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

This revision was automatically updated to reflect the committed changes. Closed by commit rL270848: Add "-gmodules" support to the test suite. (authored by tfiala). Changed prior to commit: http://reviews.llvm.org/D19998?vs=58459&id=58605#toc Repository: rL LLVM http://reviews.llvm.org/D19

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

labath accepted this revision. labath added a reviewer: labath. labath added a comment. Looks great now. Thanks. Comment at: packages/Python/lldbsuite/test/test_categories.py:62 @@ +61,3 @@ +if not gmodules.is_compiler_clang_with_gmodules(compiler_path): +

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

tfiala added a comment. Updated timings, now with OS X: | Configuration | Time (seconds)(mm:ss) | | Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, no gmodules| 377 (6:17) | | Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, with gmodules

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

tfiala added a comment. In http://reviews.llvm.org/D19998#439698, @zturner wrote: > 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. http://reviews.llvm.org/D19998 __

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

zturner accepted this revision. zturner added a reviewer: zturner. zturner added a comment. This revision is now accepted and ready to land. No problems here, I'd wait for Pavel for additional thoughts before going in. http://reviews.llvm.org/D19998 ___

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

tfiala added a comment. Timing info for Ubuntu 15.10: | Description | Time (seconds)(mm:ss) | | Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, no gmodules | 377s (6:17) | | Ubuntu 15.10 6-core, 24 GB RAM, TOT clang, with gmodules | 517s (8:37

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

tfiala added a comment. Okay, thanks Zachary. http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

zturner added a comment. Going to try this out on Windows and see how it goes. http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

tfiala marked 15 inline comments as done. tfiala added a comment. Marked all code comments as done. http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

tfiala added a comment. I believe this now accounts for everything we discussed changing. http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

tfiala updated this revision to Diff 58459. tfiala added a comment. Remove the now-unneeded second check for Windows. http://reviews.llvm.org/D19998 Files: packages/Python/lldbsuite/support/gmodules.py packages/Python/lldbsuite/test/decorators.py packages/Python/lldbsuite/test/expression

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

tfiala updated this revision to Diff 58458. tfiala added a comment. Adjustments per review, removal of decorator, fixup of incorrect build method call in inline tests. http://reviews.llvm.org/D19998 Files: packages/Python/lldbsuite/support/gmodules.py packages/Python/lldbsuite/test/decorato

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

tfiala added a comment. 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). http://reviews.llvm.org/D19998 ___ lldb-commits mailing list ll

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

tfiala added a comment. 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. http://reviews.llvm.org/D1

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

labath added inline comments. 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."""

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

tfiala updated this revision to Diff 58449. tfiala added a comment. 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. http://reviews.llvm.org/D19998 Files: packages/Python/lldbsuite/support/gmodul

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

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

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

tfiala commandeered this revision. tfiala edited reviewers, added: aprantl; removed: tfiala. tfiala added a comment. Commandeering. Thanks, Adrian! http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://list

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

aprantl abandoned this revision. aprantl added a comment. Todd, I'm abandoning the review. This should allow you to claim it as your own so you can iterate quicker. Thanks! http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@l

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

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

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

labath added a comment. I think we should make one more iteration on this, as there is still some room for improvement. In http://reviews.llvm.org/D19998#438756, @zturner wrote: > Should this be disabled by default unless explicitly requested? Seems like > "run the entire test suite N times" s

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

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. On Tue, May 24, 2016 at 6:25 PM Adrian Prantl wrote: > aprantl added a comment. > > In http

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

zturner added a comment. 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. http://reviews.llvm.org/D19998 ___

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

aprantl added a comment. In http://reviews.llvm.org/D19998#438666, @tfiala wrote: > In http://reviews.llvm.org/D19998#438614, @zturner wrote: > > > 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

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

aprantl updated this revision to Diff 58369. aprantl added a comment. Update with Todds most recent infrastructure additions. http://reviews.llvm.org/D19998 Files: packages/Python/lldbsuite/support/gmodules.py packages/Python/lldbsuite/test/decorators.py packages/Python/lldbsuite/test/ex

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

tfiala added a comment. In http://reviews.llvm.org/D19998#438614, @zturner wrote: > In http://reviews.llvm.org/D19998#438586, @tfiala wrote: > > > F1982070: gmodules-test-support-v6.diff > > > > Updated gmodules.is_compiler_clang_with_gmodules() to guard on Wind

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

zturner added a comment. In http://reviews.llvm.org/D19998#438586, @tfiala wrote: > F1982070: gmodules-test-support-v6.diff > > Updated gmodules.is_compiler_clang_with_gmodules() to guard on Windows with: > > def _gmodules_supported_internal(): > compile

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

tfiala added a comment. F1982070: gmodules-test-support-v6.diff 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:

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

tfiala added a comment. In http://reviews.llvm.org/D19998#438529, @zturner wrote: > 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

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

zturner added a comment. 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. http://reviews.llvm.org/D19998 __

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

tfiala added a comment. F1981857: gmodules-test-support-v5.diff 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

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

tfiala added a comment. F1981749: gmodules-test-support-v4.diff Patch updated. TestDeadStrip.py required xfailing for Ubuntu with gmodules debug info. http://reviews.llvm.org/D19998 ___ lldb-commits mailing list

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

tfiala added a comment. F1981558: gmodules-test-support-v3.diff 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 th

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

tfiala added a comment. F1981148: gmodules-test-support-v2.diff 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,

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

tfiala added a comment. In http://reviews.llvm.org/D19998#437820, @zturner wrote: > 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 ge

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

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 On Tue, May 24, 2016 at 9:05 AM Todd Fiala via lldb-commits < lldb-commits@lists.llvm.org> wrote: > tfiala added a comment. > > So at this point, it l

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

tfiala added a comment. 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-cla

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

tfiala added a comment. Adrian, I'm going to take a look at this with you today. I'm reviewing all the comments on this so far. -Todd http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/

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

tfiala added a comment. Oh right, I recall there was an action item on me. I'll catch up with you on that in a bit. http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

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

aprantl added a comment. I still need some expertise on how to disable the configuration on platforms with older versions of clang (or different compilers like gcc). http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.ll

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

tfiala added a comment. Hi Adrian, I think this has stalled. What do we need to resolve now? http://reviews.llvm.org/D19998 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

labath added a comment. In http://reviews.llvm.org/D19998#423541, @aprantl wrote: > In http://reviews.llvm.org/D19998#423277, @labath wrote: > > > 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 adde

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

tfiala added a comment. > 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

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

aprantl added a comment. In http://reviews.llvm.org/D19998#423277, @labath wrote: > 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

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

labath added a subscriber: labath. labath added a comment. 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)

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

tfiala added a comment. 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 lar

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

aprantl created this revision. aprantl added reviewers: granata.enrico, tfiala. aprantl added a subscriber: lldb-commits. 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