[ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[shafik]

Summary

In ImportContext(…) we may call into CompleteDecl(…) which if FromRecrord is not defined will start the definition of a ToRecord but from what I can tell at least one of the paths though here don't ensure we complete the definition.
For a RecordDecl this can be problematic since this means we won’t import base classes and we won’t have any of the methods or types we inherit from these bases.

One such path is through VisitTypedefNameDecl(…) which is exercised by the reproducer.

For LLDB expression parsing this results in expressions failing but sometimes succeeding in subsequent attempts e.g.:

(lldb) p BB->dump()
error: no member named 'dump' in 'B'
(lldb) p BB->dump()
(lldb)

This happens because the first time through FromRecord is not defined but in subsequent attempts through it may be.

[shafik]

I am open to different approaches to this problem, this is opening attempt at fixing it but I may be missing other interactions.

AFAICT starting the definition of ToRecord the way CompleteDecl(…) does when we know FromRecord is not defined is just broken for the RecordDecl case if we have bases.

It took me a lot of time to come up with this reproducer from a real life issue debugging llc.

[teemperor]

From what I understand the whole idea here is to just ask the external AST source to complete the record before we import them? If yes, then this seems like the right idea to me.

Also this seems to be testable via a Clang unit test, so I think this patch should have one.

But otherwise this LGTM. Thanks for figuring this out!

clang/lib/AST/ASTImporter.cpp

8198

I'm not sure how it can be that ASTImporter::CompleteDecl starts but never finishes the decl as it does both things at once unconditionally?

TD->startDefinition();
TD->setCompleteDefinition(true);

FWIW, this whole comment could just be Ask the external source if this is actually a complete record that just hasn't been completed yet or something like this. I think if we reach this point then it makes a complete sense to ask the external source for more info. The explanation about the base classes seems to be just a smaller detail of the whole picture here.

8205

I assume we should check here that FromRecord is now a complete definition before trying to import it's definition?

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

26

You need to make this a "//%" as otherwise this test fails (which it does right now).

[shafik]

Addressing comments

[martong]

Looks okay to me (other than the redundant import code that I have a comment about).

Also this seems to be testable via a Clang unit test, so I think this patch should have one.

Yeah, would be nice to have a Clang unit test. I believe we have the infrastructure for that in place. E.g. in LLDBLookupTest we have an lldb specific setup, that could be a good starting point.

clang/lib/AST/ASTImporter.cpp

8198

Ask the external source if this is actually a complete record that just hasn't been completed yet

FWIW this seems to be a recurring pattern, so maybe it would be worth to do this not just with RecordDecls but with other kind of decls. E.g. an EnumDecl could have an external source and not completed, couldn't it?

8204

We could merge this if with the else if at line 8164, because their body is exactly the same.
But to do that, we should move the external storage query and type completion above the enclosing if (above line 8162 and just after line 8161).

[shafik]

• Added unit test
• Modified condition for defining FromRecord to whether it has an ExternalSource

[shafik]

clang/lib/AST/ASTImporter.cpp

8198

TD->setCompleteDefinition(true); just sets a flag but it does not import the bases, which it can not do since From is not defined. See ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind) which does this after it does To->startDefinition();. So we can't really consider the definition finished if we don't do this.

Do you have a better way of describing this situation?

I think it is important to describe why we don't use CompleteDecl since the other cases do.

8198

This is definitely more costly, for the EnumDecl case I don't see how we could get in a similar situation since we don't have inheritance. I looked over the EnumDecl members and I don't see anything that would require it be defined.

[martong]

Thanks for the test Shafik, that is pretty self explanatory!

clang/lib/AST/ASTImporter.cpp

8178

What if we did this a bit differently? We could simply complete the From type if it is not completed, before getting into ImportDefinition.

if (ToRecord->isCompleteDefinition())
  return ToDC;
auto *FromRecord = cast<RecordDecl>(FromDC);
if (FromRecord->hasExternalLexicalStorage() &&
          !FromRecord->isCompleteDefinition())
        FromRecord->getASTContext().getExternalSource()->CompleteType(
            FromRecord);

This way we could get rid of the redundant calls of ImportDefinition. As we have now a call for the case when we don't have external storage and for the case when we have.

[teemperor]

Beside Gabors comment I think I'm happy with this. Thanks!

clang/unittests/AST/ASTImporterTest.cpp

5948

"is completed" -> "it completed"

5963

You can remove this as you changed the check in the ASTImporter to only check for the existence of an ExternalASTSource.

[shafik]

• Simplifying the changes in ASTImporter::ImportContext
• Removing DC->setHasExternalLexicalStorage(true); from unit test since we are checking getExternalSource()

[martong]

LGTM! Thanks!

[a_sidorin]

Hello Shafik!
The patch looks good, I only have a few stylish nits.

clang/unittests/AST/ASTImporterTest.cpp

5973

Context (starting with capital).

5989

Err (starting with capital).

[shafik]

Capitalization fixes.

[abhinavgaba]

clang/unittests/AST/ASTImporterTest.cpp

5974

Overloading CompleteType(TagDecl*) is causing a warning:
virtual void clang::ExternalASTSource::CompleteType(clang::ObjCInterfaceDecl*)’ was hidden [-Woverloaded-virtual]

One way to get rid of it would be to add this here :

private:
  using ExternalASTSource::CompleteType;

clang/lib/AST/ASTImporter.cpp

