[PATCH] Fix clang-tidy delete of stack object

[Alexander Kornienko]

On Fri, Nov 14, 2014 at 7:40 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Fri, Nov 14, 2014 at 10:32 AM, Alexander Kornienko <alexfh at google.com>
> wrote:
>
>>
>>
>> On Thu, Nov 13, 2014 at 6:39 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Thu, Nov 13, 2014 at 1:20 AM, Manuel Klimek <klimek at google.com>
>>> wrote:
>>>
>>>> +dblakie
>>>>
>>>> On Thu Nov 13 2014 at 1:53:39 AM Alexander Kornienko <alexfh at google.com>
>>>> wrote:
>>>>
>>>>> Thank you for the analysis and the proposed solution!
>>>>>
>>>>> I can reproduce the issue (with any q.cpp that is not clang-tidy
>>>>> clean):
>>>>>
>>>>> $ clang-tidy q.cpp -- --serialize-diagnostics test.dia
>>>>> *** Error in `clang_tidy': free(): invalid pointer: 0x00007fffa65bb4d8
>>>>> ***
>>>>> Aborted (core dumped)
>>>>>
>>>>>
>>>>> The patch seems correct to me and the way to distinguish between
>>>>> owning and non-owning constructors seems also fine. I'll commit the patch
>>>>> tomorrow if nobody offers a better solution.
>>>>>
>>>>
>>>> I don't really see anything better under the current restrictions.
>>>> Perhaps David has an idea, he has done a lot of the unique_ptr migrations
>>>> in llvm.
>>>>
>>>
>>> At a cursory glance, this is the "conditional ownership" issue that's
>>> come up in a few places (and currently we have solutions that both look
>>> like this one (T*+unique_ptr<T> where the latter may be null but otherwise
>>> they both point to the same object) or bool+T* where the bool indicates
>>> ownership)
>>>
>>> There is a thread on llvm/cfe-dev about whether we should introduce a
>>> reusable "conditional ownership" pointer, but the response from several
>>> people (Manuel, Chandler, and, depending on the day of the week, myself,
>>> etc) is that this kind of ownership complication is a bug in the design
>>> which we should fix at the source.
>>>
>>> I'm still not sure if that's the case (that all cases of conditional
>>> ownership like this are design bugs) - but I'm sort of curious to see how
>>> they would look if we really tried to apply that logic.
>>>
>>> As a side note: this patch looks way too subtle/dangerous as-is, even
>>> given the necessary conditional ownership semantics. Two branches of the
>>> if, one calls func(takeX()) the other calls func(unique_ptr<T>(takeX()) -
>>> that's pretty subtle (even though the "ownsClient" condition demonstrates
>>> what's going on there).
>>>
>>> I'm not sure how much it's worth making this a bit tidier/more reliable
>>> (maybe Diags::takeClient should return a unique_ptr and just return null
>>> whenever !Diags.ownsClient() - and have a separate "getClient" function
>>> that can be called to get a raw pointer, regardless of ownership (careful
>>> if we have an ordering issue there - if takeClient nulls out the Diags'
>>> client, then we'd need to call 'get' before 'take', if takeClient just sets
>>> "OwnsClient" to false, then we can call them in either order)) - or if it's
>>> just going to be a bit lame until we go the whole way and remove the
>>> conditional ownership of the DiagnosticConsumer all the way up (or add a
>>> first class maybe-owning pointer type).
>>>
>>
>> Yeah, there are too many possible options here and ideally, it would be
>> nice to unify all cases of conditional ownership or eliminate them. I
>> suppose, that the latter may be rather difficult to make as the scope of
>> the change may be wide and affect many public interfaces. But in any case,
>> we need a centralized decision on what we want to do with the conditional
>> ownership.
>>
>
> Yep :s
>
>
>> As for this patch, it should use the already existing getClient()
>> function in the non-owning branch. I can commit a fix.
>>
>
> Yeah, I think that'd help a lot.
>

Committed the take -> get fix in r222130.

>
> (if you're feeling inclined - could you also make "takeClient" return by
> unique_ptr if that seems to fit (which I really hope it does)?)
>

Sent you http://reviews.llvm.org/D6294.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141117/e864b97d/attachment.html>