HomePhabricator

[lldb] Fix that importing decls in a TagDecl end up in wrong declaration…

Description

[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