Expected<DeclContext *> ASTImporter::ImportContext(DeclContext *FromDC) {
   ExpectedDecl ToDCOrErr = Import(cast<Decl>(FromDC));
   if (!ToDCOrErr)
     return ToDCOrErr.takeError();
   auto *ToDC = cast<DeclContext>(*ToDCOrErr);
 
   // When we're using a record/enum/Objective-C class/protocol as a context, we
   // need it to have a definition.
   if (auto *ToRecord = dyn_cast<RecordDecl>(ToDC)) {
     auto *FromRecord = cast<RecordDecl>(FromDC);
-    if (ToRecord->isCompleteDefinition()) {
-      // Do nothing.
-    } else if (FromRecord->isCompleteDefinition()) {
+    if (ToRecord->isCompleteDefinition())
+      return ToDC;
+
+    // If FromRecord is not defined we need to force it to be.
+    // Simply calling CompleteDecl(...) for a RecordDecl will break some cases
+    // it will start the definition but we never finish it.
+    // If there are base classes they won't be imported and we will
+    // be missing anything that we inherit from those bases.
+    if (FromRecord->getASTContext().getExternalSource() &&
+        !FromRecord->isCompleteDefinition())
+      FromRecord->getASTContext().getExternalSource()->CompleteType(FromRecord);
+
+    if (FromRecord->isCompleteDefinition())
       if (Error Err = ASTNodeImporter(*this).ImportDefinition(
           FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
         return std::move(Err);
-    } else {
-      CompleteDecl(ToRecord);
-    }
   } else if (auto *ToEnum = dyn_cast<EnumDecl>(ToDC)) {
     auto *FromEnum = cast<EnumDecl>(FromDC);
     if (ToEnum->isCompleteDefinition()) {
       // Do nothing.
     } else if (FromEnum->isCompleteDefinition()) {
       if (Error Err = ASTNodeImporter(*this).ImportDefinition(
           FromEnum, ToEnum, ASTNodeImporter::IDK_Basic))
         return std::move(Err);
     } else {
       CompleteDecl(ToEnum);
     }
   } else if (auto *ToClass = dyn_cast<ObjCInterfaceDecl>(ToDC)) {
     auto *FromClass = cast<ObjCInterfaceDecl>(FromDC);
     if (ToClass->getDefinition()) {
       // Do nothing.
     } else if (ObjCInterfaceDecl *FromDef = FromClass->getDefinition()) {
       if (Error Err = ASTNodeImporter(*this).ImportDefinition(
           FromDef, ToClass, ASTNodeImporter::IDK_Basic))
         return std::move(Err);

clang/unittests/AST/ASTImporterTest.cpp

auto ExtendWithOptions(const T &Values, const ArgVector &Args) {
   for (ArgVector &ArgV : Copy) {
     for (const std::string &Arg : Args) {
       ArgV.push_back(Arg);
     }
   }
   return ::testing::ValuesIn(Copy);
 }
 
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+  ImportWithExternalSource() {
+    Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+                 ASTContext &FromContext, FileManager &FromFileManager,
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
+      return new ASTImporter(ToContext, ToFileManager, FromContext,
+                             FromFileManager, MinimalImport,
+                             // We use the regular lookup.
+                             /*SharedState=*/nullptr);
+    };
+  }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+  std::vector<clang::TagDecl *> &CompletedTags;
+  SourceWithCompletedTagList(std::vector<clang::TagDecl *> &CompletedTags)
+      : CompletedTags(CompletedTags) {}
+  void CompleteType(TagDecl *Tag) override {
+    auto *Record = cast<CXXRecordDecl>(Tag);
+    Record->startDefinition();
+    Record->completeDefinition();
+    CompletedTags.push_back(Tag);
+  }
+  void
+  FindExternalLexicalDecls(const DeclContext *DC,
+                           llvm::function_ref<bool(Decl::Kind)> IsKindWeWant,
+                           SmallVectorImpl<Decl *> &Result) override {}
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+  // Create an empty TU.
+  TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+  // Create and add the test ExternalASTSource.
+  std::vector<clang::TagDecl *> CompletedTags;
+  IntrusiveRefCntPtr<ExternalASTSource> source =
+      new SourceWithCompletedTagList(CompletedTags);
+  clang::ASTContext &Context = FromTU->getASTContext();
+  Context.setExternalSource(std::move(source));
+
+  // Create a dummy class by hand with external lexical storage.
+  IdentifierInfo &Ident = Context.Idents.get("test_class");
+  auto *Record = CXXRecordDecl::Create(
+      Context, TTK_Class, FromTU, SourceLocation(), SourceLocation(), &Ident);
+  Record->setHasExternalLexicalStorage();
+  FromTU->addDecl(Record);
+
+  // Do a minimal import of the created class.
+  EXPECT_EQ(0U, CompletedTags.size());
+  Import(Record, Lang_CXX);
+  EXPECT_EQ(0U, CompletedTags.size());
+
+  // Import the definition of the created class.
+  llvm::Error Err = findFromTU(Record)->Importer->ImportDefinition(Record);
+  EXPECT_FALSE((bool)Err);
+  consumeError(std::move(Err));
+
+  // Make sure the class was completed once.
+  EXPECT_EQ(1U, CompletedTags.size());
+  EXPECT_EQ(Record, CompletedTags.front());
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportSourceLocations,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportWithExternalSource,
+                        DefaultTestValuesForRunOptions, );
+
 } // end namespace ast_matchers
 } // end namespace clang

lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())

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

+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+    return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}