[ASTImporter] Improve handling of incomplete types

[spyffe]

Summary

ASTImporter has some bugs when it's importing types that themselves come from an ExternalASTSource. This is exposed particularly in the behavior when comparing complete TagDecls with forward declarations. This patch does several things:

• Adds a test case making sure that conflicting forward-declarations are resolved correctly;
• Extends the clang-import-test harness to test two-level importing, so that we make sure we complete types when necessary; and
• Fixes a few bugs I found this way. Failure to complete types was one; however, I also discovered that complete RecordDecls aren't properly added to the redecls chain for existing forward declarations.

[spyffe]

Eliminated a missing newline at end of file error

[a.sidorin]

Looks good!

lib/AST/ASTImporter.cpp

1757

By the way, should we do the same for some other kinds of Decls (like FunctionDecl)? If so, could you write a NOTE comment?

[spyffe]

• Added a FIXME per Aleksei Sidorin's suggestion.

[spyffe]

lib/AST/ASTImporter.cpp

1757

I added this as a FIXME, in the style of the other notes about incomplete implementation in this file.

[bruno]

Hi Sean,

LGTM! One comment below.

tools/clang-import-test/clang-import-test.cpp

263

No need for the curly braces here

[spyffe]

tools/clang-import-test/clang-import-test.cpp

263

Okay.

[xazax.hun]

Is this blocked on something?

[szepet]

This patch was commited in r302975. (https://reviews.llvm.org/rL302975)

include/clang/AST/ExternalASTMerger.h

public:
 
   bool FindExternalVisibleDeclsByName(const DeclContext *DC,
                                       DeclarationName Name) override;
 
   void
   FindExternalLexicalDecls(const DeclContext *DC,
                            llvm::function_ref<bool(Decl::Kind)> IsKindWeWant,
                            SmallVectorImpl<Decl *> &Result) override;
+
+   void CompleteType(TagDecl *Tag) override;
 };
 
 } // end namespace clang
 
 #endif

lib/AST/ASTImporter.cpp

   if (!SearchName && D->getTypedefNameForAnonDecl()) {
     SearchName = Importer.Import(D->getTypedefNameForAnonDecl()->getDeclName());
     IDNS = Decl::IDNS_Ordinary;
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
     IDNS |= Decl::IDNS_Ordinary;
 
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *AdoptDecl = nullptr;
+  RecordDecl *PrevDecl = nullptr;
   if (!DC->isFunctionOrMethod()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     SmallVector<NamedDecl *, 2> FoundDecls;
     DC->getRedeclContext()->localUncachedLookup(SearchName, FoundDecls);
+
+    if (!FoundDecls.empty()) {
+      // We're going to have to compare D against potentially conflicting Decls, so complete it.
+      if (D->hasExternalLexicalStorage() && !D->isCompleteDefinition())
+        D->getASTContext().getExternalSource()->CompleteType(D);
+    }
+
     for (unsigned I = 0, N = FoundDecls.size(); I != N; ++I) {
       if (!FoundDecls[I]->isInIdentifierNamespace(IDNS))
         continue;
       
       Decl *Found = FoundDecls[I];
       if (TypedefNameDecl *Typedef = dyn_cast<TypedefNameDecl>(Found)) {
         if (const TagType *Tag = Typedef->getUnderlyingType()->getAs<TagType>())
           Found = Tag->getDecl();

             if (Optional<unsigned> Index2 = StructuralEquivalenceContext::
                     findUntaggedStructOrUnionIndex(FoundRecord)) {
               if (*Index1 != *Index2)
                 continue;
             }
           }
         }
 
+        PrevDecl = FoundRecord;
+
         if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
           if ((SearchName && !D->isCompleteDefinition())
               || (D->isCompleteDefinition() &&
                   D->isAnonymousStructOrUnion()
                     == FoundDef->isAnonymousStructOrUnion() &&
                   IsStructuralMatch(D, FoundDef))) {
             // The record types structurally match, or the "from" translation
             // unit only had a forward declaration anyway; call it the same

                               DC, StartLoc, Loc, Name.getAsIdentifierInfo());
     }
     
     D2->setQualifierInfo(Importer.Import(D->getQualifierLoc()));
     D2->setLexicalDeclContext(LexicalDC);
     LexicalDC->addDeclInternal(D2);
     if (D->isAnonymousStructOrUnion())
       D2->setAnonymousStructOrUnion(true);
+    if (PrevDecl) {
+      // FIXME: do this for all Redeclarables, not just RecordDecls.
+      D2->setPreviousDecl(PrevDecl);
+    }
   }
   
   Importer.Imported(D, D2);
 
   if (D->isCompleteDefinition() && ImportDefinition(D, D2, IDK_Default))
     return nullptr;
 
   return D2;

