| Message ID | YNwVSUSNjUn3UNv3@squeak.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
| Series | [POWER10] __morestack calls from pcrel code | expand |
Alan Modra via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > Compiling gcc/testsuite/gcc.dg/split-*.c and others with -mcpu=power10 > and linking with a non-pcrel libgcc results in crashes due to the > power10 pcrel code not having r2 set for the generic-morestack.c > functions called from __morestack. There is also a problem when > non-pcrel code calls a pcrel libgcc. See the patch comments. > > A similar situation theoretically occurs with ELFv1 multi-toc > executables, when __morestack might be located in a different toc > group to its caller. This patch makes no attempt to fix that, since > the gold linker does not support multi-toc (gold is needed for proper > support of -fsplit-stack code) nor does gcc emit __morestack calls > that support multi-toc. > > Bootstrapped and regression tested power64le-linux with both > -mcpu=power10 and -mcpu=power9. OK for mainline and backporting to > gcc-11 and gcc-10? > > * config/rs6000/morestack.S (R2_SAVE): Define. > (__morestack): Save and restore r2. Set up r2 for called > functions. Thanks! This patch solved the issue I was seeing. If it gets merged, can this patch be backported to GCC 10 and 11, please?
On Wed, Jun 30, 2021 at 05:06:30PM -0300, Tulio Magno Quites Machado Filho wrote: > Alan Modra via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > > Compiling gcc/testsuite/gcc.dg/split-*.c and others with -mcpu=power10 > > and linking with a non-pcrel libgcc results in crashes due to the > > power10 pcrel code not having r2 set for the generic-morestack.c > > functions called from __morestack. There is also a problem when > > non-pcrel code calls a pcrel libgcc. See the patch comments. > > > > A similar situation theoretically occurs with ELFv1 multi-toc > > executables, when __morestack might be located in a different toc > > group to its caller. This patch makes no attempt to fix that, since > > the gold linker does not support multi-toc (gold is needed for proper > > support of -fsplit-stack code) nor does gcc emit __morestack calls > > that support multi-toc. > > > > Bootstrapped and regression tested power64le-linux with both > > -mcpu=power10 and -mcpu=power9. OK for mainline and backporting to > > gcc-11 and gcc-10? > > > > * config/rs6000/morestack.S (R2_SAVE): Define. > > (__morestack): Save and restore r2. Set up r2 for called > > functions. > > Thanks! This patch solved the issue I was seeing. > > If it gets merged, can this patch be backported to GCC 10 and 11, please? > > -- > Tulio Magno https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573978.html This patch has now been unreviewed for over two weeks. I expected a rubber stamp style approval; This assembly file is all mine, I know the ABI and how .eh_frame driven exception handling works on powerpc. So I'm going to claim the patch is obvious enough to someone with a good understanding of what is going on in morestack.S and commit under the "obvious" rule after allowing a few more days for comment.
On Wed, Jul 14, 2021 at 8:01 PM Alan Modra <amodra@gmail.com> wrote: > > On Wed, Jun 30, 2021 at 05:06:30PM -0300, Tulio Magno Quites Machado Filho wrote: > > Alan Modra via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > > > > Compiling gcc/testsuite/gcc.dg/split-*.c and others with -mcpu=power10 > > > and linking with a non-pcrel libgcc results in crashes due to the > > > power10 pcrel code not having r2 set for the generic-morestack.c > > > functions called from __morestack. There is also a problem when > > > non-pcrel code calls a pcrel libgcc. See the patch comments. > > > > > > A similar situation theoretically occurs with ELFv1 multi-toc > > > executables, when __morestack might be located in a different toc > > > group to its caller. This patch makes no attempt to fix that, since > > > the gold linker does not support multi-toc (gold is needed for proper > > > support of -fsplit-stack code) nor does gcc emit __morestack calls > > > that support multi-toc. > > > > > > Bootstrapped and regression tested power64le-linux with both > > > -mcpu=power10 and -mcpu=power9. OK for mainline and backporting to > > > gcc-11 and gcc-10? > > > > > > * config/rs6000/morestack.S (R2_SAVE): Define. > > > (__morestack): Save and restore r2. Set up r2 for called > > > functions. > > > > Thanks! This patch solved the issue I was seeing. > > > > If it gets merged, can this patch be backported to GCC 10 and 11, please? > > > > -- > > Tulio Magno > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573978.html > > This patch has now been unreviewed for over two weeks. I expected a > rubber stamp style approval; This assembly file is all mine, I know > the ABI and how .eh_frame driven exception handling works on powerpc. > So I'm going to claim the patch is obvious enough to someone with a > good understanding of what is going on in morestack.S and commit under > the "obvious" rule after allowing a few more days for comment. This patch is okay. Thanks, David
On Wed, Jul 21, 2021 at 08:59:04AM -0400, David Edelsohn wrote: > On Wed, Jul 21, 2021 at 4:29 AM Alan Modra <amodra@gmail.com> wrote: > > > > On Wed, Jul 14, 2021 at 08:24:16PM -0400, David Edelsohn wrote: > > > > > > * config/rs6000/morestack.S (R2_SAVE): Define. > > > > > > (__morestack): Save and restore r2. Set up r2 for called > > > > > > functions. > > > > > > This patch is okay. > > > > Thanks David, the patch is needed on gcc-11 and gcc-10 too. > > OK for the branches too? > > Backports are fine, but I believe that Richi is planning to cut GCC 11 > RC today, so you really should check with him about a backport at the > last minute. Hi Richard, Is this patch OK at this late stage for the gcc-11 branch? https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573978.html The impacts of the bug are segfaults and other undesirable behaviour with Go (or more generally -fsplit-stack) on power10 when libgcc is not power10 pcrel. A non-pcrel libgcc is very likely how distros will ship gcc.
On Thu, 22 Jul 2021, Alan Modra wrote: > On Wed, Jul 21, 2021 at 08:59:04AM -0400, David Edelsohn wrote: > > On Wed, Jul 21, 2021 at 4:29 AM Alan Modra <amodra@gmail.com> wrote: > > > > > > On Wed, Jul 14, 2021 at 08:24:16PM -0400, David Edelsohn wrote: > > > > > > > * config/rs6000/morestack.S (R2_SAVE): Define. > > > > > > > (__morestack): Save and restore r2. Set up r2 for called > > > > > > > functions. > > > > > > > > This patch is okay. > > > > > > Thanks David, the patch is needed on gcc-11 and gcc-10 too. > > > OK for the branches too? > > > > Backports are fine, but I believe that Richi is planning to cut GCC 11 > > RC today, so you really should check with him about a backport at the > > last minute. > > Hi Richard, > Is this patch OK at this late stage for the gcc-11 branch? > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573978.html > > The impacts of the bug are segfaults and other undesirable behaviour > with Go (or more generally -fsplit-stack) on power10 when libgcc is > not power10 pcrel. A non-pcrel libgcc is very likely how distros > will ship gcc. If you think it's safe (well, there are not many __morestack users, so based on that it's pretty safe) then go ahead. Richard.
diff --git a/libgcc/config/rs6000/morestack.S b/libgcc/config/rs6000/morestack.S index 4a07de927c5..a2e255e5c21 100644 --- a/libgcc/config/rs6000/morestack.S +++ b/libgcc/config/rs6000/morestack.S @@ -31,6 +31,7 @@ #define PARAMS 48 #endif #define MORESTACK_FRAMESIZE (PARAMS+96) +#define R2_SAVE -MORESTACK_FRAMESIZE+PARAMS-8 #define PARAMREG_SAVE -MORESTACK_FRAMESIZE+PARAMS+0 #define STATIC_CHAIN_SAVE -MORESTACK_FRAMESIZE+PARAMS+64 #define R29_SAVE -MORESTACK_FRAMESIZE+PARAMS+72 @@ -143,6 +144,17 @@ ENTRY0(__morestack_non_split) # cr7 must also be preserved. ENTRY0(__morestack) + +#if _CALL_ELF == 2 +# Functions with localentry bits of zero cannot make calls if those +# calls might change r2. This is true generally, and also true for +# __morestack with its special calling convention. When __morestack's +# caller is non-pcrel but libgcc is pcrel, the functions called here +# might modify r2. r2 must be preserved on exit, and also restored +# for the call back to our caller. + std %r2,R2_SAVE(%r1) +#endif + # Save parameter passing registers, our arguments, lr, r29 # and use r29 as a frame pointer. std %r3,PARAMREG_SAVE+0(%r1) @@ -161,10 +173,24 @@ ENTRY0(__morestack) std %r12,LINKREG_SAVE(%r1) std %r3,NEWSTACKSIZE_SAVE(%r1) # new stack size mr %r29,%r1 +#if _CALL_ELF == 2 + .cfi_offset %r2,R2_SAVE +#endif .cfi_offset %r29,R29_SAVE .cfi_def_cfa_register %r29 stdu %r1,-MORESTACK_FRAMESIZE(%r1) +#if _CALL_ELF == 2 && !defined __PCREL__ +# If this isn't a pcrel libgcc then the functions we call here will +# require r2 to be valid. If __morestack is called from pcrel code r2 +# won't be valid. Set it up. + bcl 20,31,1f +1: + mflr %r12 + addis %r2,%r12,.TOC.-1b@ha + addi %r2,%r2,.TOC.-1b@l +#endif + # void __morestack_block_signals (void) bl JUMP_TARGET(__morestack_block_signals) @@ -199,6 +225,9 @@ ENTRY0(__morestack) # instructions after __morestack's return address. # ld %r12,LINKREG_SAVE(%r29) +#if _CALL_ELF == 2 + ld %r2,R2_SAVE(%r29) +#endif ld %r3,PARAMREG_SAVE+0(%r29) # restore arg regs ld %r4,PARAMREG_SAVE+8(%r29) ld %r5,PARAMREG_SAVE+16(%r29) @@ -228,6 +257,15 @@ ENTRY0(__morestack) std %r10,PARAMREG_SAVE+56(%r29) #endif +#if _CALL_ELF == 2 && !defined __PCREL__ +# r2 was restored for calling back into our caller. Set it up again. + bcl 20,31,1f +1: + mflr %r12 + addis %r2,%r12,.TOC.-1b@ha + addi %r2,%r2,.TOC.-1b@l +#endif + bl JUMP_TARGET(__morestack_block_signals) # void *__generic_releasestack (size_t *pavailable) @@ -249,6 +287,9 @@ ENTRY0(__morestack) # Restore return value regs, and return. ld %r0,LINKREG_SAVE(%r29) mtlr %r0 +#if _CALL_ELF == 2 + ld %r2,R2_SAVE(%r29) +#endif ld %r3,PARAMREG_SAVE+0(%r29) ld %r4,PARAMREG_SAVE+8(%r29) ld %r5,PARAMREG_SAVE+16(%r29)