* [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 @ 2021-04-09 21:09 Michael Meissner 2021-04-12 22:02 ` will schmidt ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Michael Meissner @ 2021-04-09 21:09 UTC (permalink / raw) To: gcc-patches, Michael Meissner, Segher Boessenkool, David Edelsohn, Bill Schmidt, Peter Bergner, Will Schmidt 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 2021-04-09 21:09 [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 Michael Meissner @ 2021-04-12 22:02 ` will schmidt 2021-04-22 22:50 ` Segher Boessenkool 2021-04-19 19:54 ` Ping: " Michael Meissner 2021-04-22 22:56 ` Segher Boessenkool 2 siblings, 1 reply; 8+ messages in thread From: will schmidt @ 2021-04-12 22:02 UTC (permalink / raw) To: Michael Meissner, gcc-patches, Segher Boessenkool, David Edelsohn, Bill Schmidt, Peter Bergner 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 2021-04-12 22:02 ` will schmidt @ 2021-04-22 22:50 ` Segher Boessenkool 0 siblings, 0 replies; 8+ messages in thread From: Segher Boessenkool @ 2021-04-22 22:50 UTC (permalink / raw) To: will schmidt Cc: Michael Meissner, gcc-patches, David Edelsohn, Bill Schmidt, Peter Bergner 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Ping: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 2021-04-09 21:09 [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 Michael Meissner 2021-04-12 22:02 ` will schmidt @ 2021-04-19 19:54 ` Michael Meissner 2021-04-22 22:56 ` Segher Boessenkool 2 siblings, 0 replies; 8+ messages in thread From: Michael Meissner @ 2021-04-19 19:54 UTC (permalink / raw) To: Michael Meissner, gcc-patches, Segher Boessenkool, David Edelsohn, Bill Schmidt, Peter Bergner, Will Schmidt 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 2021-04-09 21:09 [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 Michael Meissner 2021-04-12 22:02 ` will schmidt 2021-04-19 19:54 ` Ping: " Michael Meissner @ 2021-04-22 22:56 ` Segher Boessenkool 2021-04-23 22:24 ` Michael Meissner 2 siblings, 1 reply; 8+ messages in thread From: Segher Boessenkool @ 2021-04-22 22:56 UTC (permalink / raw) To: Michael Meissner, gcc-patches, David Edelsohn, Bill Schmidt, Peter Bergner, Will Schmidt 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 2021-04-22 22:56 ` Segher Boessenkool @ 2021-04-23 22:24 ` Michael Meissner 2021-04-23 23:58 ` Segher Boessenkool 0 siblings, 1 reply; 8+ messages in thread From: Michael Meissner @ 2021-04-23 22:24 UTC (permalink / raw) To: Segher Boessenkool Cc: Michael Meissner, gcc-patches, David Edelsohn, Bill Schmidt, Peter Bergner, Will Schmidt [-- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 2021-04-23 22:24 ` Michael Meissner @ 2021-04-23 23:58 ` Segher Boessenkool 2021-04-25 13:45 ` Bill Schmidt 0 siblings, 1 reply; 8+ messages in thread From: Segher Boessenkool @ 2021-04-23 23:58 UTC (permalink / raw) To: Michael Meissner, gcc-patches, David Edelsohn, Bill Schmidt, Peter Bergner, Will Schmidt 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 2021-04-23 23:58 ` Segher Boessenkool @ 2021-04-25 13:45 ` Bill Schmidt 0 siblings, 0 replies; 8+ messages in thread From: Bill Schmidt @ 2021-04-25 13:45 UTC (permalink / raw) To: Segher Boessenkool, Michael Meissner, gcc-patches, David Edelsohn, Peter Bergner, Will 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-25 13:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-09 21:09 [PATCH] Fix logic error in 32-bit trampolines, PR target/98952 Michael Meissner 2021-04-12 22:02 ` will schmidt 2021-04-22 22:50 ` Segher Boessenkool 2021-04-19 19:54 ` Ping: " Michael Meissner 2021-04-22 22:56 ` Segher Boessenkool 2021-04-23 22:24 ` Michael Meissner 2021-04-23 23:58 ` Segher Boessenkool 2021-04-25 13:45 ` Bill Schmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).