<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<blockquote
cite="mid:CAENS6EtishrrLGCVCL1-XimvxJDy9u7KCE1w5AbM0Hoh4Ze9Qw@mail.gmail.com"
type="cite">
<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>
</blockquote>
Just setup linux and LLVM on vmware to measure improvement with
valgrind and found out that Takumi has already updated tests at
r215445..<br>
There are still 48 failing tests. May deleting the contents of
MultiClass::RecordVector eliminate some of them - sometimes (e.g. in
'bool TGParser::ParseDef(MultiClass *CurMultiClass)') the newly
allocated memory is pushed to RecordVector but I failed to find the
place where RecordVectors contents is deleted. <br>
<blockquote
cite="mid:CAENS6EtishrrLGCVCL1-XimvxJDy9u7KCE1w5AbM0Hoh4Ze9Qw@mail.gmail.com"
type="cite">
<div class="gmail_quote">On Aug 7, 2014 5:41 PM, "Anton Yartsev"
<<a moz-do-not-send="true"
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
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 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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
href="mailto:llvm-commits@cs.uiuc.edu"
target="_blank">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 cols="72">--
Anton</pre>
</div>
</blockquote>
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>