From d8758315cabbfdea5caa3e25531012d035838129 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 8 May 2014 04:26:47 +0000 Subject: [PATCH] Let ASTReader optionally delete its ASTDeserializationListener. Use this to fix the leak of DeserializedDeclsDumper and DeserializedDeclsChecker in FrontendAction (found by LSan), PR19560. The "delete this" bool is necessary because both PCHGenerator and ASTUnit return the same object from both getDeserializationListener() and getASTMutationListener(), so ASTReader can't just have a unique_ptr. It's also not possible to just let FrontendAction (or CompilerInstance) own these listeners due to lifetime issues (see comments on PR19560). Finally, ASTDeserializationListener can't easily be refcounted, since several of the current listeners are allocated on the stack. Having this bool isn't ideal, but it's a pattern that's used in other places in the codebase too, and it seems better than leaking. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@208277 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Frontend/CompilerInstance.h | 18 +++--- .../ASTDeserializationListener.h | 4 +- include/clang/Serialization/ASTReader.h | 4 +- lib/Frontend/CompilerInstance.cpp | 37 +++++------- lib/Frontend/FrontendAction.cpp | 48 +++++++++------ lib/Serialization/ASTReader.cpp | 60 +++++++++---------- 6 files changed, 88 insertions(+), 83 deletions(-) diff --git a/include/clang/Frontend/CompilerInstance.h b/include/clang/Frontend/CompilerInstance.h index e936fbf23b..97c140ecbd 100644 --- a/include/clang/Frontend/CompilerInstance.h +++ b/include/clang/Frontend/CompilerInstance.h @@ -570,21 +570,19 @@ public: /// Create an external AST source to read a PCH file and attach it to the AST /// context. - void createPCHExternalASTSource(StringRef Path, - bool DisablePCHValidation, + void createPCHExternalASTSource(StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors, - void *DeserializationListener); + void *DeserializationListener, + bool OwnDeserializationListener); /// Create an external AST source to read a PCH file. /// /// \return - The new object on success, or null on failure. - static ExternalASTSource * - createPCHExternalASTSource(StringRef Path, const std::string &Sysroot, - bool DisablePCHValidation, - bool AllowPCHWithCompilerErrors, - Preprocessor &PP, ASTContext &Context, - void *DeserializationListener, bool Preamble, - bool UseGlobalModuleIndex); + static ExternalASTSource *createPCHExternalASTSource( + StringRef Path, const std::string &Sysroot, bool DisablePCHValidation, + bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context, + void *DeserializationListener, bool OwnDeserializationListener, + bool Preamble, bool UseGlobalModuleIndex); /// Create a code completion consumer using the invocation; note that this /// will cause the source manager to truncate the input source file at the diff --git a/include/clang/Serialization/ASTDeserializationListener.h b/include/clang/Serialization/ASTDeserializationListener.h index 0218129fb6..1f61c8454b 100644 --- a/include/clang/Serialization/ASTDeserializationListener.h +++ b/include/clang/Serialization/ASTDeserializationListener.h @@ -27,10 +27,8 @@ class MacroInfo; class Module; class ASTDeserializationListener { -protected: - virtual ~ASTDeserializationListener(); - public: + virtual ~ASTDeserializationListener(); /// \brief The ASTReader was initialized. virtual void ReaderInitialized(ASTReader *Reader) { } diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index 5578e90f13..9ab9fb20bd 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -340,6 +340,7 @@ private: /// \brief The receiver of deserialization events. ASTDeserializationListener *DeserializationListener; + bool OwnsDeserializationListener; SourceManager &SourceMgr; FileManager &FileMgr; @@ -1382,7 +1383,8 @@ public: } /// \brief Set the AST deserialization listener. - void setDeserializationListener(ASTDeserializationListener *Listener); + void setDeserializationListener(ASTDeserializationListener *Listener, + bool TakeOwnership = false); /// \brief Determine whether this AST reader has a global index. bool hasGlobalIndex() const { return (bool)GlobalIndex; } diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp index 4a75186d97..da0ca468cd 100644 --- a/lib/Frontend/CompilerInstance.cpp +++ b/lib/Frontend/CompilerInstance.cpp @@ -307,33 +307,25 @@ void CompilerInstance::createASTContext() { // ExternalASTSource -void CompilerInstance::createPCHExternalASTSource(StringRef Path, - bool DisablePCHValidation, - bool AllowPCHWithCompilerErrors, - void *DeserializationListener){ +void CompilerInstance::createPCHExternalASTSource( + StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors, + void *DeserializationListener, bool OwnDeserializationListener) { IntrusiveRefCntPtr Source; bool Preamble = getPreprocessorOpts().PrecompiledPreambleBytes.first != 0; - Source = createPCHExternalASTSource(Path, getHeaderSearchOpts().Sysroot, - DisablePCHValidation, - AllowPCHWithCompilerErrors, - getPreprocessor(), getASTContext(), - DeserializationListener, - Preamble, - getFrontendOpts().UseGlobalModuleIndex); + Source = createPCHExternalASTSource( + Path, getHeaderSearchOpts().Sysroot, DisablePCHValidation, + AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(), + DeserializationListener, OwnDeserializationListener, Preamble, + getFrontendOpts().UseGlobalModuleIndex); ModuleManager = static_cast(Source.getPtr()); getASTContext().setExternalSource(Source); } -ExternalASTSource * -CompilerInstance::createPCHExternalASTSource(StringRef Path, - const std::string &Sysroot, - bool DisablePCHValidation, - bool AllowPCHWithCompilerErrors, - Preprocessor &PP, - ASTContext &Context, - void *DeserializationListener, - bool Preamble, - bool UseGlobalModuleIndex) { +ExternalASTSource *CompilerInstance::createPCHExternalASTSource( + StringRef Path, const std::string &Sysroot, bool DisablePCHValidation, + bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context, + void *DeserializationListener, bool OwnDeserializationListener, + bool Preamble, bool UseGlobalModuleIndex) { HeaderSearchOptions &HSOpts = PP.getHeaderSearchInfo().getHeaderSearchOpts(); std::unique_ptr Reader; @@ -346,7 +338,8 @@ CompilerInstance::createPCHExternalASTSource(StringRef Path, UseGlobalModuleIndex)); Reader->setDeserializationListener( - static_cast(DeserializationListener)); + static_cast(DeserializationListener), + /*TakeOwnership=*/OwnDeserializationListener); switch (Reader->ReadAST(Path, Preamble ? serialization::MK_Preamble : serialization::MK_PCH, diff --git a/lib/Frontend/FrontendAction.cpp b/lib/Frontend/FrontendAction.cpp index 8b05849f90..77eb1a1443 100644 --- a/lib/Frontend/FrontendAction.cpp +++ b/lib/Frontend/FrontendAction.cpp @@ -37,11 +37,16 @@ namespace { class DelegatingDeserializationListener : public ASTDeserializationListener { ASTDeserializationListener *Previous; + bool DeletePrevious; public: explicit DelegatingDeserializationListener( - ASTDeserializationListener *Previous) - : Previous(Previous) { } + ASTDeserializationListener *Previous, bool DeletePrevious) + : Previous(Previous), DeletePrevious(DeletePrevious) {} + virtual ~DelegatingDeserializationListener() { + if (DeletePrevious) + delete Previous; + } void ReaderInitialized(ASTReader *Reader) override { if (Previous) @@ -74,8 +79,9 @@ public: /// \brief Dumps deserialized declarations. class DeserializedDeclsDumper : public DelegatingDeserializationListener { public: - explicit DeserializedDeclsDumper(ASTDeserializationListener *Previous) - : DelegatingDeserializationListener(Previous) { } + explicit DeserializedDeclsDumper(ASTDeserializationListener *Previous, + bool DeletePrevious) + : DelegatingDeserializationListener(Previous, DeletePrevious) {} void DeclRead(serialization::DeclID ID, const Decl *D) override { llvm::outs() << "PCH DECL: " << D->getDeclKindName(); @@ -96,9 +102,10 @@ class DeserializedDeclsChecker : public DelegatingDeserializationListener { public: DeserializedDeclsChecker(ASTContext &Ctx, const std::set &NamesToCheck, - ASTDeserializationListener *Previous) - : DelegatingDeserializationListener(Previous), - Ctx(Ctx), NamesToCheck(NamesToCheck) { } + ASTDeserializationListener *Previous, + bool DeletePrevious) + : DelegatingDeserializationListener(Previous, DeletePrevious), Ctx(Ctx), + NamesToCheck(NamesToCheck) {} void DeclRead(serialization::DeclID ID, const Decl *D) override { if (const NamedDecl *ND = dyn_cast(D)) @@ -320,17 +327,24 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI, assert(hasPCHSupport() && "This action does not have PCH support!"); ASTDeserializationListener *DeserialListener = Consumer->GetASTDeserializationListener(); - if (CI.getPreprocessorOpts().DumpDeserializedPCHDecls) - DeserialListener = new DeserializedDeclsDumper(DeserialListener); - if (!CI.getPreprocessorOpts().DeserializedPCHDeclsToErrorOn.empty()) - DeserialListener = new DeserializedDeclsChecker(CI.getASTContext(), - CI.getPreprocessorOpts().DeserializedPCHDeclsToErrorOn, - DeserialListener); + bool DeleteDeserialListener = false; + if (CI.getPreprocessorOpts().DumpDeserializedPCHDecls) { + DeserialListener = new DeserializedDeclsDumper(DeserialListener, + DeleteDeserialListener); + DeleteDeserialListener = true; + } + if (!CI.getPreprocessorOpts().DeserializedPCHDeclsToErrorOn.empty()) { + DeserialListener = new DeserializedDeclsChecker( + CI.getASTContext(), + CI.getPreprocessorOpts().DeserializedPCHDeclsToErrorOn, + DeserialListener, DeleteDeserialListener); + DeleteDeserialListener = true; + } CI.createPCHExternalASTSource( - CI.getPreprocessorOpts().ImplicitPCHInclude, - CI.getPreprocessorOpts().DisablePCHValidation, - CI.getPreprocessorOpts().AllowPCHWithCompilerErrors, - DeserialListener); + CI.getPreprocessorOpts().ImplicitPCHInclude, + CI.getPreprocessorOpts().DisablePCHValidation, + CI.getPreprocessorOpts().AllowPCHWithCompilerErrors, DeserialListener, + DeleteDeserialListener); if (!CI.getASTContext().getExternalSource()) goto failure; } diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 1e61bd04db..10a9282c98 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -578,9 +578,10 @@ void PCHValidator::ReadCounter(const ModuleFile &M, unsigned Value) { // AST reader implementation //===----------------------------------------------------------------------===// -void -ASTReader::setDeserializationListener(ASTDeserializationListener *Listener) { +void ASTReader::setDeserializationListener(ASTDeserializationListener *Listener, + bool TakeOwnership) { DeserializationListener = Listener; + OwnsDeserializationListener = TakeOwnership; } @@ -8260,39 +8261,38 @@ void ASTReader::pushExternalDeclIntoScope(NamedDecl *D, DeclarationName Name) { } } -ASTReader::ASTReader(Preprocessor &PP, ASTContext &Context, - StringRef isysroot, bool DisableValidation, - bool AllowASTWithCompilerErrors, - bool AllowConfigurationMismatch, - bool ValidateSystemInputs, +ASTReader::ASTReader(Preprocessor &PP, ASTContext &Context, StringRef isysroot, + bool DisableValidation, bool AllowASTWithCompilerErrors, + bool AllowConfigurationMismatch, bool ValidateSystemInputs, bool UseGlobalIndex) - : Listener(new PCHValidator(PP, *this)), DeserializationListener(0), - SourceMgr(PP.getSourceManager()), FileMgr(PP.getFileManager()), - Diags(PP.getDiagnostics()), SemaObj(0), PP(PP), Context(Context), - Consumer(0), ModuleMgr(PP.getFileManager()), - isysroot(isysroot), DisableValidation(DisableValidation), - AllowASTWithCompilerErrors(AllowASTWithCompilerErrors), - AllowConfigurationMismatch(AllowConfigurationMismatch), - ValidateSystemInputs(ValidateSystemInputs), - UseGlobalIndex(UseGlobalIndex), TriedLoadingGlobalIndex(false), - CurrentGeneration(0), CurrSwitchCaseStmts(&SwitchCaseStmts), - NumSLocEntriesRead(0), TotalNumSLocEntries(0), - NumStatementsRead(0), TotalNumStatements(0), NumMacrosRead(0), - TotalNumMacros(0), NumIdentifierLookups(0), NumIdentifierLookupHits(0), - NumSelectorsRead(0), NumMethodPoolEntriesRead(0), - NumMethodPoolLookups(0), NumMethodPoolHits(0), - NumMethodPoolTableLookups(0), NumMethodPoolTableHits(0), - TotalNumMethodPoolEntries(0), - NumLexicalDeclContextsRead(0), TotalLexicalDeclContexts(0), - NumVisibleDeclContextsRead(0), TotalVisibleDeclContexts(0), - TotalModulesSizeInBits(0), NumCurrentElementsDeserializing(0), - PassingDeclsToConsumer(false), - NumCXXBaseSpecifiersLoaded(0), ReadingKind(Read_None) -{ + : Listener(new PCHValidator(PP, *this)), DeserializationListener(0), + OwnsDeserializationListener(false), SourceMgr(PP.getSourceManager()), + FileMgr(PP.getFileManager()), Diags(PP.getDiagnostics()), SemaObj(0), + PP(PP), Context(Context), Consumer(0), ModuleMgr(PP.getFileManager()), + isysroot(isysroot), DisableValidation(DisableValidation), + AllowASTWithCompilerErrors(AllowASTWithCompilerErrors), + AllowConfigurationMismatch(AllowConfigurationMismatch), + ValidateSystemInputs(ValidateSystemInputs), + UseGlobalIndex(UseGlobalIndex), TriedLoadingGlobalIndex(false), + CurrentGeneration(0), CurrSwitchCaseStmts(&SwitchCaseStmts), + NumSLocEntriesRead(0), TotalNumSLocEntries(0), NumStatementsRead(0), + TotalNumStatements(0), NumMacrosRead(0), TotalNumMacros(0), + NumIdentifierLookups(0), NumIdentifierLookupHits(0), NumSelectorsRead(0), + NumMethodPoolEntriesRead(0), NumMethodPoolLookups(0), + NumMethodPoolHits(0), NumMethodPoolTableLookups(0), + NumMethodPoolTableHits(0), TotalNumMethodPoolEntries(0), + NumLexicalDeclContextsRead(0), TotalLexicalDeclContexts(0), + NumVisibleDeclContextsRead(0), TotalVisibleDeclContexts(0), + TotalModulesSizeInBits(0), NumCurrentElementsDeserializing(0), + PassingDeclsToConsumer(false), NumCXXBaseSpecifiersLoaded(0), + ReadingKind(Read_None) { SourceMgr.setExternalSLocEntrySource(this); } ASTReader::~ASTReader() { + if (OwnsDeserializationListener) + delete DeserializationListener; + for (DeclContextVisibleUpdatesPending::iterator I = PendingVisibleUpdates.begin(), E = PendingVisibleUpdates.end(); -- 2.40.0