[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