Re: [PATCH] [RFC] PR target/52813 and target/11807
Christophe Lyon writes: > On Fri, 11 Jan 2019 at 23:59, Jeff Law wrote: >> >> On 1/8/19 5:03 AM, Richard Sandiford wrote: >> > Bernd Edlinger writes: >> >> On 1/7/19 10:23 AM, Jakub Jelinek wrote: >> >>> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote: >> - /* Clobbering the STACK POINTER register is an error. */ >> + /* Clobbered STACK POINTER register is not saved/restored by GCC, >> + which is often unexpected by users. See PR52813. */ >> if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) >> { >> - error ("Stack Pointer register clobbered by %qs in %", >> regname); >> + warning (0, "Stack Pointer register clobbered by %qs in %", >> + regname); >> + warning (0, "GCC has always ignored Stack Pointer % >> clobbers"); >> >>> >> >>> Why do we write Stack Pointer rather than stack pointer? That is really >> >>> weird. The second warning would be a note based on the first one, i.e. >> >>> if (warning ()) note (); >> >>> and better have some -W* option to silence the warning. >> >>> >> >> >> >> Yes, thanks for this suggestion. >> >> >> >> Meanwhile I found out, that the stack clobber has only been ignored up to >> >> gcc-5 (at least with lra targets, not really sure about reload targets). >> >> From gcc-6 on, with the exception of PR arm/77904 which was a regression >> >> due >> >> to the underlying lra change, but fixed later, and back-ported to >> >> gcc-6.3.0, >> >> this works for all targets I tried so far. >> >> >> >> To me, it starts to look like a rather unique and useful feature, that I >> >> would >> >> like to keep working. >> > >> > Not sure what you mean by "unique". But forcing a frame is a bit of >> > a slippery concept. Force it where? For the asm only, or the whole >> > function? This depends on optimisation and hasn't been consistent >> > across GCC versions, since it depends on the shrink-wrapping >> > optimisation. (There was a similar controversy a while ago about >> > to what extent -fno-omit-frame-pointer should "force a frame".) >> > >> > The effect on the redzone seems like something that should be specified >> > explicitly rather than as an (accidental?) side effect of listing the >> > sp in the clobber list. Maybe this would be another use for the "asm >> > attributes" proposal. "noreturn" was another attribute suggested on >> > IRC yesterday. >> > >> > But either way, the general feeling seems to be that going straight to a >> > hard error is too harsh, since there's quite a bit of existing code that >> > has the clobber. This patch implements the compromise discussed on IRC >> > yesterday of making it a -Wdeprecated warning instead. >> > >> > Tested on x86_64-linux-gnu and aarch64-linux-gnu. OK to install? >> > >> > Richard >> > >> > Dimitar: sorry the run-around on this patch, and thanks for the >> > submission. It looks from all the controversy like it was a >> > long-festering PR for a reason. :-/ >> > >> > >> > 2019-01-07 Richard Sandiford >> > >> > gcc/ >> > PR inline-asm/52813 >> > * doc/extend.texi: Document that listing the stack pointer in the >> > clobber list of an asm is a deprecated feature. >> > * common.opt (Wdeprecated): Moved from c-family/c.opt. >> > * cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated >> > warning instead of an error for clobbers of the stack pointer. >> > Add a note explaining why. >> > >> > gcc/c-family/ >> > PR inline-asm/52813 >> > * c.opt (Wdeprecated): Move documentation and variable to common.opt. >> > >> > gcc/d/ >> > PR inline-asm/52813 >> > * lang.opt (Wdeprecated): Reference common.opt instead of c.opt. >> > >> > gcc/testsuite/ >> > PR inline-asm/52813 >> > * gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a >> > -Wdeprecated warning and expect a following note:. >> OK. >> >> FWIW the number of packages affected in Fedora was in single digits, >> some of which have already been fixed. >> >> But if folks want to go with a deprecated warning instead of straight to >> an error, I won't complain. >> >> jeff > > > Hi, > > I originally complained because the arm test for pr77904.c was failing. > Since Richard's change that test emits a warning rather than an error, > but still fails. This small patch adds the missing dg-warning. > > OK? > > Thanks, > > Christophe > > 2019-01-17 Christophe Lyon > > * gcc.target/arm/pr77904.c: Add dg-warning for sp clobber. OK, thanks. Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Fri, 11 Jan 2019 at 23:59, Jeff Law wrote: > > On 1/8/19 5:03 AM, Richard Sandiford wrote: > > Bernd Edlinger writes: > >> On 1/7/19 10:23 AM, Jakub Jelinek wrote: > >>> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote: > - /* Clobbering the STACK POINTER register is an error. */ > + /* Clobbered STACK POINTER register is not saved/restored by GCC, > + which is often unexpected by users. See PR52813. */ > if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) > { > - error ("Stack Pointer register clobbered by %qs in %", > regname); > + warning (0, "Stack Pointer register clobbered by %qs in %", > + regname); > + warning (0, "GCC has always ignored Stack Pointer % > clobbers"); > >>> > >>> Why do we write Stack Pointer rather than stack pointer? That is really > >>> weird. The second warning would be a note based on the first one, i.e. > >>> if (warning ()) note (); > >>> and better have some -W* option to silence the warning. > >>> > >> > >> Yes, thanks for this suggestion. > >> > >> Meanwhile I found out, that the stack clobber has only been ignored up to > >> gcc-5 (at least with lra targets, not really sure about reload targets). > >> From gcc-6 on, with the exception of PR arm/77904 which was a regression > >> due > >> to the underlying lra change, but fixed later, and back-ported to > >> gcc-6.3.0, > >> this works for all targets I tried so far. > >> > >> To me, it starts to look like a rather unique and useful feature, that I > >> would > >> like to keep working. > > > > Not sure what you mean by "unique". But forcing a frame is a bit of > > a slippery concept. Force it where? For the asm only, or the whole > > function? This depends on optimisation and hasn't been consistent > > across GCC versions, since it depends on the shrink-wrapping > > optimisation. (There was a similar controversy a while ago about > > to what extent -fno-omit-frame-pointer should "force a frame".) > > > > The effect on the redzone seems like something that should be specified > > explicitly rather than as an (accidental?) side effect of listing the > > sp in the clobber list. Maybe this would be another use for the "asm > > attributes" proposal. "noreturn" was another attribute suggested on > > IRC yesterday. > > > > But either way, the general feeling seems to be that going straight to a > > hard error is too harsh, since there's quite a bit of existing code that > > has the clobber. This patch implements the compromise discussed on IRC > > yesterday of making it a -Wdeprecated warning instead. > > > > Tested on x86_64-linux-gnu and aarch64-linux-gnu. OK to install? > > > > Richard > > > > Dimitar: sorry the run-around on this patch, and thanks for the > > submission. It looks from all the controversy like it was a > > long-festering PR for a reason. :-/ > > > > > > 2019-01-07 Richard Sandiford > > > > gcc/ > > PR inline-asm/52813 > > * doc/extend.texi: Document that listing the stack pointer in the > > clobber list of an asm is a deprecated feature. > > * common.opt (Wdeprecated): Moved from c-family/c.opt. > > * cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated > > warning instead of an error for clobbers of the stack pointer. > > Add a note explaining why. > > > > gcc/c-family/ > > PR inline-asm/52813 > > * c.opt (Wdeprecated): Move documentation and variable to common.opt. > > > > gcc/d/ > > PR inline-asm/52813 > > * lang.opt (Wdeprecated): Reference common.opt instead of c.opt. > > > > gcc/testsuite/ > > PR inline-asm/52813 > > * gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a > > -Wdeprecated warning and expect a following note:. > OK. > > FWIW the number of packages affected in Fedora was in single digits, > some of which have already been fixed. > > But if folks want to go with a deprecated warning instead of straight to > an error, I won't complain. > > jeff Hi, I originally complained because the arm test for pr77904.c was failing. Since Richard's change that test emits a warning rather than an error, but still fails. This small patch adds the missing dg-warning. OK? Thanks, Christophe 2019-01-17 Christophe Lyon * gcc.target/arm/pr77904.c: Add dg-warning for sp clobber. diff --git a/gcc/testsuite/gcc.target/arm/pr77904.c b/gcc/testsuite/gcc.target/arm/pr77904.c index 76728c0..f279ec8 100644 --- a/gcc/testsuite/gcc.target/arm/pr77904.c +++ b/gcc/testsuite/gcc.target/arm/pr77904.c @@ -4,7 +4,8 @@ __attribute__ ((noinline, noclone)) void clobber_sp (void) { - __asm volatile ("" : : : "sp"); + __asm volatile ("" : : : "sp"); /* { dg-warning "listing the stack pointer register 'sp' in a clobber list is deprecated" } */ + } int
Re: [PATCH] [RFC] PR target/52813 and target/11807
On 1/8/19 5:03 AM, Richard Sandiford wrote: > Bernd Edlinger writes: >> On 1/7/19 10:23 AM, Jakub Jelinek wrote: >>> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote: - /* Clobbering the STACK POINTER register is an error. */ + /* Clobbered STACK POINTER register is not saved/restored by GCC, + which is often unexpected by users. See PR52813. */ if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) { - error ("Stack Pointer register clobbered by %qs in %", regname); + warning (0, "Stack Pointer register clobbered by %qs in %", + regname); + warning (0, "GCC has always ignored Stack Pointer % clobbers"); >>> >>> Why do we write Stack Pointer rather than stack pointer? That is really >>> weird. The second warning would be a note based on the first one, i.e. >>> if (warning ()) note (); >>> and better have some -W* option to silence the warning. >>> >> >> Yes, thanks for this suggestion. >> >> Meanwhile I found out, that the stack clobber has only been ignored up to >> gcc-5 (at least with lra targets, not really sure about reload targets). >> From gcc-6 on, with the exception of PR arm/77904 which was a regression due >> to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0, >> this works for all targets I tried so far. >> >> To me, it starts to look like a rather unique and useful feature, that I >> would >> like to keep working. > > Not sure what you mean by "unique". But forcing a frame is a bit of > a slippery concept. Force it where? For the asm only, or the whole > function? This depends on optimisation and hasn't been consistent > across GCC versions, since it depends on the shrink-wrapping > optimisation. (There was a similar controversy a while ago about > to what extent -fno-omit-frame-pointer should "force a frame".) > > The effect on the redzone seems like something that should be specified > explicitly rather than as an (accidental?) side effect of listing the > sp in the clobber list. Maybe this would be another use for the "asm > attributes" proposal. "noreturn" was another attribute suggested on > IRC yesterday. > > But either way, the general feeling seems to be that going straight to a > hard error is too harsh, since there's quite a bit of existing code that > has the clobber. This patch implements the compromise discussed on IRC > yesterday of making it a -Wdeprecated warning instead. > > Tested on x86_64-linux-gnu and aarch64-linux-gnu. OK to install? > > Richard > > Dimitar: sorry the run-around on this patch, and thanks for the > submission. It looks from all the controversy like it was a > long-festering PR for a reason. :-/ > > > 2019-01-07 Richard Sandiford > > gcc/ > PR inline-asm/52813 > * doc/extend.texi: Document that listing the stack pointer in the > clobber list of an asm is a deprecated feature. > * common.opt (Wdeprecated): Moved from c-family/c.opt. > * cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated > warning instead of an error for clobbers of the stack pointer. > Add a note explaining why. > > gcc/c-family/ > PR inline-asm/52813 > * c.opt (Wdeprecated): Move documentation and variable to common.opt. > > gcc/d/ > PR inline-asm/52813 > * lang.opt (Wdeprecated): Reference common.opt instead of c.opt. > > gcc/testsuite/ > PR inline-asm/52813 > * gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a > -Wdeprecated warning and expect a following note:. OK. FWIW the number of packages affected in Fedora was in single digits, some of which have already been fixed. But if folks want to go with a deprecated warning instead of straight to an error, I won't complain. jeff
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Thu, Jan 10, 2019 at 09:56:28PM +, Richard Sandiford wrote: > Jakub Jelinek writes: > > On Thu, Jan 10, 2019 at 09:23:27PM +, Richard Sandiford wrote: > >> > "noreturn"... What would that mean, *exactly*? It cannot execute any > >> > code the compiler can see, so such asm is better off as real asm anyway > >> > (not inline asm). > >> > >> "Exactly" is a strong word, and this wasn't my proposal, but... > >> I think it would act like a noreturn call to an unknown function. > >> Output operands wouldn't make sense, and arguably clobbers wouldn't > >> either. > > > > "noreturn" asm can be done already now, just use > > asm volatile ("..." ...); > > __builtin_unreachable (); > > > > I think there is no need to add a new syntax for that. Ah yes, the volatile makes this work. Cool. And as Richard says such asm shouldn't have outputs anyway, so it will always be volatile. > ISTR the point was that the PowerPC ABI places requirements on functions > with noreturn calls I don't think any of our ABIs do that? Is this about the back chain? > and the attribute would help GCC do the right thing > in those circumstances. So "noreturn" would imply a call that doesn't > return, rather than just an infinite loop. Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
Segher Boessenkool writes: >> I think it would act like a noreturn call to an unknown function. > > Except it won't behave like a call otherwise (on Power all calls force a > stack frame, for example; and on all targets noreturn calls do the same > currently I think?) I agree no current asm would behave like a call (e.g. causing leaf_p to be false, and more). The point of adding the attribute would be to change that. Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Thu, Jan 10, 2019 at 09:23:27PM +, Richard Sandiford wrote: > Segher Boessenkool writes: > > On Tue, Jan 08, 2019 at 12:03:06PM +, Richard Sandiford wrote: > >> Bernd Edlinger writes: > >> > Meanwhile I found out, that the stack clobber has only been ignored up to > >> > gcc-5 (at least with lra targets, not really sure about reload targets). > >> > From gcc-6 on, with the exception of PR arm/77904 which was a regression > >> > due > >> > to the underlying lra change, but fixed later, and back-ported to > >> > gcc-6.3.0, > >> > this works for all targets I tried so far. > >> > > >> > To me, it starts to look like a rather unique and useful feature, that I > >> > would > >> > like to keep working. > >> > >> Not sure what you mean by "unique". But forcing a frame is a bit of > >> a slippery concept. Force it where? For the asm only, or the whole > >> function? This depends on optimisation and hasn't been consistent > >> across GCC versions, since it depends on the shrink-wrapping > >> optimisation. (There was a similar controversy a while ago about > >> to what extent -fno-omit-frame-pointer should "force a frame".) > > > > It's not forcing a frame currently: it's just setting frame_pointer_needed. > > Whatever happens from that is the target's business. > > Do you mean the asm clobber or -fno-omit-frame-pointer? If the option, > then yeah, and that was exactly what was controversial :-) I meant the asm clobber. LRA sets frame_pointer_needed to 1 because the stack pointer is clobbered, on many targets anyway: /* If we modify the source of an elimination rule, disable it. Do the same if it is the destination and not the hard frame register. */ for (ep = reg_eliminate; ep < _eliminate[NUM_ELIMINABLE_REGS]; ep++) if (ep->from_rtx == XEXP (x, 0) || (ep->to_rtx == XEXP (x, 0) && ep->to_rtx != hard_frame_pointer_rtx)) setup_can_eliminate (ep, false); and setup_can_eliminate has if (! value && ep->from == FRAME_POINTER_REGNUM && ep->to == STACK_POINTER_REGNUM) frame_pointer_needed = 1; > >> The effect on the redzone seems like something that should be specified > >> explicitly rather than as an (accidental?) side effect of listing the > >> sp in the clobber list. Maybe this would be another use for the "asm > >> attributes" proposal. "noreturn" was another attribute suggested on > >> IRC yesterday. > > > > Redzone is target-dependent. > > Right. Target-dependent asm attributes wouldn't be a problem though. It's harder to document, which means it is harder to get right (and get people to _use_ it correctly), as well. > Most other things about an asm are target-dependent anyway. Very true. > > "noreturn"... What would that mean, *exactly*? It cannot execute any > > code the compiler can see, so such asm is better off as real asm anyway > > (not inline asm). > > "Exactly" is a strong word, and this wasn't my proposal, but... "Exactly", as in, "please do enough hand-waving to cover all available space" ;-) There are many details that aren't quite obvious, but they do matter for how usable and useful this extension would be. > I think it would act like a noreturn call to an unknown function. Except it won't behave like a call otherwise (on Power all calls force a stack frame, for example; and on all targets noreturn calls do the same currently I think?) > Output operands wouldn't make sense, and arguably clobbers wouldn't > either. Yeah. Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
On 1/10/19 10:23 PM, Richard Sandiford wrote: > Segher Boessenkool writes: >> On Tue, Jan 08, 2019 at 12:03:06PM +, Richard Sandiford wrote: >>> Bernd Edlinger writes: Meanwhile I found out, that the stack clobber has only been ignored up to gcc-5 (at least with lra targets, not really sure about reload targets). From gcc-6 on, with the exception of PR arm/77904 which was a regression due to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0, this works for all targets I tried so far. To me, it starts to look like a rather unique and useful feature, that I would like to keep working. >>> >>> Not sure what you mean by "unique". But forcing a frame is a bit of >>> a slippery concept. Force it where? For the asm only, or the whole >>> function? This depends on optimisation and hasn't been consistent >>> across GCC versions, since it depends on the shrink-wrapping >>> optimisation. (There was a similar controversy a while ago about >>> to what extent -fno-omit-frame-pointer should "force a frame".) >> >> It's not forcing a frame currently: it's just setting frame_pointer_needed. >> Whatever happens from that is the target's business. > > Do you mean the asm clobber or -fno-omit-frame-pointer? If the option, > then yeah, and that was exactly what was controversial :-) > Yes, what I meant is the asm clobber sets frame_pointer_needed, on the function where this asm is used, that sounded to me like it would have an impact on the frame pointer. What I also expected, is that if an asm is accessing a local via "m" then the a SP+x reference will be elimitated to a FP+x, reference, which would allow the asm to push something on the stack, and the memory references would remain valid, as long as the stack is _restored_, again in the same asm. I mean in case of register shortage. I was not thinking about noreturn at all. But if -fno-omit-frame-pointer does the same, and that is not sufficient to for forcing a frame pointer, because it is a target dependent, then I wonder how ASAN is supposed to work on such a target. But anyway I guess, your patch is fine. Thanks Bernd.
Re: [PATCH] [RFC] PR target/52813 and target/11807
Jakub Jelinek writes: > On Thu, Jan 10, 2019 at 09:23:27PM +, Richard Sandiford wrote: >> > "noreturn"... What would that mean, *exactly*? It cannot execute any >> > code the compiler can see, so such asm is better off as real asm anyway >> > (not inline asm). >> >> "Exactly" is a strong word, and this wasn't my proposal, but... >> I think it would act like a noreturn call to an unknown function. >> Output operands wouldn't make sense, and arguably clobbers wouldn't >> either. > > "noreturn" asm can be done already now, just use > asm volatile ("..." ...); > __builtin_unreachable (); > > I think there is no need to add a new syntax for that. ISTR the point was that the PowerPC ABI places requirements on functions with noreturn calls and the attribute would help GCC do the right thing in those circumstances. So "noreturn" would imply a call that doesn't return, rather than just an infinite loop. Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Thu, Jan 10, 2019 at 09:23:27PM +, Richard Sandiford wrote: > > "noreturn"... What would that mean, *exactly*? It cannot execute any > > code the compiler can see, so such asm is better off as real asm anyway > > (not inline asm). > > "Exactly" is a strong word, and this wasn't my proposal, but... > I think it would act like a noreturn call to an unknown function. > Output operands wouldn't make sense, and arguably clobbers wouldn't > either. "noreturn" asm can be done already now, just use asm volatile ("..." ...); __builtin_unreachable (); I think there is no need to add a new syntax for that. Jakub
Re: [PATCH] [RFC] PR target/52813 and target/11807
Segher Boessenkool writes: > On Tue, Jan 08, 2019 at 12:03:06PM +, Richard Sandiford wrote: >> Bernd Edlinger writes: >> > Meanwhile I found out, that the stack clobber has only been ignored up to >> > gcc-5 (at least with lra targets, not really sure about reload targets). >> > From gcc-6 on, with the exception of PR arm/77904 which was a regression >> > due >> > to the underlying lra change, but fixed later, and back-ported to >> > gcc-6.3.0, >> > this works for all targets I tried so far. >> > >> > To me, it starts to look like a rather unique and useful feature, that I >> > would >> > like to keep working. >> >> Not sure what you mean by "unique". But forcing a frame is a bit of >> a slippery concept. Force it where? For the asm only, or the whole >> function? This depends on optimisation and hasn't been consistent >> across GCC versions, since it depends on the shrink-wrapping >> optimisation. (There was a similar controversy a while ago about >> to what extent -fno-omit-frame-pointer should "force a frame".) > > It's not forcing a frame currently: it's just setting frame_pointer_needed. > Whatever happens from that is the target's business. Do you mean the asm clobber or -fno-omit-frame-pointer? If the option, then yeah, and that was exactly what was controversial :-) >> The effect on the redzone seems like something that should be specified >> explicitly rather than as an (accidental?) side effect of listing the >> sp in the clobber list. Maybe this would be another use for the "asm >> attributes" proposal. "noreturn" was another attribute suggested on >> IRC yesterday. > > Redzone is target-dependent. Right. Target-dependent asm attributes wouldn't be a problem though. Most other things about an asm are target-dependent anyway. > "noreturn"... What would that mean, *exactly*? It cannot execute any > code the compiler can see, so such asm is better off as real asm anyway > (not inline asm). "Exactly" is a strong word, and this wasn't my proposal, but... I think it would act like a noreturn call to an unknown function. Output operands wouldn't make sense, and arguably clobbers wouldn't either. Thanks, Richard >> But either way, the general feeling seems to be that going straight to a >> hard error is too harsh, since there's quite a bit of existing code that >> has the clobber. This patch implements the compromise discussed on IRC >> yesterday of making it a -Wdeprecated warning instead. > > The patch looks fine to me. Thanks! > > > Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
Hi! On Tue, Jan 08, 2019 at 12:03:06PM +, Richard Sandiford wrote: > Bernd Edlinger writes: > > Meanwhile I found out, that the stack clobber has only been ignored up to > > gcc-5 (at least with lra targets, not really sure about reload targets). > > From gcc-6 on, with the exception of PR arm/77904 which was a regression due > > to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0, > > this works for all targets I tried so far. > > > > To me, it starts to look like a rather unique and useful feature, that I > > would > > like to keep working. > > Not sure what you mean by "unique". But forcing a frame is a bit of > a slippery concept. Force it where? For the asm only, or the whole > function? This depends on optimisation and hasn't been consistent > across GCC versions, since it depends on the shrink-wrapping > optimisation. (There was a similar controversy a while ago about > to what extent -fno-omit-frame-pointer should "force a frame".) It's not forcing a frame currently: it's just setting frame_pointer_needed. Whatever happens from that is the target's business. > The effect on the redzone seems like something that should be specified > explicitly rather than as an (accidental?) side effect of listing the > sp in the clobber list. Maybe this would be another use for the "asm > attributes" proposal. "noreturn" was another attribute suggested on > IRC yesterday. Redzone is target-dependent. "noreturn"... What would that mean, *exactly*? It cannot execute any code the compiler can see, so such asm is better off as real asm anyway (not inline asm). > But either way, the general feeling seems to be that going straight to a > hard error is too harsh, since there's quite a bit of existing code that > has the clobber. This patch implements the compromise discussed on IRC > yesterday of making it a -Wdeprecated warning instead. The patch looks fine to me. Thanks! Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
Bernd Edlinger writes: > On 1/7/19 10:23 AM, Jakub Jelinek wrote: >> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote: >>> - /* Clobbering the STACK POINTER register is an error. */ >>> + /* Clobbered STACK POINTER register is not saved/restored by GCC, >>> + which is often unexpected by users. See PR52813. */ >>>if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) >>> { >>> - error ("Stack Pointer register clobbered by %qs in %", >>> regname); >>> + warning (0, "Stack Pointer register clobbered by %qs in %", >>> + regname); >>> + warning (0, "GCC has always ignored Stack Pointer % clobbers"); >> >> Why do we write Stack Pointer rather than stack pointer? That is really >> weird. The second warning would be a note based on the first one, i.e. >> if (warning ()) note (); >> and better have some -W* option to silence the warning. >> > > Yes, thanks for this suggestion. > > Meanwhile I found out, that the stack clobber has only been ignored up to > gcc-5 (at least with lra targets, not really sure about reload targets). > From gcc-6 on, with the exception of PR arm/77904 which was a regression due > to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0, > this works for all targets I tried so far. > > To me, it starts to look like a rather unique and useful feature, that I would > like to keep working. Not sure what you mean by "unique". But forcing a frame is a bit of a slippery concept. Force it where? For the asm only, or the whole function? This depends on optimisation and hasn't been consistent across GCC versions, since it depends on the shrink-wrapping optimisation. (There was a similar controversy a while ago about to what extent -fno-omit-frame-pointer should "force a frame".) The effect on the redzone seems like something that should be specified explicitly rather than as an (accidental?) side effect of listing the sp in the clobber list. Maybe this would be another use for the "asm attributes" proposal. "noreturn" was another attribute suggested on IRC yesterday. But either way, the general feeling seems to be that going straight to a hard error is too harsh, since there's quite a bit of existing code that has the clobber. This patch implements the compromise discussed on IRC yesterday of making it a -Wdeprecated warning instead. Tested on x86_64-linux-gnu and aarch64-linux-gnu. OK to install? Richard Dimitar: sorry the run-around on this patch, and thanks for the submission. It looks from all the controversy like it was a long-festering PR for a reason. :-/ 2019-01-07 Richard Sandiford gcc/ PR inline-asm/52813 * doc/extend.texi: Document that listing the stack pointer in the clobber list of an asm is a deprecated feature. * common.opt (Wdeprecated): Moved from c-family/c.opt. * cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated warning instead of an error for clobbers of the stack pointer. Add a note explaining why. gcc/c-family/ PR inline-asm/52813 * c.opt (Wdeprecated): Move documentation and variable to common.opt. gcc/d/ PR inline-asm/52813 * lang.opt (Wdeprecated): Reference common.opt instead of c.opt. gcc/testsuite/ PR inline-asm/52813 * gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a -Wdeprecated warning and expect a following note:. Index: gcc/doc/extend.texi === --- gcc/doc/extend.texi 2019-01-07 12:14:31.699490485 + +++ gcc/doc/extend.texi 2019-01-08 11:40:20.807906878 + @@ -9441,6 +9441,15 @@ When the compiler selects which register operands, it does not use any of the clobbered registers. As a result, clobbered registers are available for any use in the assembler code. +Another restriction is that the clobber list should not contain the +stack pointer register. This is because the compiler requires the +value of the stack pointer to be the same after an @code{asm} +statement as it was on entry to the statement. However, previous +versions of GCC did not enforce this rule and allowed the stack +pointer to appear in the list, with unclear semantics. This behavior +is deprecated and listing the stack pointer may become an error in +future versions of GCC@. + Here is a realistic example for the VAX showing the use of clobbered registers: Index: gcc/common.opt === --- gcc/common.opt 2019-01-04 11:39:27.178246751 + +++ gcc/common.opt 2019-01-08 11:40:20.803906912 + @@ -579,6 +579,10 @@ Wattribute-warning Common Var(warn_attribute_warning) Init(1) Warning Warn about uses of __attribute__((warning)) declarations. +Wdeprecated +Common Var(warn_deprecated) Init(1) Warning +Warn if a deprecated compiler feature, class, method, or field is used. +
Re: [PATCH] [RFC] PR target/52813 and target/11807
On 1/7/19 10:23 AM, Jakub Jelinek wrote: > On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote: >> - /* Clobbering the STACK POINTER register is an error. */ >> + /* Clobbered STACK POINTER register is not saved/restored by GCC, >> + which is often unexpected by users. See PR52813. */ >>if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) >> { >> - error ("Stack Pointer register clobbered by %qs in %", regname); >> + warning (0, "Stack Pointer register clobbered by %qs in %", >> + regname); >> + warning (0, "GCC has always ignored Stack Pointer % clobbers"); > > Why do we write Stack Pointer rather than stack pointer? That is really > weird. The second warning would be a note based on the first one, i.e. > if (warning ()) note (); > and better have some -W* option to silence the warning. > Yes, thanks for this suggestion. Meanwhile I found out, that the stack clobber has only been ignored up to gcc-5 (at least with lra targets, not really sure about reload targets). From gcc-6 on, with the exception of PR arm/77904 which was a regression due to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0, this works for all targets I tried so far. To me, it starts to look like a rather unique and useful feature, that I would like to keep working. Attached is an updated version if my patch, using the suggested warning option, and a note with the details. Bootstrapped on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. 2018-01-07 Bernd Edlinger * doc/invoke.texi: Document -Wstack-clobber. * common.opt (-Wstack-clobber): New default-enabled warning. * cfgexpand.c (asm_clobber_reg_is_valid): Emit only a warning together with an informative note when the stack pointer is clobbered. testsuite: 2018-07-01 Bernd Edlinger * gcc.target/arm/pr77904.c: Adjust test. * gcc.target/i386/pr52813.c: Adjust test. Index: gcc/cfgexpand.c === --- gcc/cfgexpand.c (revision 267653) +++ gcc/cfgexpand.c (working copy) @@ -2854,6 +2854,7 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_S asm clobber operand. Some HW registers cannot be saved/restored, hence they should not be clobbered by asm statements. */ + static bool asm_clobber_reg_is_valid (int regno, int nregs, const char *regname) { @@ -2872,11 +2873,22 @@ asm_clobber_reg_is_valid (int regno, int nregs, co error ("PIC register clobbered by %qs in %", regname); is_valid = false; } - /* Clobbering the STACK POINTER register is an error. */ + /* Clobbering the STACK POINTER register is likely an error. + However it is useful to force the use of frame pointer and prevent + the use of red zone. Thus without this clobber, pushing temporary + values onto the stack might clobber the red zone or make stack based + memory references invalid. */ if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) { - error ("Stack Pointer register clobbered by %qs in %", regname); - is_valid = false; + if (warning (OPT_Wstack_clobber, + "stack pointer register clobbered by %qs in %", + regname)) + inform (input_location, + "This does likely not do what you would expect." + " The stack pointer register still has to be restored to" + " the previous value, however it is safe to push values onto" + " the stack, when they are popped again from the stack" + " before the asm statement terminates"); } return is_valid; Index: gcc/common.opt === --- gcc/common.opt (revision 267653) +++ gcc/common.opt (working copy) @@ -702,6 +702,10 @@ Warn when one local variable shadows another local Wshadow-compatible-local Common Warning Undocumented Alias(Wshadow=compatible-local) +Wstack-clobber +Common Warning Var(warn_stack_clobber) Init(1) +Warn when asm statements try to clobber the stack pointer register. + Wstack-protector Common Var(warn_stack_protect) Warning Warn when not issuing stack smashing protection for some reason. Index: gcc/doc/invoke.texi === --- gcc/doc/invoke.texi (revision 267653) +++ gcc/doc/invoke.texi (working copy) @@ -339,7 +339,7 @@ Objective-C and Objective-C++ Dialects}. -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol -Wsign-compare -Wsign-conversion -Wfloat-conversion @gol -Wno-scalar-storage-order -Wsizeof-pointer-div @gol --Wsizeof-pointer-memaccess -Wsizeof-array-argument @gol +-Wsizeof-pointer-memaccess -Wsizeof-array-argument -Wstack-clobber @gol -Wstack-protector -Wstack-usage=@var{byte-size} -Wstrict-aliasing @gol -Wstrict-aliasing=n -Wstrict-overflow -Wstrict-overflow=@var{n} @gol -Wstringop-overflow=@var{n} -Wstringop-truncation -Wsubobject-linkage @gol @@ -7560,6 +7560,20 @@ This option is only supported
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote: > - /* Clobbering the STACK POINTER register is an error. */ > + /* Clobbered STACK POINTER register is not saved/restored by GCC, > + which is often unexpected by users. See PR52813. */ >if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) > { > - error ("Stack Pointer register clobbered by %qs in %", regname); > + warning (0, "Stack Pointer register clobbered by %qs in %", > +regname); > + warning (0, "GCC has always ignored Stack Pointer % clobbers"); Why do we write Stack Pointer rather than stack pointer? That is really weird. The second warning would be a note based on the first one, i.e. if (warning ()) note (); and better have some -W* option to silence the warning. >is_valid = false; > } > > diff --git a/gcc/testsuite/gcc.target/i386/pr52813.c > b/gcc/testsuite/gcc.target/i386/pr52813.c > index 154ebbfc423..644fef15fef 100644 > --- a/gcc/testsuite/gcc.target/i386/pr52813.c > +++ b/gcc/testsuite/gcc.target/i386/pr52813.c > @@ -5,5 +5,5 @@ > void > test1 (void) > { > - asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register > clobbered" } */ > + asm volatile ("" : : : "%esp"); /* { dg-warning "Stack Pointer register > clobbered.\+GCC has always ignored Stack Pointer 'asm' clobbers" } */ > } > -- > 2.11.0 > Jakub
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Wed, Dec 19, 2018 at 08:40:13AM +0200, Dimitar Dimitrov wrote: > On Mon, Dec 17 2018 20:15:02 EET Bernd Edlinger wrote: > > out of curiosity I looked at the clobber statement in > > gdb/nat/linux-ptrace.c: > > > >asm volatile ("pushq %0;" > > ".globl linux_ptrace_test_ret_to_nx_instr;" > > "linux_ptrace_test_ret_to_nx_instr:" > > "ret" > > : : "r" ((uint64_t) (uintptr_t) return_address) > > : "%rsp", "memory"); > > > > it turns out to be a far jump, instruction. > > GDB functionality should not be affected if SP clobber is removed, even if > the > generated code is slightly different. Please see this comment: > http://sourceware.org/ml/gdb-patches/2018-12/msg00204.html > > As I understand it, this particular code is never meant to return. It should > either stop due to the NX mapping of return_address/%0, or hit the breakpoint > placed at return_address/%0. If it doesn't return it is undefined behaviour, so anything might happen and that is perfectly alright. Defining labels is an asm is undefined, too. Maybe real assembler code is wanted here? I.e. a .s file. Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Mon, Dec 17 2018 20:15:02 EET Bernd Edlinger wrote: > out of curiosity I looked at the clobber statement in > gdb/nat/linux-ptrace.c: > >asm volatile ("pushq %0;" > ".globl linux_ptrace_test_ret_to_nx_instr;" > "linux_ptrace_test_ret_to_nx_instr:" > "ret" > : : "r" ((uint64_t) (uintptr_t) return_address) > : "%rsp", "memory"); > > it turns out to be a far jump, instruction. GDB functionality should not be affected if SP clobber is removed, even if the generated code is slightly different. Please see this comment: http://sourceware.org/ml/gdb-patches/2018-12/msg00204.html As I understand it, this particular code is never meant to return. It should either stop due to the NX mapping of return_address/%0, or hit the breakpoint placed at return_address/%0. Regards, Dimitar
Re: [PATCH] [RFC] PR target/52813 and target/11807
On 12/18/18 3:16 PM, Bernd Edlinger wrote: > Hi, > > while I looked closely at the asm statement in the gdb, > I realized that the SP clobber forces the function to use > the frame pointer, and prevents the red zone. That > makes the push / pop sequence in the asm statement safe > to use, as long as the stack is restored to the original > value. That can be a quite useful feature. And that might > have been the reason why the rsp clobber was chosen in the > first place. > > This seems to work for all targets, but it started to work > this way with gcc-6, all versions before that do ignore > this clobber stmt (as confirmed by godbolt). > > The clobber stmt make the LRA register allocator switch > frame_pointer_needed to 1, and therefore in all likelihood, > all targets should use that consistently. > > On 12/17/18 12:47 PM, Richard Sandiford wrote: >> Dimitar Dimitrov writes: >>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: Hi, if I understood that right, then clobbering sp is and has always been ignored. >> >> PR77904 was about the clobber not being ignored, so the behaviour >> hasn't been consistent. >> > > I think 77904 was a fall-out from the change in the LRA register allocator. > The patch referenced in the PR does simply honor frame_pointer_needed, > which changed with gcc-6, and caused a regression on arm. > >> I'm also not sure it was always ignored in recent sources. The clobber >> does get added to the associated rtl insn, and it'd be surprising if >> that never had an effect. >> If that is right, then I would much prefer a warning, that says exactly that, because that would also help to understand why removing that clobber statement is safe even for old gcc versions. >> >> If the asm does leave sp with a different value, then it's never been safe, >> regardless of the gcc version. That's why an error seems more appropriate. >> >>> Thank you. Looks like general consensus is to have a warning. See attached >>> patch that switches the error to a warning. >> >> I don't think there's a good reason to treat this differently from the >> preexisting PIC register error. If the argument for making it a warning >> rather than an error is that the asm might happen to work by accident, >> then the same is true for the PIC register. >> > > In the light of my findings, I believe with a good warning message that > explains that the SP needs to be restored to the previous value, that > is a useful feature, that enables the asm statement to push temporary > values on the stack which would not be safe otherwise. > > Therefore I propose not to rip it out at this time. > See my proposed patch. What do you think? > > Is it OK? > > Oops, previous version missed the fix of the PR77904 test case, which is currently broken too. Bernd- 2018-12-18 Bernd Edlinger * cfgexpand.c (asm_clobber_reg_is_valid): Emit only a warning together with an information message when the stack pointer is clobbered. testsuite: 2018-12-18 Bernd Edlinger * gcc.target/arm/pr77904.c: Adjust test. * gcc.target/i386/pr52813.c: Adjust test. Index: gcc/cfgexpand.c === --- gcc/cfgexpand.c (revision 267164) +++ gcc/cfgexpand.c (working copy) @@ -2854,6 +2854,7 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_S asm clobber operand. Some HW registers cannot be saved/restored, hence they should not be clobbered by asm statements. */ + static bool asm_clobber_reg_is_valid (int regno, int nregs, const char *regname) { @@ -2872,11 +2873,23 @@ asm_clobber_reg_is_valid (int regno, int nregs, co error ("PIC register clobbered by %qs in %", regname); is_valid = false; } - /* Clobbering the STACK POINTER register is an error. */ + /* Clobbering the STACK POINTER register is likely an error. + However it is useful to force the use of frame pointer and prevent + the use of red zone. Thus without this clobber, pushing temporary + values onto the stack might clobber the red zone or make stack based + memory references invalid. */ if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) { - error ("Stack Pointer register clobbered by %qs in %", regname); - is_valid = false; + if (warning (0, "Stack Pointer register clobbered by %qs in %", + regname)) + { + inform (input_location, + "This does likely not do what you would expect." + " The Stack Pointer register still needs to be restored to" + " the previous value, however it is safe to push values onto" + " the stack, when they are popped again from the stack" + " before the asm statement terminates"); + } } return is_valid; Index: gcc/testsuite/gcc.target/arm/pr77904.c === --- gcc/testsuite/gcc.target/arm/pr77904.c (revision 267164) +++ gcc/testsuite/gcc.target/arm/pr77904.c (working
Re: [PATCH] [RFC] PR target/52813 and target/11807
Hi, while I looked closely at the asm statement in the gdb, I realized that the SP clobber forces the function to use the frame pointer, and prevents the red zone. That makes the push / pop sequence in the asm statement safe to use, as long as the stack is restored to the original value. That can be a quite useful feature. And that might have been the reason why the rsp clobber was chosen in the first place. This seems to work for all targets, but it started to work this way with gcc-6, all versions before that do ignore this clobber stmt (as confirmed by godbolt). The clobber stmt make the LRA register allocator switch frame_pointer_needed to 1, and therefore in all likelihood, all targets should use that consistently. On 12/17/18 12:47 PM, Richard Sandiford wrote: > Dimitar Dimitrov writes: >> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: >>> Hi, >>> >>> if I understood that right, then clobbering sp is and has always been >>> ignored. > > PR77904 was about the clobber not being ignored, so the behaviour > hasn't been consistent. > I think 77904 was a fall-out from the change in the LRA register allocator. The patch referenced in the PR does simply honor frame_pointer_needed, which changed with gcc-6, and caused a regression on arm. > I'm also not sure it was always ignored in recent sources. The clobber > does get added to the associated rtl insn, and it'd be surprising if > that never had an effect. > >>> If that is right, then I would much prefer a warning, that says exactly >>> that, because that would also help to understand why removing that clobber >>> statement is safe even for old gcc versions. > > If the asm does leave sp with a different value, then it's never been safe, > regardless of the gcc version. That's why an error seems more appropriate. > >> Thank you. Looks like general consensus is to have a warning. See attached >> patch that switches the error to a warning. > > I don't think there's a good reason to treat this differently from the > preexisting PIC register error. If the argument for making it a warning > rather than an error is that the asm might happen to work by accident, > then the same is true for the PIC register. > In the light of my findings, I believe with a good warning message that explains that the SP needs to be restored to the previous value, that is a useful feature, that enables the asm statement to push temporary values on the stack which would not be safe otherwise. Therefore I propose not to rip it out at this time. See my proposed patch. What do you think? Is it OK? Thanks Bernd. 2018-12-18 Bernd Edlinger * cfgexpand.c (asm_clobber_reg_is_valid): Emit only a warning together with an information message when the stack pointer is clobbered. testsuite: 2018-12-18 Bernd Edlinger * gcc.target/i386/pr52813.c: Adjust test. Index: gcc/cfgexpand.c === --- gcc/cfgexpand.c (revision 267164) +++ gcc/cfgexpand.c (working copy) @@ -2854,6 +2854,7 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_S asm clobber operand. Some HW registers cannot be saved/restored, hence they should not be clobbered by asm statements. */ + static bool asm_clobber_reg_is_valid (int regno, int nregs, const char *regname) { @@ -2872,11 +2873,23 @@ asm_clobber_reg_is_valid (int regno, int nregs, co error ("PIC register clobbered by %qs in %", regname); is_valid = false; } - /* Clobbering the STACK POINTER register is an error. */ + /* Clobbering the STACK POINTER register is likely an error. + However it is useful to force the use of frame pointer and prevent + the use of red zone. Thus without this clobber, pushing temporary + values onto the stack might clobber the red zone or make stack based + memory references invalid. */ if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) { - error ("Stack Pointer register clobbered by %qs in %", regname); - is_valid = false; + if (warning (0, "Stack Pointer register clobbered by %qs in %", + regname)) + { + inform (input_location, + "This does likely not do what you would expect." + " The Stack Pointer register still needs to be restored to" + " the previous value, however it is safe to push values onto" + " the stack, when they are popped again from the stack" + " before the asm statement terminates"); + } } return is_valid; Index: gcc/testsuite/gcc.target/i386/pr52813.c === --- gcc/testsuite/gcc.target/i386/pr52813.c (revision 267164) +++ gcc/testsuite/gcc.target/i386/pr52813.c (working copy) @@ -1,9 +1,10 @@ /* Ensure that stack pointer cannot be an asm clobber. */ /* { dg-do compile { target { ! ia32 } } } */ -/* { dg-options "-O2" } */ +/* { dg-options "-O3 -fomit-frame-pointer" } */ void test1 (void) { - asm volatile ("" : : : "%esp"); /* {
Re: [PATCH] [RFC] PR target/52813 and target/11807
On 12/17/18 7:46 PM, Richard Sandiford wrote: > Segher Boessenkool writes: >> On Mon, Dec 17, 2018 at 11:47:42AM +, Richard Sandiford wrote: >>> Dimitar Dimitrov writes: On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: > Hi, > > if I understood that right, then clobbering sp is and has always been > ignored. >>> >>> PR77904 was about the clobber not being ignored, so the behaviour >>> hasn't been consistent. >>> >>> I'm also not sure it was always ignored in recent sources. The clobber >>> does get added to the associated rtl insn, and it'd be surprising if >>> that never had an effect. >> >> Yes, you will usually get a frame pointer. My point was that the epilogue >> will restore your stack pointer both with and without the asm clobber. > > I'm not confident that's the only effect though. > > Also, we didn't use a frame in PR77904, and using a frame would have > been the wrong thing to do. > >>> I don't think there's a good reason to treat this differently from the >>> preexisting PIC register error. If the argument for making it a warning >>> rather than an error is that the asm might happen to work by accident, >>> then the same is true for the PIC register. >> >> Yes. As well as quite a few more registers, many of those specific to >> the target. And there are many more things you can do terribly wrong in >> inline assembler, of course, most of which we can never detect. > > Right. And I don't think anyone's suggesting GCC can detect everything. > It can only police the things it knows about, which include the input, > output and clobber clauses. > > What makes the PIC register and sp worth special attention is that > changing their values would in general invalidate other code that GCC > generates itself. It's not just about whether the asm has the effect > the author wanted (whatever that was). > > FWIW, I don't think we should go on a proactive hunt for other registers > to complain about. > out of curiosity I looked at the clobber statement in gdb/nat/linux-ptrace.c: asm volatile ("pushq %0;" ".globl linux_ptrace_test_ret_to_nx_instr;" "linux_ptrace_test_ret_to_nx_instr:" "ret" : : "r" ((uint64_t) (uintptr_t) return_address) : "%rsp", "memory"); it turns out to be a far jump, instruction. And I wanted to find out what removing the %rsp clobber actually does. First there is a technical problem with that, because gcc does not print an error, it is possbile to compile the code without the sp clobber, but not to compare the code that would have been generated if the error would only be a warning. So I had to undo the patch in order to see, what the sp clobber actually does, and I think Segher mentioned that this might have an influence on the frame pointer, that turns out to be true: diff linux-ptrace-spclobber.dis linux-ptrace-noclobber.dis 449,590c449,593 < 5c0: 55 push %rbp < 5c1: 45 31 c9xor%r9d,%r9d < 5c4: 41 b8 ff ff ff ff mov$0x,%r8d < 5ca: b9 22 00 00 00 mov$0x22,%ecx < 5cf: ba 03 00 00 00 mov$0x3,%edx < 5d4: be 02 00 00 00 mov$0x2,%esi < 5d9: 31 ff xor%edi,%edi < 5db: 48 89 e5mov%rsp,%rbp < 5de: 41 57 push %r15 < 5e0: 41 56 push %r14 < 5e2: 41 55 push %r13 < 5e4: 41 54 push %r12 < 5e6: 53 push %rbx < 5e7: 48 81 ec f8 00 00 00sub$0xf8,%rsp [snip] --- > 5c0: 41 56 push %r14 > 5c2: 45 31 c9xor%r9d,%r9d > 5c5: 41 b8 ff ff ff ff mov$0x,%r8d > 5cb: b9 22 00 00 00 mov$0x22,%ecx > 5d0: 41 55 push %r13 > 5d2: ba 03 00 00 00 mov$0x3,%edx > 5d7: be 02 00 00 00 mov$0x2,%esi > 5dc: 31 ff xor%edi,%edi > 5de: 41 54 push %r12 > 5e0: 55 push %rbp > 5e1: 53 push %rbx > 5e2: 48 81 ec f0 00 00 00sub$0xf0,%rsp So for me this looks not at all trivial to see if this would work without the sp clobber, since the stack frame might be completely different without that sp clobber. I wonder what gdb developers think about the sp clobber here, if it is easy to fix or if it gives trouble to them. Thanks Bernd.
Re: [PATCH] [RFC] PR target/52813 and target/11807
Segher Boessenkool writes: > On Mon, Dec 17, 2018 at 11:47:42AM +, Richard Sandiford wrote: >> Dimitar Dimitrov writes: >> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: >> >> Hi, >> >> >> >> if I understood that right, then clobbering sp is and has always been >> >> ignored. >> >> PR77904 was about the clobber not being ignored, so the behaviour >> hasn't been consistent. >> >> I'm also not sure it was always ignored in recent sources. The clobber >> does get added to the associated rtl insn, and it'd be surprising if >> that never had an effect. > > Yes, you will usually get a frame pointer. My point was that the epilogue > will restore your stack pointer both with and without the asm clobber. I'm not confident that's the only effect though. Also, we didn't use a frame in PR77904, and using a frame would have been the wrong thing to do. >> I don't think there's a good reason to treat this differently from the >> preexisting PIC register error. If the argument for making it a warning >> rather than an error is that the asm might happen to work by accident, >> then the same is true for the PIC register. > > Yes. As well as quite a few more registers, many of those specific to > the target. And there are many more things you can do terribly wrong in > inline assembler, of course, most of which we can never detect. Right. And I don't think anyone's suggesting GCC can detect everything. It can only police the things it knows about, which include the input, output and clobber clauses. What makes the PIC register and sp worth special attention is that changing their values would in general invalidate other code that GCC generates itself. It's not just about whether the asm has the effect the author wanted (whatever that was). FWIW, I don't think we should go on a proactive hunt for other registers to complain about. Thanks, Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Mon, Dec 17, 2018 at 11:47:42AM +, Richard Sandiford wrote: > Dimitar Dimitrov writes: > > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: > >> Hi, > >> > >> if I understood that right, then clobbering sp is and has always been > >> ignored. > > PR77904 was about the clobber not being ignored, so the behaviour > hasn't been consistent. > > I'm also not sure it was always ignored in recent sources. The clobber > does get added to the associated rtl insn, and it'd be surprising if > that never had an effect. Yes, you will usually get a frame pointer. My point was that the epilogue will restore your stack pointer both with and without the asm clobber. > I don't think there's a good reason to treat this differently from the > preexisting PIC register error. If the argument for making it a warning > rather than an error is that the asm might happen to work by accident, > then the same is true for the PIC register. Yes. As well as quite a few more registers, many of those specific to the target. And there are many more things you can do terribly wrong in inline assembler, of course, most of which we can never detect. Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Sun, Dec 16, 2018 at 10:43:47AM +0200, Dimitar Dimitrov wrote: > On Fri, Dec 14 2018 2:52:17 EET Segher Boessenkool wrote: > > You need a few tweaks to what you committed. Or just one perhaps: if > > flag_pic is not set, you should not check PIC_OFFSET_TABLE_REGNUM, it is > > meaningless in that case. I'm not sure if you need to check whether the > > register is fixed or not. > The flag_pic flag is already checked by the PIC_OFFSET_TABLE_REGNUM macro. It > will return INVALID_REGNUM if flag_pic is false, so no error will be printed. No, it is not. On at least six targets the macro is simply defined as a register number. Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
On 12/17/18 2:35 PM, Richard Sandiford wrote: > Christophe Lyon writes: >> On Mon, 17 Dec 2018 at 12:47, Richard Sandiford >> wrote: >>> >>> Dimitar Dimitrov writes: On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: > Hi, > > if I understood that right, then clobbering sp is and has always been > ignored. >>> >>> PR77904 was about the clobber not being ignored, so the behaviour >>> hasn't been consistent. >>> >>> I'm also not sure it was always ignored in recent sources. The clobber >>> does get added to the associated rtl insn, and it'd be surprising if >>> that never had an effect. >>> > If that is right, then I would much prefer a warning, that says exactly > that, because that would also help to understand why removing that clobber > statement is safe even for old gcc versions. >>> >>> If the asm does leave sp with a different value, then it's never been safe, >>> regardless of the gcc version. That's why an error seems more appropriate. >>> Thank you. Looks like general consensus is to have a warning. See attached patch that switches the error to a warning. >>> >>> I don't think there's a good reason to treat this differently from the >>> preexisting PIC register error. If the argument for making it a warning >>> rather than an error is that the asm might happen to work by accident, >>> then the same is true for the PIC register. >>> >> >> If we leave the error, maybe a more explanatory message would be helpful? >> (along the lines of what I posted earlier in this thread, which may be >> too verbose) > > The message in that patch suggested removing the clobber and hoping for > the best, which IMO is bad advice. If the current message doesn't make > it clear enough that changing the sp is not allowed, how about: > > inline % statements must not change the value of the stack pointer > > ? > Yes, things could be easy, but, mot the closer one looks, the more complicated they get... Even pushing a value on the stack and popping it again in the _same_ asm statement is dangerous with red-zone targets. Maybe that would be also good to add as an advice? Bernd. > Thanks, > Richard > > >
Re: [PATCH] [RFC] PR target/52813 and target/11807
On 12/17/18 2:42 PM, Christophe Lyon wrote: > On Mon, 17 Dec 2018 at 14:35, Richard Sandiford > wrote: >> >> Christophe Lyon writes: >>> On Mon, 17 Dec 2018 at 12:47, Richard Sandiford >>> wrote: Dimitar Dimitrov writes: > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: >> Hi, >> >> if I understood that right, then clobbering sp is and has always been >> ignored. PR77904 was about the clobber not being ignored, so the behaviour hasn't been consistent. I'm also not sure it was always ignored in recent sources. The clobber does get added to the associated rtl insn, and it'd be surprising if that never had an effect. >> If that is right, then I would much prefer a warning, that says exactly >> that, because that would also help to understand why removing that >> clobber >> statement is safe even for old gcc versions. If the asm does leave sp with a different value, then it's never been safe, regardless of the gcc version. That's why an error seems more appropriate. > Thank you. Looks like general consensus is to have a warning. See attached > patch that switches the error to a warning. I don't think there's a good reason to treat this differently from the preexisting PIC register error. If the argument for making it a warning rather than an error is that the asm might happen to work by accident, then the same is true for the PIC register. >>> >>> If we leave the error, maybe a more explanatory message would be helpful? >>> (along the lines of what I posted earlier in this thread, which may be >>> too verbose) >> >> The message in that patch suggested removing the clobber and hoping for >> the best, which IMO is bad advice. If the current message doesn't make >> it clear enough that changing the sp is not allowed, how about: >> >> inline % statements must not change the value of the stack pointer >> >> ? >> > > My understanding is that in some cases, some users still want to > deliberately change SP, > so "must not" may be confusing in such cases? > What do they want to achieve, something like a longjmp ? BTW: on arm, you can add r15 (PC) to the clobber list, and that seems to be ignored as well. If you want to jump to another function, you might want to clobber "sp", "pc". PS: I know that is invalid, just guessing why there is an itch. Bernd. >> Thanks, >> Richard >> >>
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Mon, 17 Dec 2018 at 14:35, Richard Sandiford wrote: > > Christophe Lyon writes: > > On Mon, 17 Dec 2018 at 12:47, Richard Sandiford > > wrote: > >> > >> Dimitar Dimitrov writes: > >> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: > >> >> Hi, > >> >> > >> >> if I understood that right, then clobbering sp is and has always been > >> >> ignored. > >> > >> PR77904 was about the clobber not being ignored, so the behaviour > >> hasn't been consistent. > >> > >> I'm also not sure it was always ignored in recent sources. The clobber > >> does get added to the associated rtl insn, and it'd be surprising if > >> that never had an effect. > >> > >> >> If that is right, then I would much prefer a warning, that says exactly > >> >> that, because that would also help to understand why removing that > >> >> clobber > >> >> statement is safe even for old gcc versions. > >> > >> If the asm does leave sp with a different value, then it's never been safe, > >> regardless of the gcc version. That's why an error seems more appropriate. > >> > >> > Thank you. Looks like general consensus is to have a warning. See > >> > attached > >> > patch that switches the error to a warning. > >> > >> I don't think there's a good reason to treat this differently from the > >> preexisting PIC register error. If the argument for making it a warning > >> rather than an error is that the asm might happen to work by accident, > >> then the same is true for the PIC register. > >> > > > > If we leave the error, maybe a more explanatory message would be helpful? > > (along the lines of what I posted earlier in this thread, which may be > > too verbose) > > The message in that patch suggested removing the clobber and hoping for > the best, which IMO is bad advice. If the current message doesn't make > it clear enough that changing the sp is not allowed, how about: > > inline % statements must not change the value of the stack pointer > > ? > My understanding is that in some cases, some users still want to deliberately change SP, so "must not" may be confusing in such cases? > Thanks, > Richard > >
Re: [PATCH] [RFC] PR target/52813 and target/11807
Christophe Lyon writes: > On Mon, 17 Dec 2018 at 12:47, Richard Sandiford > wrote: >> >> Dimitar Dimitrov writes: >> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: >> >> Hi, >> >> >> >> if I understood that right, then clobbering sp is and has always been >> >> ignored. >> >> PR77904 was about the clobber not being ignored, so the behaviour >> hasn't been consistent. >> >> I'm also not sure it was always ignored in recent sources. The clobber >> does get added to the associated rtl insn, and it'd be surprising if >> that never had an effect. >> >> >> If that is right, then I would much prefer a warning, that says exactly >> >> that, because that would also help to understand why removing that clobber >> >> statement is safe even for old gcc versions. >> >> If the asm does leave sp with a different value, then it's never been safe, >> regardless of the gcc version. That's why an error seems more appropriate. >> >> > Thank you. Looks like general consensus is to have a warning. See attached >> > patch that switches the error to a warning. >> >> I don't think there's a good reason to treat this differently from the >> preexisting PIC register error. If the argument for making it a warning >> rather than an error is that the asm might happen to work by accident, >> then the same is true for the PIC register. >> > > If we leave the error, maybe a more explanatory message would be helpful? > (along the lines of what I posted earlier in this thread, which may be > too verbose) The message in that patch suggested removing the clobber and hoping for the best, which IMO is bad advice. If the current message doesn't make it clear enough that changing the sp is not allowed, how about: inline % statements must not change the value of the stack pointer ? Thanks, Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Mon, 17 Dec 2018 at 12:47, Richard Sandiford wrote: > > Dimitar Dimitrov writes: > > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: > >> Hi, > >> > >> if I understood that right, then clobbering sp is and has always been > >> ignored. > > PR77904 was about the clobber not being ignored, so the behaviour > hasn't been consistent. > > I'm also not sure it was always ignored in recent sources. The clobber > does get added to the associated rtl insn, and it'd be surprising if > that never had an effect. > > >> If that is right, then I would much prefer a warning, that says exactly > >> that, because that would also help to understand why removing that clobber > >> statement is safe even for old gcc versions. > > If the asm does leave sp with a different value, then it's never been safe, > regardless of the gcc version. That's why an error seems more appropriate. > > > Thank you. Looks like general consensus is to have a warning. See attached > > patch that switches the error to a warning. > > I don't think there's a good reason to treat this differently from the > preexisting PIC register error. If the argument for making it a warning > rather than an error is that the asm might happen to work by accident, > then the same is true for the PIC register. > If we leave the error, maybe a more explanatory message would be helpful? (along the lines of what I posted earlier in this thread, which may be too verbose) Christophe > Thanks, > Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
Dimitar Dimitrov writes: > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: >> Hi, >> >> if I understood that right, then clobbering sp is and has always been >> ignored. PR77904 was about the clobber not being ignored, so the behaviour hasn't been consistent. I'm also not sure it was always ignored in recent sources. The clobber does get added to the associated rtl insn, and it'd be surprising if that never had an effect. >> If that is right, then I would much prefer a warning, that says exactly >> that, because that would also help to understand why removing that clobber >> statement is safe even for old gcc versions. If the asm does leave sp with a different value, then it's never been safe, regardless of the gcc version. That's why an error seems more appropriate. > Thank you. Looks like general consensus is to have a warning. See attached > patch that switches the error to a warning. I don't think there's a good reason to treat this differently from the preexisting PIC register error. If the argument for making it a warning rather than an error is that the asm might happen to work by accident, then the same is true for the PIC register. Thanks, Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote: > Hi, > > if I understood that right, then clobbering sp is and has always been > ignored. > > If that is right, then I would much prefer a warning, that says exactly > that, because that would also help to understand why removing that clobber > statement is safe even for old gcc versions. > > Since your patch did not actually change the handling of the PIC register, > that one should of course stay an error. Thank you. Looks like general consensus is to have a warning. See attached patch that switches the error to a warning. Regards, Dimitar >From d589ebd7824b4505ab75a2404f49a7c200679545 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Sun, 16 Dec 2018 10:13:18 +0200 Subject: [PATCH] PR target/52813 Turns out there are existing programs that clobber stack pointer. To avoid disruption, switch the newly introduced error to a warning. Tested with: $ make check-gcc-c RUNTESTFLAGS="i386.exp=pr52813.c " gcc/ChangeLog: 2018-12-16 Dimitar Dimitrov * cfgexpand.c (asm_clobber_reg_is_valid): Switch error to warning. Add clarification why there is a warning. gcc/testsuite/ChangeLog: 2018-12-16 Dimitar Dimitrov * gcc.target/i386/pr52813.c (test1): Update warning message. Signed-off-by: Dimitar Dimitrov --- gcc/cfgexpand.c | 7 +-- gcc/testsuite/gcc.target/i386/pr52813.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 0d04bbcafce..1e44c9a7ad0 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2872,10 +2872,13 @@ asm_clobber_reg_is_valid (int regno, int nregs, const char *regname) error ("PIC register clobbered by %qs in %", regname); is_valid = false; } - /* Clobbering the STACK POINTER register is an error. */ + /* Clobbered STACK POINTER register is not saved/restored by GCC, + which is often unexpected by users. See PR52813. */ if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) { - error ("Stack Pointer register clobbered by %qs in %", regname); + warning (0, "Stack Pointer register clobbered by %qs in %", + regname); + warning (0, "GCC has always ignored Stack Pointer % clobbers"); is_valid = false; } diff --git a/gcc/testsuite/gcc.target/i386/pr52813.c b/gcc/testsuite/gcc.target/i386/pr52813.c index 154ebbfc423..644fef15fef 100644 --- a/gcc/testsuite/gcc.target/i386/pr52813.c +++ b/gcc/testsuite/gcc.target/i386/pr52813.c @@ -5,5 +5,5 @@ void test1 (void) { - asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */ + asm volatile ("" : : : "%esp"); /* { dg-warning "Stack Pointer register clobbered.\+GCC has always ignored Stack Pointer 'asm' clobbers" } */ } -- 2.11.0
Re: [PATCH] [RFC] PR target/52813 and target/11807
Hi, if I understood that right, then clobbering sp is and has always been ignored. If that is right, then I would much prefer a warning, that says exactly that, because that would also help to understand why removing that clobber statement is safe even for old gcc versions. Since your patch did not actually change the handling of the PIC register, that one should of course stay an error. Thanks Bernd.
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Fri, Dec 14 2018 2:52:17 EET Segher Boessenkool wrote: > You need a few tweaks to what you committed. Or just one perhaps: if > flag_pic is not set, you should not check PIC_OFFSET_TABLE_REGNUM, it is > meaningless in that case. I'm not sure if you need to check whether the > register is fixed or not. The flag_pic flag is already checked by the PIC_OFFSET_TABLE_REGNUM macro. It will return INVALID_REGNUM if flag_pic is false, so no error will be printed. Note that the PIC_OFFSET_TABLE_REGNUM behaviour is not changed by my patch. I merely moved the existing check into a separate function. > > But there are many more regs than just the PIC reg and the stack pointer > that you would want to similarly warn about, because overwriting those > registers is just as fatal: the frame pointer, the program counter, etc. > _Most_ fixed registers, but not _all_. > > So maybe it should be a target hook? OTOH that is a lot of work for such > a trivial warning, that isn't very useful anyway (a better warning for > any asm is: "Are you sure?" :-) ) I'll think about a more generic solution. But in light of the several filed PRs I think it's worth having a simple check for SP. Regards, Dimitar
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Fri, Dec 14, 2018 at 01:49:40PM +, Richard Sandiford wrote: > > OK, I read PR52813 too, but I'm not sure to fully understand the new status. > > My understanding is that since this patch was committed, if an asm statement > > clobbers sp, it is now allowed to actually declare it as clobber (this patch > > generates an error in such a case). > > So the user is now expected to lie to the compiler when writing to > > this kind of register (sp, pic register), by not declaring it as "clobber"? > > The user isn't expected to lie. The point is that GCC simply doesn't > support asms that leave the stack pointer with a different value from > before, and IMO never has. If that happened to work in some cases then > it was purely an accident. Yup. It now errors for void f(void) { asm("ohmy" ::: "sp"); } but not for void f(void) { register long x asm("sp"); asm("ohmy %0" : "=r"(x)); } which is the same problem. (I would be happier if it was a warning instead of an error btw, since there apparently is existing code that uses a clobber of sp, and GCC has always worked with that, accidentally or not). > The PRs also show disagreement about what GCC should do for an asm like > that. The asm in PR52813 temporarily changed the stack pointer and the > bug was that GCC didn't restore the original value afterwards. The asm > in PR77904 was trying to set the stack pointer to an entirely new value > and the bug was the GCC did restore the original value afterwards, > defeating the point. > > This wouldn't be the first time that there's disagreement about what > the behaviour should be. But IMO we can't support either reliably. > Spilling sp is dangerous in general because we might need the stack > for the reload, or we might accidentally try to reload something else > before restoring the stack pointer. And continuing with a new sp > provided by the asm could lead to all sorts of problems. (AIUI, the > point of PR77904 was that it would also be wrong for GCC to set up a > frame pointer and restore the sp from that frame pointer on function > exit. The new sp value was supposed to survive. So the answer isn't > simply "use a frame pointer".) Inline asm cannot do things that are not allowed by the ABI, or that touch other things in the execution environment you shouldn't touch. Apparently some people really try to clobber sp, and this new error will help them a bit. I don't know how useful that is, is it better to scare people away from using inline asm? It might well be safer for everyone... Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
(Maybe the discussion has moved on from this already -- sorry if so) Christophe Lyon writes: > On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme > wrote: >> >> So my understanding is that the original code (CMSIS library) used to >> clobber sp because the asm statement was actually changing the sp. >> That in turn led GCC to try to save and restore sp which is not what >> CMSIS was expecting to happen. Changing sp without clobber as done now >> is probably the right solution and r242693 can be reverted. That will >> remove the failing test. >> > > OK, I read PR52813 too, but I'm not sure to fully understand the new status. > My understanding is that since this patch was committed, if an asm statement > clobbers sp, it is now allowed to actually declare it as clobber (this patch > generates an error in such a case). > So the user is now expected to lie to the compiler when writing to > this kind of register (sp, pic register), by not declaring it as "clobber"? The user isn't expected to lie. The point is that GCC simply doesn't support asms that leave the stack pointer with a different value from before, and IMO never has. If that happened to work in some cases then it was purely an accident. The PRs also show disagreement about what GCC should do for an asm like that. The asm in PR52813 temporarily changed the stack pointer and the bug was that GCC didn't restore the original value afterwards. The asm in PR77904 was trying to set the stack pointer to an entirely new value and the bug was the GCC did restore the original value afterwards, defeating the point. This wouldn't be the first time that there's disagreement about what the behaviour should be. But IMO we can't support either reliably. Spilling sp is dangerous in general because we might need the stack for the reload, or we might accidentally try to reload something else before restoring the stack pointer. And continuing with a new sp provided by the asm could lead to all sorts of problems. (AIUI, the point of PR77904 was that it would also be wrong for GCC to set up a frame pointer and restore the sp from that frame pointer on function exit. The new sp value was supposed to survive. So the answer isn't simply "use a frame pointer".) Thanks, Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Thu, Dec 13, 2018 at 11:26:40PM +0200, Dimitar Dimitrov wrote: > On Thu, Dec 13, 2018 at 8:48:38 EET Segher Boessenkool wrote: > > On Wed, Dec 12, 2018 at 06:26:10PM +0200, Dimitar Dimitrov wrote: > > > I expect that if I mark a HW register as "clobber", compiler would save > > > its > > > contents before executing the asm statement, and after that it would > > > restore its contents. This is the GCC behaviour for all but the SP and > > > PIC registers. That is why I believe that PR52813 is a valid bug. > > > > It won't do it for *any* fixed registers. But you do not want to error > > or even warn for some fixed registers, for example the "flags" register > > on x86 is *always* written to by asm. > > Yes, you are correct. > > > But you never want to warn for non-fixed registers, and e.g. > > PIC_OFFSET_TABLE_REGNUM isn't always a fixed register (when flag_pic is 0 > > for example). > I could not trace how PIC_OFFSET_TABLE_REGNUM on i386 gets marked as fixed > register. I'll dig more through the source. > > > > I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered > > > in such a way that GCC will not notice (e.g. thread switching), then why > > > should GCC know about it in the first place? > > > > Up until today, GCC has always just ignored it if you claimed to clobber > > the stack pointer. > > My point is that the silent ignoring is confusing to users, as shown by > PR52813. How would you suggest me to proceed: > - Leave patch as-is. > - Revert patch. Update documentation to point that clobber marker for fixed > registers is ignored by GCC. Close PR52813 as invalid. > - Revert patch. Discuss more broadly and specify behaviour of asm clobber > for > fixed registers (and SP in particular). You need a few tweaks to what you committed. Or just one perhaps: if flag_pic is not set, you should not check PIC_OFFSET_TABLE_REGNUM, it is meaningless in that case. I'm not sure if you need to check whether the register is fixed or not. But there are many more regs than just the PIC reg and the stack pointer that you would want to similarly warn about, because overwriting those registers is just as fatal: the frame pointer, the program counter, etc. _Most_ fixed registers, but not _all_. So maybe it should be a target hook? OTOH that is a lot of work for such a trivial warning, that isn't very useful anyway (a better warning for any asm is: "Are you sure?" :-) ) So I don't know what is best to do (except that flag_pic part). Sorry. Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Thu, Dec 13, 2018 at 8:48:38 EET Segher Boessenkool wrote: > On Wed, Dec 12, 2018 at 06:26:10PM +0200, Dimitar Dimitrov wrote: > > I expect that if I mark a HW register as "clobber", compiler would save > > its > > contents before executing the asm statement, and after that it would > > restore its contents. This is the GCC behaviour for all but the SP and > > PIC registers. That is why I believe that PR52813 is a valid bug. > > It won't do it for *any* fixed registers. But you do not want to error > or even warn for some fixed registers, for example the "flags" register > on x86 is *always* written to by asm. Yes, you are correct. > > But you never want to warn for non-fixed registers, and e.g. > PIC_OFFSET_TABLE_REGNUM isn't always a fixed register (when flag_pic is 0 > for example). I could not trace how PIC_OFFSET_TABLE_REGNUM on i386 gets marked as fixed register. I'll dig more through the source. > > > I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered > > in such a way that GCC will not notice (e.g. thread switching), then why > > should GCC know about it in the first place? > > Up until today, GCC has always just ignored it if you claimed to clobber > the stack pointer. My point is that the silent ignoring is confusing to users, as shown by PR52813. How would you suggest me to proceed: - Leave patch as-is. - Revert patch. Update documentation to point that clobber marker for fixed registers is ignored by GCC. Close PR52813 as invalid. - Revert patch. Discuss more broadly and specify behaviour of asm clobber for fixed registers (and SP in particular). Thanks, Dimitar
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Wed, Dec 12, 2018 at 06:26:10PM +0200, Dimitar Dimitrov wrote: > I expect that if I mark a HW register as "clobber", compiler would save its > contents before executing the asm statement, and after that it would restore > its contents. This is the GCC behaviour for all but the SP and PIC registers. > That is why I believe that PR52813 is a valid bug. It won't do it for *any* fixed registers. But you do not want to error or even warn for some fixed registers, for example the "flags" register on x86 is *always* written to by asm. But you never want to warn for non-fixed registers, and e.g. PIC_OFFSET_TABLE_REGNUM isn't always a fixed register (when flag_pic is 0 for example). > I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered in > such a way that GCC will not notice (e.g. thread switching), then why should > GCC know about it in the first place? Up until today, GCC has always just ignored it if you claimed to clobber the stack pointer. Segher
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Wed, 12 Dec 2018 at 11:03:24 EET Christophe Lyon wrote: > And just noticed it causes a failure to build GDB for x86_64: > gdb-8.1-release/gdb/nat/linux-ptrace.c: In function 'void > linux_ptrace_init_warnings()': > gdb-8.1-release/gdb/nat/linux-ptrace.c:149:23: error: Stack Pointer > register clobbered by '%rsp' in 'asm' > 149 |: "%rsp", "memory"); > > | ^ > > Makefile:1640: recipe for target 'linux-ptrace.o' failed > > I didn't check if the GDB code is legitimate though Sorry about this. I had checked the Linux x86 kernel for SP clobbers, but forgot that GDB could also use such magic. I'll try to fix it and send a patch to GDB. It will likely take me a few days, so I hope that this breakage is not considered a P0 bug. Regards, Dimitar
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Wed, 12 Dec 2018 at 14:19:27 EET Christophe Lyon wrote: > On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme > > wrote: > > So my understanding is that the original code (CMSIS library) used to > > clobber sp because the asm statement was actually changing the sp. > > That in turn led GCC to try to save and restore sp which is not what > > CMSIS was expecting to happen. Changing sp without clobber as done now > > is probably the right solution and r242693 can be reverted. That will > > remove the failing test. > > OK, I read PR52813 too, but I'm not sure to fully understand the new status. > My understanding is that since this patch was committed, if an asm > statement clobbers sp, it is now allowed to actually declare it as clobber > (this patch generates an error in such a case). > So the user is now expected to lie to the compiler when writing to > this kind of register (sp, pic register), by not declaring it as "clobber"? Disclosure: I'm a GCC newbie. I expect that if I mark a HW register as "clobber", compiler would save its contents before executing the asm statement, and after that it would restore its contents. This is the GCC behaviour for all but the SP and PIC registers. That is why I believe that PR52813 is a valid bug. I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered in such a way that GCC will not notice (e.g. thread switching), then why should GCC know about it in the first place? Regards, Dimitar
Re: [PATCH] [RFC] PR target/52813 and target/11807
[resending from the right address] Hi Christophe, Why not simply: "Clobber of unsupported" with an accompanying change of the documentation to state the extra bit you wanted to put in that error message? Perhaps even add a reference to the section of the documentation in the error message. Best regards, Thomas On Wed, 12 Dec 2018 at 15:13, Christophe Lyon wrote: > > On Wed, 12 Dec 2018 at 14:19, Christophe Lyon > wrote: > > > > On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme > > wrote: > > > > > > So my understanding is that the original code (CMSIS library) used to > > > clobber sp because the asm statement was actually changing the sp. > > > That in turn led GCC to try to save and restore sp which is not what > > > CMSIS was expecting to happen. Changing sp without clobber as done now > > > is probably the right solution and r242693 can be reverted. That will > > > remove the failing test. > > > > > > > OK, I read PR52813 too, but I'm not sure to fully understand the new status. > > My understanding is that since this patch was committed, if an asm statement > > clobbers sp, it is now allowed to actually declare it as clobber (this patch > > generates an error in such a case). > > So the user is now expected to lie to the compiler when writing to > > this kind of register (sp, pic register), by not declaring it as "clobber"? > > > > I'm attaching a small patch which adds a more verbose error message > along the lines of what I understand of the current status. > I'm pretty sure I got (at least) the formatting wrong :) > > Christophe > > > > > > Best regards, > > > > > > Thomas > > > On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme > > > wrote: > > > > > > > > Hi Christophe, > > > > > > > > That PR was about a bug occuring when sp was clobbered so if it cannot > > > > be clobbered anymore the whole commit (r242693) can be removed. Let me > > > > check the original code that lead to the PR why it's clobbering sp > > > > though. > > > > > > > > Best regards, > > > > > > > > Thomas > > > > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon > > > > wrote: > > > > > > > > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford > > > > > wrote: > > > > > > > > > > > > Dimitar Dimitrov writes: > > > > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford > > > > > > > wrote: > > > > > > >> Dimitar Dimitrov writes: > > > > > > >> > I have tested this fix on x86_64 host, and found no regression > > > > > > >> > in the C > > > > > > >> > and C++ testsuites. I'm marking this patch as RFC simply > > > > > > >> > because I don't > > > > > > >> > have experience with other architectures, and I don't have a > > > > > > >> > setup to > > > > > > >> > test all architectures supported by GCC. > > > > > > >> > > > > > > > >> > gcc/ChangeLog: > > > > > > >> > > > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > > > >> > > > > > > > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > > > > > >> >error when stack pointer is clobbered. > > > > > > >> >(expand_asm_stmt): Refactor clobber check in separate > > > > > > >> > function. > > > > > > >> > > > > > > > >> > gcc/testsuite/ChangeLog: > > > > > > >> > > > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > > > >> > > > > > > > >> >* gcc.target/i386/pr52813.c: New test. > > > > > > >> > > > > > > > >> > Signed-off-by: Dimitar Dimitrov > > > > > > >> > > > > > > >> LGTM. Do you have a copyright assignment on file? 'Fraid this > > > > > > >> is > > > > > > >> probably big enough to need one. > > > > > > > Yes, I have copyright assignment. > > > > > > > > > > > > OK, great. I went ahead and applied the patch. > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > This patch introduces a regression on arm: > > > > > FAIL: gcc.target/arm/pr77904.c (test for excess errors) > > > > > Excess errors: > > > > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer > > > > > register clobbered by 'sp' in 'asm' > > > > > > > > > > Indeed the testcase has an explicit: > > > > > __asm volatile ("" : : : "sp"); > > > > > which is now rejected. > > > > > > > > > > Thomas, is that mandatory to test your code to fix pr77904? > > > > > > > > > > Thanks, > > > > > > > > > > Christophe > > > > > > > > > > > Thanks, > > > > > > Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Wed, 12 Dec 2018 at 14:19, Christophe Lyon wrote: > > On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme > wrote: > > > > So my understanding is that the original code (CMSIS library) used to > > clobber sp because the asm statement was actually changing the sp. > > That in turn led GCC to try to save and restore sp which is not what > > CMSIS was expecting to happen. Changing sp without clobber as done now > > is probably the right solution and r242693 can be reverted. That will > > remove the failing test. > > > > OK, I read PR52813 too, but I'm not sure to fully understand the new status. > My understanding is that since this patch was committed, if an asm statement > clobbers sp, it is now allowed to actually declare it as clobber (this patch > generates an error in such a case). > So the user is now expected to lie to the compiler when writing to > this kind of register (sp, pic register), by not declaring it as "clobber"? > I'm attaching a small patch which adds a more verbose error message along the lines of what I understand of the current status. I'm pretty sure I got (at least) the formatting wrong :) Christophe > > > Best regards, > > > > Thomas > > On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme > > wrote: > > > > > > Hi Christophe, > > > > > > That PR was about a bug occuring when sp was clobbered so if it cannot > > > be clobbered anymore the whole commit (r242693) can be removed. Let me > > > check the original code that lead to the PR why it's clobbering sp > > > though. > > > > > > Best regards, > > > > > > Thomas > > > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon > > > wrote: > > > > > > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford > > > > wrote: > > > > > > > > > > Dimitar Dimitrov writes: > > > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford > > > > > > wrote: > > > > > >> Dimitar Dimitrov writes: > > > > > >> > I have tested this fix on x86_64 host, and found no regression > > > > > >> > in the C > > > > > >> > and C++ testsuites. I'm marking this patch as RFC simply > > > > > >> > because I don't > > > > > >> > have experience with other architectures, and I don't have a > > > > > >> > setup to > > > > > >> > test all architectures supported by GCC. > > > > > >> > > > > > > >> > gcc/ChangeLog: > > > > > >> > > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > > >> > > > > > > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > > > > >> >error when stack pointer is clobbered. > > > > > >> >(expand_asm_stmt): Refactor clobber check in separate > > > > > >> > function. > > > > > >> > > > > > > >> > gcc/testsuite/ChangeLog: > > > > > >> > > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > > >> > > > > > > >> >* gcc.target/i386/pr52813.c: New test. > > > > > >> > > > > > > >> > Signed-off-by: Dimitar Dimitrov > > > > > >> > > > > > >> LGTM. Do you have a copyright assignment on file? 'Fraid this is > > > > > >> probably big enough to need one. > > > > > > Yes, I have copyright assignment. > > > > > > > > > > OK, great. I went ahead and applied the patch. > > > > > > > > > > > > > Hi, > > > > > > > > This patch introduces a regression on arm: > > > > FAIL: gcc.target/arm/pr77904.c (test for excess errors) > > > > Excess errors: > > > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer > > > > register clobbered by 'sp' in 'asm' > > > > > > > > Indeed the testcase has an explicit: > > > > __asm volatile ("" : : : "sp"); > > > > which is now rejected. > > > > > > > > Thomas, is that mandatory to test your code to fix pr77904? > > > > > > > > Thanks, > > > > > > > > Christophe > > > > > > > > > Thanks, > > > > > Richard 2018-12-12 Christophe Lyon gcc/ * cfgexpand.c (asm_clobber_reg_is_valid): Add a more descriptive error message. diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 0d04bbc..e1f2bff 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2870,12 +2870,20 @@ asm_clobber_reg_is_valid (int regno, int nregs, const char *regname) { /* ??? Diagnose during gimplification? */ error ("PIC register clobbered by %qs in %", regname); + error ("Such clobbers are not supported by GCC. " +"If you really want to overwrite the PIC register, " +"remove it from the clobber list in the % at your own risk: " +"GCC will not save it."); is_valid = false; } /* Clobbering the STACK POINTER register is an error. */ if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) { error ("Stack Pointer register clobbered by %qs in %", regname); + error ("Such clobbers are not supported by GCC. " +"If you really want to overwrite the Stack Pointer, " +"remove it from the clobber list in the % at your own risk: " +"GCC will not save it."); is_valid = false; }
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme wrote: > > So my understanding is that the original code (CMSIS library) used to > clobber sp because the asm statement was actually changing the sp. > That in turn led GCC to try to save and restore sp which is not what > CMSIS was expecting to happen. Changing sp without clobber as done now > is probably the right solution and r242693 can be reverted. That will > remove the failing test. > OK, I read PR52813 too, but I'm not sure to fully understand the new status. My understanding is that since this patch was committed, if an asm statement clobbers sp, it is now allowed to actually declare it as clobber (this patch generates an error in such a case). So the user is now expected to lie to the compiler when writing to this kind of register (sp, pic register), by not declaring it as "clobber"? > Best regards, > > Thomas > On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme > wrote: > > > > Hi Christophe, > > > > That PR was about a bug occuring when sp was clobbered so if it cannot > > be clobbered anymore the whole commit (r242693) can be removed. Let me > > check the original code that lead to the PR why it's clobbering sp > > though. > > > > Best regards, > > > > Thomas > > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon > > wrote: > > > > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford > > > wrote: > > > > > > > > Dimitar Dimitrov writes: > > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford > > > > > wrote: > > > > >> Dimitar Dimitrov writes: > > > > >> > I have tested this fix on x86_64 host, and found no regression in > > > > >> > the C > > > > >> > and C++ testsuites. I'm marking this patch as RFC simply because > > > > >> > I don't > > > > >> > have experience with other architectures, and I don't have a setup > > > > >> > to > > > > >> > test all architectures supported by GCC. > > > > >> > > > > > >> > gcc/ChangeLog: > > > > >> > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > >> > > > > > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > > > >> >error when stack pointer is clobbered. > > > > >> >(expand_asm_stmt): Refactor clobber check in separate function. > > > > >> > > > > > >> > gcc/testsuite/ChangeLog: > > > > >> > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > >> > > > > > >> >* gcc.target/i386/pr52813.c: New test. > > > > >> > > > > > >> > Signed-off-by: Dimitar Dimitrov > > > > >> > > > > >> LGTM. Do you have a copyright assignment on file? 'Fraid this is > > > > >> probably big enough to need one. > > > > > Yes, I have copyright assignment. > > > > > > > > OK, great. I went ahead and applied the patch. > > > > > > > > > > Hi, > > > > > > This patch introduces a regression on arm: > > > FAIL: gcc.target/arm/pr77904.c (test for excess errors) > > > Excess errors: > > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer > > > register clobbered by 'sp' in 'asm' > > > > > > Indeed the testcase has an explicit: > > > __asm volatile ("" : : : "sp"); > > > which is now rejected. > > > > > > Thomas, is that mandatory to test your code to fix pr77904? > > > > > > Thanks, > > > > > > Christophe > > > > > > > Thanks, > > > > Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
This breaks ia64: In file included from ../../../libgomp/config/linux/wait.h:46, from ../../../libgomp/config/linux/mutex.c:30: ../../../libgomp/config/linux/ia64/futex.h: In function 'gomp_mutex_lock_slow': ../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm' 43 | __asm __volatile ("break 0x10" | ^ ../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm' 43 | __asm __volatile ("break 0x10" | ^ ../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm' 43 | __asm __volatile ("break 0x10" | ^ ../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm' 43 | __asm __volatile ("break 0x10" | ^ ../../../libgomp/config/linux/ia64/futex.h: In function 'gomp_mutex_unlock_slow': ../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm' 43 | __asm __volatile ("break 0x10" | ^ ../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm' 43 | __asm __volatile ("break 0x10" | ^ Installed as obvious. Andreas. * config/linux/ia64/futex.h (sys_futex0): Don't mark r12 as clobbered. --- libgomp/config/linux/ia64/futex.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libgomp/config/linux/ia64/futex.h b/libgomp/config/linux/ia64/futex.h index 6efec3c813..df450f8def 100644 --- a/libgomp/config/linux/ia64/futex.h +++ b/libgomp/config/linux/ia64/futex.h @@ -45,8 +45,8 @@ sys_futex0(int *addr, int op, int val) "=r"(r8), "=r"(r10) : "r"(r15), "r"(out0), "r"(out1), "r"(out2), "r"(out3) : "memory", "out4", "out5", "out6", "out7", - /* Non-stacked integer registers, minus r8, r10, r15. */ - "r2", "r3", "r9", "r11", "r12", "r13", "r14", "r16", "r17", "r18", + /* Non-stacked integer registers, minus r8, r10, r12, r15. */ + "r2", "r3", "r9", "r11", "r13", "r14", "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23", "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31", /* Predicate registers. */ -- 2.20.0 -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: [PATCH] [RFC] PR target/52813 and target/11807
So my understanding is that the original code (CMSIS library) used to clobber sp because the asm statement was actually changing the sp. That in turn led GCC to try to save and restore sp which is not what CMSIS was expecting to happen. Changing sp without clobber as done now is probably the right solution and r242693 can be reverted. That will remove the failing test. Best regards, Thomas On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme wrote: > > Hi Christophe, > > That PR was about a bug occuring when sp was clobbered so if it cannot > be clobbered anymore the whole commit (r242693) can be removed. Let me > check the original code that lead to the PR why it's clobbering sp > though. > > Best regards, > > Thomas > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon > wrote: > > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford > > wrote: > > > > > > Dimitar Dimitrov writes: > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote: > > > >> Dimitar Dimitrov writes: > > > >> > I have tested this fix on x86_64 host, and found no regression in > > > >> > the C > > > >> > and C++ testsuites. I'm marking this patch as RFC simply because I > > > >> > don't > > > >> > have experience with other architectures, and I don't have a setup to > > > >> > test all architectures supported by GCC. > > > >> > > > > >> > gcc/ChangeLog: > > > >> > > > > >> > 2018-12-07 Dimitar Dimitrov > > > >> > > > > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > > >> >error when stack pointer is clobbered. > > > >> >(expand_asm_stmt): Refactor clobber check in separate function. > > > >> > > > > >> > gcc/testsuite/ChangeLog: > > > >> > > > > >> > 2018-12-07 Dimitar Dimitrov > > > >> > > > > >> >* gcc.target/i386/pr52813.c: New test. > > > >> > > > > >> > Signed-off-by: Dimitar Dimitrov > > > >> > > > >> LGTM. Do you have a copyright assignment on file? 'Fraid this is > > > >> probably big enough to need one. > > > > Yes, I have copyright assignment. > > > > > > OK, great. I went ahead and applied the patch. > > > > > > > Hi, > > > > This patch introduces a regression on arm: > > FAIL: gcc.target/arm/pr77904.c (test for excess errors) > > Excess errors: > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer > > register clobbered by 'sp' in 'asm' > > > > Indeed the testcase has an explicit: > > __asm volatile ("" : : : "sp"); > > which is now rejected. > > > > Thomas, is that mandatory to test your code to fix pr77904? > > > > Thanks, > > > > Christophe > > > > > Thanks, > > > Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
Hi Christophe, That PR was about a bug occuring when sp was clobbered so if it cannot be clobbered anymore the whole commit (r242693) can be removed. Let me check the original code that lead to the PR why it's clobbering sp though. Best regards, Thomas On Wed, 12 Dec 2018 at 09:43, Christophe Lyon wrote: > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford > wrote: > > > > Dimitar Dimitrov writes: > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote: > > >> Dimitar Dimitrov writes: > > >> > I have tested this fix on x86_64 host, and found no regression in the C > > >> > and C++ testsuites. I'm marking this patch as RFC simply because I > > >> > don't > > >> > have experience with other architectures, and I don't have a setup to > > >> > test all architectures supported by GCC. > > >> > > > >> > gcc/ChangeLog: > > >> > > > >> > 2018-12-07 Dimitar Dimitrov > > >> > > > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > >> >error when stack pointer is clobbered. > > >> >(expand_asm_stmt): Refactor clobber check in separate function. > > >> > > > >> > gcc/testsuite/ChangeLog: > > >> > > > >> > 2018-12-07 Dimitar Dimitrov > > >> > > > >> >* gcc.target/i386/pr52813.c: New test. > > >> > > > >> > Signed-off-by: Dimitar Dimitrov > > >> > > >> LGTM. Do you have a copyright assignment on file? 'Fraid this is > > >> probably big enough to need one. > > > Yes, I have copyright assignment. > > > > OK, great. I went ahead and applied the patch. > > > > Hi, > > This patch introduces a regression on arm: > FAIL: gcc.target/arm/pr77904.c (test for excess errors) > Excess errors: > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer > register clobbered by 'sp' in 'asm' > > Indeed the testcase has an explicit: > __asm volatile ("" : : : "sp"); > which is now rejected. > > Thomas, is that mandatory to test your code to fix pr77904? > > Thanks, > > Christophe > > > Thanks, > > Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Wed, 12 Dec 2018 at 10:42, Christophe Lyon wrote: > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford > wrote: > > > > Dimitar Dimitrov writes: > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote: > > >> Dimitar Dimitrov writes: > > >> > I have tested this fix on x86_64 host, and found no regression in the C > > >> > and C++ testsuites. I'm marking this patch as RFC simply because I > > >> > don't > > >> > have experience with other architectures, and I don't have a setup to > > >> > test all architectures supported by GCC. > > >> > > > >> > gcc/ChangeLog: > > >> > > > >> > 2018-12-07 Dimitar Dimitrov > > >> > > > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > >> >error when stack pointer is clobbered. > > >> >(expand_asm_stmt): Refactor clobber check in separate function. > > >> > > > >> > gcc/testsuite/ChangeLog: > > >> > > > >> > 2018-12-07 Dimitar Dimitrov > > >> > > > >> >* gcc.target/i386/pr52813.c: New test. > > >> > > > >> > Signed-off-by: Dimitar Dimitrov > > >> > > >> LGTM. Do you have a copyright assignment on file? 'Fraid this is > > >> probably big enough to need one. > > > Yes, I have copyright assignment. > > > > OK, great. I went ahead and applied the patch. > > > > Hi, > > This patch introduces a regression on arm: > FAIL: gcc.target/arm/pr77904.c (test for excess errors) > Excess errors: > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer > register clobbered by 'sp' in 'asm' > > Indeed the testcase has an explicit: > __asm volatile ("" : : : "sp"); > which is now rejected. > > Thomas, is that mandatory to test your code to fix pr77904? > > Thanks, > > Christophe > And just noticed it causes a failure to build GDB for x86_64: gdb-8.1-release/gdb/nat/linux-ptrace.c: In function 'void linux_ptrace_init_warnings()': gdb-8.1-release/gdb/nat/linux-ptrace.c:149:23: error: Stack Pointer register clobbered by '%rsp' in 'asm' 149 |: "%rsp", "memory"); | ^ Makefile:1640: recipe for target 'linux-ptrace.o' failed I didn't check if the GDB code is legitimate though > > Thanks, > > Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On Tue, 11 Dec 2018 at 16:52, Richard Sandiford wrote: > > Dimitar Dimitrov writes: > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote: > >> Dimitar Dimitrov writes: > >> > I have tested this fix on x86_64 host, and found no regression in the C > >> > and C++ testsuites. I'm marking this patch as RFC simply because I don't > >> > have experience with other architectures, and I don't have a setup to > >> > test all architectures supported by GCC. > >> > > >> > gcc/ChangeLog: > >> > > >> > 2018-12-07 Dimitar Dimitrov > >> > > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce > >> >error when stack pointer is clobbered. > >> >(expand_asm_stmt): Refactor clobber check in separate function. > >> > > >> > gcc/testsuite/ChangeLog: > >> > > >> > 2018-12-07 Dimitar Dimitrov > >> > > >> >* gcc.target/i386/pr52813.c: New test. > >> > > >> > Signed-off-by: Dimitar Dimitrov > >> > >> LGTM. Do you have a copyright assignment on file? 'Fraid this is > >> probably big enough to need one. > > Yes, I have copyright assignment. > > OK, great. I went ahead and applied the patch. > Hi, This patch introduces a regression on arm: FAIL: gcc.target/arm/pr77904.c (test for excess errors) Excess errors: /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer register clobbered by 'sp' in 'asm' Indeed the testcase has an explicit: __asm volatile ("" : : : "sp"); which is now rejected. Thomas, is that mandatory to test your code to fix pr77904? Thanks, Christophe > Thanks, > Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
Dimitar Dimitrov writes: > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote: >> Dimitar Dimitrov writes: >> > I have tested this fix on x86_64 host, and found no regression in the C >> > and C++ testsuites. I'm marking this patch as RFC simply because I don't >> > have experience with other architectures, and I don't have a setup to >> > test all architectures supported by GCC. >> > >> > gcc/ChangeLog: >> > >> > 2018-12-07 Dimitar Dimitrov >> > >> >* cfgexpand.c (asm_clobber_reg_is_valid): Also produce >> >error when stack pointer is clobbered. >> >(expand_asm_stmt): Refactor clobber check in separate function. >> > >> > gcc/testsuite/ChangeLog: >> > >> > 2018-12-07 Dimitar Dimitrov >> > >> >* gcc.target/i386/pr52813.c: New test. >> > >> > Signed-off-by: Dimitar Dimitrov >> >> LGTM. Do you have a copyright assignment on file? 'Fraid this is >> probably big enough to need one. > Yes, I have copyright assignment. OK, great. I went ahead and applied the patch. Thanks, Richard
Re: [PATCH] [RFC] PR target/52813 and target/11807
On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote: > Dimitar Dimitrov writes: > > I have tested this fix on x86_64 host, and found no regression in the C > > and C++ testsuites. I'm marking this patch as RFC simply because I don't > > have experience with other architectures, and I don't have a setup to > > test all architectures supported by GCC. > > > > gcc/ChangeLog: > > > > 2018-12-07 Dimitar Dimitrov > > > > * cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > error when stack pointer is clobbered. > > (expand_asm_stmt): Refactor clobber check in separate function. > > > > gcc/testsuite/ChangeLog: > > > > 2018-12-07 Dimitar Dimitrov > > > > * gcc.target/i386/pr52813.c: New test. > > > > Signed-off-by: Dimitar Dimitrov > > LGTM. Do you have a copyright assignment on file? 'Fraid this is > probably big enough to need one. Yes, I have copyright assignment. Regards, Dimitar
Re: [PATCH] [RFC] PR target/52813 and target/11807
Dimitar Dimitrov writes: > I have tested this fix on x86_64 host, and found no regression in the C > and C++ testsuites. I'm marking this patch as RFC simply because I don't > have experience with other architectures, and I don't have a setup to > test all architectures supported by GCC. > > gcc/ChangeLog: > > 2018-12-07 Dimitar Dimitrov > > * cfgexpand.c (asm_clobber_reg_is_valid): Also produce > error when stack pointer is clobbered. > (expand_asm_stmt): Refactor clobber check in separate function. > > gcc/testsuite/ChangeLog: > > 2018-12-07 Dimitar Dimitrov > > * gcc.target/i386/pr52813.c: New test. > > Signed-off-by: Dimitar Dimitrov LGTM. Do you have a copyright assignment on file? 'Fraid this is probably big enough to need one. Thanks, Richard
[PATCH] [RFC] PR target/52813 and target/11807
I have tested this fix on x86_64 host, and found no regression in the C and C++ testsuites. I'm marking this patch as RFC simply because I don't have experience with other architectures, and I don't have a setup to test all architectures supported by GCC. gcc/ChangeLog: 2018-12-07 Dimitar Dimitrov * cfgexpand.c (asm_clobber_reg_is_valid): Also produce error when stack pointer is clobbered. (expand_asm_stmt): Refactor clobber check in separate function. gcc/testsuite/ChangeLog: 2018-12-07 Dimitar Dimitrov * gcc.target/i386/pr52813.c: New test. Signed-off-by: Dimitar Dimitrov --- gcc/cfgexpand.c | 42 ++--- gcc/testsuite/gcc.target/i386/pr52813.c | 9 +++ 2 files changed, 43 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/gcc.target/i386/pr52813.c diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 5e23bc242b9..8474372a216 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2845,6 +2845,38 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_SET *clobbered_regs) return false; } +/* Check that the given REGNO spanning NREGS is a valid + asm clobber operand. Some HW registers cannot be + saved/restored, hence they should not be clobbered by + asm statements. */ +static bool +asm_clobber_reg_is_valid (int regno, int nregs, const char *regname) +{ + bool is_valid = true; + HARD_REG_SET regset; + + CLEAR_HARD_REG_SET (regset); + + add_range_to_hard_reg_set (, regno, nregs); + + /* Clobbering the PIC register is an error. */ + if (PIC_OFFSET_TABLE_REGNUM != INVALID_REGNUM + && overlaps_hard_reg_set_p (regset, Pmode, PIC_OFFSET_TABLE_REGNUM)) +{ + /* ??? Diagnose during gimplification? */ + error ("PIC register clobbered by %qs in %", regname); + is_valid = false; +} + /* Clobbering the STACK POINTER register is an error. */ + if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) +{ + error ("Stack Pointer register clobbered by %qs in %", regname); + is_valid = false; +} + + return is_valid; +} + /* Generate RTL for an asm statement with arguments. STRING is the instruction template. OUTPUTS is a list of output arguments (lvalues); INPUTS a list of inputs. @@ -2977,14 +3009,8 @@ expand_asm_stmt (gasm *stmt) else for (int reg = j; reg < j + nregs; reg++) { - /* Clobbering the PIC register is an error. */ - if (reg == (int) PIC_OFFSET_TABLE_REGNUM) - { - /* ??? Diagnose during gimplification? */ - error ("PIC register clobbered by %qs in %", - regname); - return; - } + if (!asm_clobber_reg_is_valid (reg, nregs, regname)) + return; SET_HARD_REG_BIT (clobbered_regs, reg); rtx x = gen_rtx_REG (reg_raw_mode[reg], reg); diff --git a/gcc/testsuite/gcc.target/i386/pr52813.c b/gcc/testsuite/gcc.target/i386/pr52813.c new file mode 100644 index 000..154ebbfc423 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr52813.c @@ -0,0 +1,9 @@ +/* Ensure that stack pointer cannot be an asm clobber. */ +/* { dg-do compile { target { ! ia32 } } } */ +/* { dg-options "-O2" } */ + +void +test1 (void) +{ + asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */ +} -- 2.11.0