This is an archive of the discontinued LLVM Phabricator instance.

Skip TestClassTemplateParameterPack.py on all platforms
ClosedPublic

Authored by shafik on Apr 29 2019, 9:26 AM.

Details

Summary

The TestClassTemplateParameterPack.py test does not work for the right reasons. The expressions such as:

expression -- C<int, 16>().isSixteenThirtyTwo()

work only because we are currently pulling all the local variables e.g.:

using $__lldb_local_vars::argc;
using $__lldb_local_vars::argv;
using $__lldb_local_vars::myC;
using $__lldb_local_vars::myLesserC;
using $__lldb_local_vars::myD;
using $__lldb_local_vars::myLesserD;

regardless if we use them in the expression and this causes us to for example to pull C<int,16> into the evaluation context but this is not how it should work. Once we land: https://reviews.llvm.org/D46551

This will no longer work. When clang does the call back it is looking for C but currently the debug information contains C<int,16> etc... So a long-term fix for this would require at least reworking ho that debug information is generated.

Diff Detail

Event Timeline

shafik created this revision.Apr 29 2019, 9:26 AM
teemperor accepted this revision.Apr 29 2019, 10:11 AM

So a long-term fix for this would require at least reworking ho that debug information is generated.

I believe the debug information correct but our lookup is just not finding/loading the necessary AST nodes?

Otherwise I assume the radar is keeping track of the fact that this feature is broken, so LGTM so that we unblock the other reviews.

packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-pack/TestClassTemplateParameterPack.py
7

Nit pick: Isn't there a skip or so that's more expressive than skipIf?

This revision is now accepted and ready to land.Apr 29 2019, 10:11 AM
friss requested changes to this revision.Apr 29 2019, 12:42 PM

We shouldn't skip the whole test, just the expressions that worked for bad reasons. Calling functions on local variables should work, it's just creating the templated objects that is completely broken.

This revision now requires changes to proceed.Apr 29 2019, 12:42 PM
shafik updated this revision to Diff 197178.Apr 29 2019, 1:58 PM

Fred is correct, I mistakenly thought the parts of the test that were working were being covered elsewhere but that is not the case. So I have reworked this change to instead of skipping the whole test to comment out the inline expressions that are specifically broken.

@friss updated the change to only effect those specifically broken.

friss added inline comments.Apr 29 2019, 2:35 PM
packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-pack/main.cpp
48–50

This is a little vague. Can you replace it with something like: "Disabling until we do template lookup correctly: http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180507/040689.html"

With this, this LGTM

shafik updated this revision to Diff 197360.Apr 30 2019, 9:47 AM

Updated comment to be more precise.

@friss makes sense, updated comment.

friss accepted this revision.Apr 30 2019, 9:50 AM

LGTM if no one else objects.

This revision is now accepted and ready to land.Apr 30 2019, 9:50 AM
teemperor closed this revision.May 2 2019, 3:27 AM

This has been merged as rLLDB359699 but somehow Phabriactor hasn't figured out yet that this needs to be closed. Closing this manually.