[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

On Mon, Jul 28, 2014 at 4:01 PM, Manuel Klimek <klimek at google.com> wrote:

> On Mon, Jul 28, 2014 at 3:43 PM, Manuel Klimek <klimek at google.com> wrote:
>
>> Some more context:
>> It seems like we already need to do that when building the CFG: for the
>> lifetime extended object we must not emit the destructor at the end of the
>> full expression, which we currently do.
>>
>
> And it looks like we're already trying to do that by handing
> BindToTemporary down. I'm investigating some more...
>

Found bug, separate patch sent (although right now I think it's also still
wrong).
I think we need to fix how we handle lifetime extended temporaries before
we can really do anything here - the main bug in lifetime extended
temporary handling is in connecting the MaterializeTemporaryExpr to the
right CXXBindTemporaryExpr (if it exists). Once that problem is solved, we
can simple mark the CXXBindTemporary that is lifetime extended once we hit
the MaterializeTempraryExpr in the static analyzer.


>
>
>>
>>
>> On Mon, Jul 28, 2014 at 3:03 PM, Manuel Klimek <klimek at google.com> wrote:
>>
>>> On Mon, Jul 7, 2014 at 10:15 PM, Jordan Rose <jordan_rose at apple.com>
>>> wrote:
>>>
>>>> Ah...then I'm glad we added the assertion. :-) Lifetime-extended
>>>> temporaries aren't quite implemented correctly yet, but we should probably
>>>> be removing or not even adding the state when the temporary is
>>>> lifetime-extended.
>>>>
>>>> Unfortunately I don't have this paged in, but there's logic in
>>>> processing auto destructors to handle this, and a bug (
>>>> http://llvm.org/bugs/show_bug.cgi?id=19539) about how it's wrong.
>>>>
>>>
>>> Ok, I dug into this a bit, and if I'm not missing something I think it's
>>> not possible to switch this off without implementing lifetime-extended
>>> temporaries correctly (or at least a similarly sized implementation effort):
>>> The only link between the MaterializeTempooraryExpr (which has the
>>> information whether the lifetime was extended) and the CXXBindTemporaryExpr
>>> is the underlying object storage. I looked into CodeGen, and if I
>>> understand it correctly, it looks like it basically stores for the
>>> destination whether the destructor was already handled (for example from
>>> the MaterializeTemporary flow). To get that information, I'd guess we need
>>> to do something similar in the static analyzer.
>>>
>>> I hope that my analysis is wrong and you tell me a better way to fix
>>> this :)
>>>
>>> Thanks!
>>> /Manuel
>>>
>>>
>>>>
>>>> Jordan
>>>>
>>>>
>>>> On Jul 7, 2014, at 13:13 , Manuel Klimek <klimek at google.com> wrote:
>>>>
>>>> Sigh. That triggers in Analysis/dtor.cpp for this CXXBindTemporaryExpr:
>>>> CXXBindTemporaryExpr 0x3c195d8 'class
>>>> LifetimeExtension::SaveOnVirtualDestruct' (CXXTemporary 0x3c195d0)
>>>>  `-CXXTemporaryObjectExpr 0x3c19590 'class
>>>> LifetimeExtension::SaveOnVirtualDestruct' 'void (void)'
>>>>
>>>> I assume something fishy is going on for lifetime extended
>>>> temporaries...
>>>>
>>>>
>>>> On Mon, Jul 7, 2014 at 8:38 PM, Jordan Rose <jordan_rose at apple.com>
>>>> wrote:
>>>>
>>>>>
>>>>> On Jul 7, 2014, at 11:37 , Manuel Klimek <klimek at google.com> wrote:
>>>>>
>>>>> On Mon, Jul 7, 2014 at 8:05 PM, Jordan Rose <jordan_rose at apple.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> On Jul 7, 2014, at 10:50 , Manuel Klimek <klimek at google.com> wrote:
>>>>>>
>>>>>> On Mon, Jul 7, 2014 at 7:42 PM, Jordan Rose <jordan_rose at apple.com>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jul 7, 2014, at 10:41 , Manuel Klimek <klimek at google.com> wrote:
>>>>>>>
>>>>>>> On Mon, Jul 7, 2014 at 7:38 PM, Jordan Rose <jordan_rose at apple.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> On Jul 7, 2014, at 10:37 , Manuel Klimek <klimek at google.com> wrote:
>>>>>>>>
>>>>>>>> On Mon, Jul 7, 2014 at 7:29 PM, Jordan Rose <jordan_rose at apple.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Jul 7, 2014, at 10:28 , Manuel Klimek <klimek at google.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Jul 7, 2014 at 6:48 PM, Jordan Rose <jordan_rose at apple.com
>>>>>>>>> > wrote:
>>>>>>>>>
>>>>>>>>>> Can you add an assertion at the end of a block that there are no
>>>>>>>>>> outstanding temporary destructors in the current stack frame? That seems
>>>>>>>>>> useful.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Do you mean at the end of a VisitBlockDecl?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, during the path-sensitive run, so handleBlockExit.
>>>>>>>>>
>>>>>>>>
>>>>>>>> So you mean at the end of a CFG block? But here we might have
>>>>>>>> outstanding temporary dtors open (?)
>>>>>>>>
>>>>>>>>
>>>>>>>> Oops, right. Was thinking too much in terms of AST structure. How
>>>>>>>> about at the end of a function (inlined or not)?
>>>>>>>>
>>>>>>>
>>>>>>> Could we say every time we transition from a block with a temp dtor
>>>>>>> terminator to a block that does not have a temp dtor terminator (or an
>>>>>>> unconditional terminator) we check?
>>>>>>>
>>>>>>>
>>>>>>> That sounds correct, but misses the case where we built the CFG
>>>>>>> wrong (forgetting to add the branch in the correct place and thus never
>>>>>>> getting to the temp dtor block at all).
>>>>>>>
>>>>>>
>>>>>> Makes sense. Do you have a hint where the right place on function
>>>>>> exit to check it would be? :)
>>>>>>
>>>>>>
>>>>>> *checks* ExprEngine::processEndOfFunction.
>>>>>>
>>>>>
>>>>> Hm, so we'll need to adjust the data structure to be indexed by stack
>>>>> frame somehow (use a map) instead of the pair<expr, stack-frame>?
>>>>>
>>>>>
>>>>> Eh, since it's an assertion I'd be fine with just iterating over it in
>>>>> a helper function. Or better, using std::find_if (if it has proper
>>>>> begin/end members).
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140728/f399cf71/attachment.html>