<p dir="ltr">FWIW I hadn't actually signed off on this as such, but no worries - not a big deal.</p>
<p dir="ltr">Its a pity our xfail support doesn't produce xpass results (that count as failures) when the tests fail to fail...</p>
<p dir="ltr">Assuming your changes made some of these tests not leak? Or are there more, pervasive, leaks that are still causing all of these to fail? It'd be nice to be able to measure the improvement and hold the line if we could...</p>
<div class="gmail_quote">On Aug 7, 2014 5:41 PM, "Anton Yartsev" <<a href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div>On 08.08.2014 2:00, Sean Silva wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">On Thu, Aug 7, 2014 at 2:05 PM, David
Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Is there
a simple valgrind invocation (or test suite with --vg<br>
--vg-leak, or whatever the incantations are) I can run to
verify the<br>
leaks and fixes you've provided?<br>
</blockquote>
<div><br>
</div>
<div>IIRC many of the tblgen tests have `XFAIL: vg_leak`, so
a vg_leak build supposedly should catch this. I'm sure
asan/leaksan can find these too.</div>
</div>
</div>
</div>
</blockquote>
Yes, 52 tests in \test\TableGen have 'XFAIL: vg_leak'.<br>
Committed the patch as r215176.<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>-- Sean Silva</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I'm ambivalent about the two options (haven't looked
enough at the<br>
unique/shared_ptr one to compare it to the current patch
of just<br>
adding delete) though I kind of like the idea of making
things<br>
explicit even/because it makes them uglier and makes the
badness<br>
visible so someone is more strongly reminded/incentivized
to fix it...<br>
but I'm OK with just adding delete for now, it's still
leaves a bit of<br>
a red flag for the next person to see/consider fixing...<br>
<div>
<div><br>
On Thu, Aug 7, 2014 at 1:56 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>>
wrote:<br>
> This is ugly but I think it is clearer than all
the other patches. All the<br>
> naked delete's are a red flag for future
developers. Eventually as this code<br>
> is refactored they will go away (also they can
*guide* future refactoring).<br>
> At this point, it seems like a nontrivial
refactoring is necessary to get<br>
> the ownership here under control, so I think this
patch makes sense for now.<br>
> LGTM. Please wait for David to give his LGTM as
well.<br>
><br>
> -- Sean Silva<br>
><br>
><br>
> On Wed, Aug 6, 2014 at 6:24 PM, Anton Yartsev
<<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On 07.08.2014 4:46, Sean Silva wrote:<br>
>><br>
>><br>
>><br>
>><br>
>> On Wed, Aug 6, 2014 at 5:12 PM, Anton Yartsev
<<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>> On 07.08.2014 1:01, David Blaikie wrote:<br>
>>>><br>
>>>> On Wed, Aug 6, 2014 at 1:46 PM, Anton
Yartsev <<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>> On 06.08.2014 20:39, David
Blaikie wrote:<br>
>>>>>><br>
>>>>>> On Tue, Aug 5, 2014 at 8:50
PM, Anton Yartsev<br>
>>>>>> <<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>><br>
>>>>>> wrote:<br>
>>>>>>><br>
>>>>>>> Got it. Attached is an
updated patch. First tried to use unique_ptr<br>
>>>>>>> but<br>
>>>>>>> soon<br>
>>>>>>> realized that shared_ptr
is more suitable here.<br>
>>>>>><br>
>>>>>> Hmm - let's have a bit of a
chat about this (shared_ptr usually makes<br>
>>>>>> me twitch a bit - I strongly
prefer simpler ownership (as it makes the<br>
>>>>>> code easier to understand
where it can be applied).<br>
>>>>>><br>
>>>>>> What makes shared_ptr more
suitable? From what I could see in your<br>
>>>>>> first (and this) patch the
code looked something like this:<br>
>>>>>><br>
>>>>>> owner = new T;<br>
>>>>>> container.add(owner); // pass
ownership here)<br>
>>>>>> process(owner); // do stuff
with the T, but the container will<br>
>>>>>> guarantee its survival here,
this isn't shared ownership<br>
>>>>>><br>
>>>>>> If the shared_ptrs that are
outside the container never themselves<br>
>>>>>> result in the object being
destroyed (eg: only during the the<br>
>>>>>> destruction of the
shared_ptrs in the container are the T objects ever<br>
>>>>>> destroyed) then I don't think
this is shared ownership.<br>
>>>>><br>
>>>>><br>
>>>>> I found shared_ptr handy for
situation when the ownership is not<br>
>>>>> guarantee<br>
>>>>> to be transferred.<br>
>>>>><br>
>>>>> owner = new T;<br>
>>>>> if (/*condition*/)<br>
>>>>> container.add(owner); // pass
ownership here<br>
>>>>> process(owner);<br>
>>>><br>
>>>> It still seems like there's a single
owner for this object at any<br>
>>>> given point - so using shared_ptr
seems misleading to me.<br>
>>>><br>
>>>> Also - where does this conditional
ownership passing crop-up in your<br>
>>>> patch? (just so I can more easily go
& have a look at it in-situ)<br>
>>><br>
>>> Look at bool
TGParser::ParseDef(MultiClass *CurMultiClass);
Attached is a<br>
>>> full diff.<br>
>>> Moreover, with solution that uses
unique_ptr an additional flag (or other<br>
>>> trick) is required to indicate if the
ownership was transferred or not to<br>
>>> know if we should free the memory at
returns.<br>
>>> It seems that it is better just to
manually call delete where needed.<br>
>>> Should I move in this direction?<br>
>><br>
>><br>
>> I'm starting to think that just adding the
necessary delete calls is the<br>
>> most pragmatic thing to do (how many are
needed?). It seems like a more<br>
>> thorough refactoring of the ownership here is
needed that goes beyond the<br>
>> scope of fixing the leaks here.<br>
>><br>
>> Adding the raw delete calls is also at least
in line with the philosophy<br>
>> of "every naked new and delete is a red
flag", clearly indicating where the<br>
>> ownership situation needs to be improved for
future developers.<br>
>><br>
>> Attached is the patch with the raw delete
calls added. Just here appears<br>
>> the flag indicating whether the ownership was
transferred or not..<br>
>><br>
>><br>
>><br>
>> -- Sean Silva<br>
>><br>
>>><br>
>>><br>
>>>><br>
>>>>> Using smart pointer with
reference counter here just simply guarantee<br>
>>>>> the<br>
>>>>> memory to be released if the
ownership was not taken. With shared_ptr<br>
>>>>> it is<br>
>>>>> also no need to involve
additional variable for local usage what<br>
>>>>> simplifies<br>
>>>>> the code IMHO.<br>
>>>><br>
>>>> It simplifies it in some ways but
complicates it in others.<br>
>>>> Personally, I'd avoid this use of
shared_ptr, but I imagine opinions<br>
>>>> vary on this so don't necessarily
take my word for it.<br>
>>>><br>
>>>> - David<br>
>>>><br>
>>>>><br>
>>>>>><br>
>>>>>> I'll followup to Sean's reply
with some more specific thoughts there.<br>
>>>>>><br>
>>>>>>>> Personally I'd favor:<br>
>>>>>>>><br>
>>>>>>>> auto foo =
llvm::make_unique<T>(...);<br>
>>>>>>>> foo =
llvm::make_unique<T>(...);<br>
>>>>>>>><br>
>>>>>>>> over:<br>
>>>>>>>><br>
>>>>>>>>
std::unique_ptr<T> foo(new T(...));<br>
>>>>>>>> foo.reset(new
T(...));<br>
>>>>>>>><br>
>>>>>>>> Though opinions may
vary there.<br>
>>>>>>>><br>
>>>>>>>> I'd probably consider
renaming your "CurRecLocalView" to "CurRec" so<br>
>>>>>>>> that most of the code
doesn't need to change (and CurRecUniquePtr<br>
>>>>>>>> maybe "CurRecOwner"?)<br>
>>>>>>>><br>
>>>>>>>> Also in a
preliminary/prior patch, it might be worth changing<br>
>>>>>>>> "addDef"<br>
>>>>>>>> to take a unique_ptr
by value to properly document the ownershp<br>
>>>>>>>> passing (I'm hoping
to keep focusing on any 'release()' calls and<br>
>>>>>>>> use<br>
>>>>>>>> them as a marker of
places to clean up/continue pushing unique_ptr<br>
>>>>>>>> through).<br>
>>>>>>>><br>
>>>>>>>> On Tue, Aug 5, 2014
at 12:20 PM, Anton Yartsev<br>
>>>>>>>> <<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>><br>
>>>>>>>> wrote:<br>
>>>>>>>>><br>
>>>>>>>>> Hi all,<br>
>>>>>>>>><br>
>>>>>>>>> I've recently ran
the alpha.cplusplus.NewDeleteLeaks checker over<br>
>>>>>>>>> the<br>
>>>>>>>>> LLVM<br>
>>>>>>>>> codebase to
eliminate false-positives. The checker evolved a
number<br>
>>>>>>>>> of<br>
>>>>>>>>> real<br>
>>>>>>>>> leaks. I've
decided to fix some of them.<br>
>>>>>>>>> Attached is the
patch that eliminates memory leaks found in the<br>
>>>>>>>>> TGParser.cpp.
Please review.<br>
>>>>>>>>><br>
>>>>>>>>> --<br>
>>>>>>>>> Anton<br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>>
_______________________________________________<br>
>>>>>>>>> llvm-commits
mailing list<br>
>>>>>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>>>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>>>>>><br>
>>>>>>> --<br>
>>>>>>> Anton<br>
>>>>>>><br>
>>>>><br>
>>>>> --<br>
>>>>> Anton<br>
>>>>><br>
>>><br>
>>><br>
>>> --<br>
>>> Anton<br>
>>><br>
>><br>
>><br>
>><br>
>> --<br>
>> Anton<br>
><br>
><br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
<br>
<pre cols="72">--
Anton</pre>
</div>
</blockquote></div>