On May 7, 2014, at 11:56 AM, Nico Weber <[email protected]> wrote: > Here's a patch that fixes the leak. The approach is to give ASTReader > a DeleteListener bool, since different codepaths need different > ownership semantics. (I explored two other options that didn't work > out, see comments on PR19560.)
Did you consider using a ref-counted pointer ? > > Can people live with this patch? > > On Sat, May 3, 2014 at 10:01 AM, Argyrios Kyrtzidis <[email protected]> wrote: >> On May 3, 2014, at 3:55 AM, Chandler Carruth <[email protected]> wrote: >> >> On Sat, May 3, 2014 at 3:20 AM, Chandler Carruth <[email protected]> >> wrote: >>> >>> First off, sorry for the commit necromancy, but I tried to fix a memory >>> leak here and discovered completely broken ownership semantics that really >>> need to be fixed, and need to be fixed by the person who really understands >>> the intended design here. >>> >>> On Thu, Oct 14, 2010 at 1:14 PM, Argyrios Kyrtzidis <[email protected]> >>> wrote: >>>> >>>> Author: akirtzidis >>>> Date: Thu Oct 14 15:14:18 2010 >>>> New Revision: 116503 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=116503&view=rev >>>> Log: >>>> Introduce command line option -dump-deserialized-decls which is used to >>>> print the PCH decls that got deserialized, for testing purposes. >>> >>> >>> <snip> >>> >>>> >>>> Modified: cfe/trunk/lib/Frontend/FrontendAction.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/FrontendAction.cpp?rev=116503&r1=116502&r2=116503&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/lib/Frontend/FrontendAction.cpp (original) >>>> +++ cfe/trunk/lib/Frontend/FrontendAction.cpp Thu Oct 14 15:14:18 2010 >>>> @@ -16,12 +16,43 @@ >>>> #include "clang/Frontend/CompilerInstance.h" >>>> #include "clang/Frontend/FrontendDiagnostic.h" >>>> #include "clang/Parse/ParseAST.h" >>>> +#include "clang/Serialization/ASTDeserializationListener.h" >>>> #include "llvm/Support/MemoryBuffer.h" >>>> #include "llvm/Support/Timer.h" >>>> #include "llvm/Support/ErrorHandling.h" >>>> #include "llvm/Support/raw_ostream.h" >>>> using namespace clang; >>>> >>>> +namespace { >>>> + >>>> +/// \brief Dumps deserialized declarations. >>>> +class DeserializedDeclsDumper : public ASTDeserializationListener { >>>> + ASTDeserializationListener *Previous; >>>> + >>>> +public: >>>> + DeserializedDeclsDumper(ASTDeserializationListener *Previous) >>>> + : Previous(Previous) { } >>>> + >>>> + virtual void DeclRead(serialization::DeclID ID, const Decl *D) { >>>> + llvm::outs() << "PCH DECL: " << D->getDeclKindName(); >>>> + if (const NamedDecl *ND = dyn_cast<NamedDecl>(D)) >>>> + llvm::outs() << " - " << ND->getNameAsString(); >>>> + llvm::outs() << "\n"; >>>> + >>>> + if (Previous) >>>> + Previous->DeclRead(ID, D); >>>> + } >>>> + >>>> + virtual void SetReader(ASTReader *Reader) {} >>>> + virtual void IdentifierRead(serialization::IdentID ID, IdentifierInfo >>>> *II) {} >>>> + virtual void TypeRead(serialization::TypeIdx Idx, QualType T) {} >>>> + virtual void SelectorRead(serialization::SelectorID iD, Selector Sel) >>>> {} >>>> + virtual void MacroDefinitionRead(serialization::MacroID, >>>> + MacroDefinition *MD) {} >>>> +}; >>>> + >>>> +} // end anonymous namespace >>>> + >>>> FrontendAction::FrontendAction() : Instance(0) {} >>>> >>>> FrontendAction::~FrontendAction() {} >>>> @@ -118,11 +149,15 @@ >>>> /// Use PCH? >>>> if (!CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) { >>>> assert(hasPCHSupport() && "This action does not have PCH >>>> support!"); >>>> + ASTDeserializationListener *DeserialListener >>>> + = CI.getInvocation().getFrontendOpts().ChainedPCH ? >>>> + Consumer->GetASTDeserializationListener() : 0; >>>> + if (CI.getPreprocessorOpts().DumpDeserializedPCHDecls) >>>> + DeserialListener = new >>>> DeserializedDeclsDumper(DeserialListener); >>> >>> >>> This listener leaks. So does the DeserializedDeclsChecker that follows the >>> same pattern. LSan catches this. >>> >>> I went to plug the leak by storing the listeners in the FrontendAction if >>> they end up being used, and deallocating them when the action is destroyed. >>> To my great surprise, this caused use-after-free bugs as these listeners are >>> somehow reached *after* the action is destroyed! >>> >>> Could you look into this? It should be pretty easy to reproduce. Forward >>> declare the classes (or make them private classes nested within >>> FrontendAction, or use a raw pointer and manually cast back to the derived >>> type prior to deleting) and add unique_ptrs to the action, reseting them >>> with the new listeners, and just wiring up DeserialListener to the raw >>> pointer side. If you're having trouble reproducing, let me know, and I can >>> send you a nascent patch. >>> >>> -Chandler >> >> >> Just FYI, this is also filed as PR19560. >> >> >> Thanks for bringing this to my attention! Please attach your patch to the >> PR. >> Unfortunately I'm not going to look into this soon, so I suggest disabling >> the test, that uses the flag, for LSan. >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > <clang-deserialisten.patch> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
