[ASTImporter] Improve handling of incomplete types

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.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@302975 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/AST/ExternalASTMerger.h

@@ -44,6 +44,8 @@ public:
   FindExternalLexicalDecls(const DeclContext *DC,
                            llvm::function_ref<bool(Decl::Kind)> IsKindWeWant,
                            SmallVectorImpl<Decl *> &Result) override;
+
+   void CompleteType(TagDecl *Tag) override;
 };
 
 } // end namespace clang

lib/AST/ASTImporter.cpp

@@ -1622,10 +1622,18 @@ Decl *ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
 
   // 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;
@@ -1652,6 +1660,8 @@ Decl *ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
           }
         }
 
+        PrevDecl = FoundRecord;
+
         if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
           if ((SearchName && !D->isCompleteDefinition())
               || (D->isCompleteDefinition() &&
@@ -1744,6 +1754,10 @@ Decl *ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
     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);

lib/AST/ASTStructuralEquivalence.cpp

@@ -855,6 +855,11 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
 
   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)

lib/AST/ExternalASTMerger.cpp

@@ -178,3 +178,9 @@ void ExternalASTMerger::FindExternalLexicalDecls(
         }
       });
 }
+
+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

@@ -0,0 +1,6 @@
+class T;
+
+class S {
+  T *t;
+  int a;
+};

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

@@ -0,0 +1,7 @@
+class U {
+  int b;
+};
+
+class T {
+  U u;
+};

test/Import/conflicting-struct/test.cpp

@@ -0,0 +1,7 @@
+// 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

@@ -42,6 +42,10 @@ 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"),
@@ -172,6 +176,14 @@ BuildCompilerInstance(ArrayRef<const char *> ClangArgv) {
   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>(
@@ -205,6 +217,21 @@ void AddExternalSource(
   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();
@@ -231,7 +258,8 @@ Parse(const std::string &Path,
   std::unique_ptr<ASTContext> AST =
       init_convenience::BuildASTContext(*CI, *ST, *BC);
   CI->setASTContext(AST.release());
-  AddExternalSource(*CI, Imports);
+  if (Imports.size())
+    AddExternalSource(*CI, Imports);
 
   auto LLVMCtx = llvm::make_unique<llvm::LLVMContext>();
   std::unique_ptr<CodeGenerator> CG =
@@ -268,8 +296,21 @@ int main(int argc, const char **argv) {
       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);
@@ -277,3 +318,4 @@ int main(int argc, const char **argv) {
     return 0;
   }
 }
+