diff mbox series

Fix PR64242

Message ID DB5PR08MB1030265410188ED40A00733183D20@DB5PR08MB1030.eurprd08.prod.outlook.com
State New
Headers show
Series Fix PR64242 | expand

Commit Message

Wilco Dijkstra Nov. 29, 2018, 7:26 p.m. UTC 
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.

---

Comments

Jeff Law Nov. 30, 2018, 11:07 p.m. UTC | #1 
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
Richard Biener Dec. 3, 2018, 1:34 p.m. UTC | #2 
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
Christophe Lyon Dec. 3, 2018, 1:40 p.m. UTC | #3 
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
Jakub Jelinek Dec. 3, 2018, 4:25 p.m. UTC | #4 
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
Jeff Law Dec. 3, 2018, 6:53 p.m. UTC | #5 
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
Christophe Lyon Dec. 4, 2018, 10:20 a.m. UTC | #6 
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
Jakub Jelinek Dec. 4, 2018, 10:23 a.m. UTC | #7 
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
diff mbox series

Patch

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;
+}