lib/AST/ASTStructuralEquivalence.cpp

static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   // incomplete, we assume that they are equivalent.
   D1 = D1->getDefinition();
   D2 = D2->getDefinition();
   if (!D1 || !D2)
     return true;
 
   if (CXXRecordDecl *D1CXX = dyn_cast<CXXRecordDecl>(D1)) {
     if (CXXRecordDecl *D2CXX = dyn_cast<CXXRecordDecl>(D2)) {
+      if (D1CXX->hasExternalLexicalStorage() &&
+          !D1CXX->isCompleteDefinition()) {
+        D1CXX->getASTContext().getExternalSource()->CompleteType(D1CXX);
+      }
+
       if (D1CXX->getNumBases() != D2CXX->getNumBases()) {
         if (Context.Complain) {
           Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
               << Context.ToCtx.getTypeDeclType(D2);
           Context.Diag2(D2->getLocation(), diag::note_odr_number_of_bases)
               << D2CXX->getNumBases();
           Context.Diag1(D1->getLocation(), diag::note_odr_number_of_bases)
               << D1CXX->getNumBases();

lib/AST/ExternalASTMerger.cpp

ForEachMatchingDC(
             Decl *ImportedDecl =
                 IP.Forward->Import(const_cast<Decl *>(SourceDecl));
             assert(ImportedDecl->getDeclContext() == DC);
             (void)ImportedDecl;
           }
         }
       });
 }
+
+void ExternalASTMerger::CompleteType(TagDecl *Tag) {
+  SmallVector<Decl *, 0> Result;
+  FindExternalLexicalDecls(Tag, [](Decl::Kind) { return true; }, Result);
+  Tag->setHasExternalLexicalStorage(false);
+}

test/Import/conflicting-struct/Inputs/S1.cpp

+class T;
+
+class S {
+  T *t;
+  int a;
+};

test/Import/conflicting-struct/Inputs/S2.cpp

+class U {
+  int b;
+};
+
+class T {
+  U u;
+};

test/Import/conflicting-struct/test.cpp

+// RUN: clang-import-test --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s
+void expr() {
+  S MyS;
+  T MyT;
+  MyS.a = 3;
+  MyT.u.b = 2;
+}

tools/clang-import-test/clang-import-test.cpp

 static llvm::cl::opt<std::string> Expression(
     "expression", llvm::cl::Required,
     llvm::cl::desc("Path to a file containing the expression to parse"));
 
 static llvm::cl::list<std::string>
     Imports("import", llvm::cl::ZeroOrMore,
             llvm::cl::desc("Path to a file containing declarations to import"));
 
