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

[Anton Yartsev]

On 06.08.2014 20:46, David Blaikie wrote:
> On Tue, Aug 5, 2014 at 6:04 PM, Sean Silva <chisophugis at gmail.com> wrote:
>> Thanks Anton for checking out these leaks.
>>
>>
>> On Tue, Aug 5, 2014 at 3:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> 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 think this suggestion is quite a bit cleaner.
>>
>>> I'd probably consider renaming your "CurRecLocalView" to "CurRec" so
>>> that most of the code doesn't need to change (and CurRecUniquePtr
>>> maybe "CurRecOwner"?)
>>
>> I found the CurRecLocalView to be confusing. Why is it needed? Adding tons
>> of explicit .get()'s (and .release()) is a nice way to indicate that this
>> code still needs more work to get its ownership situation cleaned up. Hiding
>> that with a local variable just... hides it.
> I don't think it's quite hiding anything, really - so far as I can
> tell from the code we've got some code that boils down to:
>
>    T *t = new T();
>    container.add(t); // ownership passed here
>    process(t); // process doesn't own the T, container owns it over the
> duration of 'process'
>
> Assuming 'process' needs to be called with 't' in the container,
> there's not much to this - you're going to need a non-owning and
> owning reference to T when you transform this into explicit ownership
> code:
>
>    auto t = make_unique<T>();
>    T &non_owning_t = *t;
>    container.add(t.release()); // or 'container.add(std::move(t))' if
> you fix container::add's ownership passing to be explicit
>    process(&t); // or fix 'process' to take T& instead of T*, if
> possible (if it doesn't need optionality, etc) so it's just
> 'process(t)'
>
> About the only other way I can imagine this is if you changed
> container.add to return a reference:
>
>    T &t = container.add(make_unique<T>());
>    process(t);
>
> But that only works if you only have one place to make a T, and I
> think this code has several different codepaths - could be factored
> into some factory function, I suppose.
>
>> Is it possible to just use a single variable `std::unique_ptr<Record>
>> CurRec;` and tons of .get()'s and .release()'s? That also ensures a "single
>> point of truth".
> I suspect not, as described above - non-owning uses are required after
> ownership is transferred into a container.
>
> Anton - is this an accurate representation of the ownership in this code?
The problem is in conditional ownership passing. The code should be 
refactored so that
1) the raw pointer should be accessible for non-owning use after 
ownership is transferred.
2) the memory should be released at all return points if the ownership 
was not transferred.

Neither the solution with the unique_ptr + .get()'s and .release()'s nor 
the solution with returning a reference meet the second requirement.
I think the solution should be no more complex then just adding 'delete' 
where needed.
>
> - David
>
>> -- Sean Silva
>>
>>>
>>> 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
>>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>


-- 
Anton