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

[rupprecht]

Summary

This reverts commit 3bf96b0329be554c67282b0d7d8da6a864b9e38f. It causes crashes as reported in PR52257 and a few other places. A reproducer is bundled with this commit to verify any fix forward.

[rupprecht]

Given that this patch has been in tree for half a year, it'd be good to get confirmation here this can be reverted given there is now a test case for causing a crash. I got an offline comment that this is OK to revert, so if nobody has objections, I'll land sometime tomorrow.

[teemperor]

LGTM, thanks for this!

[teemperor]

lldb/test/Shell/SymbolFile/DWARF/PR52257.cpp

21 ↗

(On Diff #385670)

FWIW, I think probably should be an API test (for a bunch of reasons from not relying on formatting output to remote device testing), but given this is just a revert I won't insist on that. Thanks again!

[rupprecht]

lldb/test/Shell/SymbolFile/DWARF/PR52257.cpp

21 ↗

(On Diff #385670)

Makes sense -- this was just the easiest way to copy/paste the repro from the bug. I'll convert it to an API test when landing.

[rupprecht]

• Switch crash repro from shell -> api test

[teemperor]

I actually didn't see that the patch deleted the TestCppReferenceToOuterClass test. Could you just add a @skipIf # Crashes or so above its def test... method? The test itself is still valid user code that we shouldn't crash on.

I left some nits and the test source probably needs to be made a bit more expressive in terms of what's its trying to test, but this can all be done later. Let's just land this to get the regression fixed.

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

18

nit: self.createTestTarget() (which generates useful error messages on failure)

19

nit: self.expect_expr("b", result_type="B", result_children=[ValueCheck(name="tag_set_")])

[rupprecht]

• Use better test APIs, add test back w/ a @expectedFailure

[rupprecht]

In D113449#3121070, @teemperor wrote:

I actually didn't see that the patch deleted the TestCppReferenceToOuterClass test. Could you just add a @skipIf # Crashes or so above its def test... method? The test itself is still valid user code that we shouldn't crash on.

I assume you mean something like expectedFailure, as this should unconditionally fail.

I left some nits and the test source probably needs to be made a bit more expressive in terms of what's its trying to test, but this can all be done later. Let's just land this to get the regression fixed.

OK, retesting real quick against trunk and submitting now. Thanks!

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

for (clang::Decl *candidate : lr) {
         RegisterImportedDecl(From, candidate);
         m_decls_to_ignore.insert(candidate);
         return candidate;
       }
     }
     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);
 }
 
 void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
     clang::Decl *to, clang::Decl *from) {
   // We might have a forward declaration from a shared library that we
   // gave external lexical storage so that Clang asks us about the full
   // definition when it needs it. In this case the ASTImporter isn't aware

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

if (predicate(decl->getKind())) {
                    context_named_decl->getDeclKindName(),
                    context_named_decl->getName(), decl->getDeclKindName(),
                    ast_dump);
         else
           LLDB_LOG(log, "  FELD Adding lexical {0}Decl {1}",
                    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
       // correctly propagate that we added some Decls back to Clang.
       // By leaving 'decls' empty we incorrectly return false from
       // DeclContext::LoadLexicalDeclsFromExternalStorage which might cause
       // 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;
     }
   }
 
   // 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!

lldb/test/API/commands/expression/pr52257/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

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

+"""
+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

+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

 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__)
 
+    @expectedFailure("The fix for this was reverted due to llvm.org/PR52257")
     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())