From 2b8fac8aee3f9908cb07ab9121ae3deb2ba0e9b4 Mon Sep 17 00:00:00 2001 From: Daniel Jasper Date: Wed, 11 Oct 2017 07:47:54 +0000 Subject: [PATCH] Revert r314955: "Remove PendingBody mechanism for function and ObjC method deserialization." This is breaking a build of https://github.com/abseil/abseil-cpp and so likely not really NFC. Also reverted subsequent r314956/7. I'll forward reproduction instructions to Richard. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@315439 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Serialization/ASTReader.h | 24 +++++- lib/Serialization/ASTCommon.cpp | 4 +- lib/Serialization/ASTReader.cpp | 24 ++++++ lib/Serialization/ASTReaderDecl.cpp | 106 ++++++++---------------- test/Modules/merge-lambdas.cpp | 43 ---------- 5 files changed, 79 insertions(+), 122 deletions(-) delete mode 100644 test/Modules/merge-lambdas.cpp diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index d6ae9d3a22..9c5ad00691 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -559,9 +559,13 @@ private: /// declarations that have not yet been linked to their definitions. llvm::SmallPtrSet PendingDefinitions; - /// \brief Functions or methods that are known to already have a definition - /// (that might not yet be merged into the redeclaration chain). - llvm::SmallDenseMap FunctionDefinitions; + typedef llvm::MapVector, + SmallVector, 4> > + PendingBodiesMap; + + /// \brief Functions or methods that have bodies that will be attached. + PendingBodiesMap PendingBodies; /// \brief Definitions for which we have added merged definitions but not yet /// performed deduplication. @@ -987,13 +991,25 @@ private: /// the last time we loaded information about this identifier. llvm::DenseMap IdentifierGeneration; + class InterestingDecl { + Decl *D; + bool DeclHasPendingBody; + + public: + InterestingDecl(Decl *D, bool HasBody) + : D(D), DeclHasPendingBody(HasBody) {} + Decl *getDecl() { return D; } + /// Whether the declaration has a pending body. + bool hasPendingBody() { return DeclHasPendingBody; } + }; + /// \brief Contains declarations and definitions that could be /// "interesting" to the ASTConsumer, when we get that AST consumer. /// /// "Interesting" declarations are those that have data that may /// need to be emitted, such as inline function definitions or /// Objective-C protocols. - std::deque PotentiallyInterestingDecls; + std::deque PotentiallyInterestingDecls; /// \brief The list of redeclaration chains that still need to be /// reconstructed, and the local offset to the corresponding list diff --git a/lib/Serialization/ASTCommon.cpp b/lib/Serialization/ASTCommon.cpp index 7519dc239c..9c6f03cd0b 100644 --- a/lib/Serialization/ASTCommon.cpp +++ b/lib/Serialization/ASTCommon.cpp @@ -344,8 +344,8 @@ bool serialization::needsAnonymousDeclarationNumber(const NamedDecl *D) { return true; } - // Otherwise, we only care about anonymous class members / block-scope decls. - if (D->getDeclName() || D->getLexicalDeclContext()->isFileContext()) + // Otherwise, we only care about anonymous class members. + if (D->getDeclName() || !isa(D->getLexicalDeclContext())) return false; return isa(D) || isa(D); } diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 82bf6e157b..4d77c2bb15 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -9169,6 +9169,30 @@ void ASTReader::finishPendingActions() { } PendingDefinitions.clear(); + // Load the bodies of any functions or methods we've encountered. We do + // this now (delayed) so that we can be sure that the declaration chains + // have been fully wired up (hasBody relies on this). + // FIXME: We shouldn't require complete redeclaration chains here. + for (PendingBodiesMap::iterator PB = PendingBodies.begin(), + PBEnd = PendingBodies.end(); + PB != PBEnd; ++PB) { + if (FunctionDecl *FD = dyn_cast(PB->first)) { + // FIXME: Check for =delete/=default? + // FIXME: Complain about ODR violations here? + const FunctionDecl *Defn = nullptr; + if (!getContext().getLangOpts().Modules || !FD->hasBody(Defn)) { + FD->setLazyBody(PB->second); + } else + mergeDefinitionVisibility(const_cast(Defn), FD); + continue; + } + + ObjCMethodDecl *MD = cast(PB->first); + if (!getContext().getLangOpts().Modules || !MD->hasBody()) + MD->setLazyBody(PB->second); + } + PendingBodies.clear(); + // Do some cleanup. for (auto *ND : PendingMergedDefinitionsToDeduplicate) getContext().deduplicateMergedDefinitonsFor(ND); diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 118417c930..1283b006df 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -45,6 +45,8 @@ namespace clang { GlobalDeclID NamedDeclForTagDecl; IdentifierInfo *TypedefNameForLinkage; + bool HasPendingBody; + ///\brief A flag to carry the information for a decl from the entity is /// used. We use it to delay the marking of the canonical decl as used until /// the entire declaration is deserialized and merged. @@ -214,7 +216,8 @@ namespace clang { : Reader(Reader), Record(Record), Loc(Loc), ThisDeclID(thisDeclID), ThisDeclLoc(ThisDeclLoc), TypeIDForTypeDecl(0), NamedDeclForTagDecl(0), - TypedefNameForLinkage(nullptr), IsDeclMarkedUsed(false) {} + TypedefNameForLinkage(nullptr), HasPendingBody(false), + IsDeclMarkedUsed(false) {} template static void AddLazySpecializations(T *D, @@ -262,7 +265,9 @@ namespace clang { static void markIncompleteDeclChainImpl(Redeclarable *D); static void markIncompleteDeclChainImpl(...); - FunctionDecl *TryRegisterAsFunctionDefinition(FunctionDecl *FD); + /// \brief Determine whether this declaration has a pending body. + bool hasPendingBody() const { return HasPendingBody; } + void ReadFunctionDefinition(FunctionDecl *FD); void Visit(Decl *D); @@ -415,8 +420,7 @@ public: MergedRedeclIterator &operator++() { if (Current->isFirstDecl()) { Canonical = Current; - Decl *MostRecent = ASTDeclReader::getMostRecentDecl(Canonical); - Current = MostRecent ? cast(MostRecent) : nullptr; + Current = Current->getMostRecentDecl(); } else Current = Current->getPreviousDecl(); @@ -447,70 +451,17 @@ uint64_t ASTDeclReader::GetCurrentCursorOffset() { return Loc.F->DeclsCursor.GetCurrentBitNo() + Loc.F->GlobalBitOffset; } -FunctionDecl *ASTDeclReader::TryRegisterAsFunctionDefinition(FunctionDecl *D) { - FunctionDecl *&Definition = Reader.FunctionDefinitions[D->getCanonicalDecl()]; - - if (!Definition) { - // No imported definition, but we might have a local definition. - for (auto *Redecl : merged_redecls(D)) { - // FIXME: If an existing declaration is a definition with no body - // (via =delete etc), we shouldn't permit another definition here. - if (Redecl->Body.isValid()) { - Definition = Redecl; - break; - } - } - } - - if (Definition) { - // We might have multiple update records adding definitions to the same - // declaration. - if (Definition != D) { - // Already have a different definition, merge this one into it. - Reader.MergedDeclContexts.insert(std::make_pair(D, Definition)); - Reader.mergeDefinitionVisibility(Definition, D); - } - return Definition; - } - - Definition = D; - return nullptr; -} - void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) { - // Is this definition scheduled for modular codegen? (That is, emitted to a - // separate object file for the module itself, rather than with module users.) - bool ModularCodegen = Record.readInt(); - - // Don't load this definition if we already have one. - if (auto *Definition = TryRegisterAsFunctionDefinition(FD)) { - if (ModularCodegen) { - // Request a strong definition be emitted if any merged definition does. - // FIXME: Do we need to ensure that Definition is handed to the AST - // consumer in this case? - Reader.DefinitionSource[Definition] |= - Loc.F->Kind == ModuleKind::MK_MainFile; - } - // Skip the ctor-initializers, if any. - if (isa(FD)) - if (Record.readInt()) - ReadGlobalOffset(); - // FIXME: Optionally register the duplicate definitions somewhere so we can - // check for ODR violations. - return; - } - - if (ModularCodegen) + if (Record.readInt()) Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile; - if (auto *CD = dyn_cast(FD)) { CD->NumCtorInitializers = Record.readInt(); if (CD->NumCtorInitializers) CD->CtorInitializers = ReadGlobalOffset(); } - // Store the offset of the body so we can lazily load it later. - FD->setLazyBody(GetCurrentCursorOffset()); + Reader.PendingBodies[FD] = GetCurrentCursorOffset(); + HasPendingBody = true; } void ASTDeclReader::Visit(Decl *D) { @@ -962,10 +913,10 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) { void ASTDeclReader::VisitObjCMethodDecl(ObjCMethodDecl *MD) { VisitNamedDecl(MD); if (Record.readInt()) { - // Load the body on-demand (if we don't already have a definition). Most - // clients won't care, because method definitions rarely show up in - // headers. - MD->setLazyBody(GetCurrentCursorOffset()); + // Load the body on-demand. Most clients won't care, because method + // definitions rarely show up in headers. + Reader.PendingBodies[MD] = GetCurrentCursorOffset(); + HasPendingBody = true; MD->setSelfDecl(ReadDeclAs()); MD->setCmdDecl(ReadDeclAs()); } @@ -2616,7 +2567,7 @@ inline void ASTReader::LoadedDecl(unsigned Index, Decl *D) { /// This routine should return true for anything that might affect /// code generation, e.g., inline function definitions, Objective-C /// declarations with metadata, etc. -static bool isConsumerInterestedIn(ASTContext &Ctx, Decl *D) { +static bool isConsumerInterestedIn(ASTContext &Ctx, Decl *D, bool HasBody) { // An ObjCMethodDecl is never considered as "interesting" because its // implementation container always is. @@ -2642,7 +2593,7 @@ static bool isConsumerInterestedIn(ASTContext &Ctx, Decl *D) { return Var->isFileVarDecl() && Var->isThisDeclarationADefinition() == VarDecl::Definition; if (FunctionDecl *Func = dyn_cast(D)) - return Func->doesThisDeclarationHaveABody(); + return Func->doesThisDeclarationHaveABody() || HasBody; if (auto *ES = D->getASTContext().getExternalSource()) if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never) @@ -3707,7 +3658,8 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) { // AST consumer might need to know about, queue it. // We don't pass it to the consumer immediately because we may be in recursive // loading, and some declarations may still be initializing. - PotentiallyInterestingDecls.push_back(D); + PotentiallyInterestingDecls.push_back( + InterestingDecl(D, Reader.hasPendingBody())); return D; } @@ -3730,10 +3682,10 @@ void ASTReader::PassInterestingDeclsToConsumer() { EagerlyDeserializedDecls.clear(); while (!PotentiallyInterestingDecls.empty()) { - Decl *D = PotentiallyInterestingDecls.front(); + InterestingDecl D = PotentiallyInterestingDecls.front(); PotentiallyInterestingDecls.pop_front(); - if (isConsumerInterestedIn(getContext(), D)) - PassInterestingDeclToConsumer(D); + if (isConsumerInterestedIn(getContext(), D.getDecl(), D.hasPendingBody())) + PassInterestingDeclToConsumer(D.getDecl()); } } @@ -3757,7 +3709,7 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) { // to isConsumerInterestedIn because it is unsafe to call in the // current ASTReader state. bool WasInteresting = - Record.JustLoaded || isConsumerInterestedIn(getContext(), D); + Record.JustLoaded || isConsumerInterestedIn(getContext(), D, false); for (auto &FileAndOffset : UpdateOffsets) { ModuleFile *F = FileAndOffset.first; uint64_t Offset = FileAndOffset.second; @@ -3776,8 +3728,10 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) { // We might have made this declaration interesting. If so, remember that // we need to hand it off to the consumer. - if (!WasInteresting && isConsumerInterestedIn(getContext(), D)) { - PotentiallyInterestingDecls.push_back(D); + if (!WasInteresting && + isConsumerInterestedIn(getContext(), D, Reader.hasPendingBody())) { + PotentiallyInterestingDecls.push_back( + InterestingDecl(D, Reader.hasPendingBody())); WasInteresting = true; } } @@ -4075,6 +4029,12 @@ void ASTDeclReader::UpdateDecl(Decl *D, case UPD_CXX_ADDED_FUNCTION_DEFINITION: { FunctionDecl *FD = cast(D); + if (Reader.PendingBodies[FD]) { + // FIXME: Maybe check for ODR violations. + // It's safe to stop now because this update record is always last. + return; + } + if (Record.readInt()) { // Maintain AST consistency: any later redeclarations of this function // are inline if this one is. (We might have merged another declaration diff --git a/test/Modules/merge-lambdas.cpp b/test/Modules/merge-lambdas.cpp deleted file mode 100644 index 6f243ae54c..0000000000 --- a/test/Modules/merge-lambdas.cpp +++ /dev/null @@ -1,43 +0,0 @@ -// RUN: %clang_cc1 -std=c++11 -emit-llvm-only -fmodules %s - -// PR33924: ensure that we merge together local lambas in multiple definitions -// of the same function. - -#pragma clang module build format -module format {} -#pragma clang module contents -#pragma clang module begin format -struct A { template void doFormat(T &&out) {} }; -template void format(T t) { A().doFormat([]{}); } -#pragma clang module end -#pragma clang module endbuild - -#pragma clang module build foo1 -module foo1 { export * } -#pragma clang module contents -#pragma clang module begin foo1 -#pragma clang module import format -inline void foo1() { - format(0); -} -#pragma clang module end -#pragma clang module endbuild - -#pragma clang module build foo2 -module foo2 { export * } -#pragma clang module contents -#pragma clang module begin foo2 -#pragma clang module import format -inline void foo2() { - format(0); -} -#pragma clang module end -#pragma clang module endbuild - -#pragma clang module import foo1 -#pragma clang module import foo2 - -int main() { - foo1(); - foo2(); -} -- 2.40.0