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

Todd Fiala via lldb-commits lldb-commits at lists.llvm.org
Wed May 25 09:02:27 PDT 2016

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:

I wonder if we want a combined decorator that is effectively:

that specifies that this test is only valid for those debug info entries specified.  Then the entry becomes a single decorator:

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"])`

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.


More information about the lldb-commits mailing list