[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

diff --git a/include/clang/AST/ExternalASTMerger.h b/include/clang/AST/ExternalASTMerger.h
index 51d0c30ad23bf5b7b897e126bd87229ba030e996..55459df1fe6ba9239ccfb8b9e98c0884fd6711bd 100644 (file)
--- a/include/clang/AST/ExternalASTMerger.h
+++ b/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

diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp
index 4fb6051d6f5801128efe9914b7e7ec170b6bf47f..847638b7bbeb88598b25b388e8b053e60abb35a1 100644 (file)
--- a/lib/AST/ASTImporter.cpp
+++ b/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);

diff --git a/lib/AST/ASTStructuralEquivalence.cpp b/lib/AST/ASTStructuralEquivalence.cpp
index 8fe72eac41337056d9287e175b69d835f9e54577..9376ee1d4ee49d9fd93bd9f0bbc36d81366adae4 100644 (file)
--- a/lib/AST/ASTStructuralEquivalence.cpp
+++ b/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)

diff --git a/lib/AST/ExternalASTMerger.cpp b/lib/AST/ExternalASTMerger.cpp
index 8849cfc3c80b5bb532f7506dc3e3d62defb0f882..1dc472a5f7534f71f9110cc8eab51eb873783194 100644 (file)
--- a/lib/AST/ExternalASTMerger.cpp
+++ b/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);
+}

diff --git a/test/Import/conflicting-struct/Inputs/S1.cpp b/test/Import/conflicting-struct/Inputs/S1.cpp
new file mode 100644 (file)
index 0000000..a99dba8
--- /dev/null
+++ b/test/Import/conflicting-struct/Inputs/S1.cpp
@@ -0,0 +1,6 @@
+class T;
+
+class S {
+  T *t;
+  int a;
+};

diff --git a/test/Import/conflicting-struct/Inputs/S2.cpp b/test/Import/conflicting-struct/Inputs/S2.cpp
new file mode 100644 (file)
index 0000000..de2cb6c
--- /dev/null
+++ b/test/Import/conflicting-struct/Inputs/S2.cpp
@@ -0,0 +1,7 @@
+class U {
+  int b;
+};
+
+class T {
+  U u;
+};

diff --git a/test/Import/conflicting-struct/test.cpp b/test/Import/conflicting-struct/test.cpp
new file mode 100644 (file)
index 0000000..5aad567
--- /dev/null
+++ b/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;
+}

diff --git a/tools/clang-import-test/clang-import-test.cpp b/tools/clang-import-test/clang-import-test.cpp
index d7ab18478c3234fef29b296f03cde0a4401bfd7c..567a4bb4f0a262deb882083181b30f28cc3932f5 100644 (file)
--- a/tools/clang-import-test/clang-import-test.cpp
+++ b/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;
   }
 }
+