[lldb] Disable minimal import mode for RecordDecls that back FieldDecls

Clang adds a Decl in two phases to a DeclContext. First it adds it invisible and
then it makes it visible (which will add it to the lookup data structures). It's
important that we can't do lookups into the DeclContext we are currently adding
the Decl to during this process as once the Decl has been added, any lookup will
automatically build a new lookup map and add the added Decl to it. The second
step would then add the Decl a second time to the lookup which will lead to
weird errors later one. I made adding a Decl twice to a lookup an assertion
error in D84827.

In the first step Clang also does some computations on the added Decl if it's
for example a FieldDecl that is added to a RecordDecl.

One of these computations is checking if the FieldDecl is of a record type
and the record type has a deleted constexpr destructor which will delete
the constexpr destructor of the record that got the FieldDecl.

This can lead to a bug with the way we implement MinimalImport in LLDB
and the following code:

```
struct Outer {
  typedef int HookToOuter;
  struct NestedClass {
    HookToOuter RefToOuter;
  } NestedClassMember; // We are adding this.
};
```

1. We just imported `Outer` minimally so far.
2. We are now asked to add `NestedClassMember` as a FieldDecl.
3. We import `NestedClass` minimally.
4. We add `NestedClassMember` and clang does a lookup for a constexpr dtor in
   `NestedClass`. `NestedClassMember` hasn't been added to the lookup.
5. The lookup into `NestedClass` will now load the members of `NestedClass`.
6. We try to import the type of `RefToOuter` which will try to import the `HookToOuter` typedef.
7. We import the typedef and while importing we check for conflicts in `Outer` via a lookup.
8. The lookup into `Outer` will cause the invisible `NestedClassMember` to be added to the lookup.
9. We continue normally until we get back to the `addDecl` call in step 2.
10. We now add `NestedClassMember` to the lookup even though we already did that in step 8.

The fix here is disabling the minimal import for RecordTypes from FieldDecls. We
actually already did this, but so far we only force the definition of the type
to be imported *after* we imported the FieldDecl. This just moves that code
*before* we import the FieldDecl so prevent the issue above.

Reviewed By: shafik, aprantl

Differential Revision: https://reviews.llvm.org/D102993

Author: Teemperor

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

@@ -888,6 +888,37 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
     LLDB_LOG(log, "[ClangASTImporter] Complete definition not found");
   }
 
+  // Disable the minimal import for fields that have record types. There is
+  // no point in minimally importing the record behind their type as Clang
+  // will anyway request their definition when the FieldDecl is added to the
+  // RecordDecl (as Clang will query the FieldDecl's type for things such
+  // as a deleted constexpr destructor).
+  // By importing the type ahead of time we avoid some corner cases where
+  // the FieldDecl's record is importing in the middle of Clang's
+  // `DeclContext::addDecl` logic.
+  if (clang::FieldDecl *fd = dyn_cast<FieldDecl>(From)) {
+    // This is only necessary because we do the 'minimal import'. Remove this
+    // once LLDB stopped using that mode.
+    assert(isMinimalImport() && "Only necessary for minimal import");
+    QualType field_type = fd->getType();
+    if (field_type->isRecordType()) {
+      // First get the underlying record and minimally import it.
+      clang::TagDecl *record_decl = field_type->getAsTagDecl();
+      llvm::Expected<Decl *> imported = Import(record_decl);
+      if (!imported)
+        return imported.takeError();
+      // Check how/if the import got redirected to a different AST. Now
+      // import the definition of what was actually imported. If there is no
+      // origin then that means the record was imported by just picking a
+      // compatible type in the target AST (in which case there is no more
+      // importing to do).
+      if (clang::Decl *origin = m_master.GetDeclOrigin(*imported).decl) {
+        if (llvm::Error def_err = ImportDefinition(record_decl))
+          return std::move(def_err);
+      }
+    }
+  }
+
   return ASTImporter::ImportImpl(From);
 }
 

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

@@ -479,10 +479,7 @@ void ClangASTSource::FindExternalLexicalDecls(
                    decl->getDeclKindName(), ast_dump);
       }
 
-      Decl *copied_decl = CopyDecl(decl);
-
-      if (!copied_decl)
-        continue;
+      CopyDecl(decl);
 
       // FIXME: We should add the copied decl to the 'decls' list. This would
       // add the copied Decl into the DeclContext and make sure that we
@@ -492,12 +489,6 @@ void ClangASTSource::FindExternalLexicalDecls(
       // lookup issues later on.
       // We can't just add them for now as the ASTImporter already added the
       // decl into the DeclContext and this would add it twice.
-
-      if (FieldDecl *copied_field = dyn_cast<FieldDecl>(copied_decl)) {
-        QualType copied_field_type = copied_field->getType();
-
-        m_ast_importer_sp->RequireCompleteType(copied_field_type);
-      }
     } else {
       SkippedDecls = true;
     }

lldb/test/API/lang/cpp/reference-to-outer-type/Makefile

@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py

@@ -0,0 +1,16 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+        test_var = self.expect_expr("test_var", result_type="In")
+        nested_member = test_var.GetChildMemberWithName('NestedClassMember')
+        self.assertEqual("Outer::NestedClass",
+                         nested_member.GetType().GetName())

lldb/test/API/lang/cpp/reference-to-outer-type/main.cpp

@@ -0,0 +1,23 @@
+struct Outer {
+  typedef int HookToOuter;
+  // When importing this type, we have to import all of it before adding it
+  // via the FieldDecl to 'Outer'. If we don't do this, then Clang will
+  // while adding 'NestedClassMember' ask for the full type of 'NestedClass'
+  // which then will start pulling in the 'RefToOuter' member. That member
+  // will import the typedef above and add it to 'Outer'. And adding a
+  // Decl to a DeclContext that is currently already in the process of adding
+  // another Decl will cause an inconsistent lookup.
+  struct NestedClass {
+    HookToOuter RefToOuter;
+    int SomeMember;
+  } NestedClassMember;
+};
+
+// We query the members of base classes of a type by doing a lookup via Clang.
+// As this tests is trying to find a borked lookup, we need a base class here
+// to make our 'GetChildMemberWithName' use the Clang lookup.
+struct In : Outer {};
+
+In test_var;
+
+int main() { return test_var.NestedClassMember.SomeMember; }