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

[David Blaikie]

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?

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
>
>