Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952
On 4/23/21 6:58 PM, Segher Boessenkool wrote: On Fri, Apr 23, 2021 at 06:24:07PM -0400, Michael Meissner wrote: On Thu, Apr 22, 2021 at 05:56:32PM -0500, Segher Boessenkool wrote: As Will says, it looks like the ELFv2 version has the same bug. Please fix that the same way. Yes it has the same bug. However in practice it would never be hit, since this bug is 32-bit, and we only build 64-bit systems with ELF v2. I did fix it. Hrm, in that case, why do we have that code at all?! Okay for trunk. Okay for backport to 11 when that branch opens again. Does this need more backports? (Those should follow after 11 of course). Bill mentioned we may want to backport this to earlier branches before they are frozen. Tulio, are backports to earlier revisions important? Well, the bug has been there since the original commit to (then) tramp.asm, which was 25 years ago, and only now people noticed ;-) We should have a backport to GCC 11 at least. Older is up to you (and Tulio). This was reported to us as a compatibility problem with Clang that was holding up porting a language runtime to Power. Since this is very obviously a bug, I would like to be aggressive about backporting it to previous releases to avoid any other such problems. Thanks for considering! Bill Segher
Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952
On Fri, Apr 23, 2021 at 06:24:07PM -0400, Michael Meissner wrote: > On Thu, Apr 22, 2021 at 05:56:32PM -0500, Segher Boessenkool wrote: > > As Will says, it looks like the ELFv2 version has the same bug. Please > > fix that the same way. > > Yes it has the same bug. However in practice it would never be hit, since > this > bug is 32-bit, and we only build 64-bit systems with ELF v2. I did fix it. Hrm, in that case, why do we have that code at all?! > > Okay for trunk. Okay for backport to 11 when that branch opens again. > > Does this need more backports? (Those should follow after 11 of > > course). > > Bill mentioned we may want to backport this to earlier branches before they > are > frozen. Tulio, are backports to earlier revisions important? Well, the bug has been there since the original commit to (then) tramp.asm, which was 25 years ago, and only now people noticed ;-) We should have a backport to GCC 11 at least. Older is up to you (and Tulio). Segher
Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952
On Thu, Apr 22, 2021 at 05:56:32PM -0500, Segher Boessenkool wrote:
> On Fri, Apr 09, 2021 at 05:09:07PM -0400, Michael Meissner wrote:
> > Fix logic error in 32-bit trampolines, PR target/98952.
> >
> > The test in the PowerPC 32-bit trampoline support is backwards. It aborts
> > if the trampoline size is greater than the expected size. It should abort
> > when the trampoline size is less than the expected size.
>
> > PR target/98952
> > * config/rs6000/tramp.S (__trampoline_setup): Fix trampoline size
> > comparison in 32-bit.
>
> > --- a/libgcc/config/rs6000/tramp.S
> > +++ b/libgcc/config/rs6000/tramp.S
> > @@ -64,8 +64,7 @@ FUNC_START(__trampoline_setup)
> > mflr r11
> > addi r7,r11,trampoline_initial-4-.LCF0 /* trampoline address
> > -4 */
> >
> > - li r8,trampoline_size /* verify that the trampoline is big
> > enough */
> > - cmpwcr1,r8,r4
> > + cmpwi cr1,r4,trampoline_size /* verify that the trampoline is big
> > enough */
> > srwir4,r4,2 /* # words to move */
> > addir9,r3,-4/* adjust pointer for lwzu */
> > mtctr r4
>
> As Will says, it looks like the ELFv2 version has the same bug. Please
> fix that the same way.
Yes it has the same bug. However in practice it would never be hit, since this
bug is 32-bit, and we only build 64-bit systems with ELF v2. I did fix it.
> In the commit message and the changelog, point out that you folded the
> cmp with the li while you were at it. It is easier to read code like
> this so the change is fine, but do point it out.
>
> Can you test this in a testcase somehow? That would have found the
> ELFv2 case, for example.
I created a test case calling __trampoline_setup with a larger buffer. If it
doesn't abort the test passes.
> Okay for trunk. Okay for backport to 11 when that branch opens again.
> Does this need more backports? (Those should follow after 11 of
> course).
Bill mentioned we may want to backport this to earlier branches before they are
frozen. Tulio, are backports to earlier revisions important?
I will attach the patch that I just commited.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
>From 9a30a3f06b908e4e781324c2e813cd1db87119df Mon Sep 17 00:00:00 2001
From: Michael Meissner
Date: Fri, 23 Apr 2021 18:16:03 -0400
Subject: [PATCH] Fix logic error in 32-bit trampolines.
The test in the PowerPC 32-bit trampoline support is backwards. It aborts
if the trampoline size is greater than the expected size. It should abort
when the trampoline size is less than the expected size. I fixed the test
so the operands are reversed. I then folded the load immediate into the
compare instruction.
I verified this by creating a 32-bit trampoline program and manually
changing the size of the trampoline to be 48 instead of 40. The program
aborted with the larger size. I updated this code and ran the test again
and it passed.
I added a test case that runs on PowerPC 32-bit Linux systems and it calls
the __trampoline_setup function with a larger buffer size than the
compiler uses. The test is not run on 64-bit systems, since the function
__trampoline_setup is not called. I also limited the test to just Linux
systems, in case trampolines are handled differently in other systems.
libgcc/
2021-04-23 Michael Meissner
PR target/98952
* config/rs6000/tramp.S (__trampoline_setup, elfv1 #ifdef): Fix
trampoline size comparison in 32-bit by reversing test and
combining load immediate with compare.
(__trampoline_setup, elfv2 #ifdef): Fix trampoline size comparison
in 32-bit by reversing test and combining load immediate with
compare.
gcc/testsuite/
2021-04-23 Michael Meissner
PR target/98952
* gcc.target/powerpc/pr98952.c: New test.
---
gcc/testsuite/gcc.target/powerpc/pr98952.c | 28 ++
libgcc/config/rs6000/tramp.S | 6 ++---
2 files changed, 30 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98952.c
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98952.c
b/gcc/testsuite/gcc.target/powerpc/pr98952.c
new file mode 100644
index 000..c487fbc403e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98952.c
@@ -0,0 +1,28 @@
+/* { dg-do run { target { powerpc*-*-linux* && ilp32 } } } */
+/* { dg-options "-O2" } */
+
+/* PR 96983 reported that the test in libgcc's tramp.S was backwards and it
+ would abort if the trampoline size passed to the function was greater than
+ the size the runtime was expecting (40). It should abort if the size is
less
+ than 40, not greater than 40. This test creates a call to
__trampoline_setup
+ with a much larger buffer to make sure the function does not abort.
+
+ We do not run this test on 64-bit since __trampoline_setup is not present in
+
Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952
On Fri, Apr 09, 2021 at 05:09:07PM -0400, Michael Meissner wrote: > Fix logic error in 32-bit trampolines, PR target/98952. > > The test in the PowerPC 32-bit trampoline support is backwards. It aborts > if the trampoline size is greater than the expected size. It should abort > when the trampoline size is less than the expected size. > PR target/98952 > * config/rs6000/tramp.S (__trampoline_setup): Fix trampoline size > comparison in 32-bit. > --- a/libgcc/config/rs6000/tramp.S > +++ b/libgcc/config/rs6000/tramp.S > @@ -64,8 +64,7 @@ FUNC_START(__trampoline_setup) > mflr r11 > addi r7,r11,trampoline_initial-4-.LCF0 /* trampoline address -4 */ > > - li r8,trampoline_size /* verify that the trampoline is big > enough */ > - cmpwcr1,r8,r4 > + cmpwi cr1,r4,trampoline_size /* verify that the trampoline is big > enough */ > srwir4,r4,2 /* # words to move */ > addir9,r3,-4/* adjust pointer for lwzu */ > mtctr r4 As Will says, it looks like the ELFv2 version has the same bug. Please fix that the same way. In the commit message and the changelog, point out that you folded the cmp with the li while you were at it. It is easier to read code like this so the change is fine, but do point it out. Can you test this in a testcase somehow? That would have found the ELFv2 case, for example. Okay for trunk. Okay for backport to 11 when that branch opens again. Does this need more backports? (Those should follow after 11 of course). Thanks, Segher
Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952
On Mon, Apr 12, 2021 at 05:02:38PM -0500, will schmidt wrote: > On Fri, 2021-04-09 at 17:09 -0400, Michael Meissner wrote: > > - li r8,trampoline_size /* verify that the trampoline is big > > enough */ > > - cmpwcr1,r8,r4 > > + cmpwi cr1,r4,trampoline_size /* verify that the trampoline is big > > enough */ > > Hmm, I spent several minutes trying to determine how cmpw behaves > differently than cmpwi before noticing you also swapped the > order of the r4,r8 operands. > > That seems OK. > > A statement in the description indicating that you used a cmpwi instead > of a cmpw since you were in the neighborhood would help call that out. In general, don't do two unrelated things, esp. when not pointing it out explicitly. > The #elif _CALL_ELF == 2 portion of tramp.S (line 159 or so) has a > similar compare stanza with respect to the order of operands on the > compare. Will this also have a backwards greater-than less-than issue? > > li r8,trampoline_size /* verify that the trampoline is big > enough */ > cmpwcr1,r8,r4 > srwir4,r4,3 /* # doublewords to move */ > addir9,r3,-8/* adjust pointer for stdu */ > mtctr r4 > blt cr1,.Labort Not sure... It should use cmpwi as well though, and then it is easier to see. Segher
Ping: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952
Ping the patch for a logic error in setting up 32-bit trampolines. | Subject: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 | Message-ID: <20210409210907.ga5...@ibm-toto.the-meissners.org> | User-Agent: Mutt/1.5.21 (2010-09-15) https://gcc.gnu.org/pipermail/gcc-patches/2021-April/567849.html -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952
On Fri, 2021-04-09 at 17:09 -0400, Michael Meissner wrote: > Fix logic error in 32-bit trampolines, PR target/98952. > > The test in the PowerPC 32-bit trampoline support is backwards. It aborts > if the trampoline size is greater than the expected size. It should abort > when the trampoline size is less than the expected size. > > I verified this by creating a 32-bit trampoline program and manually > changing the size of the trampoline to be 48 instead of 40. The program > aborted with the larger size. I updated this code and ran the test again > and it passed. > > I did a bootstrap build on a big endian power8 system that supports both > 32-bit and 64-bit executables, and there were no regressions. Can I check > this patch into the trunk? > > libgcc/ > 2021-04-09 Michael Meissner > > PR target/98952 > * config/rs6000/tramp.S (__trampoline_setup): Fix trampoline size > comparison in 32-bit. > --- > libgcc/config/rs6000/tramp.S | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/libgcc/config/rs6000/tramp.S b/libgcc/config/rs6000/tramp.S > index 4236a82b402..6b61d892da6 100644 > --- a/libgcc/config/rs6000/tramp.S > +++ b/libgcc/config/rs6000/tramp.S > @@ -64,8 +64,7 @@ FUNC_START(__trampoline_setup) > mflr r11 > addi r7,r11,trampoline_initial-4-.LCF0 /* trampoline address -4 */ > > - li r8,trampoline_size /* verify that the trampoline is big > enough */ > - cmpwcr1,r8,r4 > + cmpwi cr1,r4,trampoline_size /* verify that the trampoline is big > enough */ Hmm, I spent several minutes trying to determine how cmpw behaves differently than cmpwi before noticing you also swapped the order of the r4,r8 operands. That seems OK. A statement in the description indicating that you used a cmpwi instead of a cmpw since you were in the neighborhood would help call that out. The #elif _CALL_ELF == 2 portion of tramp.S (line 159 or so) has a similar compare stanza with respect to the order of operands on the compare. Will this also have a backwards greater-than less-than issue? li r8,trampoline_size /* verify that the trampoline is big enough */ cmpwcr1,r8,r4 srwir4,r4,3 /* # doublewords to move */ addir9,r3,-8/* adjust pointer for stdu */ mtctr r4 blt cr1,.Labort thanks -Will > srwir4,r4,2 /* # words to move */ > addir9,r3,-4/* adjust pointer for lwzu */ > mtctr r4 > -- > 2.22.0 > >
[PATCH] Fix logic error in 32-bit trampolines, PR target/98952
Fix logic error in 32-bit trampolines, PR target/98952. The test in the PowerPC 32-bit trampoline support is backwards. It aborts if the trampoline size is greater than the expected size. It should abort when the trampoline size is less than the expected size. I verified this by creating a 32-bit trampoline program and manually changing the size of the trampoline to be 48 instead of 40. The program aborted with the larger size. I updated this code and ran the test again and it passed. I did a bootstrap build on a big endian power8 system that supports both 32-bit and 64-bit executables, and there were no regressions. Can I check this patch into the trunk? libgcc/ 2021-04-09 Michael Meissner PR target/98952 * config/rs6000/tramp.S (__trampoline_setup): Fix trampoline size comparison in 32-bit. --- libgcc/config/rs6000/tramp.S | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libgcc/config/rs6000/tramp.S b/libgcc/config/rs6000/tramp.S index 4236a82b402..6b61d892da6 100644 --- a/libgcc/config/rs6000/tramp.S +++ b/libgcc/config/rs6000/tramp.S @@ -64,8 +64,7 @@ FUNC_START(__trampoline_setup) mflr r11 addi r7,r11,trampoline_initial-4-.LCF0 /* trampoline address -4 */ - li r8,trampoline_size /* verify that the trampoline is big enough */ - cmpwcr1,r8,r4 + cmpwi cr1,r4,trampoline_size /* verify that the trampoline is big enough */ srwir4,r4,2 /* # words to move */ addir9,r3,-4/* adjust pointer for lwzu */ mtctr r4 -- 2.22.0 -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
