[PATCH][tablegen] Eliminate memory leaks in TGParser.cpp

[David Blaikie]

FWIW I hadn't actually signed off on this as such, but no worries - not a
big deal.

Its a pity our xfail support doesn't produce xpass results (that count as
failures) when the tests fail to fail...

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...
On Aug 7, 2014 5:41 PM, "Anton Yartsev" <anton.yartsev at gmail.com> wrote:

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