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
