On Wed, May 7, 2014 at 7:38 PM, Nico Weber <[email protected]> wrote:
> 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. > …after actually trying this: A ref-counted ptr would mean that the deserialization listeners have to be allocated on the heap, and many of them currently aren't. > > >> >> > >> > 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
