From 8ff60edd11d3f6740018e18f89836d15fd4cd65e Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Sat, 24 Jan 2015 01:07:20 +0000 Subject: [PATCH] [modules] Sometimes we can deserialize a class member but not have yet encountered any definition for the class; this happens when the definition is added by an update record that is not yet loaded. In such a case, eagerly pick the original parent of the member as the canonical definition of the class rather than muddling through with the canonical declaration (the latter can lead to us failing to merge properly later if the canonical definition turns out to be some other declaration). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@226977 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Serialization/ASTReader.h | 4 + lib/AST/ASTDumper.cpp | 11 ++- lib/Serialization/ASTReader.cpp | 7 +- lib/Serialization/ASTReaderDecl.cpp | 77 ++++++++++++++----- .../Modules/Inputs/merge-nested-templates/a.h | 1 + .../Modules/Inputs/merge-nested-templates/b.h | 2 + .../Modules/Inputs/merge-nested-templates/c.h | 3 + .../merge-nested-templates/module.modulemap | 3 + .../Inputs/merge-nested-templates/string.ii | 14 ++++ test/Modules/merge-nested-templates.cpp | 4 + 10 files changed, 100 insertions(+), 26 deletions(-) create mode 100644 test/Modules/Inputs/merge-nested-templates/a.h create mode 100644 test/Modules/Inputs/merge-nested-templates/b.h create mode 100644 test/Modules/Inputs/merge-nested-templates/c.h create mode 100644 test/Modules/Inputs/merge-nested-templates/module.modulemap create mode 100644 test/Modules/Inputs/merge-nested-templates/string.ii create mode 100644 test/Modules/merge-nested-templates.cpp diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index 91ad34bd1c..8dd9b145c0 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -435,6 +435,10 @@ private: llvm::SmallVector, 16> PendingUpdateRecords; + /// \brief The DefinitionData pointers that we faked up for class definitions + /// that we needed but hadn't loaded yet. + llvm::SmallPtrSet PendingFakeDefinitionData; + struct ReplacedDeclInfo { ModuleFile *Mod; uint64_t Offset; diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp index ebf5e651ef..edd836041f 100644 --- a/lib/AST/ASTDumper.cpp +++ b/lib/AST/ASTDumper.cpp @@ -1099,10 +1099,13 @@ void ASTDumper::VisitFunctionDecl(const FunctionDecl *D) { E = D->getDeclsInPrototypeScope().end(); I != E; ++I) dumpDecl(*I); - for (FunctionDecl::param_const_iterator I = D->param_begin(), - E = D->param_end(); - I != E; ++I) - dumpDecl(*I); + if (!D->param_begin() && D->getNumParams()) + dumpChild([=] { OS << "<getNumParams() << ">>"; }); + else + for (FunctionDecl::param_const_iterator I = D->param_begin(), + E = D->param_end(); + I != E; ++I) + dumpDecl(*I); if (const CXXConstructorDecl *C = dyn_cast(D)) for (CXXConstructorDecl::init_const_iterator I = C->init_begin(), diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 416164ebb7..f7822b7476 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -8298,7 +8298,12 @@ void ASTReader::finishPendingActions() { loadDeclUpdateRecords(Update.first, Update.second); } } - + + // At this point, all update records for loaded decls are in place, so any + // fake class definitions should have become real. + assert(PendingFakeDefinitionData.empty() && + "faked up a class definition but never saw the real one"); + // If we deserialized any C++ or Objective-C class definitions, any // Objective-C protocol definitions, or any redeclarable templates, make sure // that all redeclarations point to the definitions. Note that this can only diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index d43f3eaacb..754b0a0a53 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -107,7 +107,7 @@ namespace clang { void ReadCXXDefinitionData(struct CXXRecordDecl::DefinitionData &Data, const RecordData &R, unsigned &I); void MergeDefinitionData(CXXRecordDecl *D, - struct CXXRecordDecl::DefinitionData &NewDD); + struct CXXRecordDecl::DefinitionData &&NewDD); static NamedDecl *getAnonymousDeclForMerging(ASTReader &Reader, DeclContext *DC, @@ -205,6 +205,8 @@ namespace clang { operator T*() const { return dyn_cast_or_null(Existing); } }; + static DeclContext *getPrimaryContextForMerging(ASTReader &Reader, + DeclContext *DC); FindExistingResult findExisting(NamedDecl *D); public: @@ -1353,11 +1355,25 @@ void ASTDeclReader::ReadCXXDefinitionData( } void ASTDeclReader::MergeDefinitionData( - CXXRecordDecl *D, struct CXXRecordDecl::DefinitionData &MergeDD) { + CXXRecordDecl *D, struct CXXRecordDecl::DefinitionData &&MergeDD) { assert(D->DefinitionData.getNotUpdated() && "merging class definition into non-definition"); auto &DD = *D->DefinitionData.getNotUpdated(); + if (Reader.PendingFakeDefinitionData.count(&DD)) { + // We faked up this definition data because we found a class for which we'd + // not yet loaded the definition. Replace it with the real thing now. + Reader.PendingFakeDefinitionData.erase(&DD); + assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?"); + + // Don't change which declaration is the definition; that is required + // to be invariant once we select it. + auto *Def = DD.Definition; + DD = std::move(MergeDD); + DD.Definition = Def; + return; + } + // If the new definition has new special members, let the name lookup // code know that it needs to look in the new definition too. // @@ -1460,17 +1476,18 @@ void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D) { // We might already have a definition for this record. This can happen either // because we're reading an update record, or because we've already done some // merging. Either way, just merge into it. - if (auto *CanonDD = D->DefinitionData.getNotUpdated()) { + CXXRecordDecl *Canon = D->getCanonicalDecl(); + if (auto *CanonDD = Canon->DefinitionData.getNotUpdated()) { if (CanonDD->Definition != DD->Definition) Reader.MergedDeclContexts.insert( std::make_pair(DD->Definition, CanonDD->Definition)); - MergeDefinitionData(D, *DD); + MergeDefinitionData(Canon, std::move(*DD)); + D->DefinitionData = Canon->DefinitionData; return; } // Propagate the DefinitionData pointer to the canonical declaration, so // that all other deserialized declarations will see it. - CXXRecordDecl *Canon = D->getCanonicalDecl(); if (Canon == D) { D->DefinitionData = DD; D->IsCompleteDefinition = true; @@ -1482,7 +1499,7 @@ void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D) { std::make_pair(D, CanonDD->Definition)); D->DefinitionData = Canon->DefinitionData; D->IsCompleteDefinition = false; - MergeDefinitionData(D, *DD); + MergeDefinitionData(D, std::move(*DD)); } else { Canon->DefinitionData = DD; D->DefinitionData = Canon->DefinitionData; @@ -1827,7 +1844,7 @@ ASTDeclReader::VisitClassTemplateSpecializationDeclImpl( // definition. if (auto *DDD = D->DefinitionData.getNotUpdated()) { if (auto *CanonDD = CanonSpec->DefinitionData.getNotUpdated()) { - MergeDefinitionData(CanonSpec, *DDD); + MergeDefinitionData(CanonSpec, std::move(*DDD)); Reader.PendingDefinitions.erase(D); Reader.MergedDeclContexts.insert( std::make_pair(D, CanonDD->Definition)); @@ -2125,6 +2142,7 @@ void ASTDeclReader::mergeTemplatePattern(RedeclarableTemplateDecl *D, auto *ExistingPattern = Existing->getTemplatedDecl(); RedeclarableResult Result(Reader, DPattern->getCanonicalDecl()->getGlobalID(), DPattern->getKind()); + if (auto *DClass = dyn_cast(DPattern)) { // Merge with any existing definition. // FIXME: This is duplicated in several places. Refactor. @@ -2132,7 +2150,7 @@ void ASTDeclReader::mergeTemplatePattern(RedeclarableTemplateDecl *D, cast(ExistingPattern)->getCanonicalDecl(); if (auto *DDD = DClass->DefinitionData.getNotUpdated()) { if (auto *ExistingDD = ExistingClass->DefinitionData.getNotUpdated()) { - MergeDefinitionData(ExistingClass, *DDD); + MergeDefinitionData(ExistingClass, std::move(*DDD)); Reader.PendingDefinitions.erase(DClass); Reader.MergedDeclContexts.insert( std::make_pair(DClass, ExistingDD->Definition)); @@ -2533,18 +2551,33 @@ static bool isSameEntity(NamedDecl *X, NamedDecl *Y) { /// Find the context in which we should search for previous declarations when /// looking for declarations to merge. -static DeclContext *getPrimaryContextForMerging(DeclContext *DC) { +DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader, + DeclContext *DC) { if (NamespaceDecl *ND = dyn_cast(DC)) return ND->getOriginalNamespace(); - // There is one tricky case here: if DC is a class with no definition, then - // we're merging a declaration whose definition is added by an update record, - // but we've not yet loaded that update record. In this case, we use the - // canonical declaration for merging until we get a real definition. - // FIXME: When we add a definition, we may need to move the partial lookup - // information from the canonical declaration onto the chosen definition. - if (CXXRecordDecl *RD = dyn_cast(DC)) - return RD->getPrimaryContext(); + if (CXXRecordDecl *RD = dyn_cast(DC)) { + // Try to dig out the definition. + auto *DD = RD->DefinitionData.getNotUpdated(); + if (!DD) + DD = RD->getCanonicalDecl()->DefinitionData.getNotUpdated(); + + // If there's no definition yet, then DC's definition is added by an update + // record, but we've not yet loaded that update record. In this case, we + // commit to DC being the canonical definition now, and will fix this when + // we load the update record. + if (!DD) { + DD = new (Reader.Context) struct CXXRecordDecl::DefinitionData(RD); + RD->IsCompleteDefinition = true; + RD->DefinitionData = DD; + RD->getCanonicalDecl()->DefinitionData = DD; + + // Track that we did this horrible thing so that we can fix it later. + Reader.PendingFakeDefinitionData.insert(DD); + } + + return DD->Definition; + } if (EnumDecl *ED = dyn_cast(DC)) return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition() @@ -2574,7 +2607,7 @@ ASTDeclReader::FindExistingResult::~FindExistingResult() { AnonymousDeclNumber, New); } else if (DC->isTranslationUnit() && Reader.SemaObj) { Reader.SemaObj->IdResolver.tryAddTopLevelDecl(New, Name); - } else if (DeclContext *MergeDC = getPrimaryContextForMerging(DC)) { + } else if (DeclContext *MergeDC = getPrimaryContextForMerging(Reader, DC)) { // Add the declaration to its redeclaration context so later merging // lookups will find it. MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true); @@ -2720,7 +2753,7 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) { return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber, TypedefNameForLinkage); } - } else if (DeclContext *MergeDC = getPrimaryContextForMerging(DC)) { + } else if (DeclContext *MergeDC = getPrimaryContextForMerging(Reader, DC)) { DeclContext::lookup_result R = MergeDC->noload_lookup(Name); for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E; ++I) { if (NamedDecl *Existing = getDeclForMerging(*I, TypedefNameForLinkage)) @@ -3605,11 +3638,13 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile, case UPD_CXX_INSTANTIATED_CLASS_DEFINITION: { auto *RD = cast(D); - bool HadDefinition = RD->getDefinition(); + bool HadRealDefinition = RD->getDefinition() && + !Reader.PendingFakeDefinitionData.count( + RD->DefinitionData.getNotUpdated()); ReadCXXRecordDefinition(RD); // Visible update is handled separately. uint64_t LexicalOffset = Record[Idx++]; - if (!HadDefinition && LexicalOffset) { + if (!HadRealDefinition && LexicalOffset) { RD->setHasExternalLexicalStorage(true); Reader.ReadDeclContextStorage(ModuleFile, ModuleFile.DeclsCursor, std::make_pair(LexicalOffset, 0), diff --git a/test/Modules/Inputs/merge-nested-templates/a.h b/test/Modules/Inputs/merge-nested-templates/a.h new file mode 100644 index 0000000000..826d2571fd --- /dev/null +++ b/test/Modules/Inputs/merge-nested-templates/a.h @@ -0,0 +1 @@ +#include "string.ii" diff --git a/test/Modules/Inputs/merge-nested-templates/b.h b/test/Modules/Inputs/merge-nested-templates/b.h new file mode 100644 index 0000000000..516694e38e --- /dev/null +++ b/test/Modules/Inputs/merge-nested-templates/b.h @@ -0,0 +1,2 @@ +#include "a.h" +std::wstring::iterator j; diff --git a/test/Modules/Inputs/merge-nested-templates/c.h b/test/Modules/Inputs/merge-nested-templates/c.h new file mode 100644 index 0000000000..ab95e145ef --- /dev/null +++ b/test/Modules/Inputs/merge-nested-templates/c.h @@ -0,0 +1,3 @@ +#include "string.ii" +std::wstring::iterator i; +#include "b.h" diff --git a/test/Modules/Inputs/merge-nested-templates/module.modulemap b/test/Modules/Inputs/merge-nested-templates/module.modulemap new file mode 100644 index 0000000000..77e0a89e39 --- /dev/null +++ b/test/Modules/Inputs/merge-nested-templates/module.modulemap @@ -0,0 +1,3 @@ +module a { header "a.h" export * } +module b { header "b.h" export * } +module c { header "c.h" export * } diff --git a/test/Modules/Inputs/merge-nested-templates/string.ii b/test/Modules/Inputs/merge-nested-templates/string.ii new file mode 100644 index 0000000000..136d8e7008 --- /dev/null +++ b/test/Modules/Inputs/merge-nested-templates/string.ii @@ -0,0 +1,14 @@ +namespace std { + template struct normal_iterator { + normal_iterator() {} + + template + normal_iterator(normal_iterator) {} + }; + + template struct basic_string { + typedef normal_iterator iterator; + }; + + typedef basic_string wstring; +} diff --git a/test/Modules/merge-nested-templates.cpp b/test/Modules/merge-nested-templates.cpp new file mode 100644 index 0000000000..42764eaed6 --- /dev/null +++ b/test/Modules/merge-nested-templates.cpp @@ -0,0 +1,4 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/merge-nested-templates -verify %s +// expected-no-diagnostics +#include "c.h" -- 2.40.0