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; } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
