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

[Sean Silva]

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.

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