Re: [Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Todd Fiala via lldb-commits]

tfiala closed this revision.
tfiala added a comment.

Closing this out as it appears to be resolved one way or another.


Repository:
  rL LLVM

https://reviews.llvm.org/D21032



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Eugene Zelenko via lldb-commits]

Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.

Appear to be reverted in r272062.


Repository:
  rL LLVM

https://reviews.llvm.org/D21032



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Todd Fiala via lldb-commits]

tfiala added a comment.

Did this one go in?  If so, can we close the review?

Thanks!


Repository:
  rL LLVM

http://reviews.llvm.org/D21032



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Zachary Turner via lldb-commits]

For the diffing issue seems like we should be able to generate makefiles
with stable separators. I haven't seen the generation code, but it seems
like we could write a function lldbsuite.support.fs.unixpath() that works
like normpath but always uses /, never \. Would that fix it?

When you say it doesn't support in place renames, the only issue I'm aware
of is that you can't use os.rename if the destination already exists. As
such, you also can't implement atomic rename. As long as that isn't
required, you should be able to unlink first, then rename. I thought we had
a function in lldbsuite.support.fs that does this, but I can't remember
On Tue, Jun 7, 2016 at 2:48 PM Pavel Labath  wrote:

> labath added subscribers: zturner, labath.
> labath added a comment.
>
> Hi, I have reverted this commit, as it makes a number of assumptions,
> which are not true on windows. Please see comments for details.
>
> If you need help testing out a revised version on windows, let me know. I
> think Zachary will be able to help with that as well (:P).
>
>
> 
> Comment at: packages/Python/lldbsuite/test/lldbinline.py:135
> @@ +134,3 @@
> +if os.path.exists("Makefile"):
> +if not filecmp.cmp("Makefile", "Makefile.tmp"):
> +sys.exit("Existing Makefile doesn't match generated
> Makefile!")
> 
> This files will not end up being identical, due to different path
> separators and newlines when this is being run on windows.
>
> I like the idea of diffing, but it needs to be done in a way that it will
> work on windows.
>
> 
> Comment at: packages/Python/lldbsuite/test/lldbinline.py:136
> @@ +135,3 @@
> +if not filecmp.cmp("Makefile", "Makefile.tmp"):
> +sys.exit("Existing Makefile doesn't match generated
> Makefile!")
> +
> 
> This will not cause the error to be reported in the test runner. See <
> http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/6268/steps/test1/logs/stdio>
> where, this code is triggered, but the test is still not marked as failed
> in the summary at the end. I think throwing an exception here would do the
> expected thing.
>
> 
> Comment at: packages/Python/lldbsuite/test/lldbinline.py:138
> @@ +137,3 @@
> +
> +os.rename("Makefile.tmp", "Makefile")
> +
> 
> Windows does not support in-place renames, so this will just fail.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D21032
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Zachary Turner via lldb-commits]

zturner added a comment.

For the diffing issue seems like we should be able to generate makefiles
with stable separators. I haven't seen the generation code, but it seems
like we could write a function lldbsuite.support.fs.unixpath() that works
like normpath but always uses /, never \. Would that fix it?

When you say it doesn't support in place renames, the only issue I'm aware
of is that you can't use os.rename if the destination already exists. As
such, you also can't implement atomic rename. As long as that isn't
required, you should be able to unlink first, then rename. I thought we had
a function in lldbsuite.support.fs that does this, but I can't remember


Repository:
  rL LLVM

http://reviews.llvm.org/D21032



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Pavel Labath via lldb-commits]

labath added subscribers: zturner, labath.
labath added a comment.

Hi, I have reverted this commit, as it makes a number of assumptions, which are 
not true on windows. Please see comments for details.

If you need help testing out a revised version on windows, let me know. I think 
Zachary will be able to help with that as well (:P).



Comment at: packages/Python/lldbsuite/test/lldbinline.py:135
@@ +134,3 @@
+if os.path.exists("Makefile"):
+if not filecmp.cmp("Makefile", "Makefile.tmp"):
+sys.exit("Existing Makefile doesn't match generated Makefile!")

This files will not end up being identical, due to different path separators 
and newlines when this is being run on windows.

I like the idea of diffing, but it needs to be done in a way that it will work 
on windows.


Comment at: packages/Python/lldbsuite/test/lldbinline.py:136
@@ +135,3 @@
+if not filecmp.cmp("Makefile", "Makefile.tmp"):
+sys.exit("Existing Makefile doesn't match generated Makefile!")
+

This will not cause the error to be reported in the test runner. See 

 where, this code is triggered, but the test is still not marked as failed in 
the summary at the end. I think throwing an exception here would do the 
expected thing.


Comment at: packages/Python/lldbsuite/test/lldbinline.py:138
@@ +137,3 @@
+
+os.rename("Makefile.tmp", "Makefile")
+

Windows does not support in-place renames, so this will just fail.


Repository:
  rL LLVM

http://reviews.llvm.org/D21032



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Todd Fiala via lldb-commits]

tfiala accepted this revision.
tfiala added a comment.
This revision is now accepted and ready to land.

Looks good, Sean!


Repository:
  rL LLVM

http://reviews.llvm.org/D21032



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D21032: Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time

[Sean Callanan via lldb-commits]

spyffe created this revision.
spyffe added a reviewer: tfiala.
spyffe added a subscriber: lldb-commits.
spyffe set the repository for this revision to rL LLVM.

To eliminate problems where 'lldbinline.py'-generated Makefiles are re-used, 
I've standardized the generation of them.  The testsuite now asserts if there 
is a discrepancy between what is there and what 'lldbinline.py' would generate.

