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.)
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
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
