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

[Anton Yartsev]

On 07.08.2014 4:33, Anton Yartsev wrote:
> 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.
Sorry, I was confused. There is no need to free the memory if a shared 
pointer is used and the ownership was not transferred...
>>
>> - 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