* [PATCH] Fix PR64242
@ 2018-11-29 19:26 Wilco Dijkstra
2018-11-30 23:07 ` Jeff Law
0 siblings, 1 reply; 8+ messages in thread
From: Wilco Dijkstra @ 2018-11-29 19:26 UTC (permalink / raw)
To: GCC Patches; +Cc: nd
Fix PR64242 - the middle end expansion for long jmp updates the
hard frame pointer before it reads the new stack pointer. This
results in a corrupted stack pointer if the jmp buffer is a local.
The obvious fix is to insert a temporary.
AArch64 bootstrap & regress pass. OK to commit?
ChangeLog:
2018-11-29 Wilco Dijkstra <wdijkstr@arm.com>
gcc/
PR middle-end/64242
* builtins.c (expand_builtin_longjmp): Use a temporary when restoring
the frame pointer.
(expand_builtin_nonlocal_goto): Likewise.
testsuite/
PR middle-end/64242
* gcc.c-torture/execute/pr64242.c: New test.
---
diff --git a/gcc/builtins.c b/gcc/builtins.c
index ebde2db6e6494cf7e4441f6834e65fcb81e1629c..5c80c60378fc4fbb3faf8e04672d7091ac071624 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -1142,8 +1142,11 @@ expand_builtin_longjmp (rtx buf_addr, rtx value)
emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
emit_clobber (gen_rtx_MEM (BLKmode, hard_frame_pointer_rtx));
- emit_move_insn (hard_frame_pointer_rtx, fp);
+ /* Restore the frame pointer and stack pointer. We must use a
+ temporary since the setjmp buffer may be a local. */
+ fp = copy_to_reg (fp);
emit_stack_restore (SAVE_NONLOCAL, stack);
+ emit_move_insn (hard_frame_pointer_rtx, fp);
emit_use (hard_frame_pointer_rtx);
emit_use (stack_pointer_rtx);
@@ -1286,9 +1289,11 @@ expand_builtin_nonlocal_goto (tree exp)
emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
emit_clobber (gen_rtx_MEM (BLKmode, hard_frame_pointer_rtx));
- /* Restore frame pointer for containing function. */
- emit_move_insn (hard_frame_pointer_rtx, r_fp);
+ /* Restore the frame pointer and stack pointer. We must use a
+ temporary since the setjmp buffer may be a local. */
+ r_fp = copy_to_reg (r_fp);
emit_stack_restore (SAVE_NONLOCAL, r_sp);
+ emit_move_insn (hard_frame_pointer_rtx, r_fp);
/* USE of hard_frame_pointer_rtx added for consistency;
not clear if really needed. */
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr64242.c b/gcc/testsuite/gcc.c-torture/execute/pr64242.c
new file mode 100644
index 0000000000000000000000000000000000000000..72dab5709203437b50514a70c523d104706e4bda
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr64242.c
@@ -0,0 +1,30 @@
+/* { dg-require-effective-target indirect_jumps } */
+
+extern void abort (void);
+
+__attribute ((noinline)) void
+broken_longjmp(void *p)
+{
+ void *buf[5];
+ __builtin_memcpy (buf, p, 5 * sizeof (void*));
+ /* Corrupts stack pointer... */
+ __builtin_longjmp (buf, 1);
+}
+
+volatile int x = 0;
+volatile void *p;
+int
+main (void)
+{
+ void *buf[5];
+ p = __builtin_alloca (x);
+
+ if (!__builtin_setjmp (buf))
+ broken_longjmp (buf);
+
+ /* Fails if stack pointer corrupted. */
+ if (p != __builtin_alloca (x))
+ abort();
+
+ return 0;
+}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR64242
2018-11-29 19:26 [PATCH] Fix PR64242 Wilco Dijkstra
@ 2018-11-30 23:07 ` Jeff Law
2018-12-03 13:34 ` Richard Biener
2018-12-03 16:26 ` Jakub Jelinek
0 siblings, 2 replies; 8+ messages in thread
From: Jeff Law @ 2018-11-30 23:07 UTC (permalink / raw)
To: Wilco Dijkstra, GCC Patches; +Cc: nd
On 11/29/18 12:26 PM, Wilco Dijkstra wrote:
> Fix PR64242 - the middle end expansion for long jmp updates the
> hard frame pointer before it reads the new stack pointer. This
> results in a corrupted stack pointer if the jmp buffer is a local.
> The obvious fix is to insert a temporary.
>
> AArch64 bootstrap & regress pass. OK to commit?
>
> ChangeLog:
> 2018-11-29 Wilco Dijkstra <wdijkstr@arm.com>
>
> gcc/
> PR middle-end/64242
> * builtins.c (expand_builtin_longjmp): Use a temporary when restoring
> the frame pointer.
> (expand_builtin_nonlocal_goto): Likewise.
>
> testsuite/
> PR middle-end/64242
> * gcc.c-torture/execute/pr64242.c: New test.
THanks for tracking this down. I'd like to have this run through my
next testing cycle, so I went ahead and installed it for you.
Thanks again,
Jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR64242
2018-11-30 23:07 ` Jeff Law
@ 2018-12-03 13:34 ` Richard Biener
2018-12-03 13:40 ` Christophe Lyon
2018-12-03 16:26 ` Jakub Jelinek
1 sibling, 1 reply; 8+ messages in thread
From: Richard Biener @ 2018-12-03 13:34 UTC (permalink / raw)
To: Jeff Law; +Cc: Wilco Dijkstra, GCC Patches, nd
On Sat, Dec 1, 2018 at 12:07 AM Jeff Law <law@redhat.com> wrote:
>
> On 11/29/18 12:26 PM, Wilco Dijkstra wrote:
> > Fix PR64242 - the middle end expansion for long jmp updates the
> > hard frame pointer before it reads the new stack pointer. This
> > results in a corrupted stack pointer if the jmp buffer is a local.
> > The obvious fix is to insert a temporary.
> >
> > AArch64 bootstrap & regress pass. OK to commit?
> >
> > ChangeLog:
> > 2018-11-29 Wilco Dijkstra <wdijkstr@arm.com>
> >
> > gcc/
> > PR middle-end/64242
> > * builtins.c (expand_builtin_longjmp): Use a temporary when restoring
> > the frame pointer.
> > (expand_builtin_nonlocal_goto): Likewise.
> >
> > testsuite/
> > PR middle-end/64242
> > * gcc.c-torture/execute/pr64242.c: New test.
> THanks for tracking this down. I'd like to have this run through my
> next testing cycle, so I went ahead and installed it for you.
The testcase runfails on x86_64 with -m32 and -m64.
Richard.
> Thanks again,
> Jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR64242
2018-12-03 13:34 ` Richard Biener
@ 2018-12-03 13:40 ` Christophe Lyon
0 siblings, 0 replies; 8+ messages in thread
From: Christophe Lyon @ 2018-12-03 13:40 UTC (permalink / raw)
To: Richard Biener; +Cc: Jeff Law, Wilco Dijkstra, gcc Patches, nd
On Mon, 3 Dec 2018 at 14:35, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Sat, Dec 1, 2018 at 12:07 AM Jeff Law <law@redhat.com> wrote:
> >
> > On 11/29/18 12:26 PM, Wilco Dijkstra wrote:
> > > Fix PR64242 - the middle end expansion for long jmp updates the
> > > hard frame pointer before it reads the new stack pointer. This
> > > results in a corrupted stack pointer if the jmp buffer is a local.
> > > The obvious fix is to insert a temporary.
> > >
> > > AArch64 bootstrap & regress pass. OK to commit?
> > >
> > > ChangeLog:
> > > 2018-11-29 Wilco Dijkstra <wdijkstr@arm.com>
> > >
> > > gcc/
> > > PR middle-end/64242
> > > * builtins.c (expand_builtin_longjmp): Use a temporary when restoring
> > > the frame pointer.
> > > (expand_builtin_nonlocal_goto): Likewise.
> > >
> > > testsuite/
> > > PR middle-end/64242
> > > * gcc.c-torture/execute/pr64242.c: New test.
> > THanks for tracking this down. I'd like to have this run through my
> > next testing cycle, so I went ahead and installed it for you.
>
> The testcase runfails on x86_64 with -m32 and -m64.
>
And on some arm targets, as I mentioned in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64242#c7
> Richard.
>
> > Thanks again,
> > Jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR64242
2018-11-30 23:07 ` Jeff Law
2018-12-03 13:34 ` Richard Biener
@ 2018-12-03 16:26 ` Jakub Jelinek
2018-12-03 18:53 ` Jeff Law
2018-12-04 10:20 ` Christophe Lyon
1 sibling, 2 replies; 8+ messages in thread
From: Jakub Jelinek @ 2018-12-03 16:26 UTC (permalink / raw)
To: Jeff Law; +Cc: Wilco Dijkstra, GCC Patches, nd
Hi!
Here is a fix for the testcase, so that it doesn't FAIL pretty much
everywhere.
On Fri, Nov 30, 2018 at 04:07:31PM -0700, Jeff Law wrote:
> > PR middle-end/64242
> > * gcc.c-torture/execute/pr64242.c: New test.
> THanks for tracking this down. I'd like to have this run through my
> next testing cycle, so I went ahead and installed it for you.
What I've tested:
1) x86_64-linux {-m32,-m64} - without the testcase patch, the testcase FAILs
without or with the builtins.c change; with the testcase patch and
witout the builtins.c change, there is
FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test
FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test
FAIL: gcc.c-torture/execute/pr64242.c -Os execution test
for -m32 and no FAILs for -m64, with the builtins.c change the tests
passes on both -m32 and -m64
2) powerpc64-linux {-m32,-m64} - without the testcase patch, the testcase
FAILs without and with the builtins.c change for -m32. With the testcase
patch and without the builtins.c change, there is
FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
FAIL: gcc.c-torture/execute/pr64242.c -O1 execution test
FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test
FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test
FAIL: gcc.c-torture/execute/pr64242.c -Os execution test
for -m32 and
FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
FAIL: gcc.c-torture/execute/pr64242.c -O1 execution test
for -m64, with the builtins.c change everything passes
3) aarch64-linux - both without and with the testcase patch, the
testcase FAILs without the builtins.c change and passes with it
Ok for trunk?
2018-12-03 Jakub Jelinek <jakub@redhat.com>
PR middle-end/64242
* gcc.c-torture/execute/pr64242.c (foo, bar): New functions.
(p): Make it void *volatile instead of volatile void *.
(q): New variable.
(main): Add a dummy 32-byte aligned variable and escape its address.
Don't require that the two __builtin_alloca (0) calls return the
same address, just require that their difference is smaller than
1024 bytes.
--- gcc/testsuite/gcc.c-torture/execute/pr64242.c.jj 2018-12-01 00:25:08.082009500 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr64242.c 2018-12-03 16:51:51.869797742 +0100
@@ -3,7 +3,7 @@
extern void abort (void);
__attribute ((noinline)) void
-broken_longjmp(void *p)
+broken_longjmp (void *p)
{
void *buf[5];
__builtin_memcpy (buf, p, 5 * sizeof (void*));
@@ -11,20 +11,41 @@ broken_longjmp(void *p)
__builtin_longjmp (buf, 1);
}
+__attribute ((noipa)) __UINTPTR_TYPE__
+foo (void *p)
+{
+ return (__UINTPTR_TYPE__) p;
+}
+
+__attribute ((noipa)) void
+bar (void *p)
+{
+ asm volatile ("" : : "r" (p));
+}
+
volatile int x = 0;
-volatile void *p;
+void *volatile p;
+void *volatile q;
+
int
-main (void)
+main ()
{
void *buf[5];
+ struct __attribute__((aligned (32))) S { int a[4]; } s;
+ bar (&s);
p = __builtin_alloca (x);
-
if (!__builtin_setjmp (buf))
broken_longjmp (buf);
/* Fails if stack pointer corrupted. */
- if (p != __builtin_alloca (x))
- abort();
+ q = __builtin_alloca (x);
+ if (foo (p) < foo (q))
+ {
+ if (foo (q) - foo (p) >= 1024)
+ abort ();
+ }
+ else if (foo (p) - foo (q) >= 1024)
+ abort ();
return 0;
}
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR64242
2018-12-03 16:26 ` Jakub Jelinek
@ 2018-12-03 18:53 ` Jeff Law
2018-12-04 10:20 ` Christophe Lyon
1 sibling, 0 replies; 8+ messages in thread
From: Jeff Law @ 2018-12-03 18:53 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Wilco Dijkstra, GCC Patches, nd
On 12/3/18 9:25 AM, Jakub Jelinek wrote:
> Hi!
>
> Here is a fix for the testcase, so that it doesn't FAIL pretty much
> everywhere.
>
> On Fri, Nov 30, 2018 at 04:07:31PM -0700, Jeff Law wrote:
>>> PR middle-end/64242
>>> * gcc.c-torture/execute/pr64242.c: New test.
>> THanks for tracking this down. I'd like to have this run through my
>> next testing cycle, so I went ahead and installed it for you.
>
> What I've tested:
> 1) x86_64-linux {-m32,-m64} - without the testcase patch, the testcase FAILs
> without or with the builtins.c change; with the testcase patch and
> witout the builtins.c change, there is
> FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test
> FAIL: gcc.c-torture/execute/pr64242.c -Os execution test
> for -m32 and no FAILs for -m64, with the builtins.c change the tests
> passes on both -m32 and -m64
> 2) powerpc64-linux {-m32,-m64} - without the testcase patch, the testcase
> FAILs without and with the builtins.c change for -m32. With the testcase
> patch and without the builtins.c change, there is
> FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O1 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test
> FAIL: gcc.c-torture/execute/pr64242.c -Os execution test
> for -m32 and
> FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O1 execution test
> for -m64, with the builtins.c change everything passes
> 3) aarch64-linux - both without and with the testcase patch, the
> testcase FAILs without the builtins.c change and passes with it
>
> Ok for trunk?
>
> 2018-12-03 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/64242
> * gcc.c-torture/execute/pr64242.c (foo, bar): New functions.
> (p): Make it void *volatile instead of volatile void *.
> (q): New variable.
> (main): Add a dummy 32-byte aligned variable and escape its address.
> Don't require that the two __builtin_alloca (0) calls return the
> same address, just require that their difference is smaller than
> 1024 bytes.
Yea, my tester fell over the new test on multiple targets. THanks for
fixing it up.
OK
jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR64242
2018-12-03 16:26 ` Jakub Jelinek
2018-12-03 18:53 ` Jeff Law
@ 2018-12-04 10:20 ` Christophe Lyon
2018-12-04 10:23 ` Jakub Jelinek
1 sibling, 1 reply; 8+ messages in thread
From: Christophe Lyon @ 2018-12-04 10:20 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jeff Law, Wilco Dijkstra, gcc Patches, nd
On Mon, 3 Dec 2018 at 17:26, Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> Here is a fix for the testcase, so that it doesn't FAIL pretty much
> everywhere.
>
> On Fri, Nov 30, 2018 at 04:07:31PM -0700, Jeff Law wrote:
> > > PR middle-end/64242
> > > * gcc.c-torture/execute/pr64242.c: New test.
> > THanks for tracking this down. I'd like to have this run through my
> > next testing cycle, so I went ahead and installed it for you.
>
> What I've tested:
> 1) x86_64-linux {-m32,-m64} - without the testcase patch, the testcase FAILs
> without or with the builtins.c change; with the testcase patch and
> witout the builtins.c change, there is
> FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test
> FAIL: gcc.c-torture/execute/pr64242.c -Os execution test
> for -m32 and no FAILs for -m64, with the builtins.c change the tests
> passes on both -m32 and -m64
> 2) powerpc64-linux {-m32,-m64} - without the testcase patch, the testcase
> FAILs without and with the builtins.c change for -m32. With the testcase
> patch and without the builtins.c change, there is
> FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O1 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test
> FAIL: gcc.c-torture/execute/pr64242.c -Os execution test
> for -m32 and
> FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
> FAIL: gcc.c-torture/execute/pr64242.c -O1 execution test
> for -m64, with the builtins.c change everything passes
> 3) aarch64-linux - both without and with the testcase patch, the
> testcase FAILs without the builtins.c change and passes with it
>
> Ok for trunk?
>
> 2018-12-03 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/64242
> * gcc.c-torture/execute/pr64242.c (foo, bar): New functions.
> (p): Make it void *volatile instead of volatile void *.
> (q): New variable.
> (main): Add a dummy 32-byte aligned variable and escape its address.
> Don't require that the two __builtin_alloca (0) calls return the
> same address, just require that their difference is smaller than
> 1024 bytes.
>
Hi Jakub,
This commit didn't fix the failure I reported on some arm targets, and
it introduced
a regression on aarch64-none-elf with -mabi=ilp32:
FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
Il seems the test timed-out.
Higher optimization levels still pass.
Christophe
> --- gcc/testsuite/gcc.c-torture/execute/pr64242.c.jj 2018-12-01 00:25:08.082009500 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr64242.c 2018-12-03 16:51:51.869797742 +0100
> @@ -3,7 +3,7 @@
> extern void abort (void);
>
> __attribute ((noinline)) void
> -broken_longjmp(void *p)
> +broken_longjmp (void *p)
> {
> void *buf[5];
> __builtin_memcpy (buf, p, 5 * sizeof (void*));
> @@ -11,20 +11,41 @@ broken_longjmp(void *p)
> __builtin_longjmp (buf, 1);
> }
>
> +__attribute ((noipa)) __UINTPTR_TYPE__
> +foo (void *p)
> +{
> + return (__UINTPTR_TYPE__) p;
> +}
> +
> +__attribute ((noipa)) void
> +bar (void *p)
> +{
> + asm volatile ("" : : "r" (p));
> +}
> +
> volatile int x = 0;
> -volatile void *p;
> +void *volatile p;
> +void *volatile q;
> +
> int
> -main (void)
> +main ()
> {
> void *buf[5];
> + struct __attribute__((aligned (32))) S { int a[4]; } s;
> + bar (&s);
> p = __builtin_alloca (x);
> -
> if (!__builtin_setjmp (buf))
> broken_longjmp (buf);
>
> /* Fails if stack pointer corrupted. */
> - if (p != __builtin_alloca (x))
> - abort();
> + q = __builtin_alloca (x);
> + if (foo (p) < foo (q))
> + {
> + if (foo (q) - foo (p) >= 1024)
> + abort ();
> + }
> + else if (foo (p) - foo (q) >= 1024)
> + abort ();
>
> return 0;
> }
>
> Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR64242
2018-12-04 10:20 ` Christophe Lyon
@ 2018-12-04 10:23 ` Jakub Jelinek
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Jelinek @ 2018-12-04 10:23 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Jeff Law, Wilco Dijkstra, gcc Patches, nd
On Tue, Dec 04, 2018 at 11:20:38AM +0100, Christophe Lyon wrote:
> This commit didn't fix the failure I reported on some arm targets, and
> it introduced
> a regression on aarch64-none-elf with -mabi=ilp32:
> FAIL: gcc.c-torture/execute/pr64242.c -O0 execution test
> Il seems the test timed-out.
> Higher optimization levels still pass.
That just means the problem wasn't really fully fixed on those targets,
in the PR there are some comments about missing scheduling barriers.
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-12-04 10:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-29 19:26 [PATCH] Fix PR64242 Wilco Dijkstra
2018-11-30 23:07 ` Jeff Law
2018-12-03 13:34 ` Richard Biener
2018-12-03 13:40 ` Christophe Lyon
2018-12-03 16:26 ` Jakub Jelinek
2018-12-03 18:53 ` Jeff Law
2018-12-04 10:20 ` Christophe Lyon
2018-12-04 10:23 ` Jakub Jelinek
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).