diff mbox series

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

Message ID 20210409210907.GA5325@ibm-toto.the-meissners.org
State New
Headers show
Series Fix logic error in 32-bit trampolines, PR target/98952 | expand

Commit Message

Michael Meissner April 9, 2021, 9:09 p.m. UTC 
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(-)

Comments

will schmidt April 12, 2021, 10:02 p.m. UTC | #1 
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
> 
>
Michael Meissner April 19, 2021, 7:54 p.m. UTC | #2 
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
Segher Boessenkool April 22, 2021, 10:50 p.m. UTC | #3 
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
Segher Boessenkool April 22, 2021, 10:56 p.m. UTC | #4 
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
Michael Meissner April 23, 2021, 10:24 p.m. UTC | #5 
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.
Segher Boessenkool April 23, 2021, 11:58 p.m. UTC | #6 
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
Li, Pan2 via Gcc-patches April 25, 2021, 1:45 p.m. UTC | #7 
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
diff mbox series

Patch

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