<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 08.08.2014 2:00, Sean Silva wrote:<br>
</div>
<blockquote
cite="mid:CAHnXoanQprOKK7GyffyaS7yziUMr5CbiPcyrvL5t6fnd2Zo2Hw@mail.gmail.com"
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 moz-do-not-send="true"
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
cite="mid:CAHnXoanQprOKK7GyffyaS7yziUMr5CbiPcyrvL5t6fnd2Zo2Hw@mail.gmail.com"
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 class="HOEnZb">
<div class="h5"><br>
On Thu, Aug 7, 2014 at 1:56 PM, Sean Silva <<a
moz-do-not-send="true"
href="mailto:chisophugis@gmail.com">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 moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">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 moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">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 moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">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 moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">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
moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">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
moz-do-not-send="true"
href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>>>>>>>> <a
moz-do-not-send="true"
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 class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>