I've also eliminated all cases where what is there is legitimately different 
from what 'lldbinline.py' would generate.

Repository:
  rL LLVM

http://reviews.llvm.org/D21032

Files:
  
packages/Python/lldbsuite/test/functionalities/data-formatter/dump_dynamic/Makefile
  
packages/Python/lldbsuite/test/functionalities/data-formatter/setvaluefromcstring/Makefile
  
packages/Python/lldbsuite/test/functionalities/data-formatter/stringprinter/Makefile
  
packages/Python/lldbsuite/test/functionalities/data-formatter/typedef_array/Makefile
  packages/Python/lldbsuite/test/lang/c/struct_types/Makefile
  packages/Python/lldbsuite/test/lang/cpp/const_this/Makefile
  packages/Python/lldbsuite/test/lang/cpp/extern_c/Makefile
  packages/Python/lldbsuite/test/lang/objc/objc-runtime-ivars/Makefile
  packages/Python/lldbsuite/test/lldbinline.py
  packages/Python/lldbsuite/test/python_api/sbvalue_const_addrof/Makefile

Index: packages/Python/lldbsuite/test/python_api/sbvalue_const_addrof/Makefile
===
--- packages/Python/lldbsuite/test/python_api/sbvalue_const_addrof/Makefile
+++ packages/Python/lldbsuite/test/python_api/sbvalue_const_addrof/Makefile
@@ -1,4 +1,13 @@
 LEVEL = ../../make
 CXX_SOURCES := main.cpp
 CXXFLAGS += -std=c++11
+ifneq (,$(findstring clang,$(CC)))
+CFLAGS_EXTRAS += -fno-limit-debug-info
+endif
+
 include $(LEVEL)/Makefile.rules
+
+
+cleanup:
+	rm -f Makefile *.d
+
Index: packages/Python/lldbsuite/test/lldbinline.py
===
--- packages/Python/lldbsuite/test/lldbinline.py
+++ packages/Python/lldbsuite/test/lldbinline.py
@@ -2,7 +2,9 @@
 from __future__ import absolute_import
 
 # System modules
+import filecmp
 import os
+import sys
 
 # Third-party modules
 
@@ -89,9 +91,6 @@
 return "-N dsym %s" % (self.mydir)
 
 def BuildMakefile(self):
-if os.path.exists("Makefile"):
-return
-
 categories = {}
 
 for f in os.listdir(os.getcwd()):
@@ -102,7 +101,7 @@
 else:
 categories[t] = [f]
 
-makefile = open("Makefile", 'w+')
+makefile = open("Makefile.tmp", 'w+')
 
 level = os.sep.join([".."] * len(self.mydir.split(os.sep))) + os.sep + "make"
 
@@ -118,11 +117,26 @@
 if ('CXX_SOURCES' in list(categories.keys())):
 makefile.write("CXXFLAGS += -std=c++11\n")
 
-makefile.write("include $(LEVEL)/Makefile.rules\n")
+# clang-3.5+ outputs FullDebugInfo by default for Darwin/FreeBSD 
+# targets.  Other targets do not, which causes this test to fail.
+# This flag enables FullDebugInfo for all targets.
+
+makefile.write("ifneq (,$(findstring clang,$(CC)))\n")
+makefile.write("CFLAGS_EXTRAS += -fno-limit-debug-info\n")
+makefile.write("endif\n\n")
+
+makefile.write("include $(LEVEL)/Makefile.rules\n\n")
+
 makefile.write("\ncleanup:\n\trm -f Makefile *.d\n\n")
 makefile.flush()
 makefile.close()
 
+if os.path.exists("Makefile"):
+if not filecmp.cmp("Makefile", "Makefile.tmp"):
+sys.exit("Existing Makefile doesn't match generated Makefile!")
+
+os.rename("Makefile.tmp", "Makefile")
+
 @skipUnlessDarwin
 def __test_with_dsym(self):
 self.using_dsym = True
Index: packages/Python/lldbsuite/test/lang/objc/objc-runtime-ivars/Makefile
===
--- packages/Python/lldbsuite/test/lang/objc/objc-runtime-ivars/Makefile
+++ packages/Python/lldbsuite/test/lang/objc/objc-runtime-ivars/Makefile
@@ -1,6 +1,13 @@
 LEVEL = ../../../make
-
 OBJC_SOURCES := main.m
 LDFLAGS = $(CFLAGS) -lobjc -framework Foundation
+ifneq (,$(findstring clang,$(CC)))
+CFLAGS_EXTRAS += -fno-limit-debug-info
+endif
 
 include $(LEVEL)/Makefile.rules
+
+
+cleanup:
+	rm -f Makefile *.d
+
Index: packages/Python/lldbsuite/test/lang/cpp/extern_c/Makefile
===
--- packages/Python/lldbsuite/test/lang/cpp/extern_c/Makefile
+++ packages/Python/lldbsuite/test/lang/cpp/extern_c/Makefile
@@ -1,3 +1,13 @@
 LEVEL = ../../../make
 CXX_SOURCES := main.cpp
+CXXFLAGS += -std=c++11
+ifneq (,$(findstring clang,$(CC)))
+CFLAGS_EXTRAS += -fno-limit-debug-info
+endif
+
 include $(LEVEL)/Makefile.rules
+
+
+cleanup:
+	rm -f Makefile *.d
+
Index: packages/Python/lldbsuite/test/lang/cpp/const_this/Makefile