[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:
> 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 @@
- 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.)
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 @@
"""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).
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 @@
> 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 @@
> `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