| Message ID | CAAkRFZK_27tveOTLioz-YwfF_4w4qtTJkRFjno7r8risRVQsnQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
OK. Jason
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
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.
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
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
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.
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
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
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
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
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
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
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
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.
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
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.
> 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.
(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
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
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
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
This looks fine to me, if nobody has any more objections. Jason
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/
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/
>
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
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.