On Wed, May 7, 2014 at 6:34 PM, Argyrios Kyrtzidis <[email protected]>wrote:
> 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 ? > Sure, if you prefer that that'd do the trick too. It seemed more intrusive to me. > > > > > 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
