[PATCH] Fix logic error in 32-bit trampolines, PR target/98952

[PATCH] Fix logic error in 32-bit trampolines, PR target/98952

[Michael Meissner]

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  <meissner@linux.ibm.com>

	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 */
-	cmpw	cr1,r8,r4
+	cmpwi	cr1,r4,trampoline_size	/* verify that the trampoline is big enough */
 	srwi	r4,r4,2		/* # words to move */
 	addi	r9,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: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952

[will schmidt]

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  <meissner@linux.ibm.com>
> 
> 	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 */
> -	cmpw	cr1,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 */
	cmpw	cr1,r8,r4
	srwi	r4,r4,3		/* # doublewords to move */
	addi	r9,r3,-8	/* adjust pointer for stdu */
	mtctr	r4
	blt	cr1,.Labort




thanks
-Will


>  	srwi	r4,r4,2		/* # words to move */
>  	addi	r9,r3,-4	/* adjust pointer for lwzu */
>  	mtctr	r4
> -- 
> 2.22.0
> 
> 

Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952

[Segher Boessenkool]

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 */
> > -	cmpw	cr1,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 */
> 	cmpw	cr1,r8,r4
> 	srwi	r4,r4,3		/* # doublewords to move */
> 	addi	r9,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

[Michael Meissner]

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.GA5325@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: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952

[Segher Boessenkool]

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 */
> -	cmpw	cr1,r8,r4
> +	cmpwi	cr1,r4,trampoline_size	/* verify that the trampoline is big enough */
>  	srwi	r4,r4,2		/* # words to move */
>  	addi	r9,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

[Michael Meissner]

[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]

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 */
> > -	cmpw	cr1,r8,r4
> > +	cmpwi	cr1,r4,trampoline_size	/* verify that the trampoline is big enough */
> >  	srwi	r4,r4,2		/* # words to move */
> >  	addi	r9,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: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[-- Attachment #2: pr98952.patch001b --]
[-- Type: text/plain, Size: 4043 bytes --]

From 9a30a3f06b908e4e781324c2e813cd1db87119df Mon Sep 17 00:00:00 2001
From: Michael Meissner <meissner@linux.ibm.com>
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  <meissner@linux.ibm.com>

	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  <meissner@linux.ibm.com>

	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 00000000000..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
+   64-bit systems.
+
+   We only run the test under Linux in case the other systems have some
+   different variant for __trampoline_setup.  */
+
+#ifndef SIZE
+#define SIZE 100
+#endif
+
+extern void __trampoline_setup (int *, unsigned, void *, void *);
+
+int main (void)
+{
+  int tramp[SIZE / sizeof (int)];
+
+  __trampoline_setup (tramp, SIZE, 0, 0);
+  return 0;
+}
diff --git a/libgcc/config/rs6000/tramp.S b/libgcc/config/rs6000/tramp.S
index 4236a82b402..68baf16de9f 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 */
-	cmpw	cr1,r8,r4
+	cmpwi	cr1,r4,trampoline_size	/* verify that the trampoline is big enough */
 	srwi	r4,r4,2		/* # words to move */
 	addi	r9,r3,-4	/* adjust pointer for lwzu */
 	mtctr	r4
@@ -156,8 +155,7 @@ FUNC_START(__trampoline_setup)
 	ld 7,.LC0@toc@l(7)	/* trampoline address -8 */
 #endif
 
-	li	r8,trampoline_size	/* verify that the trampoline is big enough */
-	cmpw	cr1,r8,r4
+	cmpwi	cr1,r4,trampoline_size	/* verify that the trampoline is big enough */
 	srwi	r4,r4,3		/* # doublewords to move */
 	addi	r9,r3,-8	/* adjust pointer for stdu */
 	mtctr	r4
-- 
2.22.0

Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952

[Segher Boessenkool]

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

[Bill Schmidt]

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