Page MenuHomePhabricator

Fix evaluation of nested classes with parent from other CU
ClosedPublic

Authored by jarin on Oct 1 2019, 8:37 AM.

Details

Summary

This makes sure that we associate DIEs that are imported from other CUs with the appropriate decl context.

Without this fix, nested classes can be dumped directly into their CU context if their parent was imported from another CU.

Diff Detail

Repository
rL LLVM

Event Timeline

jarin created this revision.Oct 1 2019, 8:37 AM
jarin added a reviewer: labath.Oct 1 2019, 8:38 AM
labath accepted this revision.Oct 1 2019, 10:02 AM
labath added reviewers: clayborg, JDevlieghere.

Just a bit more context (we were discussing this with Jaroslav offline): without this line, the GetClangDeclContextForDie method will fail to locate the clang decl context for the OuterA::Inner class because the class OuterA has been parsed and linked to the DIE in other.cpp's debug info when the first expression was evaluated. I've looked through the code and this fix makes sense to me, but I am not super-familiar with this, so I'd appreciate if someone could have another look.

packages/Python/lldbsuite/test/lang/cpp/nested-class-other-compilation-unit/Makefile
1–2 ↗(On Diff #222630)

This level stuff is no longer necessary.

This revision is now accepted and ready to land.Oct 1 2019, 10:02 AM
teemperor requested changes to this revision.Oct 1 2019, 10:26 AM

A few short comments what the role of the different classes/members play in the test case would be helpful. E.g. "This member/variable/expressions triggers the loading of Decl Foo in that CU". I'll take a closer look tomorrow, but on the first looks this patch LGTM. Thanks!

packages/Python/lldbsuite/test/lang/cpp/nested-class-other-compilation-unit/TestNestedClassWithParentInAnotherCU.py
2 ↗(On Diff #222630)

That comment is still from the original test case.

packages/Python/lldbsuite/test/lang/cpp/nested-class-other-compilation-unit/other.cpp
9 ↗(On Diff #222630)

nit pick: there are two spaces between return and new (we sadly don't use clang-format for tests)

This revision now requires changes to proceed.Oct 1 2019, 10:26 AM

The fix makes sense to me, I wonder if we have a similar issue w/ namespaces or perhaps they are handled differently.

jarin updated this revision to Diff 222756.Oct 1 2019, 10:51 PM

I have added some comments to the test (I hope it is not too overboard), removed the LEVEL stuff from the Makefile and fixed the formatting.

jarin updated this revision to Diff 222757.Oct 1 2019, 10:54 PM

Fixed a typo.

Just a few nitpicks about some minor typos, otherwise this LGTM. Thanks for the patch! I assume you need someone to commit this for you?

packages/Python/lldbsuite/test/lang/cpp/nested-class-other-compilation-unit/TestNestedClassWithParentInAnotherCU.py
2 ↗(On Diff #222757)

missing 'the' between 'that' and 'expression evaluator'. Also a missing 'a' between 'of' and 'nested'.

packages/Python/lldbsuite/test/lang/cpp/nested-class-other-compilation-unit/main.cpp
11 ↗(On Diff #222757)

Nit pick: 'foward' -> 'forward'.

teemperor accepted this revision.Oct 2 2019, 3:02 AM
This revision is now accepted and ready to land.Oct 2 2019, 3:02 AM
jarin updated this revision to Diff 222803.Oct 2 2019, 5:05 AM
jarin marked 5 inline comments as done.

Fixed the nits, thanks for the careful review!

I will indeed need someone to submit this for me. Thanks in advance :-)

jarin added inline comments.Oct 2 2019, 5:07 AM
packages/Python/lldbsuite/test/lang/cpp/nested-class-other-compilation-unit/TestNestedClassWithParentInAnotherCU.py
2 ↗(On Diff #222757)

Fixing "the expression evaluator"; "of a nested classes" does not sound right, though.

teemperor added inline comments.Oct 2 2019, 5:17 AM
packages/Python/lldbsuite/test/lang/cpp/nested-class-other-compilation-unit/TestNestedClassWithParentInAnotherCU.py
2 ↗(On Diff #222757)

Whoops, right, my bad.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 6:44 AM