[cfe-commits] [Review] Partial fix for PR 11645: put destructor calls before a return statement.

[Xu Zhongxing]

于 2012/1/18 19:04, Erik Verbruggen 写道:
> On 17-1-12 17:37, Ted Kremenek wrote:
>> On Tuesday, January 17, 2012 at 8:22 AM, Erik Verbruggen wrote:
>>> On 17-1-12 7:02, Chandler Carruth wrote:
>>>> On Mon, Jan 16, 2012 at 9:29 PM, Ted Kremenek<kremenek at apple.com
>>>> <mailto:kremenek at apple.com>
>>>> <mailto:kremenek at apple.com>>  wrote:
>>>>
>>>> Hi Erik,
>>>>
>>>> Thanks for working on this. I've commented in the PR.
>>>>
>>>> My concern is that I am having second thoughts about whether or not
>>>> this is the right direction. If there are temporaries in the return
>>>> expression, should those destructors appear before the 'return'
>>>> statement (but after the subexpression of 'return' gets evaluated)?
>>>> I can see it both ways.
>>>>
>>>> For example:
>>>>
>>>> return A();
>>>>
>>>> where 'A' converts to whatever the return value type should be, and
>>>> ~A() is non-trivial. Technically, ~A() evaluates after the
>>>> enclosing statement. We're giving 'return' special treatment in
>>>> your patch.
>>>>
>>>> One clean representation of this is to put all the destructors after
>>>> the 'return' in the CFGBlock. The other way is to have the
>>>> destructors appear after the subexpression of 'return', but before
>>>> the 'return' itself. The former requires clients of the CFG to
>>>> rethink where they expect 'return' to be in a CFGBlock.
>>>>
>>>> What do you think?
>>>>
>>>>
>>>> If I can jump in from the peanut gallery...
>>>>
>>>> This doesn't seem a necessarily unique problem to return statements. Any
>>>> statement which forms a CFGBlock terminator and which contains
>>>> temporaries with destructors should hit the same issue. The other
>>>> example is I suspect throw. It may even be possible to trigger this with
>>>> an indirect goto if the index into the label-address array involves
>>>> temporary objects.
>>> For a throw it is actually slightly worse: no implicit destructors calls
>>> get added to the CFG. I did not check the indirect goto, because I could
>>> not come up with a good example..
>>>
>> For now, just assume that everything with EH in the CFG is wrong or
>> incomplete. That's a discussion on its own.
>>>> I think the C++ standard* provides some help in making a choice about
>>>> how to represent this. It doesn't actually say that ~A() evaluates after
>>>> the enclosing statement. Instead it defines 'full-expressions' as an
>>>> expression which is not a subexpression of any other expression
>>>> ([intro.execution] 1.9p10). As *part* of this full-expression, all
>>>> temporary objects' non-trivial destructors must be called
>>>> ([class.temporary] 12.2p3). To me, that indicates that the destructor
>>>> should be placed after the complete subexpression in the return
>>>> statement, but within the return statement itself. This has the nice
>>>> property of preserving the (very useful!) invariant that terminator
>>>> statements are the last statement in the CFG block.
>>> This would mean that a full expression of "return SomeClass();" would
>>> get split in two: all the implicit destructors calls for the scope get
>>> placed between the return value calculation and the actual return of
>>> that value.
>> Yes. Exactly. That's the correct semantics
>>> If you would want to base code generation on the CFG, it
>>> would force an extra pass to identify whether calculated values should
>>> be constructed in a return value slot or not. So I think that using
>>> return/throw (and possibly goto) as markers, would be a simplification.
>> Concerning "goto" and "throw"; those are terminators and already can
>> serve as markers. I'll elaborate with the case of "return". I'm also not
>> certain what you mean by an "extra pass", but it *might* require some work.
>>
>> My concern is that, unlike "goto", the "return" in the CFG is not a
>> terminator; it doesn't represent a transfer of control. That's what
>> terminators in CFGBlocks are for. Instead, it appears in the CFGBlock as
>> an element, which means (in that context) it represents a statement that
>> gets executed. Since it doesn't represent the transfer of control in
>> that context, the sole role of the "return" in the CFG is to represent
>> the binding of the return value.
>>
>> That said, we could place the "return" as a terminator, in *addition* to
>> what we do now, and have that terminator terminate the CFGBlock with the
>> destructor calls in it. That terminator would be on a CFGBlock that
>> transitioned to the Exit block. Doing this isn't absolutely necessary,
>> but it would annotate the end of the CFGBlock with the return statement,
>> possibly making it more amendable to code generation while still
>> preserving the invariants of the CFG.
> So if I understand it correctly, the following example should generate
> the CFG below:
>
> class A {
> public:
>       A();
>       ~A();
> };
>
> class B {
> public:
>       B();
>       ~B();
> };
>
> B test() {
>       A a;
>       return B();
> }
>
>    [B3 (ENTRY)]
>      Succs (1): B2
>
>    [B1]
>      1: [B2.2].~A() (Implicit destructor)
>      Succs (1): B0
>
>    [B2]
>      1:  (CXXConstructExpr, class A)
>      2: A a;
>      3: B() (CXXConstructExpr, class B)
>      4: [B2.3] (BindTemporary)
>      5: [B2.4] (ImplicitCastExpr, NoOp, const class B)
>      6: [B2.5]
>      7: [B2.6] (CXXConstructExpr, class B)

Do we need a 'return [B2.7]' here to indicate the copying of the return 
value to the return-value slot? Otherwise how do we know whether to 
destruct the temporary object when we reach the next element?
>      8: ~B() (Temporary object destructor)
>      9: return [B2.7];
>      Preds (1): B3
>      Succs (1): B1
>
>    [B0 (EXIT)]
>      Preds (2): B1
>
> (Note: this is manually "corrected" CFG.)
>
> Is that right?
>
> -- Erik.
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits