[PATCH][tablegen] Eliminate memory leaks in TGParser.cpp
Anton Yartsev
anton.yartsev at gmail.com
Thu Aug 7 17:41:22 PDT 2014
On 08.08.2014 2:00, Sean Silva wrote:
>
>
>
> On Thu, Aug 7, 2014 at 2:05 PM, David Blaikie <dblaikie at gmail.com
> <mailto: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
> <mailto: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 <mailto: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 <mailto: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 <mailto: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 <mailto: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 <mailto: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 <mailto: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/20140808/63f4ede2/attachment.html>
More information about the llvm-commits
mailing list