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