[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

On Tue, Aug 5, 2014 at 7:15 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> I'm sorry, I don't understand what you mean by non-leaking. Do you mean
> that if we decide not to run a destructor, we shouldn't leave that trace in
> the ProgramState? I think it's okay because it will just get postponed to
> when the destructor is actually run later, right?
>

I mean that until the lifetime extension is fixed, there are lifetime
extended temporaries for which we will mark the ProgramState, but never
clear it, as with the current modeling of lifetime extended temporaries the
destructors for them look like normal destructors, not like temporary
destructors (if we correctly run into them at all, which also has some
bugs).


>
> Jordan
>
>
> On Jul 30, 2014, at 4:08 , 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/29988026/attachment.html>