This is an archive of the discontinued LLVM Phabricator instance.

Add a "-gmodules" category to the test suite.
ClosedPublic

Authored by tfiala on May 5 2016, 2:51 PM.

Details

Summary

This is a work-in-progress patch for adding a "-gmodules" category to the testsuite. It adds clang module debugging as a configuration alongside .dSYM and DWO to ensure that module debug info is getting broader coverage. When enabled it will add "-fmodules -gmodules" to CFLAGS (but not CXXFLAGS). It will not invoke dsymutil, since dsymutil undoes most of the module-related intricacies.

In the current form, this passes the test suite on Darwin, and it should also work on any other platform that uses clang as the host compiler.

I do have a couple of remaining questions regarding the implementation:

  • Is there a way to detect that clang is the host compiler, so we can disable the category on platforms that use a different compiler?
  • The "-fmodules" and "-gmodules" are only well supported by clang-3.9 and later. Is there a good way to test for this in is_supported_on_platform() or somewhere else?

Diff Detail

Event Timeline

aprantl updated this revision to Diff 56351.May 5 2016, 2:51 PM
aprantl retitled this revision from to Add a "-gmodules" category to the test suite..
aprantl updated this object.
aprantl added reviewers: granata.enrico, tfiala.
aprantl added a subscriber: lldb-commits.
tfiala edited edge metadata.May 5 2016, 4:24 PM

I think we can limit the tests to skip unless clang of a certain version, which will probably take care of it. I'll see if I can get the right decorator incantation for this. (It's a bit complicated by the fact that official Apple clang releases show up as the much larger version numbers, while the same clang built on the local machine will show up with the LLVM.org version numbers).

labath added a subscriber: labath.May 6 2016, 2:09 AM

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)
  • there is a @skipUnlessClangModules decorator in decorators.py. As far as I can see, this patch should now make it obsolete. It seems that it can be removed and all invocations replaced with add_test_categories(["gmodules"])

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).

packages/Python/lldbsuite/test/lldbtest.py
1488

Shouldn't this be gmodules_test_method?

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.

  • there is a @skipUnlessClangModules decorator in decorators.py. As far as I can see, this patch should now make it obsolete. It seems that it can be removed and all invocations replaced with add_test_categories(["gmodules"])

Good catch. I didn't notice that!

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.

tfiala added a comment.May 6 2016, 9:16 AM

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 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.

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...

packages/Python/lldbsuite/test/plugins/builder_base.py
144

shouldn't this be buildModules or something?

Hi Adrian,

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

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

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

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

Adrian,

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

-Todd

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.

packages/Python/lldbsuite/test/lldbtest.py
1488

I agree with @labath

packages/Python/lldbsuite/test/plugins/builder_base.py
144

buildGModules() sounds right here, particularly since it matches the define being passed in. And adjust the comment below to reflect.

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

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.

I just uploaded a modified patch. This patch does nothing more than fix invalid behavior in the initial patch, as it had several issues that would cause runtime errors.

With the adjusted patch, which is based against lldb r270570, I see the following failures on OS X 10.11.5 with Xcode 7.3.1, building test inferiors with the in-tree clang (with both compiler-rt and libcxx from top of tree):

FAIL: test_NSString_expr_commands_gmodules (lang/objc/foundation/TestObjCMethods2.py)
FAIL: test_break_gmodules (lang/objc/foundation/TestRuntimeTypes.py)
FAIL: test_c_global_variables_gmodules (lang/c/global_variables/TestGlobalVariables.py)
ERROR: test_top_level_expressions_gmodules (expression_command/top-level/TestTopLevelExprs.py)

As it stands right now, it looks like it will always try to run gmodules. I'll adjust that now per previous comments.

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.

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

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.

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.

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.

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

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.

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.

aprantl updated this revision to Diff 58369.May 24 2016, 6:09 PM
aprantl edited edge metadata.

Update with Todds most recent infrastructure additions.

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

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.

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

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.

packages/Python/lldbsuite/support/gmodules.py
19 ↗(On Diff #58369)

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?

22 ↗(On Diff #58369)

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.)

packages/Python/lldbsuite/test/decorators.py
527 ↗(On Diff #58369)

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).

packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
13 ↗(On Diff #58369)

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

packages/Python/lldbsuite/test/lldbinline.py
148

buildGModules() ?

@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.

packages/Python/lldbsuite/support/gmodules.py
19 ↗(On Diff #58369)

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.

22 ↗(On Diff #58369)

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.

packages/Python/lldbsuite/test/decorators.py
527 ↗(On Diff #58369)

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?

packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
13 ↗(On Diff #58369)

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).

packages/Python/lldbsuite/test/lldbinline.py
148

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.

aprantl abandoned this revision.May 25 2016, 9:10 AM

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

tfiala commandeered this revision.May 25 2016, 9:44 AM
tfiala edited reviewers, added: aprantl; removed: tfiala.

Commandeering.

Thanks, Adrian!

@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).

tfiala reclaimed this revision.May 25 2016, 10:04 AM
tfiala updated this revision to Diff 58449.

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.

labath added inline comments.May 25 2016, 10:11 AM
packages/Python/lldbsuite/test/decorators.py
527 ↗(On Diff #58369)

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.

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.

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).

tfiala updated this revision to Diff 58458.May 25 2016, 11:04 AM

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

tfiala updated this revision to Diff 58459.May 25 2016, 11:06 AM

Remove the now-unneeded second check for Windows.

I believe this now accounts for everything we discussed changing.

tfiala marked 15 inline comments as done.May 25 2016, 11:08 AM

Marked all code comments as done.

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

Okay, thanks Zachary.

Timing info for Ubuntu 15.10:

DescriptionTime (seconds)(mm:ss)
Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, no gmodules377s (6:17)
Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, with gmodules517s (8:37)

Net increase: 37%.

No new failures with Ubuntu 15.10 x86_64 with the inline test fix.

zturner accepted this revision.May 25 2016, 12:52 PM
zturner added a reviewer: zturner.

No problems here, I'd wait for Pavel for additional thoughts before going in.

This revision is now accepted and ready to land.May 25 2016, 12:52 PM

No problems here, I'd wait for Pavel for additional thoughts before going in.

Yep that's fine. I am in no hurry to get this in.

tfiala added a comment.EditedMay 25 2016, 1:09 PM

Updated timings, now with OS X:

ConfigurationTime (seconds)(mm:ss)
Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, no gmodules377 (6:17)
Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, with gmodules517 (8:37)
OS X 11.5 8-core, 32 GB RAM, Xcode 7.3.1 clang, no gmodules361 (6:01)
OS X 11.5 8-core, 32 GB RAM, Xcode 7.3.1 clang, with gmodules500 (8:20)
ConfigurationNet increase (%)
Ubuntu 15.10 6-core, 24 GB RAM, TOT clang37
OS X 11.5 8-core, 32 GB RAM, Xcode 7.3.1 clang39
labath accepted this revision.May 26 2016, 1:41 AM
labath added a reviewer: labath.

Looks great now. Thanks.

packages/Python/lldbsuite/test/test_categories.py
62

So, one way to achieve that would be to move the logic that decides whether to run these tests to a later stage -- to check the conditions during test running, not test generation. This would make this behave the same way as all the other skip decorators -- the test would be skipped instead of "not generated at all".

But I don't think we should do this now.

This revision was automatically updated to reflect the committed changes.