[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

Jordan, do you think we need to make the marking non-leaking before
checking this in?

I think the right solution to fix the leakage of markers is to fix the
handling of lifetime extended temporaries, which unfortunately needs a fix
to handling lifetime of temporaries (and inlining temp dtors) first, which
is another patch that will probably take a while to work through and get in.

If you have a different proposal on how to fix the marking only, I'm all
ears; so far I haven't been able to think of an easy one - we could try to
encode the relationship between the materializetemporary and the
cxxbindtemporary, but that is super-complex (conditional operators, etc),
and I think I won't get it right before having all tests passing for the
handling of lifetime extended temporaries in the cfg.

Cheers,
/Manuel



On Tue, Jul 29, 2014 at 11:41 AM, Manuel Klimek <klimek at google.com> wrote:

> On Mon, Jul 28, 2014 at 9:24 PM, Manuel Klimek <klimek at google.com> wrote:
>
>> On Mon, Jul 28, 2014 at 8:09 PM, Jordan Rose <jordan_rose at apple.com>
>> wrote:
>>
>>>
>>> On Jul 28, 2014, at 9:36 , Manuel Klimek <klimek at google.com> wrote:
>>>
>>> On Mon, Jul 28, 2014 at 6:27 PM, Jordan Rose <jordan_rose at apple.com>
>>> wrote:
>>>
>>>>
>>>> On Jul 28, 2014, at 7:56 , Manuel Klimek <klimek at google.com> wrote:
>>>>
>>>> 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.
>>>>
>>>>
>>>> I'd really like the CFG to be correct, not something that we have to
>>>> reconstruct correctness for in the analyzer.
>>>>
>>>
>>> Yes, but it's currently not correct.
>>>
>>>
>>>> We should be able to statically tell which temporary a
>>>> MaterializeTemporaryExpr is going to bind to, right? Or rather, we should
>>>> be able to tell whether or not a given CXXBindTemporaryExpr will be
>>>> destroyed or bound if it is executed.
>>>>
>>>
>>> Yes, fully agreed, but first we have to fix how to handle lifetime
>>> extended temporaries - the fix *is* getting that information correctly out
>>> of the AST.
>>> I'd rather not mix this patch with a fix for lifetime extended
>>> temporaries, and I'm not sure continuing to maintain this patch until after
>>> I fixed lifetime extended temps is a good idea.
>>>
>>>
>>>
>>> Agreed. I was just objecting to "mark the CXXBindTemporary...once we hit
>>> the MaterializeTemporaryExpr in the static analyzer". It seems like we
>>> would *never* add a destructor for that temporary in the first place,
>>> so we wouldn't have to mark it as skipped. Or did you mean "when building
>>> the CFG"?
>>>
>>
>> Ah, I meant that we need to mark the constructor to be able to not mark
>> the destructor from the constructor. If that makes sense :) Currently we
>> mark the constructor as "executed" when we hit it - we don't want to do
>> that if it's executed and then lifetime extended.
>> The alternative is to mark the temporary constructor as executed, but
>> then delete that mark when we hit the lifetime extension.
>>
>
> After some digging through codgen, I found that codegen uses a similar
> approach:
> Basically, when hitting the MaterializeTemporaryExpr, it will first skip
> through to the correctly typed subexpr, and then do an aggregate expression
> visitation, which will implicitly either hit the CXXBindTemporary (which
> then belongs to that MaterializeTemporaryExpr) or go back to the outer
> visitation (which will then lose the connection to the
> MaterializeTemoprary).
>
> So, multiple possible approaches:
> 1. When hitting a MaterializeTemporaryExpr in the static analyzer, we
> drill through to the CXXBindTemporaryExpr (using the cases from codegen
> that let's us continue visitation) and then reset the state that was set
> when we visited the CXXBindTemporaryExpr.
> 2. When building the CFG, somehow introduce a pointer from the
> CXXBindTemporaryExpr to its MaterializeTemporaryExpr (or just note down its
> storage duration); I have no idea how to do that from a data structure
> point of view though...
>
> Cheers,
> /Manuel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140730/1fb811e6/attachment.html>