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

[Sean Silva]

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/c95045e9/attachment.html>