Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL270848: Add "-gmodules" support to the test suite. (authored 
by tfiala).

Changed prior to commit:
  http://reviews.llvm.org/D19998?vs=58459=58605#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D19998

Files:
  lldb/trunk/packages/Python/lldbsuite/support/gmodules.py
  lldb/trunk/packages/Python/lldbsuite/test/decorators.py
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/top-level/TestTopLevelExprs.py
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/dead-strip/TestDeadStrip.py
  
lldb/trunk/packages/Python/lldbsuite/test/lang/c/global_variables/TestGlobalVariables.py
  
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
  
lldb/trunk/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods2.py
  
lldb/trunk/packages/Python/lldbsuite/test/lang/objc/foundation/TestRuntimeTypes.py
  lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py
  lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
  lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/trunk/packages/Python/lldbsuite/test/plugins/builder_base.py
  lldb/trunk/packages/Python/lldbsuite/test/test_categories.py

Index: lldb/trunk/packages/Python/lldbsuite/test/plugins/builder_base.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/plugins/builder_base.py
+++ lldb/trunk/packages/Python/lldbsuite/test/plugins/builder_base.py
@@ -143,6 +143,17 @@
 # True signifies that we can handle building dwo.
 return True
 
+def buildGModules(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
+"""Build the binaries with dwarf debug info."""
+commands = []
+if clean:
+commands.append([getMake(), "clean", getCmdLine(dictionary)])
+commands.append([getMake(), "MAKE_DSYM=NO", "MAKE_GMODULES=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
+
+lldbtest.system(commands, sender=sender)
+# True signifies that we can handle building with gmodules.
+return True
+
 def cleanup(sender=None, dictionary=None):
 """Perform a platform-specific cleanup after the test."""
 #import traceback
Index: lldb/trunk/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/trunk/packages/Python/lldbsuite/test/test_categories.py
@@ -11,16 +11,19 @@
 # Third-party modules
 
 # LLDB modules
+from lldbsuite.support import gmodules
+
 
 debug_info_categories = [
-'dwarf', 'dwo', 'dsym'
+'dwarf', 'dwo', 'dsym', 'gmodules'
 ]
 
 all_categories = {
 'dataformatters': 'Tests related to the type command and the data formatters subsystem',
 'dwarf' : 'Tests that can be run with DWARF debug information',
 'dwo'   : 'Tests that can be run with DWO debug information',
 'dsym'  : 'Tests that can be run with DSYM debug information',
+'gmodules'  : 'Tests that can be run with -gmodules debug information',
 'expression': 'Tests related to the expression parser',
 'objc'  : 'Tests related to the Objective-C programming language support',
 'pyapi' : 'Tests related to the Python API',
@@ -42,12 +45,27 @@
 candidate = item
 return candidate
 
-def is_supported_on_platform(category, platform):
+
+def is_supported_on_platform(category, platform, compiler_paths):
 if category == "dwo":
 # -gsplit-dwarf is not implemented by clang on Windows.
 return platform in ["linux", "freebsd"]
 elif category == "dsym":
 return platform in ["darwin", "macosx", "ios"]
+elif category == "gmodules":
+# First, check to see if the platform can even support gmodules.
+if platform not in ["linux", "freebsd", "darwin", "macosx", "ios"]:
+return False
+# If all compilers specified support gmodules, we'll enable it.
+for compiler_path in compiler_paths:
+if not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+# Ideally in a multi-compiler scenario during a single test run, this would
+# allow gmodules on compilers that support it and not on ones that don't.
+# However, I didn't see an easy way for all the callers of this to know
+# the compiler being used for a test invocation.  As we tend to run with
+# a single compiler per test run, this shouldn't be a major issue.
+return False
+return True
 return True
 
 def validate(categories, exact_match):
Index: lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
+++ 

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

Okay, thanks Zachary.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Zachary Turner via lldb-commits]

zturner added a comment.

Going to try this out on Windows and see how it goes.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala marked 15 inline comments as done.
tfiala added a comment.

Marked all code comments as done.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

I believe this now accounts for everything we discussed changing.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala updated this revision to Diff 58459.
tfiala added a comment.

Remove the now-unneeded second check for Windows.


http://reviews.llvm.org/D19998

Files:
  packages/Python/lldbsuite/support/gmodules.py
  packages/Python/lldbsuite/test/decorators.py
  
packages/Python/lldbsuite/test/expression_command/top-level/TestTopLevelExprs.py
  packages/Python/lldbsuite/test/functionalities/dead-strip/TestDeadStrip.py
  packages/Python/lldbsuite/test/lang/c/global_variables/TestGlobalVariables.py
  packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods2.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestRuntimeTypes.py
  packages/Python/lldbsuite/test/lldbinline.py
  packages/Python/lldbsuite/test/lldbtest.py
  packages/Python/lldbsuite/test/make/Makefile.rules
  packages/Python/lldbsuite/test/plugins/builder_base.py
  packages/Python/lldbsuite/test/test_categories.py

Index: packages/Python/lldbsuite/test/test_categories.py
===
--- packages/Python/lldbsuite/test/test_categories.py
+++ packages/Python/lldbsuite/test/test_categories.py
@@ -11,16 +11,19 @@
 # Third-party modules
 
 # LLDB modules
+from lldbsuite.support import gmodules
+
 
 debug_info_categories = [
-'dwarf', 'dwo', 'dsym'
+'dwarf', 'dwo', 'dsym', 'gmodules'
 ]
 
 all_categories = {
 'dataformatters': 'Tests related to the type command and the data formatters subsystem',
 'dwarf' : 'Tests that can be run with DWARF debug information',
 'dwo'   : 'Tests that can be run with DWO debug information',
 'dsym'  : 'Tests that can be run with DSYM debug information',
+'gmodules'  : 'Tests that can be run with -gmodules debug information',
 'expression': 'Tests related to the expression parser',
 'objc'  : 'Tests related to the Objective-C programming language support',
 'pyapi' : 'Tests related to the Python API',
@@ -42,12 +45,27 @@
 candidate = item
 return candidate
 
-def is_supported_on_platform(category, platform):
+
+def is_supported_on_platform(category, platform, compiler_paths):
 if category == "dwo":
 # -gsplit-dwarf is not implemented by clang on Windows.
 return platform in ["linux", "freebsd"]
 elif category == "dsym":
 return platform in ["darwin", "macosx", "ios"]
+elif category == "gmodules":
+# First, check to see if the platform can even support gmodules.
+if platform not in ["linux", "freebsd", "darwin", "macosx", "ios"]:
+return False
+# If all compilers specified support gmodules, we'll enable it.
+for compiler_path in compiler_paths:
+if not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+# Ideally in a multi-compiler scenario during a single test run, this would
+# allow gmodules on compilers that support it and not on ones that don't.
+# However, I didn't see an easy way for all the callers of this to know
+# the compiler being used for a test invocation.  As we tend to run with
+# a single compiler per test run, this shouldn't be a major issue.
+return False
+return True
 return True
 
 def validate(categories, exact_match):
Index: packages/Python/lldbsuite/test/plugins/builder_base.py
===
--- packages/Python/lldbsuite/test/plugins/builder_base.py
+++ packages/Python/lldbsuite/test/plugins/builder_base.py
@@ -143,6 +143,17 @@
 # True signifies that we can handle building dwo.
 return True
 
+def buildGModules(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
+"""Build the binaries with dwarf debug info."""
+commands = []
+if clean:
+commands.append([getMake(), "clean", getCmdLine(dictionary)])
+commands.append([getMake(), "MAKE_DSYM=NO", "MAKE_GMODULES=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
+
+lldbtest.system(commands, sender=sender)
+# True signifies that we can handle building with gmodules.
+return True
+
 def cleanup(sender=None, dictionary=None):
 """Perform a platform-specific cleanup after the test."""
 #import traceback
Index: packages/Python/lldbsuite/test/make/Makefile.rules
===
--- packages/Python/lldbsuite/test/make/Makefile.rules
+++ packages/Python/lldbsuite/test/make/Makefile.rules
@@ -212,8 +212,13 @@
 	CFLAGS += -gsplit-dwarf
 endif
 
+ifeq "$(MAKE_GMODULES)" "YES"
+	CFLAGS += -fmodules -gmodules
+endif
+
 CXXFLAGS += -std=c++11
-CXXFLAGS += $(CFLAGS)
+# FIXME: C++ modules aren't supported on all platforms.
+CXXFLAGS += $(subst -fmodules,, $(CFLAGS))
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += 

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala updated this revision to Diff 58458.
tfiala added a comment.

Adjustments per review, removal of decorator, fixup of
incorrect build method call in inline tests.


http://reviews.llvm.org/D19998

Files:
  packages/Python/lldbsuite/support/gmodules.py
  packages/Python/lldbsuite/test/decorators.py
  
packages/Python/lldbsuite/test/expression_command/top-level/TestTopLevelExprs.py
  packages/Python/lldbsuite/test/functionalities/dead-strip/TestDeadStrip.py
  packages/Python/lldbsuite/test/lang/c/global_variables/TestGlobalVariables.py
  packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods2.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestRuntimeTypes.py
  packages/Python/lldbsuite/test/lldbinline.py
  packages/Python/lldbsuite/test/lldbtest.py
  packages/Python/lldbsuite/test/make/Makefile.rules
  packages/Python/lldbsuite/test/plugins/builder_base.py
  packages/Python/lldbsuite/test/test_categories.py

Index: packages/Python/lldbsuite/test/test_categories.py
===
--- packages/Python/lldbsuite/test/test_categories.py
+++ packages/Python/lldbsuite/test/test_categories.py
@@ -11,16 +11,19 @@
 # Third-party modules
 
 # LLDB modules
+from lldbsuite.support import gmodules
+
 
 debug_info_categories = [
-'dwarf', 'dwo', 'dsym'
+'dwarf', 'dwo', 'dsym', 'gmodules'
 ]
 
 all_categories = {
 'dataformatters': 'Tests related to the type command and the data formatters subsystem',
 'dwarf' : 'Tests that can be run with DWARF debug information',
 'dwo'   : 'Tests that can be run with DWO debug information',
 'dsym'  : 'Tests that can be run with DSYM debug information',
+'gmodules'  : 'Tests that can be run with -gmodules debug information',
 'expression': 'Tests related to the expression parser',
 'objc'  : 'Tests related to the Objective-C programming language support',
 'pyapi' : 'Tests related to the Python API',
@@ -42,12 +45,27 @@
 candidate = item
 return candidate
 
-def is_supported_on_platform(category, platform):
+
+def is_supported_on_platform(category, platform, compiler_paths):
 if category == "dwo":
 # -gsplit-dwarf is not implemented by clang on Windows.
 return platform in ["linux", "freebsd"]
 elif category == "dsym":
 return platform in ["darwin", "macosx", "ios"]
+elif category == "gmodules":
+# First, check to see if the platform can even support gmodules.
+if platform not in ["linux", "freebsd", "darwin", "macosx", "ios"]:
+return False
+# If all compilers specified support gmodules, we'll enable it.
+for compiler_path in compiler_paths:
+if not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+# Ideally in a multi-compiler scenario during a single test run, this would
+# allow gmodules on compilers that support it and not on ones that don't.
+# However, I didn't see an easy way for all the callers of this to know
+# the compiler being used for a test invocation.  As we tend to run with
+# a single compiler per test run, this shouldn't be a major issue.
+return False
+return True
 return True
 
 def validate(categories, exact_match):
Index: packages/Python/lldbsuite/test/plugins/builder_base.py
===
--- packages/Python/lldbsuite/test/plugins/builder_base.py
+++ packages/Python/lldbsuite/test/plugins/builder_base.py
@@ -143,6 +143,17 @@
 # True signifies that we can handle building dwo.
 return True
 
+def buildGModules(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
+"""Build the binaries with dwarf debug info."""
+commands = []
+if clean:
+commands.append([getMake(), "clean", getCmdLine(dictionary)])
+commands.append([getMake(), "MAKE_DSYM=NO", "MAKE_GMODULES=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
+
+lldbtest.system(commands, sender=sender)
+# True signifies that we can handle building with gmodules.
+return True
+
 def cleanup(sender=None, dictionary=None):
 """Perform a platform-specific cleanup after the test."""
 #import traceback
Index: packages/Python/lldbsuite/test/make/Makefile.rules
===
--- packages/Python/lldbsuite/test/make/Makefile.rules
+++ packages/Python/lldbsuite/test/make/Makefile.rules
@@ -212,8 +212,13 @@
 	CFLAGS += -gsplit-dwarf
 endif
 
+ifeq "$(MAKE_GMODULES)" "YES"
+	CFLAGS += -fmodules -gmodules
+endif
+
 CXXFLAGS += -std=c++11
-CXXFLAGS += $(CFLAGS)
+# FIXME: C++ modules aren't supported on all platforms.
+CXXFLAGS += $(subst -fmodules,, $(CFLAGS))
 LD 

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

I also stuck a print in to verify the gmodules build was being run everywhere I 
expected (i.e. for normal tests, inline tests, and the gmodules-explicit tests).


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

Yep, I see how it works now.

I'll do a quick follow-up review after this to get that other issue with the 
debug info multiplicator (you may have just coined a word ;-)) fixed since I'm 
already in this code.

Just finishing testing it on OS X.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Pavel Labath via lldb-commits]

labath added inline comments.


Comment at: packages/Python/lldbsuite/test/decorators.py:527
@@ -525,3 +526,3 @@
 
-def skipUnlessClangModules():
+def skipUnlessClangModules(func):
 """Decorate the item to skip test unless Clang -gmodules flag is 
supported."""

tfiala wrote:
> labath wrote:
> > This whole decorator can be removed. It's invocations can be replaced with 
> > `add_test_categories(["gmodules"])` (cf. `lldbtest.py:1453`, it will avoid 
> > duplicating the test if it is already annotated with categories).
> Yes, I see now.  I did not understand add_test_categories(["gmodules"]) was a 
> valid way to mark a test as gmodules-only.
> 
> It does look a little weird that we need to do:
> @no_debug_info_test
> @add_test_categories(["gmodules"])
> 
> I wonder if we want a combined decorator that is effectively:
> @valid_debug_info([...])
> 
> that specifies that this test is only valid for those debug info entries 
> specified.  Then the entry becomes a single decorator:
> @valid_debug_info(["gmodules"])
> 
> What do you think?
You shouldn't need both decorators. The test multiplicator will first check 
whether whether the test is already explicitly annotated with some debug info 
category (lldbtest.py:1446) and multiply only into the categories that were not 
annotated.

Now that I look at it closer, I see that the code there is not entirely correct 
in case you annotate a test with two categories (it will create two tests, but 
both of them will be annotated with both categories). However, this is a topic 
for another change, and should not prevent you from using it now.

So, yes, you should only need one decorator, and things should just work. As 
for the current syntax, I am open to making any changes to it, but I want to 
avoid having two ways of doing this in parallel.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala updated this revision to Diff 58449.
tfiala added a comment.

Just updating the patch set to what I had intended to give Adrian in r6.  This 
does not yet contain any of the suggestions from the latest round.


http://reviews.llvm.org/D19998

Files:
  packages/Python/lldbsuite/support/gmodules.py
  packages/Python/lldbsuite/test/decorators.py
  
packages/Python/lldbsuite/test/expression_command/top-level/TestTopLevelExprs.py
  packages/Python/lldbsuite/test/functionalities/dead-strip/TestDeadStrip.py
  packages/Python/lldbsuite/test/lang/c/global_variables/TestGlobalVariables.py
  packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods2.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestRuntimeTypes.py
  packages/Python/lldbsuite/test/lldbinline.py
  packages/Python/lldbsuite/test/lldbtest.py
  packages/Python/lldbsuite/test/make/Makefile.rules
  packages/Python/lldbsuite/test/plugins/builder_base.py
  packages/Python/lldbsuite/test/test_categories.py

Index: packages/Python/lldbsuite/test/test_categories.py
===
--- packages/Python/lldbsuite/test/test_categories.py
+++ packages/Python/lldbsuite/test/test_categories.py
@@ -11,16 +11,19 @@
 # Third-party modules
 
 # LLDB modules
+from lldbsuite.support import gmodules
+
 
 debug_info_categories = [
-'dwarf', 'dwo', 'dsym'
+'dwarf', 'dwo', 'dsym', 'gmodules'
 ]
 
 all_categories = {
 'dataformatters': 'Tests related to the type command and the data formatters subsystem',
 'dwarf' : 'Tests that can be run with DWARF debug information',
 'dwo'   : 'Tests that can be run with DWO debug information',
 'dsym'  : 'Tests that can be run with DSYM debug information',
+'gmodules'  : 'Tests that can be run with -gmodules debug information',
 'expression': 'Tests related to the expression parser',
 'objc'  : 'Tests related to the Objective-C programming language support',
 'pyapi' : 'Tests related to the Python API',
@@ -42,12 +45,26 @@
 candidate = item
 return candidate
 
-def is_supported_on_platform(category, platform):
+
+def is_supported_on_platform(category, platform, compiler_paths):
 if category == "dwo":
 # -gsplit-dwarf is not implemented by clang on Windows.
 return platform in ["linux", "freebsd"]
 elif category == "dsym":
 return platform in ["darwin", "macosx", "ios"]
+elif category == "gmodules":
+# First, check to see if the platform can even support gmodules.
+if platform not in ["linux", "freebsd", "darwin", "macosx", "ios"]:
+return False
+# If all compilers specified support gmodules, we'll enable it.
+for compiler_path in compiler_paths:
+if not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+# Ideally in a multi-compiler scenario during a single test run, this would
+# allow gmodules on compilers that support it and not on ones that don't.
+# However, I didn't see an easy way for all the callers of this to know
+# the compiler being used for a test invocation.
+return False
+return True
 return True
 
 def validate(categories, exact_match):
Index: packages/Python/lldbsuite/test/plugins/builder_base.py
===
--- packages/Python/lldbsuite/test/plugins/builder_base.py
+++ packages/Python/lldbsuite/test/plugins/builder_base.py
@@ -143,6 +143,17 @@
 # True signifies that we can handle building dwo.
 return True
 
+def buildGModules(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
+"""Build the binaries with dwarf debug info."""
+commands = []
+if clean:
+commands.append([getMake(), "clean", getCmdLine(dictionary)])
+commands.append([getMake(), "MAKE_DSYM=NO", "MAKE_GMODULES=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
+
+lldbtest.system(commands, sender=sender)
+# True signifies that we can handle building with gmodules.
+return True
+
 def cleanup(sender=None, dictionary=None):
 """Perform a platform-specific cleanup after the test."""
 #import traceback
Index: packages/Python/lldbsuite/test/make/Makefile.rules
===
--- packages/Python/lldbsuite/test/make/Makefile.rules
+++ packages/Python/lldbsuite/test/make/Makefile.rules
@@ -212,8 +212,13 @@
 	CFLAGS += -gsplit-dwarf
 endif
 
+ifeq "$(MAKE_GMODULES)" "YES"
+	CFLAGS += -fmodules -gmodules
+endif
+
 CXXFLAGS += -std=c++11
-CXXFLAGS += $(CFLAGS)
+# FIXME: C++ modules aren't supported on all platforms.
+CXXFLAGS += $(subst -fmodules,, $(CFLAGS))
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS)
@@ 

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

> @aprantl, the lldbsuite/support/gmodules.py file didn't make it into your 
> patch set here. (It was the top file in the -v6 diff).


Totally incorrect.  The top file is the new file, but it is malformed and is 
showing a diff rather than the whole file.  Not sure exactly how I produced 
that diff, but that is something I'm about to rectify.  I'll put up a fresh 
patch set after I address the issues in the review.  (It may take a while as I 
will also be testing with adjustments for the inline tests, that did not use 
the gmodules build steps --- regular tests did, just not inline ones, which we 
happen to have a lot of).


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala commandeered this revision.
tfiala edited reviewers, added: aprantl; removed: tfiala.
tfiala added a comment.

Commandeering.

Thanks, Adrian!


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Adrian Prantl via lldb-commits]

aprantl abandoned this revision.
aprantl added a comment.

Todd, I'm abandoning the review. This should allow you to claim it as your own 
so you can iterate quicker. Thanks!


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

@aprantl, the lldbsuite/support/gmodules.py file didn't make it into your patch 
set here.  (It was the top file in the -v6 diff).

I'm going to adjust per comments above.  @labath, see question on 
add_test_categories.

I may end up filing this as a separate review since I'm WFH and we can probably 
iterate faster without getting @aprantl to have to keep putting up patch sets 
for me.  (I didn't see a way for phabricator to allow multiple people to put up 
a diff set on the same review - if that existed I'd use that).

@zturner, with the changes @labath suggested, that one decorator that was used 
by that one test will go away, and then the is_supported-style check for 
gmodules (as currently written) will not permit gmodules on Windows.  It is set 
to support macosx, ios, linux and freebsd.



Comment at: packages/Python/lldbsuite/support/gmodules.py:19
@@ -19,2 +19,2 @@
 compiler = os.path.basename(compiler_path)
 if "clang" not in compiler:

labath wrote:
> This diff looks broken. This appears to be a new file (I certainly don't have 
> it in my tree), and it is being shown as a diff. Could you reupload a correct 
> diff against the current top of tree?
Yes it is missing from the patch set.  It is in the -v6.diff file I attached 
earlier (top entry in the diff).  We'll get this updated.


Comment at: packages/Python/lldbsuite/support/gmodules.py:22
@@ -21,3 +21,3 @@
 return False
-clang_help = os.popen("%s --help" % compiler_path).read()
-return GMODULES_HELP_REGEX.search(clang_help, re.DOTALL) is not None
+elif os.name == "nt":
+# gmodules support is broken on Windows

labath wrote:
> This check should be folded into `test_categories.is_supported_on_platform`. 
> (Also, it is incorrect as it should presumably be checking against the target 
> platform and not the host.)
It actually is in is_supported_on_platform (by virtue of windows not being 
included).

I had misunderstood your earlier comment on how add_categories worked.  I did 
not know that I could make a debuginfo-specific test work by adding the 
category.  That makes sense now, but I had kept the other decorator around only 
because I didn't see this as an option.

I get it now.  Good idea.


Comment at: packages/Python/lldbsuite/test/decorators.py:527
@@ -525,3 +526,3 @@
 
-def skipUnlessClangModules():
+def skipUnlessClangModules(func):
 """Decorate the item to skip test unless Clang -gmodules flag is 
supported."""

labath wrote:
> This whole decorator can be removed. It's invocations can be replaced with 
> `add_test_categories(["gmodules"])` (cf. `lldbtest.py:1453`, it will avoid 
> duplicating the test if it is already annotated with categories).
Yes, I see now.  I did not understand add_test_categories(["gmodules"]) was a 
valid way to mark a test as gmodules-only.

It does look a little weird that we need to do:
@no_debug_info_test
@add_test_categories(["gmodules"])

I wonder if we want a combined decorator that is effectively:
@valid_debug_info([...])

that specifies that this test is only valid for those debug info entries 
specified.  Then the entry becomes a single decorator:
@valid_debug_info(["gmodules"])

What do you think?


Comment at: 
packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py:13
@@ -12,1 +12,3 @@
+@skipUnlessClangModules
 def test_specialized_typedef_from_pch(self):
+self.buildGModules()

labath wrote:
> replace these two decorators with `add_test_categories(["gmodules"])`
Yup.

Well we would need @no_debug_info_test still, wouldn't we?  Otherwise we'll fan 
out to all the debug info variants?  (Or is add_test_categories() replace all 
test categories?  I assumed it was additive since it starts with "add_", in 
which case I'd expect we'd still have all the normal debug info entries show 
up).


Comment at: packages/Python/lldbsuite/test/lldbinline.py:148
@@ +147,3 @@
+self.BuildMakefile()
+self.buildDwarf()
+self.do_test()

labath wrote:
> `buildGModules()` ?
Yeah that's wrong.  Good catch.

That also means the testing I did on OS X and Linux failed to do real gmodule 
debugging for inline tests.  I'll need to rerun.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Pavel Labath via lldb-commits]

labath added a comment.

I think we should make one more iteration on this, as there is still some room 
for improvement.

In http://reviews.llvm.org/D19998#438756, @zturner wrote:

> Should this be disabled by default unless explicitly requested? Seems like
>  "run the entire test suite N times" should be opt in, not opt out.


I think it makes sense to run multiple variants of some tests, as "gmodules" is 
a regular debugger feature and we want to make sure we support that feature. 
What I think is a problem here is the "whole test suite" part, as I think a lot 
of tests (think, all process control and thread tests) don't (or shouldn't) 
depend on debug info (see discussion early on in this thread). My feeling is 
that we should start restricting the duplication more aggressively, and then it 
won't matter that **some** tests get run multiple times.

That said, if you as the windows maintainer say that you don't want to support 
gmodules debugging on windows at the moment, then I think it's fine to skip 
those tests on this platform.



Comment at: packages/Python/lldbsuite/support/gmodules.py:19
@@ -19,2 +19,2 @@
 compiler = os.path.basename(compiler_path)
 if "clang" not in compiler:

This diff looks broken. This appears to be a new file (I certainly don't have 
it in my tree), and it is being shown as a diff. Could you reupload a correct 
diff against the current top of tree?


Comment at: packages/Python/lldbsuite/support/gmodules.py:22
@@ -21,3 +21,3 @@
 return False
-clang_help = os.popen("%s --help" % compiler_path).read()
-return GMODULES_HELP_REGEX.search(clang_help, re.DOTALL) is not None
+elif os.name == "nt":
+# gmodules support is broken on Windows

This check should be folded into `test_categories.is_supported_on_platform`. 
(Also, it is incorrect as it should presumably be checking against the target 
platform and not the host.)


Comment at: packages/Python/lldbsuite/test/decorators.py:527
@@ -525,3 +526,3 @@
 
-def skipUnlessClangModules():
+def skipUnlessClangModules(func):
 """Decorate the item to skip test unless Clang -gmodules flag is 
supported."""

This whole decorator can be removed. It's invocations can be replaced with 
`add_test_categories(["gmodules"])` (cf. `lldbtest.py:1453`, it will avoid 
duplicating the test if it is already annotated with categories).


Comment at: 
packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py:13
@@ -12,1 +12,3 @@
+@skipUnlessClangModules
 def test_specialized_typedef_from_pch(self):
+self.buildGModules()

replace these two decorators with `add_test_categories(["gmodules"])`


Comment at: packages/Python/lldbsuite/test/lldbinline.py:148
@@ +147,3 @@
+self.BuildMakefile()
+self.buildDwarf()
+self.do_test()

`buildGModules()` ?


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Zachary Turner via lldb-commits]

Should this be disabled by default unless explicitly requested? Seems like
"run the entire test suite N times" should be opt in, not opt out.

If its opt in, I don't mind removing the os check for Windows.
On Tue, May 24, 2016 at 6:25 PM Adrian Prantl  wrote:

> aprantl added a comment.
>
> In http://reviews.llvm.org/D19998#438666, @tfiala wrote:
>
> > In http://reviews.llvm.org/D19998#438614, @zturner wrote:
> >
> > > So I asked some of the guys here, and they said modules debug info (in
> particular -gmodules) will not work anywhere but OSX.
> >
> >
> > I don't think that's right.  I ran all these tests on Linux, and our
> debug info guy (Adrian Prantl) thinks it is better supported on Linux than
> OS X.  ?  I'll let Adrian weigh in on that though.  I got all but the one
> dead code stripping test to run fine on Ubuntu 14.04 x86_64.
>
>
> Clang modules themselves (-fmodules) can be thought of as almost a build
> system / preprocessor feature that does not or rather can not have any
> effect on the debugger at all. Darwin (OS X, iOS, ...) supports clang
> modules for C and Objective-C very well. C++ clang modules are not
> supported on Darwin. On Linux clang modules are supported, including C++
> modules. I believe I also heard of people using clang modules on FreeBSD.
> On Windows, clang can build and use modules, but I don't think anyone is
> using/testing this actively.
>
> Clang module debugging (-fmodules -gmodules) affects the debugger (debug
> info for module types is emitted only once in the compiled module and
> referred to by reference in the object files). It is fully supported on
> Darwin. On Linux, can in theory support it (there may be some unresolved
> bugs with DWOs), but nobody tested this so far and this patch would make it
> possible to figure out where we're at. I think tests should be XFAIL'ed on
> an individual basis and considered bugs if they don't work on Linux.
>
> - adrian
>
>
> http://reviews.llvm.org/D19998
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Zachary Turner via lldb-commits]

zturner added a comment.

Should this be disabled by default unless explicitly requested? Seems like
"run the entire test suite N times" should be opt in, not opt out.

If its opt in, I don't mind removing the os check for Windows.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Adrian Prantl via lldb-commits]

aprantl added a comment.

In http://reviews.llvm.org/D19998#438666, @tfiala wrote:

> In http://reviews.llvm.org/D19998#438614, @zturner wrote:
>
> > So I asked some of the guys here, and they said modules debug info (in 
> > particular -gmodules) will not work anywhere but OSX.
>
>
> I don't think that's right.  I ran all these tests on Linux, and our debug 
> info guy (Adrian Prantl) thinks it is better supported on Linux than OS X.  ? 
>  I'll let Adrian weigh in on that though.  I got all but the one dead code 
> stripping test to run fine on Ubuntu 14.04 x86_64.


Clang modules themselves (-fmodules) can be thought of as almost a build system 
/ preprocessor feature that does not or rather can not have any effect on the 
debugger at all. Darwin (OS X, iOS, ...) supports clang modules for C and 
Objective-C very well. C++ clang modules are not supported on Darwin. On Linux 
clang modules are supported, including C++ modules. I believe I also heard of 
people using clang modules on FreeBSD. On Windows, clang can build and use 
modules, but I don't think anyone is using/testing this actively.

Clang module debugging (-fmodules -gmodules) affects the debugger (debug info 
for module types is emitted only once in the compiled module and referred to by 
reference in the object files). It is fully supported on Darwin. On Linux, can 
in theory support it (there may be some unresolved bugs with DWOs), but nobody 
tested this so far and this patch would make it possible to figure out where 
we're at. I think tests should be XFAIL'ed on an individual basis and 
considered bugs if they don't work on Linux.

- adrian


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Adrian Prantl via lldb-commits]

aprantl updated this revision to Diff 58369.
aprantl added a comment.

Update with Todds most recent infrastructure additions.


http://reviews.llvm.org/D19998

Files:
  packages/Python/lldbsuite/support/gmodules.py
  packages/Python/lldbsuite/test/decorators.py
  
packages/Python/lldbsuite/test/expression_command/top-level/TestTopLevelExprs.py
  packages/Python/lldbsuite/test/functionalities/dead-strip/TestDeadStrip.py
  packages/Python/lldbsuite/test/lang/c/global_variables/TestGlobalVariables.py
  packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods2.py
  packages/Python/lldbsuite/test/lang/objc/foundation/TestRuntimeTypes.py
  packages/Python/lldbsuite/test/lldbinline.py
  packages/Python/lldbsuite/test/lldbtest.py
  packages/Python/lldbsuite/test/make/Makefile.rules
  packages/Python/lldbsuite/test/plugins/builder_base.py
  packages/Python/lldbsuite/test/test_categories.py

Index: packages/Python/lldbsuite/test/test_categories.py
===
--- packages/Python/lldbsuite/test/test_categories.py
+++ packages/Python/lldbsuite/test/test_categories.py
@@ -11,9 +11,11 @@
 # Third-party modules
 
 # LLDB modules
+from lldbsuite.support import gmodules
+
 
 debug_info_categories = [
-'dwarf', 'dwo', 'dsym'
+'dwarf', 'dwo', 'dsym', 'gmodules'
 ]
 
 all_categories = {
@@ -21,6 +23,7 @@
 'dwarf' : 'Tests that can be run with DWARF debug information',
 'dwo'   : 'Tests that can be run with DWO debug information',
 'dsym'  : 'Tests that can be run with DSYM debug information',
+'gmodules'  : 'Tests that can be run with -gmodules debug information',
 'expression': 'Tests related to the expression parser',
 'objc'  : 'Tests related to the Objective-C programming language support',
 'pyapi' : 'Tests related to the Python API',
@@ -42,12 +45,26 @@
 candidate = item
 return candidate
 
-def is_supported_on_platform(category, platform):
+
+def is_supported_on_platform(category, platform, compiler_paths):
 if category == "dwo":
 # -gsplit-dwarf is not implemented by clang on Windows.
 return platform in ["linux", "freebsd"]
 elif category == "dsym":
 return platform in ["darwin", "macosx", "ios"]
+elif category == "gmodules":
+# First, check to see if the platform can even support gmodules.
+if platform not in ["linux", "freebsd", "darwin", "macosx", "ios"]:
+return False
+# If all compilers specified support gmodules, we'll enable it.
+for compiler_path in compiler_paths:
+if not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+# Ideally in a multi-compiler scenario during a single test run, this would
+# allow gmodules on compilers that support it and not on ones that don't.
+# However, I didn't see an easy way for all the callers of this to know
+# the compiler being used for a test invocation.
+return False
+return True
 return True
 
 def validate(categories, exact_match):
Index: packages/Python/lldbsuite/test/plugins/builder_base.py
===
--- packages/Python/lldbsuite/test/plugins/builder_base.py
+++ packages/Python/lldbsuite/test/plugins/builder_base.py
@@ -143,6 +143,17 @@
 # True signifies that we can handle building dwo.
 return True
 
+def buildGModules(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
+"""Build the binaries with dwarf debug info."""
+commands = []
+if clean:
+commands.append([getMake(), "clean", getCmdLine(dictionary)])
+commands.append([getMake(), "MAKE_DSYM=NO", "MAKE_GMODULES=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
+
+lldbtest.system(commands, sender=sender)
+# True signifies that we can handle building with gmodules.
+return True
+
 def cleanup(sender=None, dictionary=None):
 """Perform a platform-specific cleanup after the test."""
 #import traceback
Index: packages/Python/lldbsuite/test/make/Makefile.rules
===
--- packages/Python/lldbsuite/test/make/Makefile.rules
+++ packages/Python/lldbsuite/test/make/Makefile.rules
@@ -212,8 +212,13 @@
 	CFLAGS += -gsplit-dwarf
 endif
 
+ifeq "$(MAKE_GMODULES)" "YES"
+	CFLAGS += -fmodules -gmodules
+endif
+
 CXXFLAGS += -std=c++11
-CXXFLAGS += $(CFLAGS)
+# FIXME: C++ modules aren't supported on all platforms.
+CXXFLAGS += $(subst -fmodules,, $(CFLAGS))
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS)
@@ -519,7 +524,7 @@
 $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
 	$(CXX) $(CXXFLAGS) -x c++-header -o $(PCH_OUTPUT) $(PCH_CXX_SOURCE)
 %.o : %.cpp $(PCH_OUTPUT)
-	$(CXX) $(PCHFLAGS) 

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

In http://reviews.llvm.org/D19998#438614, @zturner wrote:

> In http://reviews.llvm.org/D19998#438586, @tfiala wrote:
>
> > F1982070: gmodules-test-support-v6.diff 
> >
> > Updated gmodules.is_compiler_clang_with_gmodules() to guard on Windows with:
> >
> >   def _gmodules_supported_internal():
> >   compiler = os.path.basename(compiler_path)
> >   if "clang" not in compiler:
> >   return False
> >   elif os.name == "nt":
> >   # gmodules support is broken on Windows
> >   return False
> >   else:
> >   # Check the compiler help for the -gmodules option.
> >   clang_help = os.popen("%s --help" % compiler_path).read()
> >   return GMODULES_HELP_REGEX.search(clang_help, re.DOTALL) is not 
> > None
> >   
>
>
> So I asked some of the guys here, and they said modules debug info (in 
> particular -gmodules) will not work anywhere but OSX.


I don't think that's right.  I ran all these tests on Linux, and our debug info 
guy (Adrian Prantl) thinks it is better supported on Linux than OS X.  ?  I'll 
let Adrian weigh in on that though.  I got all but the one dead code stripping 
test to run fine on Ubuntu 14.04 x86_64.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Zachary Turner via lldb-commits]

zturner added a comment.

In http://reviews.llvm.org/D19998#438586, @tfiala wrote:

> F1982070: gmodules-test-support-v6.diff 
>
> Updated gmodules.is_compiler_clang_with_gmodules() to guard on Windows with:
>
>   def _gmodules_supported_internal():
>   compiler = os.path.basename(compiler_path)
>   if "clang" not in compiler:
>   return False
>   elif os.name == "nt":
>   # gmodules support is broken on Windows
>   return False
>   else:
>   # Check the compiler help for the -gmodules option.
>   clang_help = os.popen("%s --help" % compiler_path).read()
>   return GMODULES_HELP_REGEX.search(clang_help, re.DOTALL) is not None
>   


So I asked some of the guys here, and they said modules debug info (in 
particular -gmodules) will not work anywhere but OSX.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

F1982070: gmodules-test-support-v6.diff 

Updated gmodules.is_compiler_clang_with_gmodules() to guard on Windows with:

  def _gmodules_supported_internal():
  compiler = os.path.basename(compiler_path)
  if "clang" not in compiler:
  return False
  elif os.name == "nt":
  # gmodules support is broken on Windows
  return False
  else:
  # Check the compiler help for the -gmodules option.
  clang_help = os.popen("%s --help" % compiler_path).read()
  return GMODULES_HELP_REGEX.search(clang_help, re.DOTALL) is not None


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

In http://reviews.llvm.org/D19998#438529, @zturner wrote:

> In `is_compiler_clang_with_gmodules` you will need to explicitly return false 
> if the target is Windows, because clang help will still show the command line 
> option as being valid, even though it doesn't work properly.


Ah okay.

Two code paths call that, one is guarded against Windows, but the very last 
change for that one single gmodules-only test uses the decorator that doesn't 
guard.  (I had mis-expected that -gmodules would just be missing entirely from 
the Windows end).

Adrian is about to put up the patch set.  I'll catch him and we'll modify that 
before it goes up.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Zachary Turner via lldb-commits]

zturner added a comment.

In `is_compiler_clang_with_gmodules` you will need to explicitly return false 
if the target is Windows, because clang help will still show the command line 
option as being valid, even though it doesn't work properly.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

F1981857: gmodules-test-support-v5.diff 

This is the final patch for the way I'd like to see this.  It breaks out 
gmodules checking into a separate support module (in 
lldbsuite/support/gmodules.py).  The skipUnless for gmodules now uses this 
mechanism, and the one previous gmodules-specific test is now set to run only 
for gmodules debug info.

@aprantl, if you can put up this patch set here, then we're good to get a final 
pass on the review from others.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

F1981749: gmodules-test-support-v4.diff 

Patch updated.  TestDeadStrip.py required xfailing for Ubuntu with gmodules 
debug info.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

F1981558: gmodules-test-support-v3.diff 

The attached patch contains an implementation that properly checks for gmodules 
support and runs clean on OS X.  I filed a few bugs and marked a few tests as 
failing with gmodules debug info in this patch.

I'm going to give it a run on Linux and see if we have more tests that need to 
be marked xfail with gmodules support.

@aprantl, can you update the patch sets with the latest patch?  It is built 
against r270593.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

In http://reviews.llvm.org/D19998#437820, @zturner wrote:

> Just chiming in to say modules is unsupported on Windows even with clang,
>  so please make sure the gmodules support function takes this into account


Okay, thanks Zachary.  We'll make sure it doesn't somehow get enabled there.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Zachary Turner via lldb-commits]

Just chiming in to say modules is unsupported on Windows even with clang,
so please make sure the gmodules support function takes this into account
On Tue, May 24, 2016 at 9:05 AM Todd Fiala via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> tfiala added a comment.
>
> So at this point, it looks like we're just missing a mechanism to check
> for gmodules support, and then add it to the supported_categories list if
> it is available.
>
> I'll have a look a that.
>
> This might be the time where we want to break out an apple-clang vs.
> llvm.org-clang when we talk about version numbers.  We're asking for
> trouble when we treat both as the same, yet they have drastically different
> version numbers that will eventually collide as llvm.org version numbers
> increase.
>
>
> 
> Comment at: packages/Python/lldbsuite/test/lldbtest.py:1488
> @@ +1487,3 @@
> +@wraps(attrvalue)
> +def dwarf_test_method(self, attrvalue=attrvalue):
> +self.debug_info = "gmodules"
> 
> labath wrote:
> > Shouldn't this be `gmodules_test_method`?
> I agree with @labath
>
> 
> Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:144
> @@ -143,1 +143,3 @@
>
> +def buildDwarf(sender=None, architecture=None, compiler=None,
> dictionary=None, clean=True):
> +"""Build the binaries with dwarf debug info."""
> 
> labath wrote:
> > shouldn't this be `buildModules` or something?
> buildGModules() sounds right here, particularly since it matches the
> define being passed in.  And adjust the comment below to reflect.
>
>
> http://reviews.llvm.org/D19998
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

So at this point, it looks like we're just missing a mechanism to check for 
gmodules support, and then add it to the supported_categories list if it is 
available.

I'll have a look a that.

This might be the time where we want to break out an apple-clang vs. 
llvm.org-clang when we talk about version numbers.  We're asking for trouble 
when we treat both as the same, yet they have drastically different version 
numbers that will eventually collide as llvm.org version numbers increase.



Comment at: packages/Python/lldbsuite/test/lldbtest.py:1488
@@ +1487,3 @@
+@wraps(attrvalue)
+def dwarf_test_method(self, attrvalue=attrvalue):
+self.debug_info = "gmodules"

labath wrote:
> Shouldn't this be `gmodules_test_method`?
I agree with @labath


Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:144
@@ -143,1 +143,3 @@
 
+def buildDwarf(sender=None, architecture=None, compiler=None, dictionary=None, 
clean=True):
+"""Build the binaries with dwarf debug info."""

labath wrote:
> shouldn't this be `buildModules` or something?
buildGModules() sounds right here, particularly since it matches the define 
being passed in.  And adjust the comment below to reflect.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

Adrian,

I'm going to take a look at this with you today.  I'm reviewing all the 
comments on this so far.

-Todd


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

Oh right, I recall there was an action item on me.

I'll catch up with you on that in a bit.


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Adrian Prantl via lldb-commits]

aprantl added a comment.

I still need some expertise on how to disable the configuration on platforms 
with older versions of clang (or different compilers like gcc).


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

Hi Adrian,

I think this has stalled.  What do we need to resolve now?


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Pavel Labath via lldb-commits]

labath added a comment.

In http://reviews.llvm.org/D19998#423541, @aprantl wrote:

> In http://reviews.llvm.org/D19998#423277, @labath wrote:
>
> > I am glad to see more testing of the modules debugging. I have a couple of 
> > small comments though:
> >
> > - `-fmodules`: Why is it not being added to CXXFLAGS? Is this how clang is 
> > supposed to be invoked? (I am not very familiar clang modules)
>
>
> C++ modules are still a work in progress and not supported on all platforms 
> (particularly on Darwin due to the way the system module maps interact with 
> libc++, see https://llvm.org/bugs/show_bug.cgi?id=26928 for examples). On the 
> platforms where C++ modules work well (such as Linux) on the other hand, 
> module debugging hasn't been productized so far. Due to the way module 
> debugging reuses DWO mechanisms I don't expect it to work without some 
> fine-tuning.


Thank you for the comprehensive explanation. This information is interesting. 
Does that mean that in case of tests with c++ source code, there will be no 
difference in the test executables between modules and non-modules versions of 
the tests? I am wondering whether we should avoid running the modules tests in 
this case... I just did a quick count and we seem to have 129 C source files 
vs. 262 C++ files, so the difference is not actually as big as I expected, but 
it is still a substantial amount of test time going to waste. Maybe it's not 
that important though, we can possibly optimize that later, if it turns out to 
be necessary.

> > And one meta-comment not directly related to this patch:

> 

> >  We already run most of the tests two times. Now we will be doing it once 
> > more, which will increase the test times even more. I think it's important 
> > for each debug info format to have good coverage, but I also feel that 
> > there are tests which have nothing to do with debug info (or their 
> > connection to debug info is only very peripheral), and it does not make to 
> > sense to slow down the tests runs by running those tests so many times. We 
> > already have a (not very elegant, but working) mechanism to avoid this 
> > (`NO_DEBUG_INFO_TESTCASE` member). I propose that we be more aggressive in 
> > using it for new tests which do not specifically test debug info. Also when 
> > looking at existing tests, we should re-evaluate whether the test really 
> > needs to be run that many times (right now, the largest candidate that 
> > comes to mind is TestConcurrentEvents, but I am sure there are others I 
> > can't think of by name now).

> 

> 

> That sounds like an all-around good idea, but probably out of scope for this 
> patch.


Yes, I am definitely not requesting you make any changes like that here. I 
using just testing the waters about what people think about this idea. It's a 
bit of a hijack though, so sorry about that. We can move the discussion 
outside, if it starts to get more involved.

In http://reviews.llvm.org/D19998#423562, @tfiala wrote:

> >   I propose that we be more aggressive in using it for new tests which do 
> > not specifically test debug info. 
>
>
> I totally agree, but I also caution that, as a debugger, the heart of much of 
> what we do does use debug info.  So we need to be careful to make sure that 
> we don't disable the fan-out across all debug styles if we are in fact using 
> debug info.


I guess the question here is what do we consider as "using the debug info". To 
take TestConcurrentEvents as an example, it certainly *uses* debug info, in the 
sense that it sets a breakpoint at a certain line, and sets some variables, but 
I don't think this is what the test is about. It is about testing that the 
debugger handles the situation where there are multiple events happening in the 
target concurrently, which should not be related to the debug info at all. Any 
failure it this test which would be caused by a debug info problem should 
already be caught by some other test which actually targets these things.

At least, that's my opinion...



Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:144
@@ -143,1 +143,3 @@
 
+def buildDwarf(sender=None, architecture=None, compiler=None, dictionary=None, 
clean=True):
+"""Build the binaries with dwarf debug info."""

shouldn't this be `buildModules` or something?


http://reviews.llvm.org/D19998



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

Re: [Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Todd Fiala via lldb-commits]

tfiala added a comment.

>   I propose that we be more aggressive in using it for new tests which do not 
> specifically test debug info. 


I totally agree, but I also caution that, as a debugger, the heart of much of 
what we do does use debug info.  So we need to be careful to make sure that we 
don't disable the fan-out across all debug styles if we are in fact using debug 
info.


http://reviews.llvm.org/D19998



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