| Message ID | 32da4d9c196bd30988776e19c4f210381b630c92.camel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] Fix STATIC_CHAIN_REGNUM for v850 port | expand |
On Sat, 29 Feb 2020, Jeff Law wrote: > > Wow, I think I wrote the v850 port back in circa 1997 and this bug has been > latent all this time. Vlad's IRA changes twiddled register allocation in just > the right way to expose this bug. > > I'm not sure what I was thinking, but apparently I made a spectacularly bad > choice for the STATIC_CHAIN_REGNUM in choosing a call-saved register (r20). > > It's simply wrong to use a call-saved register for the static chain. Heh. I did that mistake too, for CRIS. :/ A comment from RTH below my (incorrect) comment in cris.c above cris_asm_trampoline_template alludes to there being an ABI-neutral solution: "??? See the i386 regparm=3 implementation that pushes the static chain value to the stack in the trampoline, and uses a call-saved register when called directly." ... but IIRC it either didn't apply for CRIS or I didn't look into it thoroughly enough. Or that's also buggy. brgds, H-P PS. Perhaps a doc update with a warning is a suitable penance? :)
On Tue, 2020-03-03 at 13:38 -0500, Hans-Peter Nilsson wrote: > On Sat, 29 Feb 2020, Jeff Law wrote: > > Wow, I think I wrote the v850 port back in circa 1997 and this bug has been > > latent all this time. Vlad's IRA changes twiddled register allocation in > > just > > the right way to expose this bug. > > > > I'm not sure what I was thinking, but apparently I made a spectacularly bad > > choice for the STATIC_CHAIN_REGNUM in choosing a call-saved register (r20). > > > > It's simply wrong to use a call-saved register for the static chain. > > Heh. I did that mistake too, for CRIS. :/ > > A comment from RTH below my (incorrect) comment in cris.c above > cris_asm_trampoline_template alludes to there being an > ABI-neutral solution: "??? See the i386 regparm=3 implementation > that pushes the static chain value to the stack in the > trampoline, and uses a call-saved register when called > directly." ... but IIRC it either didn't apply for CRIS or I > didn't look into it thoroughly enough. Or that's also buggy. > > brgds, H-P > PS. Perhaps a doc update with a warning is a suitable penance? :) That was already the plan. I took care of the gcc-10 changes.html bit today. jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 7d95db8623f..2a69c680a9b 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2020-02-28 Jeff Law <law@redhat.com> + + * config/v850/v850.h (STATIC_CHAIN_REGNUM): Change to r19. + * config/v850/v850.c (v850_asm_trampoline_template): Update + accordingly. + 2020-02-28 Michael Meissner <meissner@linux.ibm.com> PR target/93937 diff --git a/gcc/config/v850/v850.c b/gcc/config/v850/v850.c index 074adf87687..4b0e28c1786 100644 --- a/gcc/config/v850/v850.c +++ b/gcc/config/v850/v850.c @@ -2961,7 +2961,7 @@ static void v850_asm_trampoline_template (FILE *f) { fprintf (f, "\tjarl .+4,r12\n"); - fprintf (f, "\tld.w 12[r12],r20\n"); + fprintf (f, "\tld.w 12[r12],r19\n"); fprintf (f, "\tld.w 16[r12],r12\n"); fprintf (f, "\tjmp [r12]\n"); fprintf (f, "\tnop\n"); diff --git a/gcc/config/v850/v850.h b/gcc/config/v850/v850.h index 823bc5e17e3..7ae583c7df2 100644 --- a/gcc/config/v850/v850.h +++ b/gcc/config/v850/v850.h @@ -438,8 +438,9 @@ enum reg_class /* Base register for access to arguments of the function. */ #define ARG_POINTER_REGNUM 35 -/* Register in which static-chain is passed to a function. */ -#define STATIC_CHAIN_REGNUM 20 +/* Register in which static-chain is passed to a function. + This must be a call used register. */ +#define STATIC_CHAIN_REGNUM 19 /* If defined, this macro specifies a table of register pairs used to eliminate unneeded registers that point into the stack frame. If
Wow, I think I wrote the v850 port back in circa 1997 and this bug has been latent all this time. Vlad's IRA changes twiddled register allocation in just the right way to expose this bug. I'm not sure what I was thinking, but apparently I made a spectacularly bad choice for the STATIC_CHAIN_REGNUM in choosing a call-saved register (r20). It's simply wrong to use a call-saved register for the static chain. Think about what the case if we take the address of a nested function. We actually get the address of the trampoline. Then assume we call through that function pointer at some point deeper in the call stack. At the call site we have to express that the static chain register was changed, but there's no way to know at the call site -- that's the whole point of using the trampoline, it looks just like a normal indirect call. The only was I can see to fix this is to fix the static chain register to be a call clobbered register which is an ABI change. Thankfully the combination of v850 and nested functions probably isn't used terribly much. I've verified this fixed the recent v850 regressions. Committing to the trunk. Jeff commit c7dbc54958321d296ca4e283f26f279f6a5342a7 Author: Jeff Law <law@redhat.com> Date: Sat Feb 29 13:45:37 2020 -0700 Make STATIC_CHAIN_REGNUM a call used register. * config/v850/v850.h (STATIC_CHAIN_REGNUM): Change to r19. * config/v850/v850.c (v850_asm_trampoline_template): Update accordingly.