New option to turn off stack reuse for temporaries

New option to turn off stack reuse for temporaries

[Xinliang David Li]

One of the most common runtime errors we have seen in gcc-4_7 is
caused by dangling references to temporaries whole life time have
ended

e.g,

 const A& a = foo();

or
foo (A());// where temp's address is saved and used after foo.

Of course this is user error according to the standard, triaging of
bugs like this is pretty time consuming to triage. This patch tries to
introduce an option to disable stack reuse for temporaries, which can
be used to debugging purpose.

Is this good for trunk?

thanks,

David

2012-06-20  Xinliang David Li  <davidxl@google.com>

        * common.opt: -ftemp-reuse-stack option.
        * gimplify.c (gimplify_target_expr): Check new flag.



Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi     (revision 188362)
+++ doc/invoke.texi     (working copy)
@@ -1003,6 +1003,7 @@ See S/390 and zSeries Options.
 -fstack-limit-register=@var{reg}  -fstack-limit-symbol=@var{sym} @gol
 -fno-stack-limit -fsplit-stack @gol
 -fleading-underscore  -ftls-model=@var{model} @gol
+-ftemp-stack-reuse @gol
 -ftrapv  -fwrapv  -fbounds-check @gol
 -fvisibility -fstrict-volatile-bitfields}
 @end table
@@ -19500,6 +19501,10 @@ indices used to access arrays are within
 currently only supported by the Java and Fortran front ends, where
 this option defaults to true and false respectively.

+@item -ftemp-stack-reuse
+@opindex ftemp_stack_reuse
+This option enables stack space reuse for temporaries. The default is on.
+
 @item -ftrapv
 @opindex ftrapv
 This option generates traps for signed overflow on addition, subtraction,
