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

This reverts commit 3bf96b0329.

It causes crashes as reported in PR52257 and a few other places. A reproducer is bundled with this commit to verify any fix forward. The original test is left in place, but marked XFAIL as it now produces the wrong result.

Reviewed By: teemperor

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

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

@@ -888,37 +888,6 @@ 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,7 +479,10 @@ void ClangASTSource::FindExternalLexicalDecls(
                    decl->getDeclKindName(), ast_dump);
       }
 
-      CopyDecl(decl);
+      Decl *copied_decl = CopyDecl(decl);
+
+      if (!copied_decl)
+        continue;
 
       // 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
@@ -489,6 +492,12 @@ 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/commands/expression/pr52257/Makefile

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

lldb/test/API/commands/expression/pr52257/TestExprCrash.py

@@ -0,0 +1,18 @@
+"""
+Verify that LLDB doesn't crash during expression evaluation.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ExprCrashTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test_pr52257(self):
+        self.build()
+        self.createTestTarget()
+        self.expect_expr("b", result_type="B", result_children=[ValueCheck(name="tag_set_")])

lldb/test/API/commands/expression/pr52257/main.cpp

@@ -0,0 +1,12 @@
+template <typename> struct pair {};
+struct A {
+  using iterator = pair<char *>;
+  pair<char *> a_[];
+};
+struct B {
+  using iterator = A::iterator;
+  iterator begin();
+  A *tag_set_;
+};
+B b;
+int main() {};

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

@@ -7,6 +7,7 @@ class TestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
+    @expectedFailure("The fix for this was reverted due to llvm.org/PR52257")
     def test(self):
         self.build()
         self.dbg.CreateTarget(self.getBuildArtifact("a.out"))