[lldb] Fix that importing decls in a TagDecl end up in wrong declaration context (partly reverts D61333)

[teemperor]

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.

[teemperor]

I plan to match this soon-ish as this is essentially just a revert.

[martong]

Thanks for the thorough explanation about the different ASTContexts in LLDB.
The hack is indeed hideous, but seems good to me... for now until the real solution is born.

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

Note that LLDB uses the lenient ODR violation handling strategy (ASTImporter::ODRHandlingType::Liberal).
With the other, stricter ODR violation handling strategy, when the to be imported std::pair turns out to be nonequivalent with the existing one we would get an error instead of a new decl.
Would it make sense to change the odr handling strategy before the formatter turns to the ExternalASTSource?
It would not solve this particular issue, but perhaps could make discovering bugs like this easier.

My preferred solution would be to complete all declarations in E-AST before they get moved to P-AST

That sounds reasonable to me.

When doing expr m we do a minimal import of std::map from D-AST to E-AST just do the type checking/codegen.

There are some cases, when codegen do need the completed tagdecl, not just the skeleton we get via minimal import. So, there must be cases when in the E-AST we have a full, completed type, right? What would happen if we imported everything as a full declaration in the first place, instead of using the minimal import? Could that work?
Perhaps it is even more intrusive then your preferred solution, but could greatly reduce the import related code and complexity in LLDB.

[martong]

BTW, I tried to access the bug reports rdar://55502701 and rdar://55129537, but could not.
I tried at openradar, but there the search for the ID was not successful.
I wonder if there is a publicly accessible (for non Apple employees) URL for these bugs?

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py

def test_with_run_command(self):
             self.runCmd(
                 "settings set target.max-children-count 256",
                 check=False)
 
         # Execute the cleanup function during test case tear down.
         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] = ',
                              'first = 0',
                              'second = 0',
                              '[1] = ',
                              'first = 1',
                              'second = 1'])
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
 
         self.expect('frame variable ii',
                     substrs=['%s::map' % ns, 'size=4',
                              '[2] = ',
                              'first = 2',
                              'second = 0',
                              '[3] = ',
                              'first = 3',
                              'second = 1'])
 
         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',
                              'second = 0',
                              '[7] = ',
                              'first = 7',
                              'second = 1'])
 
-        self.expect("p ii",
+        self.expect("frame var ii",
                     substrs=['%s::map' % ns, 'size=8',
                              '[5] = ',
                              'first = 5',
                              'second = 0',
                              '[7] = ',
                              'first = 7',
                              'second = 1'])
 

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

if (predicate(decl->getKind())) {
       if (!copied_decl)
         continue;
 
       if (FieldDecl *copied_field = dyn_cast<FieldDecl>(copied_decl)) {
         QualType copied_field_type = copied_field->getType();
 
         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;
     }
   }
 
   // CopyDecl may build a lookup table which may set up ExternalLexicalStorage
   // to false.  However, since we skipped some of the external Decls we must
   // set it back!