Index: gimplify.c
===================================================================
--- gimplify.c  (revision 188362)
+++ gimplify.c  (working copy)
@@ -5487,7 +5487,8 @@ gimplify_target_expr (tree *expr_p, gimp
       /* Add a clobber for the temporary going out of scope, like
         gimplify_bind_expr.  */
       if (gimplify_ctxp->in_cleanup_point_expr
-         && needs_to_live_in_memory (temp))
+         && needs_to_live_in_memory (temp)
+         && flag_temp_stack_reuse)
        {
          tree clobber = build_constructor (TREE_TYPE (temp), NULL);
          TREE_THIS_VOLATILE (clobber) = true;
Index: common.opt
===================================================================
--- common.opt  (revision 188362)
+++ common.opt  (working copy)
@@ -1322,6 +1322,10 @@ fif-conversion2
 Common Report Var(flag_if_conversion2) Optimization
 Perform conversion of conditional jumps to conditional execution

+ftemp-stack-reuse
+Common Report Var(flag_temp_stack_reuse) Init(1)
+Enable stack reuse for compiler generated temps
+
 ftree-loop-if-convert
 Common Report Var(flag_tree_loop_if_convert) Init(-1) Optimization
 Convert conditional jumps in innermost loops to branchless equivalents

Re: New option to turn off stack reuse for temporaries

[Jason Merrill]

The documentation needs to explain more what the option controls, and 
why you might want it on or off.  Other than that it looks fine.

Jason

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

I modified the documentation and it now looks like this:

@item -ftemp-stack-reuse
@opindex ftemp_stack_reuse
This option enables stack space reuse for temporaries. The default is on.
The lifetime of a compiler generated temporary is well defined by the C++
standard. When a lifetime of a temporary ends, and if the temporary lives
in memory, an optimizing compiler has the freedom to reuse its stack
space with other temporaries or scoped local variables whose live range
does not overlap with it. However some of the legacy code relies on
the behavior of older compilers in which temporaries' stack space is
not reused, the aggressive stack reuse can lead to runtime errors. This
option is used to control the temporary stack reuse optimization.

Does it look ok?

thanks,

David

On Wed, Jun 20, 2012 at 5:29 PM, Jason Merrill <jason@redhat.com> wrote:
> The documentation needs to explain more what the option controls, and why
> you might want it on or off.  Other than that it looks fine.
>
> Jason

Re: New option to turn off stack reuse for temporaries

[Jason Merrill]

OK.

Jason

Re: New option to turn off stack reuse for temporaries

[Richard Guenther]

On Thu, Jun 21, 2012 at 7:28 AM, Xinliang David Li <davidxl@google.com> wrote:
> I modified the documentation and it now looks like this:
>
> @item -ftemp-stack-reuse
> @opindex ftemp_stack_reuse
> This option enables stack space reuse for temporaries. The default is on.
> The lifetime of a compiler generated temporary is well defined by the C++
> standard. When a lifetime of a temporary ends, and if the temporary lives
> in memory, an optimizing compiler has the freedom to reuse its stack
> space with other temporaries or scoped local variables whose live range
> does not overlap with it. However some of the legacy code relies on
> the behavior of older compilers in which temporaries' stack space is
> not reused, the aggressive stack reuse can lead to runtime errors. This
> option is used to control the temporary stack reuse optimization.
>
> Does it look ok?

The flag is not restricted to the C++ compiler and applies to all automatic
variables.  The description is very much C++ specific though - I think
it should mention the concept of scopes.

Also even with this flag there is no guarantee we cannot figure out lifetime
in other ways, for example if the temporary gets promoted to a register.
Also with this patch you remove code motion barriers which might cause
other issues.

A more "proper" place to fix this is when we actually do the stack reuse,
in cfgexpand.

So no, I don't think the patch is ok as-is.

Thanks,
Richard.

> thanks,
>
> David
>
> On Wed, Jun 20, 2012 at 5:29 PM, Jason Merrill <jason@redhat.com> wrote:
>> The documentation needs to explain more what the option controls, and why
>> you might want it on or off.  Other than that it looks fine.
>>
>> Jason

Re: New option to turn off stack reuse for temporaries

[Michael Matz]

Hi,

On Thu, 21 Jun 2012, Richard Guenther wrote:

> The flag is not restricted to the C++ compiler and applies to all 
> automatic variables.

The use of that flag in the gimplifier (->in_cleanup_expr) makes it 
actually c++ specific.

> Also even with this flag there is no guarantee we cannot figure out 
> lifetime in other ways, for example if the temporary gets promoted to a 
> register. Also with this patch you remove code motion barriers which 
> might cause other issues.
> 
> A more "proper" place to fix this is when we actually do the stack 
> reuse, in cfgexpand.

That is true, though.  It would then also enable debugging help for 
pointers to things that go out-of-scope.


Ciao,
Michael.

Re: New option to turn off stack reuse for temporaries

[Richard Guenther]

On Thu, Jun 21, 2012 at 5:53 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Thu, 21 Jun 2012, Richard Guenther wrote:
>
>> The flag is not restricted to the C++ compiler and applies to all
>> automatic variables.
>
> The use of that flag in the gimplifier (->in_cleanup_expr) makes it
> actually c++ specific.

We don't have any other users of WITH_CLEANUP_EXPR?  Indeed.

>> Also even with this flag there is no guarantee we cannot figure out
>> lifetime in other ways, for example if the temporary gets promoted to a
>> register. Also with this patch you remove code motion barriers which
>> might cause other issues.
>>
>> A more "proper" place to fix this is when we actually do the stack
>> reuse, in cfgexpand.
>
> That is true, though.  It would then also enable debugging help for
> pointers to things that go out-of-scope.

Yes.  So I think if the flag is supposed to be used for debugging (instead
of as fix or workaround for invalid programs) then we should go one step
further and have it disable stack slot sharing alltogether - without any
other side-effect on pre-RTL optimizations (which undoubtedly not having
the CLOBBERS have).

Richard.

>
> Ciao,
> Michael.

Re: New option to turn off stack reuse for temporaries

[Jason Merrill]

On 06/21/2012 02:21 AM, Richard Guenther wrote:
> The flag is not restricted to the C++ compiler and applies to all automatic
> variables.

This only affects the clobbers for C++ temporary objects, not clobbers 
for automatic variables going out of scope.

> Also with this patch you remove code motion barriers which might cause
> other issues.

How so?

> A more "proper" place to fix this is when we actually do the stack reuse,
> in cfgexpand.

How would that distinguish between the clobbers for temporaries vs. 
automatics?

Jason

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

On Thu, Jun 21, 2012 at 2:21 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Jun 21, 2012 at 7:28 AM, Xinliang David Li <davidxl@google.com> wrote:
>> I modified the documentation and it now looks like this:
>>
>> @item -ftemp-stack-reuse
>> @opindex ftemp_stack_reuse
>> This option enables stack space reuse for temporaries. The default is on.
>> The lifetime of a compiler generated temporary is well defined by the C++
>> standard. When a lifetime of a temporary ends, and if the temporary lives
>> in memory, an optimizing compiler has the freedom to reuse its stack
>> space with other temporaries or scoped local variables whose live range
>> does not overlap with it. However some of the legacy code relies on
>> the behavior of older compilers in which temporaries' stack space is
>> not reused, the aggressive stack reuse can lead to runtime errors. This
>> option is used to control the temporary stack reuse optimization.
>>
>> Does it look ok?
>
> The flag is not restricted to the C++ compiler and applies to all automatic
> variables.  The description is very much C++ specific though - I think
> it should mention the concept of scopes.
>
> Also even with this flag there is no guarantee we cannot figure out lifetime
> in other ways, for example if the temporary gets promoted to a register.

That should not be an issue then -- if the compiler can figure out the
live range via data flow analysis (instead of relying on
assertions/markers), the stack reuse or register promotion based on
that should always be safe (assuming no bugs in the analysis).

> Also with this patch you remove code motion barriers which might cause
> other issues.

What other issues? It enables more potential code motion, but on the
other hand, causes more conservative stack reuse. As far I can tell,
the handling of temporaries is added independently after the clobber
for scoped variables are introduced. This option can be used to
restore the older behavior (in handling temps).

thanks,

David

>
> A more "proper" place to fix this is when we actually do the stack reuse,
> in cfgexpand.
>
> So no, I don't think the patch is ok as-is.
>
> Thanks,
> Richard.
>
>> thanks,
>>
>> David
>>
>> On Wed, Jun 20, 2012 at 5:29 PM, Jason Merrill <jason@redhat.com> wrote:
>>> The documentation needs to explain more what the option controls, and why
>>> you might want it on or off.  Other than that it looks fine.
>>>
>>> Jason

Re: New option to turn off stack reuse for temporaries

[Richard Guenther]

On Thu, Jun 21, 2012 at 8:27 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Jun 21, 2012 at 2:21 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jun 21, 2012 at 7:28 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> I modified the documentation and it now looks like this:
>>>
>>> @item -ftemp-stack-reuse
>>> @opindex ftemp_stack_reuse
>>> This option enables stack space reuse for temporaries. The default is on.
>>> The lifetime of a compiler generated temporary is well defined by the C++
>>> standard. When a lifetime of a temporary ends, and if the temporary lives
>>> in memory, an optimizing compiler has the freedom to reuse its stack
>>> space with other temporaries or scoped local variables whose live range
>>> does not overlap with it. However some of the legacy code relies on
>>> the behavior of older compilers in which temporaries' stack space is
>>> not reused, the aggressive stack reuse can lead to runtime errors. This
>>> option is used to control the temporary stack reuse optimization.
>>>
>>> Does it look ok?
>>
>> The flag is not restricted to the C++ compiler and applies to all automatic
>> variables.  The description is very much C++ specific though - I think
>> it should mention the concept of scopes.
>>
>> Also even with this flag there is no guarantee we cannot figure out lifetime
>> in other ways, for example if the temporary gets promoted to a register.
>
> That should not be an issue then -- if the compiler can figure out the
> live range via data flow analysis (instead of relying on
> assertions/markers), the stack reuse or register promotion based on
> that should always be safe (assuming no bugs in the analysis).
>
>> Also with this patch you remove code motion barriers which might cause
>> other issues.
>
> What other issues? It enables more potential code motion, but on the
> other hand, causes more conservative stack reuse. As far I can tell,
> the handling of temporaries is added independently after the clobber
> for scoped variables are introduced. This option can be used to
> restore the older behavior (in handling temps).

Well, it does not really restore the old behavior (if you mean before adding
CLOBBERS, not before the single patch that might have used those for
gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
for those decls but it also does other things via side-effects of no longer
emitting the CLOBBER.  I say it's better to disable the stack-slot sharing.

Richard.

> thanks,
>
> David
>
>>
>> A more "proper" place to fix this is when we actually do the stack reuse,
>> in cfgexpand.
>>
>> So no, I don't think the patch is ok as-is.
>>
>> Thanks,
>> Richard.
>>
>>> thanks,
>>>
>>> David
>>>
>>> On Wed, Jun 20, 2012 at 5:29 PM, Jason Merrill <jason@redhat.com> wrote:
>>>> The documentation needs to explain more what the option controls, and why
>>>> you might want it on or off.  Other than that it looks fine.
>>>>
>>>> Jason

Re: New option to turn off stack reuse for temporaries

[Jason Merrill]

On 06/22/2012 01:30 AM, Richard Guenther wrote:
>> What other issues? It enables more potential code motion, but on the
>> other hand, causes more conservative stack reuse. As far I can tell,
>> the handling of temporaries is added independently after the clobber
>> for scoped variables are introduced. This option can be used to
>> restore the older behavior (in handling temps).
>
> Well, it does not really restore the old behavior (if you mean before adding
> CLOBBERS, not before the single patch that might have used those for
> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
> for those decls but it also does other things via side-effects of no longer
> emitting the CLOBBER.  I say it's better to disable the stack-slot sharing.

The patch exactly restores the behavior of temporaries from before my 
change to add CLOBBERs for temporaries.  The primary effect of that 
change was to provide stack-slot sharing, but if there are other effects 
they are probably desirable as well, since the broken code depended on 
the old behavior.

Jason

Re: New option to turn off stack reuse for temporaries

[Richard Guenther]

On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>
>>> What other issues? It enables more potential code motion, but on the
>>> other hand, causes more conservative stack reuse. As far I can tell,
>>> the handling of temporaries is added independently after the clobber
>>> for scoped variables are introduced. This option can be used to
>>> restore the older behavior (in handling temps).
>>
>>
>> Well, it does not really restore the old behavior (if you mean before
>> adding
>> CLOBBERS, not before the single patch that might have used those for
>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>> for those decls but it also does other things via side-effects of no
>> longer
>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>> sharing.
>
>
> The patch exactly restores the behavior of temporaries from before my change
> to add CLOBBERs for temporaries.  The primary effect of that change was to
> provide stack-slot sharing, but if there are other effects they are probably
> desirable as well, since the broken code depended on the old behavior.

So you see it as workaround option, like -fno-strict-aliasing, rather than
debugging aid?

Richard.

> Jason

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

On Fri, Jun 22, 2012 at 2:39 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>>
>>>> What other issues? It enables more potential code motion, but on the
>>>> other hand, causes more conservative stack reuse. As far I can tell,
>>>> the handling of temporaries is added independently after the clobber
>>>> for scoped variables are introduced. This option can be used to
>>>> restore the older behavior (in handling temps).
>>>
>>>
>>> Well, it does not really restore the old behavior (if you mean before
>>> adding
>>> CLOBBERS, not before the single patch that might have used those for
>>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>>> for those decls but it also does other things via side-effects of no
>>> longer
>>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>>> sharing.
>>
>>
>> The patch exactly restores the behavior of temporaries from before my change
>> to add CLOBBERs for temporaries.  The primary effect of that change was to
>> provide stack-slot sharing, but if there are other effects they are probably
>> desirable as well, since the broken code depended on the old behavior.
>
> So you see it as workaround option, like -fno-strict-aliasing, rather than
> debugging aid?

It can be used for both purposes -- if the violations are as pervasive
as strict-aliasing cases (which looks like so).

thanks,

David

>
> Richard.
>
>> Jason

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

Are there any more concerns about this patch? If not, I'd like to check it in.

thanks,

David

On Fri, Jun 22, 2012 at 8:51 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Jun 22, 2012 at 2:39 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
>>> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>>>
>>>>> What other issues? It enables more potential code motion, but on the
>>>>> other hand, causes more conservative stack reuse. As far I can tell,
>>>>> the handling of temporaries is added independently after the clobber
>>>>> for scoped variables are introduced. This option can be used to
>>>>> restore the older behavior (in handling temps).
>>>>
>>>>
>>>> Well, it does not really restore the old behavior (if you mean before
>>>> adding
>>>> CLOBBERS, not before the single patch that might have used those for
>>>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>>>> for those decls but it also does other things via side-effects of no
>>>> longer
>>>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>>>> sharing.
>>>
>>>
>>> The patch exactly restores the behavior of temporaries from before my change
>>> to add CLOBBERs for temporaries.  The primary effect of that change was to
>>> provide stack-slot sharing, but if there are other effects they are probably
>>> desirable as well, since the broken code depended on the old behavior.
>>
>> So you see it as workaround option, like -fno-strict-aliasing, rather than
>> debugging aid?
>
> It can be used for both purposes -- if the violations are as pervasive
> as strict-aliasing cases (which looks like so).
>
> thanks,
>
> David
>
>>
>> Richard.
>>
>>> Jason

Re: New option to turn off stack reuse for temporaries

[Richard Guenther]

On Mon, Jun 25, 2012 at 6:25 PM, Xinliang David Li <davidxl@google.com> wrote:
> Are there any more concerns about this patch? If not, I'd like to check it in.

No - the fact that the flag is C++ specific but in common.opt is odd enough
and -ftemp-reuse-stack sounds very very generic - which in fact it is not,
it's a no-op in C.  Is there a more formal phrase for the temporary kind that
is affected?  For me "temp" is synonymous to "auto" so I'd have expected
the switch to turn off stack slot sharing for

 {
   int a[5];
 }
 {
   int a[5];
 }

but that is not what it does.  So - a little kludgy but probably more to what
I'd like it to be would be to move the option to c-family/c.opt enabled only
for C++ and Obj-C++ and export it to the middle-end via a new langhook
(the gimplifier code should be in Frontend code that lowers to GENERIC
really and the WITH_CLEANUP_EXPR code should be C++ frontend specific ...).

Thanks,
Richard.

> thanks,
>
> David
>
> On Fri, Jun 22, 2012 at 8:51 AM, Xinliang David Li <davidxl@google.com> wrote:
>> On Fri, Jun 22, 2012 at 2:39 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
>>>> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>>>>
>>>>>> What other issues? It enables more potential code motion, but on the
>>>>>> other hand, causes more conservative stack reuse. As far I can tell,
>>>>>> the handling of temporaries is added independently after the clobber
>>>>>> for scoped variables are introduced. This option can be used to
>>>>>> restore the older behavior (in handling temps).
>>>>>
>>>>>
>>>>> Well, it does not really restore the old behavior (if you mean before
>>>>> adding
>>>>> CLOBBERS, not before the single patch that might have used those for
>>>>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>>>>> for those decls but it also does other things via side-effects of no
>>>>> longer
>>>>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>>>>> sharing.
>>>>
>>>>
>>>> The patch exactly restores the behavior of temporaries from before my change
>>>> to add CLOBBERs for temporaries.  The primary effect of that change was to
>>>> provide stack-slot sharing, but if there are other effects they are probably
>>>> desirable as well, since the broken code depended on the old behavior.
>>>
>>> So you see it as workaround option, like -fno-strict-aliasing, rather than
>>> debugging aid?
>>
>> It can be used for both purposes -- if the violations are as pervasive
>> as strict-aliasing cases (which looks like so).
>>
>> thanks,
>>
>> David
>>
>>>
>>> Richard.
>>>
>>>> Jason

Re: New option to turn off stack reuse for temporaries

[Jason Merrill]

On 06/26/2012 04:28 AM, Richard Guenther wrote:
> No - the fact that the flag is C++ specific but in common.opt is odd enough
> and -ftemp-reuse-stack sounds very very generic - which in fact it is not,
> it's a no-op in C.  Is there a more formal phrase for the temporary kind that
> is affected?

Not that I'm aware of.  This is a temporary introduced by TARGET_EXPR, 
which lives until the end of the enclosing CLEANUP_POINT_EXPR.

> So - a little kludgy but probably more to what
> I'd like it to be would be to move the option to c-family/c.opt enabled only
> for C++ and Obj-C++ and export it to the middle-end via a new langhook

Hmm, that does seem rather kludgy for something that affects the 
behavior of middle-end code working on GENERIC.

> (the gimplifier code should be in Frontend code that lowers to GENERIC
> really and the WITH_CLEANUP_EXPR code should be C++ frontend specific ...).

TARGET_EXPR has been a back-end code since the dawn of GCC version 
control; if it's still only used by the C++ front end I guess it could 
move to cp-tree.def, but we can't really lower it until gimplification 
time because we need to strip away enclosing COMPOUND_EXPRs and such so 
we can see that it's on the RHS of an initialization and optimize away 
the temporary in that case.  And now that GIMPLE isn't a subset of 
GENERIC, we can't just use the gimplifier at genericization time.  And 
I'd rather not duplicate the entire gimplifier in the front end.

Jason

Re: New option to turn off stack reuse for temporaries

[Michael Matz]

Hi,

On Tue, 26 Jun 2012, Jason Merrill wrote:

> > (the gimplifier code should be in Frontend code that lowers to GENERIC 
> > really and the WITH_CLEANUP_EXPR code should be C++ frontend specific 
> > ...).
> 
> TARGET_EXPR has been a back-end code since the dawn of GCC version 
> control; if it's still only used by the C++ front end I guess it could 
> move to cp-tree.def, but we can't really lower it until gimplification 
> time because we need to strip away enclosing COMPOUND_EXPRs and such so 
> we can see that it's on the RHS of an initialization and optimize away 
> the temporary in that case. And now that GIMPLE isn't a subset of 
> GENERIC, we can't just use the gimplifier at genericization time.  And 
> I'd rather not duplicate the entire gimplifier in the front end.

I agree with Jason.  TARGET_EXPR and CLEANUP_POINT_EXPR might currently be 
used only for C++, but I think they are sensible general constructs to be 
supported by the gimplifier.

But I also think that the option to disable stack slot sharing should be 
moved to cfgexpand to trigger non-sharing of everything, not just these 
cleanup temporaries.  After all using the (c++)temporary after expression 
end is a source bug that the option is supposed to work around, just like 
this is:

  char *p; { char str[50]; p = str; } use(p);

So, IMO the option should also work around this source bug.  We had at 
least one example of that in our own code base.


Ciao,
Michael.

Re: New option to turn off stack reuse for temporaries

[Jakub Jelinek]

On Tue, Jun 26, 2012 at 06:07:09PM +0200, Michael Matz wrote:
> I agree with Jason.  TARGET_EXPR and CLEANUP_POINT_EXPR might currently be 
> used only for C++, but I think they are sensible general constructs to be 
> supported by the gimplifier.
> 
> But I also think that the option to disable stack slot sharing should be 
> moved to cfgexpand to trigger non-sharing of everything, not just these 
> cleanup temporaries.  After all using the (c++)temporary after expression 
> end is a source bug that the option is supposed to work around, just like 
> this is:

If you move it solely to cfgexpand time, broken code will still often not
work the way it happened to work with 4.6 and earlier.  You'd need to both
disable the sharing and disable additions of gimple clobbers.
Because otherwise DCE/DSE and other passes happily optimize (broken) code
away.  So, if we want a -fno-strict-aliasing like option to work around
broken code, we should IMHO do both of those.
> 
>   char *p; { char str[50]; p = str; } use(p);
> 
> So, IMO the option should also work around this source bug.  We had at 
> least one example of that in our own code base.

Yeah, gengtype I think.

	Jakub

Re: New option to turn off stack reuse for temporaries

[Mike Stump]

On Jun 26, 2012, at 9:07 AM, Michael Matz wrote:
> I agree with Jason.  TARGET_EXPR and CLEANUP_POINT_EXPR might currently be 
> used only for C++, but I think they are sensible general constructs to be 
> supported by the gimplifier.

As do I.  The intent was for Ada and every other language with things like temporaries and cleanups to reuse the backend constructs, so that instead of writing optimizers, one for each language, to instead share the optimizer across languages.  To me, the middle end and the backend are the best places for these.

Re: New option to turn off stack reuse for temporaries

[Eric Botcazou]

> As do I.  The intent was for Ada and every other language with things like
> temporaries and cleanups to reuse the backend constructs, so that instead
> of writing optimizers, one for each language, to instead share the
> optimizer across languages.  To me, the middle end and the backend are the
> best places for these.

Both are very high-level constructs though.  By the time the AST is converted 
to GENERIC in the Ada compiler, it is already too lowered to make use of them.

-- 
Eric Botcazou

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

(re-post in plain text)

Moving this to cfgexpand time is simple and it can also be extended to
handle scoped variables. However Jakub raised a good point about this
being too late as stack space overlay is not the only way to cause
trouble when the lifetime of a stack object is extended beyond the
clobber stmt.

thanks,

David

On Tue, Jun 26, 2012 at 1:28 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Jun 25, 2012 at 6:25 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Are there any more concerns about this patch? If not, I'd like to check it in.
>
> No - the fact that the flag is C++ specific but in common.opt is odd enough
> and -ftemp-reuse-stack sounds very very generic - which in fact it is not,
> it's a no-op in C.  Is there a more formal phrase for the temporary kind that
> is affected?  For me "temp" is synonymous to "auto" so I'd have expected
> the switch to turn off stack slot sharing for
>
>  {
>   int a[5];
>  }
>  {
>   int a[5];
>  }
>
> but that is not what it does.  So - a little kludgy but probably more to what
> I'd like it to be would be to move the option to c-family/c.opt enabled only
> for C++ and Obj-C++ and export it to the middle-end via a new langhook
> (the gimplifier code should be in Frontend code that lowers to GENERIC
> really and the WITH_CLEANUP_EXPR code should be C++ frontend specific ...).
>
> Thanks,
> Richard.
>
>> thanks,
>>
>> David
>>
>> On Fri, Jun 22, 2012 at 8:51 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Fri, Jun 22, 2012 at 2:39 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
>>>>> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>>>>>
>>>>>>> What other issues? It enables more potential code motion, but on the
>>>>>>> other hand, causes more conservative stack reuse. As far I can tell,
>>>>>>> the handling of temporaries is added independently after the clobber
>>>>>>> for scoped variables are introduced. This option can be used to
>>>>>>> restore the older behavior (in handling temps).
>>>>>>
>>>>>>
>>>>>> Well, it does not really restore the old behavior (if you mean before
>>>>>> adding
>>>>>> CLOBBERS, not before the single patch that might have used those for
>>>>>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>>>>>> for those decls but it also does other things via side-effects of no
>>>>>> longer
>>>>>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>>>>>> sharing.
>>>>>
>>>>>
>>>>> The patch exactly restores the behavior of temporaries from before my change
>>>>> to add CLOBBERs for temporaries.  The primary effect of that change was to
>>>>> provide stack-slot sharing, but if there are other effects they are probably
>>>>> desirable as well, since the broken code depended on the old behavior.
>>>>
>>>> So you see it as workaround option, like -fno-strict-aliasing, rather than
>>>> debugging aid?
>>>
>>> It can be used for both purposes -- if the violations are as pervasive
>>> as strict-aliasing cases (which looks like so).
>>>
>>> thanks,
>>>
>>> David
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Jason

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

[-- Attachment #1: Type: text/plain, Size: 4129 bytes --]

I extended the patch a little so that the option can be used to set
multiple stack reuse levels: -fstack-reuse=[all|name_vars|none]

all: enable stack reuse for all local vars (named vars and compiler
generated temporaries) which live in memory;
name_vars: enable stack reuse only for user declared local vars with names;
none: disable stack reuse completely.

Note the patch still chooses to suppress clobber statement generation
instead of just ignoring them in stack layout. This has the additional
advantage of allowing more aggressive code motion when stack use is
disabled.

The documentation will be updated when the patch is agreed upon.

thanks,

David


On Thu, Jun 28, 2012 at 10:43 PM, Xinliang David Li <davidxl@google.com> wrote:
> (re-post in plain text)
>
> Moving this to cfgexpand time is simple and it can also be extended to
> handle scoped variables. However Jakub raised a good point about this
> being too late as stack space overlay is not the only way to cause
> trouble when the lifetime of a stack object is extended beyond the
> clobber stmt.
>
> thanks,
>
> David
>
> On Tue, Jun 26, 2012 at 1:28 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Mon, Jun 25, 2012 at 6:25 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> Are there any more concerns about this patch? If not, I'd like to check it in.
>>
>> No - the fact that the flag is C++ specific but in common.opt is odd enough
>> and -ftemp-reuse-stack sounds very very generic - which in fact it is not,
>> it's a no-op in C.  Is there a more formal phrase for the temporary kind that
>> is affected?  For me "temp" is synonymous to "auto" so I'd have expected
>> the switch to turn off stack slot sharing for
>>
>>  {
>>   int a[5];
>>  }
>>  {
>>   int a[5];
>>  }
>>
>> but that is not what it does.  So - a little kludgy but probably more to what
>> I'd like it to be would be to move the option to c-family/c.opt enabled only
>> for C++ and Obj-C++ and export it to the middle-end via a new langhook
>> (the gimplifier code should be in Frontend code that lowers to GENERIC
>> really and the WITH_CLEANUP_EXPR code should be C++ frontend specific ...).
>>
>> Thanks,
>> Richard.
>>
>>> thanks,
>>>
>>> David
>>>
>>> On Fri, Jun 22, 2012 at 8:51 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Fri, Jun 22, 2012 at 2:39 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
>>>>>> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>>>>>>
>>>>>>>> What other issues? It enables more potential code motion, but on the
>>>>>>>> other hand, causes more conservative stack reuse. As far I can tell,
>>>>>>>> the handling of temporaries is added independently after the clobber
>>>>>>>> for scoped variables are introduced. This option can be used to
>>>>>>>> restore the older behavior (in handling temps).
>>>>>>>
>>>>>>>
>>>>>>> Well, it does not really restore the old behavior (if you mean before
>>>>>>> adding
>>>>>>> CLOBBERS, not before the single patch that might have used those for
>>>>>>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>>>>>>> for those decls but it also does other things via side-effects of no
>>>>>>> longer
>>>>>>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>>>>>>> sharing.
>>>>>>
>>>>>>
>>>>>> The patch exactly restores the behavior of temporaries from before my change
>>>>>> to add CLOBBERs for temporaries.  The primary effect of that change was to
>>>>>> provide stack-slot sharing, but if there are other effects they are probably
>>>>>> desirable as well, since the broken code depended on the old behavior.
>>>>>
>>>>> So you see it as workaround option, like -fno-strict-aliasing, rather than
>>>>> debugging aid?
>>>>
>>>> It can be used for both purposes -- if the violations are as pervasive
>>>> as strict-aliasing cases (which looks like so).
>>>>
>>>> thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Jason

[-- Attachment #2: reuse-stack.p --]
[-- Type: application/octet-stream, Size: 2387 bytes --]

Index: gimplify.c
===================================================================
--- gimplify.c	(revision 188909)
+++ gimplify.c	(working copy)
@@ -1247,7 +1247,8 @@ gimplify_bind_expr (tree *expr_p, gimple
 	  && !DECL_HAS_VALUE_EXPR_P (t)
 	  /* Only care for variables that have to be in memory.  Others
 	     will be rewritten into SSA names, hence moved to the top-level.  */
-	  && !is_gimple_reg (t))
+	  && !is_gimple_reg (t)
+	  && flag_stack_reuse != SR_NONE)
 	{
 	  tree clobber = build_constructor (TREE_TYPE (t), NULL);
 	  TREE_THIS_VOLATILE (clobber) = 1;
@@ -5634,7 +5635,8 @@ gimplify_target_expr (tree *expr_p, gimp
       /* Add a clobber for the temporary going out of scope, like
 	 gimplify_bind_expr.  */
       if (gimplify_ctxp->in_cleanup_point_expr
-	  && needs_to_live_in_memory (temp))
+	  && needs_to_live_in_memory (temp)
+	  && flag_stack_reuse == SR_ALL)
 	{
 	  tree clobber = build_constructor (TREE_TYPE (temp), NULL);
 	  TREE_THIS_VOLATILE (clobber) = true;
Index: common.opt
===================================================================
--- common.opt	(revision 188909)
+++ common.opt	(working copy)
@@ -1243,6 +1243,22 @@ fif-conversion2
 Common Report Var(flag_if_conversion2) Optimization
 Perform conversion of conditional jumps to conditional execution
 
+fstack-reuse=
+Common Joined RejectNegative Enum(stack_reuse_level) Var(flag_stack_reuse) Init(SR_ALL)
+-fstack-reuse=[all|named_vars|none] Set stack reuse level for local variables.
+
+Enum
+Name(stack_reuse_level) Type(enum stack_reuse_level) UnknownError(unknown Stack Reuse Level %qs)
+
+EnumValue
+Enum(stack_reuse_level) String(all) Value(SR_ALL)
+
+EnumValue
+Enum(stack_reuse_level) String(named_vars) Value(SR_NAMED_VARS)
+
+EnumValue
+Enum(stack_reuse_level) String(none) Value(SR_NONE)
+
 ftree-loop-if-convert
 Common Report Var(flag_tree_loop_if_convert) Init(-1) Optimization
 Convert conditional jumps in innermost loops to branchless equivalents
Index: flag-types.h
===================================================================
--- flag-types.h	(revision 188909)
+++ flag-types.h	(working copy)
@@ -106,6 +106,14 @@ enum symbol_visibility
 };
 #endif
 
+/* The stack reuse level.  */
+enum stack_reuse_level
+{
+  SR_NONE,
+  SR_NAMED_VARS,
+  SR_ALL
+};
+
 /* The algorithm used for the integrated register allocator (IRA).  */
 enum ira_algorithm
 {

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

Comment?

David

On Mon, Jul 2, 2012 at 4:30 PM, Xinliang David Li <davidxl@google.com> wrote:
> I extended the patch a little so that the option can be used to set
> multiple stack reuse levels: -fstack-reuse=[all|name_vars|none]
>
> all: enable stack reuse for all local vars (named vars and compiler
> generated temporaries) which live in memory;
> name_vars: enable stack reuse only for user declared local vars with names;
> none: disable stack reuse completely.
>
> Note the patch still chooses to suppress clobber statement generation
> instead of just ignoring them in stack layout. This has the additional
> advantage of allowing more aggressive code motion when stack use is
> disabled.
>
> The documentation will be updated when the patch is agreed upon.
>
> thanks,
>
> David
>
>
> On Thu, Jun 28, 2012 at 10:43 PM, Xinliang David Li <davidxl@google.com> wrote:
>> (re-post in plain text)
>>
>> Moving this to cfgexpand time is simple and it can also be extended to
>> handle scoped variables. However Jakub raised a good point about this
>> being too late as stack space overlay is not the only way to cause
>> trouble when the lifetime of a stack object is extended beyond the
>> clobber stmt.
>>
>> thanks,
>>
>> David
>>
>> On Tue, Jun 26, 2012 at 1:28 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, Jun 25, 2012 at 6:25 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> Are there any more concerns about this patch? If not, I'd like to check it in.
>>>
>>> No - the fact that the flag is C++ specific but in common.opt is odd enough
>>> and -ftemp-reuse-stack sounds very very generic - which in fact it is not,
>>> it's a no-op in C.  Is there a more formal phrase for the temporary kind that
>>> is affected?  For me "temp" is synonymous to "auto" so I'd have expected
>>> the switch to turn off stack slot sharing for
>>>
>>>  {
>>>   int a[5];
>>>  }
>>>  {
>>>   int a[5];
>>>  }
>>>
>>> but that is not what it does.  So - a little kludgy but probably more to what
>>> I'd like it to be would be to move the option to c-family/c.opt enabled only
>>> for C++ and Obj-C++ and export it to the middle-end via a new langhook
>>> (the gimplifier code should be in Frontend code that lowers to GENERIC
>>> really and the WITH_CLEANUP_EXPR code should be C++ frontend specific ...).
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> thanks,
>>>>
>>>> David
>>>>
>>>> On Fri, Jun 22, 2012 at 8:51 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Fri, Jun 22, 2012 at 2:39 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
>>>>>>> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>>>>>>>
>>>>>>>>> What other issues? It enables more potential code motion, but on the
>>>>>>>>> other hand, causes more conservative stack reuse. As far I can tell,
>>>>>>>>> the handling of temporaries is added independently after the clobber
>>>>>>>>> for scoped variables are introduced. This option can be used to
>>>>>>>>> restore the older behavior (in handling temps).
>>>>>>>>
>>>>>>>>
>>>>>>>> Well, it does not really restore the old behavior (if you mean before
>>>>>>>> adding
>>>>>>>> CLOBBERS, not before the single patch that might have used those for
>>>>>>>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>>>>>>>> for those decls but it also does other things via side-effects of no
>>>>>>>> longer
>>>>>>>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>>>>>>>> sharing.
>>>>>>>
>>>>>>>
>>>>>>> The patch exactly restores the behavior of temporaries from before my change
>>>>>>> to add CLOBBERs for temporaries.  The primary effect of that change was to
>>>>>>> provide stack-slot sharing, but if there are other effects they are probably
>>>>>>> desirable as well, since the broken code depended on the old behavior.
>>>>>>
>>>>>> So you see it as workaround option, like -fno-strict-aliasing, rather than
>>>>>> debugging aid?
>>>>>
>>>>> It can be used for both purposes -- if the violations are as pervasive
>>>>> as strict-aliasing cases (which looks like so).
>>>>>
>>>>> thanks,
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>> Jason

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

Ping ..

On Wed, Jul 4, 2012 at 8:01 AM, Xinliang David Li <davidxl@google.com> wrote:
> Comment?
>
> David
>
> On Mon, Jul 2, 2012 at 4:30 PM, Xinliang David Li <davidxl@google.com> wrote:
>> I extended the patch a little so that the option can be used to set
>> multiple stack reuse levels: -fstack-reuse=[all|name_vars|none]
>>
>> all: enable stack reuse for all local vars (named vars and compiler
>> generated temporaries) which live in memory;
>> name_vars: enable stack reuse only for user declared local vars with names;
>> none: disable stack reuse completely.
>>
>> Note the patch still chooses to suppress clobber statement generation
>> instead of just ignoring them in stack layout. This has the additional
>> advantage of allowing more aggressive code motion when stack use is
>> disabled.
>>
>> The documentation will be updated when the patch is agreed upon.
>>
>> thanks,
>>
>> David
>>
>>
>> On Thu, Jun 28, 2012 at 10:43 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> (re-post in plain text)
>>>
>>> Moving this to cfgexpand time is simple and it can also be extended to
>>> handle scoped variables. However Jakub raised a good point about this
>>> being too late as stack space overlay is not the only way to cause
>>> trouble when the lifetime of a stack object is extended beyond the
>>> clobber stmt.
>>>
>>> thanks,
>>>
>>> David
>>>
>>> On Tue, Jun 26, 2012 at 1:28 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Mon, Jun 25, 2012 at 6:25 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> Are there any more concerns about this patch? If not, I'd like to check it in.
>>>>
>>>> No - the fact that the flag is C++ specific but in common.opt is odd enough
>>>> and -ftemp-reuse-stack sounds very very generic - which in fact it is not,
>>>> it's a no-op in C.  Is there a more formal phrase for the temporary kind that
>>>> is affected?  For me "temp" is synonymous to "auto" so I'd have expected
>>>> the switch to turn off stack slot sharing for
>>>>
>>>>  {
>>>>   int a[5];
>>>>  }
>>>>  {
>>>>   int a[5];
>>>>  }
>>>>
>>>> but that is not what it does.  So - a little kludgy but probably more to what
>>>> I'd like it to be would be to move the option to c-family/c.opt enabled only
>>>> for C++ and Obj-C++ and export it to the middle-end via a new langhook
>>>> (the gimplifier code should be in Frontend code that lowers to GENERIC
>>>> really and the WITH_CLEANUP_EXPR code should be C++ frontend specific ...).
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> thanks,
>>>>>
>>>>> David
>>>>>
>>>>> On Fri, Jun 22, 2012 at 8:51 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> On Fri, Jun 22, 2012 at 2:39 AM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
>>>>>>>> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>>>>>>>>
>>>>>>>>>> What other issues? It enables more potential code motion, but on the
>>>>>>>>>> other hand, causes more conservative stack reuse. As far I can tell,
>>>>>>>>>> the handling of temporaries is added independently after the clobber
>>>>>>>>>> for scoped variables are introduced. This option can be used to
>>>>>>>>>> restore the older behavior (in handling temps).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Well, it does not really restore the old behavior (if you mean before
>>>>>>>>> adding
>>>>>>>>> CLOBBERS, not before the single patch that might have used those for
>>>>>>>>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>>>>>>>>> for those decls but it also does other things via side-effects of no
>>>>>>>>> longer
>>>>>>>>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>>>>>>>>> sharing.
>>>>>>>>
>>>>>>>>
>>>>>>>> The patch exactly restores the behavior of temporaries from before my change
>>>>>>>> to add CLOBBERs for temporaries.  The primary effect of that change was to
>>>>>>>> provide stack-slot sharing, but if there are other effects they are probably
>>>>>>>> desirable as well, since the broken code depended on the old behavior.
>>>>>>>
>>>>>>> So you see it as workaround option, like -fno-strict-aliasing, rather than
>>>>>>> debugging aid?
>>>>>>
>>>>>> It can be used for both purposes -- if the violations are as pervasive
>>>>>> as strict-aliasing cases (which looks like so).
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>> Jason

Re: New option to turn off stack reuse for temporaries

[Jason Merrill]

This looks fine to me, if nobody has any more objections.

Jason

Re: New option to turn off stack reuse for temporaries

[Olivier Ballereau]

Hello David,

Sorry to come so late into the discussion, but...

On 21/06/12 00:50, Xinliang David Li wrote:
> One of the most common runtime errors we have seen in gcc-4_7 is
> caused by dangling references to temporaries whole life time have
> ended
> 
> e.g,
> 
>  const A& a = foo();
> 
> or
> foo (A());// where temp's address is saved and used after foo.
> 
> Of course this is user error according to the standard,
> [...]

... is the first of your 2 examples really a user error? If so, it
breaks GotW #88: A Candidate For the “Most Important const” [1]. Can you
please clarify?

Thanks in advance!
Olivier

[1]
http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/

Re: New option to turn off stack reuse for temporaries

[Xinliang David Li]

My first example is not correct --- according to the standard, the
lifetime of the temporary should be extended. The second example is an
user error.

 C++ standard says this in 12.2.5:

"
The second context is when a reference is bound to a temporary. The
temporary to which the reference is
bound or the temporary that is the complete object of a subobject to
which the reference is bound persists
for the lifetime of the reference except:
— A temporary bound to a reference member in a constructor’s
ctor-initializer (12.6.2) persists until the
constructor exits.
— A temporary bound to a reference parameter in a function call
(5.2.2) persists until the completion of
the full-expression containing the call.
— The lifetime of a temporary bound to the returned value in a
function return statement (6.6.3) is not
extended; the temporary is destroyed at the end of the full-expression
in the return statement.
— A temporary bound to a reference in a new-initializer (5.3.4)
persists until the completion of the
full-expression containing the new-initializer. [Example:
struct S { int mi; const std::pair<int,int>& mp; };
S a { 1, {2,3} };
S* p = new S{ 1, {2,3} }; // Creates dangling reference
— end example ] [ Note: This may introduce a dangling reference, and
implementations are encouraged
to issue a warning in such a case. — end note ]
"

David




On Sun, Dec 2, 2012 at 4:31 AM, Olivier Ballereau
<Olivier.Ballereau@gmx.net> wrote:
> Hello David,
>
> Sorry to come so late into the discussion, but...
>
> On 21/06/12 00:50, Xinliang David Li wrote:
>> One of the most common runtime errors we have seen in gcc-4_7 is
>> caused by dangling references to temporaries whole life time have
>> ended
>>
>> e.g,
>>
>>  const A& a = foo();
>>
>> or
>> foo (A());// where temp's address is saved and used after foo.
>>
>> Of course this is user error according to the standard,
>> [...]
>
> ... is the first of your 2 examples really a user error? If so, it
> breaks GotW #88: A Candidate For the “Most Important const” [1]. Can you
> please clarify?
>
> Thanks in advance!
> Olivier
>
> [1]
> http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
>

Re: New option to turn off stack reuse for temporaries

[Jason Merrill]

Yes.

-------- Original Message --------
 From: Richard Guenther <richard.guenther@gmail.com>
 Sent: Fri, Jun 22, 2012 02:39 AM
 To: Jason Merrill <jason@redhat.com>
 CC: Xinliang David Li <davidxl@google.com>; GCC Patches <gcc-patches@gcc.gnu.org>; Michael Matz <matz@suse.de>
 Subject: Re: New option to turn off stack reuse for temporaries

On Fri, Jun 22, 2012 at 11:29 AM, Jason Merrill <jason@redhat.com> wrote:
> On 06/22/2012 01:30 AM, Richard Guenther wrote:
>>>
>>> What other issues? It enables more potential code motion, but on the
>>> other hand, causes more conservative stack reuse. As far I can tell,
>>> the handling of temporaries is added independently after the clobber
>>> for scoped variables are introduced. This option can be used to
>>> restore the older behavior (in handling temps).
>>
>>
>> Well, it does not really restore the old behavior (if you mean before
>> adding
>> CLOBBERS, not before the single patch that might have used those for
>> gimplifying WITH_CLEANUP_EXPR).  You say it disables stack-slot sharing
>> for those decls but it also does other things via side-effects of no
>> longer
>> emitting the CLOBBER.  I say it's better to disable the stack-slot
>> sharing.
>
>
> The patch exactly restores the behavior of temporaries from before my change
> to add CLOBBERs for temporaries.  The primary effect of that change was to
> provide stack-slot sharing, but if there are other effects they are probably
> desirable as well, since the broken code depended on the old behavior.

So you see it as workaround option, like -fno-strict-aliasing, rather than
debugging aid?

Richard.

> Jason