[Lldb-commits] [lldb] r372549 - [lldb] Fix that importing decls in a TagDecl end up in wrong declaration context (partly reverts D61333)
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Mon Sep 23 00:27:14 PDT 2019
Author: teemperor
Date: Mon Sep 23 00:27:14 2019
New Revision: 372549
URL: http://llvm.org/viewvc/llvm-project?rev=372549&view=rev
Log:
[lldb] Fix that importing decls in a TagDecl end up in wrong declaration context (partly reverts D61333)
Summary:
In D61333 we dropped some code from ClangASTSource that checks if imported declarations
ended up in the right DeclContext. While this code wasn't tested by the test suite (or better, it was hit
by the test suite but we didn't have any checks that were affected) and the code seems pointless
(as usually Decls should end up in the right DeclContext), it actually broke the data formatters in LLDB
and causes a bunch of obscure bugs where structs suddenly miss all their members. The first report we got about
this was that printing a std::map doesn't work anymore when simply doing "expr m" (m is the std::map).
This patch reverts D61333 partly and reintroduces the check in a more stricter way (we actually check now that
we *move* the Decl and it is in a single DeclContext). This should fix all the problems we currently have until
we figure out how to properly fix the underlying issues. I changed the order of some std::map formatter tests
which is currently the most reliable way to test this problem (it's a tricky setup, see description below).
Fixes rdar://55502701 and rdar://55129537
--------------------------------------
Some more explanation what is actually going on and what is going wrong:
The situation we have is that if we have a `std::map m` and do a `expr m`, we end up seeing an empty map
(even if `m` has elements). The reason for this is that our data formatter sees that std::pair<int, int> has no
members. However, `frame var m` works just fine (and fixes all following `expr m` calls).
The reason for why `expr` breaks std::map is that we actually copy the std::map nodes in two steps in the
three ASTContexts that are involved: The debug information ASTContext (D-AST), the expression ASTContext
we created for the current expression (E-AST) and the persistent ASTContext we use for our $variables (P-AST).
When doing `expr m` we do a minimal import of `std::map` from D-AST to E-AST just do the type checking/codegen.
This copies std::map itself and does a minimal.import of `std::pair<int, int>` (that is, we don't actually import
the `first` and `second` members as we don't need them for anything). After the expression is done, we take
the expression result and copy it from E-AST to P-AST. This imports the E-AST's `std::pair` into P-AST which still
has no `first` and `second` as they are still undeserialized. Once we are in P-AST, the data formatter tries to
inspect `std::map` (and also `std::pair` as that's what the elements are) and it asks for the `std::pair` members.
We see that `std::pair` has undeserialized members and go to the ExternalASTSource to ask for them. However,
P-ASTs ExternalASTSource points to D-AST (and not E-AST, which `std::pair` came from). It can't point to E-AST
as that is only temporary and already gone (and also doesn't actually contain all decls we have in P-AST).
So we go to D-AST to get the `std::pair` members. The ASTImporter is asked to copy over `std::pair` members
and first checks if `std::pair` is already in P-AST. However, it only finds the std::pair we got from E-AST, so it
can't use it's map of already imported declarations and does a comparison between the `std::pair` decls we have
Because the ASTImporter thinks they are different declarations, it creates a second `std::pair` and fills in the
members `first` and `second` into the second `std::pair`. However, the data formatter is looking at the first
`std::pair` which still has no members as they are in the other decl. Now we pretend we have no declarations
and just print an empty map as a fallback.
The hack we had before fixed this issue by moving `first` and `second` to the first declaration which makes
the formatters happy as they can now see the members in the DeclContext they are querying.
Obviously this is a temporary patch until we get a real fix but I'm not sure what's the best way to fix this.
Implementing that the ClassTemplateSpecializationDecl actually understands that the two std::pair's are the same
decl fixes the issue, but this doesn't fix the bug for all declarations. My preferred solution would be to
complete all declarations in E-AST before they get moved to P-AST (as we anyway have to do this from what I can
tell), but that might have unintended side-effects and not sure what's the best way to implement this.
Reviewers: friss, martong
Reviewed By: martong
Subscribers: aprantl, rnkovacs, christof, abidh, JDevlieghere, lldb-commits, shafik
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D67803
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
Modified: lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py?rev=372549&r1=372548&r2=372549&view=diff
==============================================================================
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py Mon Sep 23 00:27:14 2019
@@ -52,13 +52,26 @@ class LibcxxMapDataFormatterTestCase(Tes
self.addTearDownHook(cleanup)
ns = self.namespace
- self.expect('frame variable ii',
+ self.expect('p ii',
+ substrs=['%s::map' % ns,
+ 'size=0',
+ '{}'])
+ self.expect('frame var ii',
substrs=['%s::map' % ns,
'size=0',
'{}'])
lldbutil.continue_to_breakpoint(self.process(), bkpt)
+ self.expect('p ii',
+ substrs=['%s::map' % ns, 'size=2',
+ '[0] = ',
+ 'first = 0',
+ 'second = 0',
+ '[1] = ',
+ 'first = 1',
+ 'second = 1'])
+
self.expect('frame variable ii',
substrs=['%s::map' % ns, 'size=2',
'[0] = ',
@@ -81,7 +94,7 @@ class LibcxxMapDataFormatterTestCase(Tes
lldbutil.continue_to_breakpoint(self.process(), bkpt)
- self.expect("frame variable ii",
+ self.expect("p ii",
substrs=['%s::map' % ns, 'size=8',
'[5] = ',
'first = 5',
@@ -90,7 +103,7 @@ class LibcxxMapDataFormatterTestCase(Tes
'first = 7',
'second = 1'])
- self.expect("p ii",
+ self.expect("frame var ii",
substrs=['%s::map' % ns, 'size=8',
'[5] = ',
'first = 5',
Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp?rev=372549&r1=372548&r2=372549&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp (original)
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp Mon Sep 23 00:27:14 2019
@@ -650,6 +650,20 @@ void ClangASTSource::FindExternalLexical
m_ast_importer_sp->RequireCompleteType(copied_field_type);
}
+ auto decl_context_non_const = const_cast<DeclContext *>(decl_context);
+
+ // The decl ended up in the wrong DeclContext. Let's fix that so
+ // the decl we copied will actually be found.
+ // FIXME: This is a horrible hack that shouldn't be necessary. However
+ // it seems our current setup sometimes fails to copy decls to the right
+ // place. See rdar://55129537.
+ if (copied_decl->getDeclContext() != decl_context) {
+ assert(copied_decl->getDeclContext()->containsDecl(copied_decl));
+ copied_decl->getDeclContext()->removeDecl(copied_decl);
+ copied_decl->setDeclContext(decl_context_non_const);
+ assert(!decl_context_non_const->containsDecl(copied_decl));
+ decl_context_non_const->addDeclInternal(copied_decl);
+ }
} else {
SkippedDecls = true;
}
More information about the lldb-commits
mailing list