+static llvm::cl::opt<bool>
+    Direct("direct", llvm::cl::Optional,
+             llvm::cl::desc("Use the parsed declarations without indirection"));
+
 static llvm::cl::list<std::string>
     ClangArgs("Xcc", llvm::cl::ZeroOrMore,
               llvm::cl::desc("Argument to pass to the CompilerInvocation"),
               llvm::cl::CommaSeparated);
 
 namespace init_convenience {
 class TestDiagnosticConsumer : public DiagnosticConsumer {
 private:
BuildCompilerInstance(ArrayRef<const char *> ClangArgv) {
   Ins->getTarget().adjust(Ins->getLangOpts());
   Ins->createFileManager();
   Ins->createSourceManager(Ins->getFileManager());
   Ins->createPreprocessor(TU_Complete);
 
   return Ins;
 }
 
+std::unique_ptr<CompilerInstance>
+BuildCompilerInstance(ArrayRef<std::string> ClangArgs) {
+  std::vector<const char *> ClangArgv(ClangArgs.size());
+  std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+                 [](const std::string &s) -> const char * { return s.data(); });
+  return init_convenience::BuildCompilerInstance(ClangArgv);
+}
+
 std::unique_ptr<ASTContext>
 BuildASTContext(CompilerInstance &CI, SelectorTable &ST, Builtin::Context &BC) {
   auto AST = llvm::make_unique<ASTContext>(
       CI.getLangOpts(), CI.getSourceManager(),
       CI.getPreprocessor().getIdentifierTable(), ST, BC);
   AST->InitBuiltinTypes(CI.getTarget());
   return AST;
 }
void AddExternalSource(
   for (const std::unique_ptr<CompilerInstance> &CI : Imports) {
     Sources.push_back({CI->getASTContext(), CI->getFileManager()});
   }
   auto ES = llvm::make_unique<ExternalASTMerger>(Target, Sources);
   CI.getASTContext().setExternalSource(ES.release());
   CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage();
 }
 
+std::unique_ptr<CompilerInstance> BuildIndirect(std::unique_ptr<CompilerInstance> &CI) {
+  std::vector<const char *> ClangArgv(ClangArgs.size());
+  std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+                 [](const std::string &s) -> const char * { return s.data(); });
+  std::unique_ptr<CompilerInstance> IndirectCI =
+      init_convenience::BuildCompilerInstance(ClangArgv);
+  auto ST = llvm::make_unique<SelectorTable>();
+  auto BC = llvm::make_unique<Builtin::Context>();
+  std::unique_ptr<ASTContext> AST =
+      init_convenience::BuildASTContext(*IndirectCI, *ST, *BC);
+  IndirectCI->setASTContext(AST.release());
+  AddExternalSource(*IndirectCI, CI);
+  return IndirectCI;
+}
+
 llvm::Error ParseSource(const std::string &Path, CompilerInstance &CI,
                         CodeGenerator &CG) {
   SourceManager &SM = CI.getSourceManager();
   const FileEntry *FE = CI.getFileManager().getFile(Path);
   if (!FE) {
     return llvm::make_error<llvm::StringError>(
         llvm::Twine("Couldn't open ", Path), std::error_code());
   }
std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
                  [](const std::string &s) -> const char * { return s.data(); });
   std::unique_ptr<CompilerInstance> CI =
       init_convenience::BuildCompilerInstance(ClangArgv);
   auto ST = llvm::make_unique<SelectorTable>();
   auto BC = llvm::make_unique<Builtin::Context>();
   std::unique_ptr<ASTContext> AST =
       init_convenience::BuildASTContext(*CI, *ST, *BC);
   CI->setASTContext(AST.release());
+  if (Imports.size()) {
     AddExternalSource(*CI, Imports);
+  }
 
   auto LLVMCtx = llvm::make_unique<llvm::LLVMContext>();
   std::unique_ptr<CodeGenerator> CG =
       init_convenience::BuildCodeGen(*CI, *LLVMCtx);
   CG->Initialize(CI->getASTContext());
 
   CI->getDiagnosticClient().BeginSourceFile(CI->getLangOpts(),
                                             &CI->getPreprocessor());
for (auto I : Imports) {
     llvm::Expected<std::unique_ptr<CompilerInstance>> ImportCI = Parse(I, {});
     if (auto E = ImportCI.takeError()) {
       llvm::errs() << llvm::toString(std::move(E));
       exit(-1);
     } else {
       ImportCIs.push_back(std::move(*ImportCI));
     }
   }
+  std::vector<std::unique_ptr<CompilerInstance>> IndirectCIs;
+  if (!Direct) {
+    for (auto &ImportCI : ImportCIs) {
+      llvm::Expected<std::unique_ptr<CompilerInstance>> IndirectCI =
+          BuildIndirect(ImportCI);
+      if (auto E = IndirectCI.takeError()) {
+        llvm::errs() << llvm::toString(std::move(E));
+        exit(-1);
+      } else {
+        IndirectCIs.push_back(std::move(*IndirectCI));
+      }
+    }
+  }
   llvm::Expected<std::unique_ptr<CompilerInstance>> ExpressionCI =
-      Parse(Expression, ImportCIs);
+      Parse(Expression, Direct ? ImportCIs : IndirectCIs);
   if (auto E = ExpressionCI.takeError()) {
     llvm::errs() << llvm::toString(std::move(E));
     exit(-1);
   } else {
     return 0;
   }
 }
+