[PATCH][tablegen] Eliminate memory leaks in TGParser.cpp
Anton Yartsev
anton.yartsev at gmail.com
Wed Aug 13 18:55:53 PDT 2014
> 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...
>
Just setup linux and LLVM on vmware to measure improvement with valgrind
and found out that Takumi has already updated tests at r215445..
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.
> On Aug 7, 2014 5:41 PM, "Anton Yartsev" <anton.yartsev at gmail.com
> <mailto: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
>> <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
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140814/720aa319/attachment.html>
More information about the llvm-commits
mailing list