From bce52aed345c52715744ec7aef4942b3a66bfc04 Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Fri, 22 Jul 2016 21:08:24 +0000 Subject: [PATCH] [modules] Teach the ASTWriter to ignore mutations coming from the ASTReader. Processing update records (and loading a module, in general) might trigger unexpected calls to the ASTWriter (being a mutation listener). Now we have a mechanism to suppress those calls to the ASTWriter but notify other possible mutation listeners. Fixes https://llvm.org/bugs/show_bug.cgi?id=28332 Patch by Cristina Cristescu and me. Reviewed by Richard Smith (D21800). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@276473 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Serialization/ASTReader.h | 22 ++++++++++++++++++++ lib/Serialization/ASTReader.cpp | 2 ++ lib/Serialization/ASTReaderDecl.cpp | 13 ++++-------- lib/Serialization/ASTWriter.cpp | 18 +++++++++++++++- test/Modules/Inputs/PR28332/TextualInclude.h | 7 +++++++ test/Modules/Inputs/PR28332/a.h | 8 +++++++ test/Modules/Inputs/PR28332/b.h | 3 +++ test/Modules/Inputs/PR28332/c.h | 2 ++ test/Modules/Inputs/PR28332/module.modulemap | 3 +++ test/Modules/pr28332.cpp | 8 +++++++ 10 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 test/Modules/Inputs/PR28332/TextualInclude.h create mode 100644 test/Modules/Inputs/PR28332/a.h create mode 100644 test/Modules/Inputs/PR28332/b.h create mode 100644 test/Modules/Inputs/PR28332/c.h create mode 100644 test/Modules/Inputs/PR28332/module.modulemap create mode 100644 test/Modules/pr28332.cpp diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index 143314ced6..b931f92570 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -842,6 +842,9 @@ private: /// \brief Whether we have tried loading the global module index yet. bool TriedLoadingGlobalIndex; + ///\brief Whether we are currently processing update records. + bool ProcessingUpdateRecords; + typedef llvm::DenseMap SwitchCaseMapTy; /// \brief Mapping from switch-case IDs in the chain to switch-case statements /// @@ -1041,6 +1044,23 @@ private: ~ReadingKindTracker() { Reader.ReadingKind = PrevKind; } }; + /// \brief RAII object to mark the start of processing updates. + class ProcessingUpdatesRAIIObj { + ASTReader &Reader; + bool PrevState; + + ProcessingUpdatesRAIIObj(const ProcessingUpdatesRAIIObj &) = delete; + void operator=(const ProcessingUpdatesRAIIObj &) = delete; + + public: + ProcessingUpdatesRAIIObj(ASTReader &reader) + : Reader(reader), PrevState(Reader.ProcessingUpdateRecords) { + Reader.ProcessingUpdateRecords = true; + } + + ~ProcessingUpdatesRAIIObj() { Reader.ProcessingUpdateRecords = PrevState; } + }; + /// \brief Suggested contents of the predefines buffer, after this /// PCH file has been processed. /// @@ -2129,6 +2149,8 @@ public: /// \brief Loads comments ranges. void ReadComments() override; + + bool isProcessingUpdateRecords() { return ProcessingUpdateRecords; } }; /// \brief Helper class that saves the current stream position and diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 6bfae07b82..27b7048f37 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -8644,6 +8644,7 @@ void ASTReader::FinishedDeserializing() { auto Updates = std::move(PendingExceptionSpecUpdates); PendingExceptionSpecUpdates.clear(); for (auto Update : Updates) { + ProcessingUpdatesRAIIObj ProcessingUpdates(*this); auto *FPT = Update.second->getType()->castAs(); auto ESI = FPT->getExtProtoInfo().ExceptionSpec; if (auto *Listener = Context.getASTMutationListener()) @@ -8714,6 +8715,7 @@ ASTReader::ASTReader( AllowConfigurationMismatch(AllowConfigurationMismatch), ValidateSystemInputs(ValidateSystemInputs), UseGlobalIndex(UseGlobalIndex), TriedLoadingGlobalIndex(false), + ProcessingUpdateRecords(false), CurrSwitchCaseStmts(&SwitchCaseStmts), NumSLocEntriesRead(0), TotalNumSLocEntries(0), NumStatementsRead(0), TotalNumStatements(0), NumMacrosRead(0), TotalNumMacros(0), NumIdentifierLookups(0), diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 184e7efe84..b5384bbc39 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -3489,6 +3489,7 @@ void ASTReader::loadDeclUpdateRecords(serialization::DeclID ID, Decl *D) { // The declaration may have been modified by files later in the chain. // If this is the case, read the record containing the updates from each file // and pass it to ASTDeclReader to make the modifications. + ProcessingUpdatesRAIIObj ProcessingUpdates(*this); DeclUpdateOffsetsMap::iterator UpdI = DeclUpdateOffsets.find(ID); if (UpdI != DeclUpdateOffsets.end()) { auto UpdateOffsets = std::move(UpdI->second); @@ -3907,11 +3908,8 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile, } case UPD_DECL_MARKED_USED: { - // FIXME: This doesn't send the right notifications if there are - // ASTMutationListeners other than an ASTWriter. - // Maintain AST consistency: any later redeclarations are used too. - D->setIsUsed(); + D->markUsed(Reader.Context); break; } @@ -3935,11 +3933,8 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile, Exported = TD->getDefinition(); Module *Owner = SubmoduleID ? Reader.getSubmodule(SubmoduleID) : nullptr; if (Reader.getContext().getLangOpts().ModulesLocalVisibility) { - // FIXME: This doesn't send the right notifications if there are - // ASTMutationListeners other than an ASTWriter. - Reader.getContext().mergeDefinitionIntoModule( - cast(Exported), Owner, - /*NotifyListeners*/ false); + Reader.getContext().mergeDefinitionIntoModule(cast(Exported), + Owner); Reader.PendingMergedDefinitionsToDeduplicate.insert( cast(Exported)); } else if (Owner && Owner->NameVisibility != Module::AllVisible) { diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp index dbba5fd191..9a1e4f6778 100644 --- a/lib/Serialization/ASTWriter.cpp +++ b/lib/Serialization/ASTWriter.cpp @@ -5657,6 +5657,7 @@ void ASTWriter::ModuleRead(serialization::SubmoduleID ID, Module *Mod) { } void ASTWriter::CompletedTagDefinition(const TagDecl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(D->isCompleteDefinition()); assert(!WritingAST && "Already writing the AST!"); if (auto *RD = dyn_cast(D)) { @@ -5683,7 +5684,8 @@ static bool isImportedDeclContext(ASTReader *Chain, const Decl *D) { } void ASTWriter::AddedVisibleDecl(const DeclContext *DC, const Decl *D) { - assert(DC->isLookupContext() && + if (Chain && Chain->isProcessingUpdateRecords()) return; + assert(DC->isLookupContext() && "Should not add lookup results to non-lookup contexts!"); // TU is handled elsewhere. @@ -5717,6 +5719,7 @@ void ASTWriter::AddedVisibleDecl(const DeclContext *DC, const Decl *D) { } void ASTWriter::AddedCXXImplicitMember(const CXXRecordDecl *RD, const Decl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(D->isImplicit()); // We're only interested in cases where a local declaration is added to an @@ -5734,6 +5737,7 @@ void ASTWriter::AddedCXXImplicitMember(const CXXRecordDecl *RD, const Decl *D) { } void ASTWriter::ResolvedExceptionSpec(const FunctionDecl *FD) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!DoneWritingDeclsAndTypes && "Already done writing updates!"); if (!Chain) return; Chain->forEachImportedKeyDecl(FD, [&](const Decl *D) { @@ -5748,6 +5752,7 @@ void ASTWriter::ResolvedExceptionSpec(const FunctionDecl *FD) { } void ASTWriter::DeducedReturnType(const FunctionDecl *FD, QualType ReturnType) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!Chain) return; Chain->forEachImportedKeyDecl(FD, [&](const Decl *D) { @@ -5758,6 +5763,7 @@ void ASTWriter::DeducedReturnType(const FunctionDecl *FD, QualType ReturnType) { void ASTWriter::ResolvedOperatorDelete(const CXXDestructorDecl *DD, const FunctionDecl *Delete) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); assert(Delete && "Not given an operator delete"); if (!Chain) return; @@ -5767,6 +5773,7 @@ void ASTWriter::ResolvedOperatorDelete(const CXXDestructorDecl *DD, } void ASTWriter::CompletedImplicitDefinition(const FunctionDecl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!D->isFromASTFile()) return; // Declaration not imported from PCH. @@ -5776,6 +5783,7 @@ void ASTWriter::CompletedImplicitDefinition(const FunctionDecl *D) { } void ASTWriter::FunctionDefinitionInstantiated(const FunctionDecl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!D->isFromASTFile()) return; @@ -5784,6 +5792,7 @@ void ASTWriter::FunctionDefinitionInstantiated(const FunctionDecl *D) { } void ASTWriter::StaticDataMemberInstantiated(const VarDecl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!D->isFromASTFile()) return; @@ -5796,6 +5805,7 @@ void ASTWriter::StaticDataMemberInstantiated(const VarDecl *D) { } void ASTWriter::DefaultArgumentInstantiated(const ParmVarDecl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!D->isFromASTFile()) return; @@ -5806,6 +5816,7 @@ void ASTWriter::DefaultArgumentInstantiated(const ParmVarDecl *D) { void ASTWriter::AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD, const ObjCInterfaceDecl *IFD) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!IFD->isFromASTFile()) return; // Declaration not imported from PCH. @@ -5816,6 +5827,7 @@ void ASTWriter::AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD, } void ASTWriter::DeclarationMarkedUsed(const Decl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); // If there is *any* declaration of the entity that's not from an AST file, @@ -5829,6 +5841,7 @@ void ASTWriter::DeclarationMarkedUsed(const Decl *D) { } void ASTWriter::DeclarationMarkedOpenMPThreadPrivate(const Decl *D) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!D->isFromASTFile()) return; @@ -5838,6 +5851,7 @@ void ASTWriter::DeclarationMarkedOpenMPThreadPrivate(const Decl *D) { void ASTWriter::DeclarationMarkedOpenMPDeclareTarget(const Decl *D, const Attr *Attr) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!D->isFromASTFile()) return; @@ -5847,6 +5861,7 @@ void ASTWriter::DeclarationMarkedOpenMPDeclareTarget(const Decl *D, } void ASTWriter::RedefinedHiddenDefinition(const NamedDecl *D, Module *M) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); assert(D->isHidden() && "expected a hidden declaration"); DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_EXPORTED, M)); @@ -5854,6 +5869,7 @@ void ASTWriter::RedefinedHiddenDefinition(const NamedDecl *D, Module *M) { void ASTWriter::AddedAttributeToRecord(const Attr *Attr, const RecordDecl *Record) { + if (Chain && Chain->isProcessingUpdateRecords()) return; assert(!WritingAST && "Already writing the AST!"); if (!Record->isFromASTFile()) return; diff --git a/test/Modules/Inputs/PR28332/TextualInclude.h b/test/Modules/Inputs/PR28332/TextualInclude.h new file mode 100644 index 0000000000..e4d2580230 --- /dev/null +++ b/test/Modules/Inputs/PR28332/TextualInclude.h @@ -0,0 +1,7 @@ +#ifndef LLVM_ADT_SMALLVECTORIMPL_H +#define LLVM_ADT_SMALLVECTORIMPL_H +class SmallVectorImpl { +public: + ~SmallVectorImpl(); +}; +#endif \ No newline at end of file diff --git a/test/Modules/Inputs/PR28332/a.h b/test/Modules/Inputs/PR28332/a.h new file mode 100644 index 0000000000..1dc96c80fc --- /dev/null +++ b/test/Modules/Inputs/PR28332/a.h @@ -0,0 +1,8 @@ +#include "b.h" + +class A { + SmallVector LegalIntWidths; + A() {} +}; + +#include "c.h" diff --git a/test/Modules/Inputs/PR28332/b.h b/test/Modules/Inputs/PR28332/b.h new file mode 100644 index 0000000000..e1e07e8920 --- /dev/null +++ b/test/Modules/Inputs/PR28332/b.h @@ -0,0 +1,3 @@ +#include "TextualInclude.h" +template class SmallVector : SmallVectorImpl {}; + diff --git a/test/Modules/Inputs/PR28332/c.h b/test/Modules/Inputs/PR28332/c.h new file mode 100644 index 0000000000..e18bdaccca --- /dev/null +++ b/test/Modules/Inputs/PR28332/c.h @@ -0,0 +1,2 @@ +#include "TextualInclude.h" + diff --git a/test/Modules/Inputs/PR28332/module.modulemap b/test/Modules/Inputs/PR28332/module.modulemap new file mode 100644 index 0000000000..8c3f4ecab4 --- /dev/null +++ b/test/Modules/Inputs/PR28332/module.modulemap @@ -0,0 +1,3 @@ +module "c.h" { header "c.h" export * } +module "b.h" { header "b.h" export * } +module "a.h" { header "a.h" } diff --git a/test/Modules/pr28332.cpp b/test/Modules/pr28332.cpp new file mode 100644 index 0000000000..596dd24600 --- /dev/null +++ b/test/Modules/pr28332.cpp @@ -0,0 +1,8 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/PR28332 -verify %s +// RUN: %clang_cc1 -std=c++11 -fmodules -fmodule-map-file=%S/Inputs/PR28332/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28332 -verify %s + +#include "a.h" + +// expected-no-diagnostics + -- 2.40.0