[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

Ping.


On Wed, Jul 30, 2014 at 1:08 PM, Manuel Klimek <klimek at google.com> wrote:

> 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/20140805/dec94295/attachment.html>