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

[Sean Silva]

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.

-- 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140807/0aa9a7aa/attachment.html>