From 848b631d3a1e857e2ad2fe61f09ad6de50e89c25 Mon Sep 17 00:00:00 2001 From: Sean Callanan Date: Sat, 13 May 2017 00:46:33 +0000 Subject: [PATCH] [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 | 2 + lib/AST/ASTImporter.cpp | 14 ++++++ lib/AST/ASTStructuralEquivalence.cpp | 5 ++ lib/AST/ExternalASTMerger.cpp | 6 +++ test/Import/conflicting-struct/Inputs/S1.cpp | 6 +++ test/Import/conflicting-struct/Inputs/S2.cpp | 7 +++ test/Import/conflicting-struct/test.cpp | 7 +++ tools/clang-import-test/clang-import-test.cpp | 46 ++++++++++++++++++- 8 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 test/Import/conflicting-struct/Inputs/S1.cpp create mode 100644 test/Import/conflicting-struct/Inputs/S2.cpp create mode 100644 test/Import/conflicting-struct/test.cpp diff --git a/include/clang/AST/ExternalASTMerger.h b/include/clang/AST/ExternalASTMerger.h index 51d0c30ad2..55459df1fe 100644 --- a/include/clang/AST/ExternalASTMerger.h +++ b/include/clang/AST/ExternalASTMerger.h @@ -44,6 +44,8 @@ public: FindExternalLexicalDecls(const DeclContext *DC, llvm::function_ref IsKindWeWant, SmallVectorImpl &Result) override; + + void CompleteType(TagDecl *Tag) override; }; } // end namespace clang diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp index 4fb6051d6f..847638b7bb 100644 --- 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 ConflictingDecls; SmallVector 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 8fe72eac41..9376ee1d4e 100644 --- a/lib/AST/ASTStructuralEquivalence.cpp +++ b/lib/AST/ASTStructuralEquivalence.cpp @@ -855,6 +855,11 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context, if (CXXRecordDecl *D1CXX = dyn_cast(D1)) { if (CXXRecordDecl *D2CXX = dyn_cast(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 8849cfc3c8..1dc472a5f7 100644 --- a/lib/AST/ExternalASTMerger.cpp +++ b/lib/AST/ExternalASTMerger.cpp @@ -178,3 +178,9 @@ void ExternalASTMerger::FindExternalLexicalDecls( } }); } + +void ExternalASTMerger::CompleteType(TagDecl *Tag) { + SmallVector 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 index 0000000000..a99dba8c78 --- /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 index 0000000000..de2cb6cd03 --- /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 index 0000000000..5aad567cd7 --- /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 d7ab18478c..567a4bb4f0 100644 --- 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 Imports("import", llvm::cl::ZeroOrMore, llvm::cl::desc("Path to a file containing declarations to import")); +static llvm::cl::opt + Direct("direct", llvm::cl::Optional, + llvm::cl::desc("Use the parsed declarations without indirection")); + static llvm::cl::list ClangArgs("Xcc", llvm::cl::ZeroOrMore, llvm::cl::desc("Argument to pass to the CompilerInvocation"), @@ -172,6 +176,14 @@ BuildCompilerInstance(ArrayRef ClangArgv) { return Ins; } +std::unique_ptr +BuildCompilerInstance(ArrayRef ClangArgs) { + std::vector 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 BuildASTContext(CompilerInstance &CI, SelectorTable &ST, Builtin::Context &BC) { auto AST = llvm::make_unique( @@ -205,6 +217,21 @@ void AddExternalSource( CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage(); } +std::unique_ptr BuildIndirect(std::unique_ptr &CI) { + std::vector ClangArgv(ClangArgs.size()); + std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(), + [](const std::string &s) -> const char * { return s.data(); }); + std::unique_ptr IndirectCI = + init_convenience::BuildCompilerInstance(ClangArgv); + auto ST = llvm::make_unique(); + auto BC = llvm::make_unique(); + std::unique_ptr 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 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(); std::unique_ptr CG = @@ -268,8 +296,21 @@ int main(int argc, const char **argv) { ImportCIs.push_back(std::move(*ImportCI)); } } + std::vector> IndirectCIs; + if (!Direct) { + for (auto &ImportCI : ImportCIs) { + llvm::Expected> 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> 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; } } + -- 2.40.0