[PATCH] Fix PR64242

[PATCH] Fix PR64242

[Wilco Dijkstra]

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

Re: [PATCH] Fix PR64242

[Jeff Law]

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

Re: [PATCH] Fix PR64242

[Richard Biener]

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

Re: [PATCH] Fix PR64242

[Christophe Lyon]

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

Re: [PATCH] Fix PR64242

[Jakub Jelinek]

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

Re: [PATCH] Fix PR64242

[Jeff Law]

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

Re: [PATCH] Fix PR64242

[Christophe Lyon]

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

Re: [PATCH] Fix PR64242

[Jakub Jelinek]

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