I went ahead and landed this patch as-is in r208277 with a somewhat length commit message. I'm happy to address any feedback anyone has after reading that message, just reply to the commit :-)
On Wed, May 7, 2014 at 8:11 PM, Nico Weber <[email protected]> wrote: > 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
