From 9092d4795d987d748183fd32ba39035c91949234 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. llvm-svn: 302975 --- clang/include/clang/AST/ExternalASTMerger.h | 2 + clang/lib/AST/ASTImporter.cpp | 14 +++++++ clang/lib/AST/ASTStructuralEquivalence.cpp | 5 +++ clang/lib/AST/ExternalASTMerger.cpp | 6 +++ clang/test/Import/conflicting-struct/Inputs/S1.cpp | 6 +++ clang/test/Import/conflicting-struct/Inputs/S2.cpp | 7 ++++ clang/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 clang/test/Import/conflicting-struct/Inputs/S1.cpp create mode 100644 clang/test/Import/conflicting-struct/Inputs/S2.cpp create mode 100644 clang/test/Import/conflicting-struct/test.cpp diff --git a/clang/include/clang/AST/ExternalASTMerger.h b/clang/include/clang/AST/ExternalASTMerger.h index 51d0c30..55459df 100644 --- a/clang/include/clang/AST/ExternalASTMerger.h +++ b/clang/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/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index 4fb6051..847638b 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/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 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/clang/lib/AST/ASTStructuralEquivalence.cpp b/clang/lib/AST/ASTStructuralEquivalence.cpp index 8fe72ea..9376ee1 100644 --- a/clang/lib/AST/ASTStructuralEquivalence.cpp +++ b/clang/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/clang/lib/AST/ExternalASTMerger.cpp b/clang/lib/AST/ExternalASTMerger.cpp index 8849cfc..1dc472a 100644 --- a/clang/lib/AST/ExternalASTMerger.cpp +++ b/clang/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/clang/test/Import/conflicting-struct/Inputs/S1.cpp b/clang/test/Import/conflicting-struct/Inputs/S1.cpp new file mode 100644 index 0000000..a99dba8 --- /dev/null +++ b/clang/test/Import/conflicting-struct/Inputs/S1.cpp @@ -0,0 +1,6 @@ +class T; + +class S { + T *t; + int a; +}; diff --git a/clang/test/Import/conflicting-struct/Inputs/S2.cpp b/clang/test/Import/conflicting-struct/Inputs/S2.cpp new file mode 100644 index 0000000..de2cb6c --- /dev/null +++ b/clang/test/Import/conflicting-struct/Inputs/S2.cpp @@ -0,0 +1,7 @@ +class U { + int b; +}; + +class T { + U u; +}; diff --git a/clang/test/Import/conflicting-struct/test.cpp b/clang/test/Import/conflicting-struct/test.cpp new file mode 100644 index 0000000..5aad567 --- /dev/null +++ b/clang/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/clang/tools/clang-import-test/clang-import-test.cpp b/clang/tools/clang-import-test/clang-import-test.cpp index d7ab184..567a4bb 100644 --- a/clang/tools/clang-import-test/clang-import-test.cpp +++ b/clang/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.7.4