[PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Sudakshina Das]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

Hi

This patch is another partial fix for PR 84521. This is adding a 
definition to one of the target hooks used in the SJLJ implemetation so 
that AArch64 defines the hard_frame_pointer_rtx as the 
TARGET_BUILTIN_SETJMP_FRAME_VALUE. As pointed out by Wilco there is 
still a lot more work to be done for these builtins in the future.

Testing: Bootstrapped and regtested on aarch64-none-linux-gnu and added 
new test.

Is this ok for trunk?

Sudi


*** gcc/ChangeLog ***

2018-03-14  Sudakshina Das  <sudi.das@arm.com>

	* builtins.c (expand_builtin_setjmp_receiver): Update condition
	to restore frame pointer.
	* config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update
	comment.
	* config/aarch64/aarch64.c (aarch64_builtin_setjmp_frame_value):
	New.
	(TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.

*** gcc/testsuite/ChangeLog ***

2018-03-14  Sudakshina Das  <sudi.das@arm.com>

	* gcc.c-torture/execute/pr84521.c: New test.

[-- Attachment #2: pr84521.diff --]
[-- Type: text/x-patch, Size: 3096 bytes --]

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 85affa7..640f1a9 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -898,7 +898,8 @@ expand_builtin_setjmp_receiver (rtx receiver_label)
 
   /* Now put in the code to restore the frame pointer, and argument
      pointer, if needed.  */
-  if (! targetm.have_nonlocal_goto ())
+  if (! targetm.have_nonlocal_goto ()
+      && targetm.builtin_setjmp_frame_value () != hard_frame_pointer_rtx)
     {
       /* First adjust our frame pointer to its actual value.  It was
 	 previously set to the start of the virtual area corresponding to
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index e3c52f6..7a21c14 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -474,7 +474,9 @@ extern unsigned aarch64_architecture_version;
 #define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
 #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
 
-/* Don't use __builtin_setjmp until we've defined it.  */
+/* Don't use __builtin_setjmp until we've defined it.
+   CAUTION: This macro is only used during exception unwinding.
+   Don't fall for its name.  */
 #undef DONT_USE_BUILTIN_SETJMP
 #define DONT_USE_BUILTIN_SETJMP 1
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index e1fb87f..e7ac0fe 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -12128,6 +12128,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
   expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
 }
 
+/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
+static rtx
+aarch64_builtin_setjmp_frame_value (void)
+{
+  return hard_frame_pointer_rtx;
+}
+
 /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
 
 static tree
@@ -17505,6 +17512,9 @@ aarch64_run_selftests (void)
 #undef TARGET_FOLD_BUILTIN
 #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
 
+#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
+#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
+
 #undef TARGET_FUNCTION_ARG
 #define TARGET_FUNCTION_ARG aarch64_function_arg
 
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
new file mode 100644
index 0000000..76b10d2
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
@@ -0,0 +1,49 @@
+/* { dg-require-effective-target indirect_jumps } */
+
+#include <setjmp.h>
+
+jmp_buf buf;
+
+int uses_longjmp (void)
+{
+  __builtin_longjmp (buf, 1);
+}
+
+int gl;
+void after_longjmp (void)
+{
+  gl = 5;
+}
+
+int
+test_1 (int n)
+{
+  volatile int *p = alloca (n);
+  if (__builtin_setjmp (buf))
+    {
+      after_longjmp ();
+    }
+  else
+    {
+      uses_longjmp ();
+    }
+
+  return 0;
+}
+
+int __attribute__ ((optimize ("no-omit-frame-pointer")))
+test_2 (int n)
+{
+  int i;
+  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
+  for (i = 0; i < n; i++)
+    ptr[i] = i;
+  test_1 (n);
+  return 0;
+}
+
+int main (int argc, const char **argv)
+{
+  __builtin_memset (&buf, 0xaf, sizeof (buf));
+  test_2 (100);
+}

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[James Greenhalgh]

On Wed, Mar 14, 2018 at 05:40:49PM +0000, Sudi Das wrote:
> Hi
> 
> This patch is another partial fix for PR 84521. This is adding a 
> definition to one of the target hooks used in the SJLJ implemetation so 
> that AArch64 defines the hard_frame_pointer_rtx as the 
> TARGET_BUILTIN_SETJMP_FRAME_VALUE. As pointed out by Wilco there is 
> still a lot more work to be done for these builtins in the future.
> 
> Testing: Bootstrapped and regtested on aarch64-none-linux-gnu and added 
> new test.
> 
> Is this ok for trunk?

OK.

Thanks,
James

> *** gcc/ChangeLog ***
> 
> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
> 
> 	* builtins.c (expand_builtin_setjmp_receiver): Update condition
> 	to restore frame pointer.
> 	* config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update
> 	comment.
> 	* config/aarch64/aarch64.c (aarch64_builtin_setjmp_frame_value):
> 	New.
> 	(TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
> 
> 	* gcc.c-torture/execute/pr84521.c: New test.

> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index 85affa7..640f1a9 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -898,7 +898,8 @@ expand_builtin_setjmp_receiver (rtx receiver_label)
>  
>    /* Now put in the code to restore the frame pointer, and argument
>       pointer, if needed.  */
> -  if (! targetm.have_nonlocal_goto ())
> +  if (! targetm.have_nonlocal_goto ()
> +      && targetm.builtin_setjmp_frame_value () != hard_frame_pointer_rtx)
>      {
>        /* First adjust our frame pointer to its actual value.  It was
>  	 previously set to the start of the virtual area corresponding to
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index e3c52f6..7a21c14 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -474,7 +474,9 @@ extern unsigned aarch64_architecture_version;
>  #define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
>  #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
>  
> -/* Don't use __builtin_setjmp until we've defined it.  */
> +/* Don't use __builtin_setjmp until we've defined it.
> +   CAUTION: This macro is only used during exception unwinding.
> +   Don't fall for its name.  */
>  #undef DONT_USE_BUILTIN_SETJMP
>  #define DONT_USE_BUILTIN_SETJMP 1
>  
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index e1fb87f..e7ac0fe 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -12128,6 +12128,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
>    expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
>  }
>  
> +/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
> +static rtx
> +aarch64_builtin_setjmp_frame_value (void)
> +{
> +  return hard_frame_pointer_rtx;
> +}
> +
>  /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
>  
>  static tree
> @@ -17505,6 +17512,9 @@ aarch64_run_selftests (void)
>  #undef TARGET_FOLD_BUILTIN
>  #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
>  
> +#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
> +#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
> +
>  #undef TARGET_FUNCTION_ARG
>  #define TARGET_FUNCTION_ARG aarch64_function_arg
>  
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> new file mode 100644
> index 0000000..76b10d2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> @@ -0,0 +1,49 @@
> +/* { dg-require-effective-target indirect_jumps } */
> +
> +#include <setjmp.h>
> +
> +jmp_buf buf;
> +
> +int uses_longjmp (void)
> +{
> +  __builtin_longjmp (buf, 1);
> +}
> +
> +int gl;
> +void after_longjmp (void)
> +{
> +  gl = 5;
> +}
> +
> +int
> +test_1 (int n)
> +{
> +  volatile int *p = alloca (n);
> +  if (__builtin_setjmp (buf))
> +    {
> +      after_longjmp ();
> +    }
> +  else
> +    {
> +      uses_longjmp ();
> +    }
> +
> +  return 0;
> +}
> +
> +int __attribute__ ((optimize ("no-omit-frame-pointer")))
> +test_2 (int n)
> +{
> +  int i;
> +  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
> +  for (i = 0; i < n; i++)
> +    ptr[i] = i;
> +  test_1 (n);
> +  return 0;
> +}
> +
> +int main (int argc, const char **argv)
> +{
> +  __builtin_memset (&buf, 0xaf, sizeof (buf));
> +  test_2 (100);
> +}

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Sudakshina Das]

On 19/03/18 12:11, James Greenhalgh wrote:
> On Wed, Mar 14, 2018 at 05:40:49PM +0000, Sudi Das wrote:
>> Hi
>>
>> This patch is another partial fix for PR 84521. This is adding a
>> definition to one of the target hooks used in the SJLJ implemetation so
>> that AArch64 defines the hard_frame_pointer_rtx as the
>> TARGET_BUILTIN_SETJMP_FRAME_VALUE. As pointed out by Wilco there is
>> still a lot more work to be done for these builtins in the future.
>>
>> Testing: Bootstrapped and regtested on aarch64-none-linux-gnu and added
>> new test.
>>
>> Is this ok for trunk?
> 
> OK.

Thanks James but I realized I marked this wrong as only AArch64 patch. 
This also has a mid change so cc'ing more people for approval.

Sudi

> 
> Thanks,
> James
> 
>> *** gcc/ChangeLog ***
>>
>> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
>>
>> 	* builtins.c (expand_builtin_setjmp_receiver): Update condition
>> 	to restore frame pointer.
>> 	* config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update
>> 	comment.
>> 	* config/aarch64/aarch64.c (aarch64_builtin_setjmp_frame_value):
>> 	New.
>> 	(TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
>>
>> 	* gcc.c-torture/execute/pr84521.c: New test.
> 
>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>> index 85affa7..640f1a9 100644
>> --- a/gcc/builtins.c
>> +++ b/gcc/builtins.c
>> @@ -898,7 +898,8 @@ expand_builtin_setjmp_receiver (rtx receiver_label)
>>   
>>     /* Now put in the code to restore the frame pointer, and argument
>>        pointer, if needed.  */
>> -  if (! targetm.have_nonlocal_goto ())
>> +  if (! targetm.have_nonlocal_goto ()
>> +      && targetm.builtin_setjmp_frame_value () != hard_frame_pointer_rtx)
>>       {
>>         /* First adjust our frame pointer to its actual value.  It was
>>   	 previously set to the start of the virtual area corresponding to
>> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
>> index e3c52f6..7a21c14 100644
>> --- a/gcc/config/aarch64/aarch64.h
>> +++ b/gcc/config/aarch64/aarch64.h
>> @@ -474,7 +474,9 @@ extern unsigned aarch64_architecture_version;
>>   #define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
>>   #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
>>   
>> -/* Don't use __builtin_setjmp until we've defined it.  */
>> +/* Don't use __builtin_setjmp until we've defined it.
>> +   CAUTION: This macro is only used during exception unwinding.
>> +   Don't fall for its name.  */
>>   #undef DONT_USE_BUILTIN_SETJMP
>>   #define DONT_USE_BUILTIN_SETJMP 1
>>   
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index e1fb87f..e7ac0fe 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -12128,6 +12128,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
>>     expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
>>   }
>>   
>> +/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
>> +static rtx
>> +aarch64_builtin_setjmp_frame_value (void)
>> +{
>> +  return hard_frame_pointer_rtx;
>> +}
>> +
>>   /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
>>   
>>   static tree
>> @@ -17505,6 +17512,9 @@ aarch64_run_selftests (void)
>>   #undef TARGET_FOLD_BUILTIN
>>   #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
>>   
>> +#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
>> +#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
>> +
>>   #undef TARGET_FUNCTION_ARG
>>   #define TARGET_FUNCTION_ARG aarch64_function_arg
>>   
>> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
>> new file mode 100644
>> index 0000000..76b10d2
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
>> @@ -0,0 +1,49 @@
>> +/* { dg-require-effective-target indirect_jumps } */
>> +
>> +#include <setjmp.h>
>> +
>> +jmp_buf buf;
>> +
>> +int uses_longjmp (void)
>> +{
>> +  __builtin_longjmp (buf, 1);
>> +}
>> +
>> +int gl;
>> +void after_longjmp (void)
>> +{
>> +  gl = 5;
>> +}
>> +
>> +int
>> +test_1 (int n)
>> +{
>> +  volatile int *p = alloca (n);
>> +  if (__builtin_setjmp (buf))
>> +    {
>> +      after_longjmp ();
>> +    }
>> +  else
>> +    {
>> +      uses_longjmp ();
>> +    }
>> +
>> +  return 0;
>> +}
>> +
>> +int __attribute__ ((optimize ("no-omit-frame-pointer")))
>> +test_2 (int n)
>> +{
>> +  int i;
>> +  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
>> +  for (i = 0; i < n; i++)
>> +    ptr[i] = i;
>> +  test_1 (n);
>> +  return 0;
>> +}
>> +
>> +int main (int argc, const char **argv)
>> +{
>> +  __builtin_memset (&buf, 0xaf, sizeof (buf));
>> +  test_2 (100);
>> +}
> 

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Jeff Law]

On 03/14/2018 11:40 AM, Sudakshina Das wrote:
> Hi
> 
> This patch is another partial fix for PR 84521. This is adding a
> definition to one of the target hooks used in the SJLJ implemetation so
> that AArch64 defines the hard_frame_pointer_rtx as the
> TARGET_BUILTIN_SETJMP_FRAME_VALUE. As pointed out by Wilco there is
> still a lot more work to be done for these builtins in the future.
> 
> Testing: Bootstrapped and regtested on aarch64-none-linux-gnu and added
> new test.
> 
> Is this ok for trunk?
> 
> Sudi
> 
> 
> *** gcc/ChangeLog ***
> 
> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
> 
>     * builtins.c (expand_builtin_setjmp_receiver): Update condition
>     to restore frame pointer.
>     * config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update
>     comment.
>     * config/aarch64/aarch64.c (aarch64_builtin_setjmp_frame_value):
>     New.
>     (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
> 
>     * gcc.c-torture/execute/pr84521.c: New test.
So just to be clear, you do _not_ want the frame pointer restored here?
Right?

aarch64_builtin_setjmp_frame_value always returns hard_frame_pointer_rtx
which will cause the generic code in builtins.c to not restore the frame
pointer.

Have you looked at other targets which define builtin_setjmp_frame_value
to determine if they'll do the right thing.  x86 and sparc are the most
important.  I see that arc, vax and avr also define that hook, but are
obviously harder to test.

jeff

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Sudakshina Das]

On 02/05/18 18:28, Jeff Law wrote:
> On 03/14/2018 11:40 AM, Sudakshina Das wrote:
>> Hi
>>
>> This patch is another partial fix for PR 84521. This is adding a
>> definition to one of the target hooks used in the SJLJ implemetation so
>> that AArch64 defines the hard_frame_pointer_rtx as the
>> TARGET_BUILTIN_SETJMP_FRAME_VALUE. As pointed out by Wilco there is
>> still a lot more work to be done for these builtins in the future.
>>
>> Testing: Bootstrapped and regtested on aarch64-none-linux-gnu and added
>> new test.
>>
>> Is this ok for trunk?
>>
>> Sudi
>>
>>
>> *** gcc/ChangeLog ***
>>
>> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * builtins.c (expand_builtin_setjmp_receiver): Update condition
>>      to restore frame pointer.
>>      * config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update
>>      comment.
>>      * config/aarch64/aarch64.c (aarch64_builtin_setjmp_frame_value):
>>      New.
>>      (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2018-03-14  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * gcc.c-torture/execute/pr84521.c: New test.
> So just to be clear, you do _not_ want the frame pointer restored here?
> Right?
> 
> aarch64_builtin_setjmp_frame_value always returns hard_frame_pointer_rtx
> which will cause the generic code in builtins.c to not restore the frame
> pointer.
> 
> Have you looked at other targets which define builtin_setjmp_frame_value
> to determine if they'll do the right thing.  x86 and sparc are the most
> important.  I see that arc, vax and avr also define that hook, but are
> obviously harder to test.
>

Sorry this fell off my radar. I have reg-tested it on x86 and tried it
on the sparc machine from the gcc farm but I think I couldn't finished
the run and now its showing to he unreachable.

Sudi

> jeff
> 
> 

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Eric Botcazou]

> Sorry this fell off my radar. I have reg-tested it on x86 and tried it
> on the sparc machine from the gcc farm but I think I couldn't finished
> the run and now its showing to he unreachable.

The patch is a no-op for SPARC because it defines the nonlocal_goto pattern.

But I would nevertheless strongly suggest _not_ fiddling with the generic code 
like that and just defining the nonlocal_goto pattern for Aarch64 instead.

-- 
Eric Botcazou

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Sudakshina Das]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

Hi Eric

On 07/06/18 16:33, Eric Botcazou wrote:
>> Sorry this fell off my radar. I have reg-tested it on x86 and tried it
>> on the sparc machine from the gcc farm but I think I couldn't finished
>> the run and now its showing to he unreachable.
> 
> The patch is a no-op for SPARC because it defines the nonlocal_goto pattern.
> 
> But I would nevertheless strongly suggest _not_ fiddling with the generic code
> like that and just defining the nonlocal_goto pattern for Aarch64 instead.
> 

Thank you for the suggestion, I have edited the patch accordingly and
defined the nonlocal_goto pattern for AArch64. This has also helped take
care of the issue with __builtin_longjmp that Wilco had mentioned in his
comment on the PR (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84521#c19).

I have also modified the test case according to Wilco's comment to add 
an extra jump buffer. This test case passes with AArch64 but fails on
x86 trunk as follows (It may fail on other targets as well):

FAIL: gcc.c-torture/execute/pr84521.c   -O1  execution test
FAIL: gcc.c-torture/execute/pr84521.c   -O2  execution test
FAIL: gcc.c-torture/execute/pr84521.c   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
FAIL: gcc.c-torture/execute/pr84521.c   -O3 -g  execution test
FAIL: gcc.c-torture/execute/pr84521.c   -Os  execution test
FAIL: gcc.c-torture/execute/pr84521.c   -O2 -flto -fno-use-linker-plugin
-flto-partition=none  execution test
FAIL: gcc.c-torture/execute/pr84521.c   -O2 -flto -fuse-linker-plugin
-fno-fat-lto-objects  execution test

Testing: Bootstrapped and regtested on aarch64-none-linux-gnu.

Is this ok for trunk?

Sudi

*** gcc/ChangeLog ***

2018-06-14  Sudakshina Das  <sudi.das@arm.com>

	PR target/84521
	* config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update comment.
	* config/aarch64/aarch64.c (aarch64_needs_frame_chain): Add
	cfun->has_nonlocal_label to force frame chain.
	(aarch64_builtin_setjmp_frame_value): New.
	(TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
	* config/aarch64/aarch64.md (nonlocal_goto): New.

*** gcc/testsuite/ChangeLog ***

2018-06-14  Sudakshina Das  <sudi.das@arm.com>

	PR target/84521
	* gcc.c-torture/execute/pr84521.c: New test.

[-- Attachment #2: pr84521-new.diff --]
[-- Type: text/x-patch, Size: 3996 bytes --]

diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 976f9af..f042def 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -474,7 +474,9 @@ extern unsigned aarch64_architecture_version;
 #define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
 #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
 
-/* Don't use __builtin_setjmp until we've defined it.  */
+/* Don't use __builtin_setjmp until we've defined it.
+   CAUTION: This macro is only used during exception unwinding.
+   Don't fall for its name.  */
 #undef DONT_USE_BUILTIN_SETJMP
 #define DONT_USE_BUILTIN_SETJMP 1
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index bd0ac2f..95f7fe3 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3998,7 +3998,7 @@ static bool
 aarch64_needs_frame_chain (void)
 {
   /* Force a frame chain for EH returns so the return address is at FP+8.  */
-  if (frame_pointer_needed || crtl->calls_eh_return)
+  if (frame_pointer_needed || crtl->calls_eh_return || cfun->has_nonlocal_label)
     return true;
 
   /* A leaf function cannot have calls or write LR.  */
@@ -12213,6 +12213,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
   expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
 }
 
+/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
+static rtx
+aarch64_builtin_setjmp_frame_value (void)
+{
+  return hard_frame_pointer_rtx;
+}
+
 /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
 
 static tree
@@ -17829,6 +17836,9 @@ aarch64_run_selftests (void)
 #undef TARGET_FOLD_BUILTIN
 #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
 
+#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
+#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
+
 #undef TARGET_FUNCTION_ARG
 #define TARGET_FUNCTION_ARG aarch64_function_arg
 
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 830f976..381fd83 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -6081,6 +6081,30 @@
   DONE;
 })
 
+;; This is broadly similar to the builtins.c except that it uses
+;; temporaries to load the incoming SP and FP.
+(define_expand "nonlocal_goto"
+  [(use (match_operand 0 "general_operand"))
+   (use (match_operand 1 "general_operand"))
+   (use (match_operand 2 "general_operand"))
+   (use (match_operand 3 "general_operand"))]
+  ""
+{
+    rtx label_in = copy_to_reg (operands[1]);
+    rtx fp_in = copy_to_reg (operands[3]);
+    rtx sp_in = copy_to_reg (operands[2]);
+
+    emit_move_insn (hard_frame_pointer_rtx, fp_in);
+    emit_stack_restore (SAVE_NONLOCAL, sp_in);
+
+    emit_use (hard_frame_pointer_rtx);
+    emit_use (stack_pointer_rtx);
+
+    emit_indirect_jump (label_in);
+
+    DONE;
+})
+
 ;; Helper for aarch64.c code.
 (define_expand "set_clobber_cc"
   [(parallel [(set (match_operand 0)
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
new file mode 100644
index 0000000..c20ff5e
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
@@ -0,0 +1,53 @@
+/* { dg-require-effective-target indirect_jumps } */
+
+#include <string.h>
+#include <alloca.h>
+#include <setjmp.h>
+
+jmp_buf buf;
+
+int uses_longjmp (void)
+{
+  jmp_buf buf2;
+  memcpy (buf2, buf, sizeof (buf));
+  __builtin_longjmp (buf2, 1);
+}
+
+int gl;
+void after_longjmp (void)
+{
+  gl = 5;
+}
+
+int
+test_1 (int n)
+{
+  volatile int *p = alloca (n);
+  if (__builtin_setjmp (buf))
+    {
+      after_longjmp ();
+    }
+  else
+    {
+      uses_longjmp ();
+    }
+
+  return 0;
+}
+
+int __attribute__ ((optimize ("no-omit-frame-pointer")))
+test_2 (int n)
+{
+  int i;
+  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
+  for (i = 0; i < n; i++)
+    ptr[i] = i;
+  test_1 (n);
+  return 0;
+}
+
+int main (int argc, const char **argv)
+{
+  __builtin_memset (&buf, 0xaf, sizeof (buf));
+  test_2 (100);
+}

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Sudakshina Das]

PING!

On 14/06/18 12:10, Sudakshina Das wrote:
> Hi Eric
> 
> On 07/06/18 16:33, Eric Botcazou wrote:
>>> Sorry this fell off my radar. I have reg-tested it on x86 and tried it
>>> on the sparc machine from the gcc farm but I think I couldn't finished
>>> the run and now its showing to he unreachable.
>>
>> The patch is a no-op for SPARC because it defines the nonlocal_goto 
>> pattern.
>>
>> But I would nevertheless strongly suggest _not_ fiddling with the 
>> generic code
>> like that and just defining the nonlocal_goto pattern for Aarch64 
>> instead.
>>
> 
> Thank you for the suggestion, I have edited the patch accordingly and
> defined the nonlocal_goto pattern for AArch64. This has also helped take
> care of the issue with __builtin_longjmp that Wilco had mentioned in his
> comment on the PR (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84521#c19).
> 
> I have also modified the test case according to Wilco's comment to add 
> an extra jump buffer. This test case passes with AArch64 but fails on
> x86 trunk as follows (It may fail on other targets as well):
> 
> FAIL: gcc.c-torture/execute/pr84521.c   -O1  execution test
> FAIL: gcc.c-torture/execute/pr84521.c   -O2  execution test
> FAIL: gcc.c-torture/execute/pr84521.c   -O3 -fomit-frame-pointer
> -funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
> FAIL: gcc.c-torture/execute/pr84521.c   -O3 -g  execution test
> FAIL: gcc.c-torture/execute/pr84521.c   -Os  execution test
> FAIL: gcc.c-torture/execute/pr84521.c   -O2 -flto -fno-use-linker-plugin
> -flto-partition=none  execution test
> FAIL: gcc.c-torture/execute/pr84521.c   -O2 -flto -fuse-linker-plugin
> -fno-fat-lto-objects  execution test
> 
> Testing: Bootstrapped and regtested on aarch64-none-linux-gnu.
> 
> Is this ok for trunk?
> 
> Sudi
> 
> *** gcc/ChangeLog ***
> 
> 2018-06-14  Sudakshina Das  <sudi.das@arm.com>
> 
>      PR target/84521
>      * config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update comment.
>      * config/aarch64/aarch64.c (aarch64_needs_frame_chain): Add
>      cfun->has_nonlocal_label to force frame chain.
>      (aarch64_builtin_setjmp_frame_value): New.
>      (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
>      * config/aarch64/aarch64.md (nonlocal_goto): New.
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2018-06-14  Sudakshina Das  <sudi.das@arm.com>
> 
>      PR target/84521
>      * gcc.c-torture/execute/pr84521.c: New test.

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[James Greenhalgh]

On Mon, Jun 25, 2018 at 04:24:14AM -0500, Sudakshina Das wrote:
> PING!
> 
> On 14/06/18 12:10, Sudakshina Das wrote:
> > Hi Eric
> > 
> > On 07/06/18 16:33, Eric Botcazou wrote:
> >>> Sorry this fell off my radar. I have reg-tested it on x86 and tried it
> >>> on the sparc machine from the gcc farm but I think I couldn't finished
> >>> the run and now its showing to he unreachable.
> >>
> >> The patch is a no-op for SPARC because it defines the nonlocal_goto 
> >> pattern.
> >>
> >> But I would nevertheless strongly suggest _not_ fiddling with the 
> >> generic code
> >> like that and just defining the nonlocal_goto pattern for Aarch64 
> >> instead.
> >>
> > 
> > Thank you for the suggestion, I have edited the patch accordingly and
> > defined the nonlocal_goto pattern for AArch64. This has also helped take
> > care of the issue with __builtin_longjmp that Wilco had mentioned in his
> > comment on the PR (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84521#c19).
> > 
> > I have also modified the test case according to Wilco's comment to add 
> > an extra jump buffer. This test case passes with AArch64 but fails on
> > x86 trunk as follows (It may fail on other targets as well):
> > 
> > FAIL: gcc.c-torture/execute/pr84521.c   -O1  execution test
> > FAIL: gcc.c-torture/execute/pr84521.c   -O2  execution test
> > FAIL: gcc.c-torture/execute/pr84521.c   -O3 -fomit-frame-pointer
> > -funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
> > FAIL: gcc.c-torture/execute/pr84521.c   -O3 -g  execution test
> > FAIL: gcc.c-torture/execute/pr84521.c   -Os  execution test
> > FAIL: gcc.c-torture/execute/pr84521.c   -O2 -flto -fno-use-linker-plugin
> > -flto-partition=none  execution test
> > FAIL: gcc.c-torture/execute/pr84521.c   -O2 -flto -fuse-linker-plugin
> > -fno-fat-lto-objects  execution test
> > 
> > Testing: Bootstrapped and regtested on aarch64-none-linux-gnu.
> > 
> > Is this ok for trunk?

The AArch64 parts are OK. I've been holding off approving the patch while
I wait for Eric to reply on the x86_64 fails with your new testcase.

Thanks,
James



> > 
> > Sudi
> > 
> > *** gcc/ChangeLog ***
> > 
> > 2018-06-14  Sudakshina Das  <sudi.das@arm.com>
> > 
> >      PR target/84521
> >      * config/aarch64/aarch64.h (DONT_USE_BUILTIN_SETJMP): Update comment.
> >      * config/aarch64/aarch64.c (aarch64_needs_frame_chain): Add
> >      cfun->has_nonlocal_label to force frame chain.
> >      (aarch64_builtin_setjmp_frame_value): New.
> >      (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
> >      * config/aarch64/aarch64.md (nonlocal_goto): New.
> > 
> > *** gcc/testsuite/ChangeLog ***
> > 
> > 2018-06-14  Sudakshina Das  <sudi.das@arm.com>
> > 
> >      PR target/84521
> >      * gcc.c-torture/execute/pr84521.c: New test.
> 

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Eric Botcazou]

> The AArch64 parts are OK. I've been holding off approving the patch while
> I wait for Eric to reply on the x86_64 fails with your new testcase.

The test is not portable in any case since it uses the "optimize" attribute so 
I'd just make it specific to Aarch64 and install the patch.

-- 
Eric Botcazou

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Wilco Dijkstra]

Eric Botcazou wrote:

> > The AArch64 parts are OK. I've been holding off approving the patch while
> > I wait for Eric to reply on the x86_64 fails with your new testcase.
>
> The test is not portable in any case since it uses the "optimize" attribute so 
> I'd just make it specific to Aarch64 and install the patch.

This test can easily be changed not to use optimize since it doesn't look like it
needs it. We really need to tests these builtins properly, otherwise they will
continue to fail on most targets.

Wilco
    

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Eric Botcazou]

> This test can easily be changed not to use optimize since it doesn't look
> like it needs it. We really need to tests these builtins properly,
> otherwise they will continue to fail on most targets.

As far as I can see PR target/84521 has been reported only for Aarch64 so I'd 
just leave the other targets alone (and avoid propagating FUD if possible).

-- 
Eric Botcazou

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Wilco Dijkstra]

Eric Botcazou wrote:

>> This test can easily be changed not to use optimize since it doesn't look
>> like it needs it. We really need to tests these builtins properly,
>> otherwise they will continue to fail on most targets.
>
> As far as I can see PR target/84521 has been reported only for Aarch64 so I'd 
> just leave the other targets alone (and avoid propagating FUD if possible).

It's quite obvious from PR84521 that this is an issue affecting all targets.
Adding better generic tests for __builtin_setjmp can only be a good thing.

Wilco

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Sudakshina Das]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

Hi Eric

On 27/06/18 12:22, Wilco Dijkstra wrote:
> Eric Botcazou wrote:
> 
>>> This test can easily be changed not to use optimize since it doesn't look
>>> like it needs it. We really need to tests these builtins properly,
>>> otherwise they will continue to fail on most targets.
>>
>> As far as I can see PR target/84521 has been reported only for Aarch64 so I'd
>> just leave the other targets alone (and avoid propagating FUD if possible).
> 
> It's quite obvious from PR84521 that this is an issue affecting all targets.
> Adding better generic tests for __builtin_setjmp can only be a good thing.
> 
> Wilco
> 

This conversation seems to have died down and I would like to
start it again. I would agree with Wilco's suggestion about
keeping the test in the generic folder. I have removed the
optimize attribute and the effect is still the same. It passes
on AArch64 with this patch and it currently fails on x86
trunk (gcc version 9.0.0 20180712 (experimental) (GCC))
on -O1 and above.

Thanks
Sudi

[-- Attachment #2: pr84521-new.diff --]
[-- Type: text/x-patch, Size: 3943 bytes --]

diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index f284e74..9792d28 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -473,7 +473,9 @@ extern unsigned aarch64_architecture_version;
 #define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
 #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
 
-/* Don't use __builtin_setjmp until we've defined it.  */
+/* Don't use __builtin_setjmp until we've defined it.
+   CAUTION: This macro is only used during exception unwinding.
+   Don't fall for its name.  */
 #undef DONT_USE_BUILTIN_SETJMP
 #define DONT_USE_BUILTIN_SETJMP 1
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 01f35f8..4266a3d 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3998,7 +3998,7 @@ static bool
 aarch64_needs_frame_chain (void)
 {
   /* Force a frame chain for EH returns so the return address is at FP+8.  */
-  if (frame_pointer_needed || crtl->calls_eh_return)
+  if (frame_pointer_needed || crtl->calls_eh_return || cfun->has_nonlocal_label)
     return true;
 
   /* A leaf function cannot have calls or write LR.  */
@@ -12218,6 +12218,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
   expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
 }
 
+/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
+static rtx
+aarch64_builtin_setjmp_frame_value (void)
+{
+  return hard_frame_pointer_rtx;
+}
+
 /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
 
 static tree
@@ -17744,6 +17751,9 @@ aarch64_run_selftests (void)
 #undef TARGET_FOLD_BUILTIN
 #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
 
+#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
+#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
+
 #undef TARGET_FUNCTION_ARG
 #define TARGET_FUNCTION_ARG aarch64_function_arg
 
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index a014a01..d5f33d8 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -6087,6 +6087,30 @@
   DONE;
 })
 
+;; This is broadly similar to the builtins.c except that it uses
+;; temporaries to load the incoming SP and FP.
+(define_expand "nonlocal_goto"
+  [(use (match_operand 0 "general_operand"))
+   (use (match_operand 1 "general_operand"))
+   (use (match_operand 2 "general_operand"))
+   (use (match_operand 3 "general_operand"))]
+  ""
+{
+    rtx label_in = copy_to_reg (operands[1]);
+    rtx fp_in = copy_to_reg (operands[3]);
+    rtx sp_in = copy_to_reg (operands[2]);
+
+    emit_move_insn (hard_frame_pointer_rtx, fp_in);
+    emit_stack_restore (SAVE_NONLOCAL, sp_in);
+
+    emit_use (hard_frame_pointer_rtx);
+    emit_use (stack_pointer_rtx);
+
+    emit_indirect_jump (label_in);
+
+    DONE;
+})
+
 ;; Helper for aarch64.c code.
 (define_expand "set_clobber_cc"
   [(parallel [(set (match_operand 0)
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
new file mode 100644
index 0000000..564ef14
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
@@ -0,0 +1,53 @@
+/* { dg-require-effective-target indirect_jumps } */
+
+#include <string.h>
+#include <alloca.h>
+#include <setjmp.h>
+
+jmp_buf buf;
+
+int uses_longjmp (void)
+{
+  jmp_buf buf2;
+  memcpy (buf2, buf, sizeof (buf));
+  __builtin_longjmp (buf2, 1);
+}
+
+int gl;
+void after_longjmp (void)
+{
+  gl = 5;
+}
+
+int
+test_1 (int n)
+{
+  volatile int *p = alloca (n);
+  if (__builtin_setjmp (buf))
+    {
+      after_longjmp ();
+    }
+  else
+    {
+      uses_longjmp ();
+    }
+
+  return 0;
+}
+
+int
+test_2 (int n)
+{
+  int i;
+  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
+  for (i = 0; i < n; i++)
+    ptr[i] = i;
+  test_1 (n);
+  return 0;
+}
+
+int main (int argc, const char **argv)
+{
+  __builtin_memset (&buf, 0xaf, sizeof (buf));
+  test_2 (100);
+}

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[James Greenhalgh]

On Thu, Jul 12, 2018 at 12:01:09PM -0500, Sudakshina Das wrote:
> Hi Eric
> 
> On 27/06/18 12:22, Wilco Dijkstra wrote:
> > Eric Botcazou wrote:
> > 
> >>> This test can easily be changed not to use optimize since it doesn't look
> >>> like it needs it. We really need to tests these builtins properly,
> >>> otherwise they will continue to fail on most targets.
> >>
> >> As far as I can see PR target/84521 has been reported only for Aarch64 so I'd
> >> just leave the other targets alone (and avoid propagating FUD if possible).
> > 
> > It's quite obvious from PR84521 that this is an issue affecting all targets.
> > Adding better generic tests for __builtin_setjmp can only be a good thing.
> > 
> > Wilco
> > 
> 
> This conversation seems to have died down and I would like to
> start it again. I would agree with Wilco's suggestion about
> keeping the test in the generic folder. I have removed the
> optimize attribute and the effect is still the same. It passes
> on AArch64 with this patch and it currently fails on x86
> trunk (gcc version 9.0.0 20180712 (experimental) (GCC))
> on -O1 and above.


I don't see where the FUD comes in here; either this builtin has a defined
semantics across targets and they are adhered to, or the builtin doesn't have
well defined semantics, or the targets fail to implement those semantics.

I think this should go in as is. If other targets are unhappy with the
failing test they should fix their target or skip the test if it is not
appropriate.

You may want to CC some of the maintainers of platforms you know to fail as
a courtesy on the PR (add your testcase, and add failing targets and their
maintainers to that PR) before committing so it doesn't come as a complete
surprise.

This is OK with some attempt to get target maintainers involved in the
conversation before commit.

Thanks,
James

> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index f284e74..9792d28 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -473,7 +473,9 @@ extern unsigned aarch64_architecture_version;
>  #define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
>  #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
>  
> -/* Don't use __builtin_setjmp until we've defined it.  */
> +/* Don't use __builtin_setjmp until we've defined it.
> +   CAUTION: This macro is only used during exception unwinding.
> +   Don't fall for its name.  */
>  #undef DONT_USE_BUILTIN_SETJMP
>  #define DONT_USE_BUILTIN_SETJMP 1
>  
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 01f35f8..4266a3d 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -3998,7 +3998,7 @@ static bool
>  aarch64_needs_frame_chain (void)
>  {
>    /* Force a frame chain for EH returns so the return address is at FP+8.  */
> -  if (frame_pointer_needed || crtl->calls_eh_return)
> +  if (frame_pointer_needed || crtl->calls_eh_return || cfun->has_nonlocal_label)
>      return true;
>  
>    /* A leaf function cannot have calls or write LR.  */
> @@ -12218,6 +12218,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
>    expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
>  }
>  
> +/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
> +static rtx
> +aarch64_builtin_setjmp_frame_value (void)
> +{
> +  return hard_frame_pointer_rtx;
> +}
> +
>  /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
>  
>  static tree
> @@ -17744,6 +17751,9 @@ aarch64_run_selftests (void)
>  #undef TARGET_FOLD_BUILTIN
>  #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
>  
> +#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
> +#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
> +
>  #undef TARGET_FUNCTION_ARG
>  #define TARGET_FUNCTION_ARG aarch64_function_arg
>  
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index a014a01..d5f33d8 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -6087,6 +6087,30 @@
>    DONE;
>  })
>  
> +;; This is broadly similar to the builtins.c except that it uses
> +;; temporaries to load the incoming SP and FP.
> +(define_expand "nonlocal_goto"
> +  [(use (match_operand 0 "general_operand"))
> +   (use (match_operand 1 "general_operand"))
> +   (use (match_operand 2 "general_operand"))
> +   (use (match_operand 3 "general_operand"))]
> +  ""
> +{
> +    rtx label_in = copy_to_reg (operands[1]);
> +    rtx fp_in = copy_to_reg (operands[3]);
> +    rtx sp_in = copy_to_reg (operands[2]);
> +
> +    emit_move_insn (hard_frame_pointer_rtx, fp_in);
> +    emit_stack_restore (SAVE_NONLOCAL, sp_in);
> +
> +    emit_use (hard_frame_pointer_rtx);
> +    emit_use (stack_pointer_rtx);
> +
> +    emit_indirect_jump (label_in);
> +
> +    DONE;
> +})
> +
>  ;; Helper for aarch64.c code.
>  (define_expand "set_clobber_cc"
>    [(parallel [(set (match_operand 0)
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> new file mode 100644
> index 0000000..564ef14
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> @@ -0,0 +1,53 @@
> +/* { dg-require-effective-target indirect_jumps } */
> +
> +#include <string.h>
> +#include <alloca.h>
> +#include <setjmp.h>
> +
> +jmp_buf buf;
> +
> +int uses_longjmp (void)
> +{
> +  jmp_buf buf2;
> +  memcpy (buf2, buf, sizeof (buf));
> +  __builtin_longjmp (buf2, 1);
> +}
> +
> +int gl;
> +void after_longjmp (void)
> +{
> +  gl = 5;
> +}
> +
> +int
> +test_1 (int n)
> +{
> +  volatile int *p = alloca (n);
> +  if (__builtin_setjmp (buf))
> +    {
> +      after_longjmp ();
> +    }
> +  else
> +    {
> +      uses_longjmp ();
> +    }
> +
> +  return 0;
> +}
> +
> +int
> +test_2 (int n)
> +{
> +  int i;
> +  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
> +  for (i = 0; i < n; i++)
> +    ptr[i] = i;
> +  test_1 (n);
> +  return 0;
> +}
> +
> +int main (int argc, const char **argv)
> +{
> +  __builtin_memset (&buf, 0xaf, sizeof (buf));
> +  test_2 (100);
> +}

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Andrew Pinski]

On Tue, Jul 31, 2018 at 2:43 PM James Greenhalgh
<james.greenhalgh@arm.com> wrote:
>
> On Thu, Jul 12, 2018 at 12:01:09PM -0500, Sudakshina Das wrote:
> > Hi Eric
> >
> > On 27/06/18 12:22, Wilco Dijkstra wrote:
> > > Eric Botcazou wrote:
> > >
> > >>> This test can easily be changed not to use optimize since it doesn't look
> > >>> like it needs it. We really need to tests these builtins properly,
> > >>> otherwise they will continue to fail on most targets.
> > >>
> > >> As far as I can see PR target/84521 has been reported only for Aarch64 so I'd
> > >> just leave the other targets alone (and avoid propagating FUD if possible).
> > >
> > > It's quite obvious from PR84521 that this is an issue affecting all targets.
> > > Adding better generic tests for __builtin_setjmp can only be a good thing.
> > >
> > > Wilco
> > >
> >
> > This conversation seems to have died down and I would like to
> > start it again. I would agree with Wilco's suggestion about
> > keeping the test in the generic folder. I have removed the
> > optimize attribute and the effect is still the same. It passes
> > on AArch64 with this patch and it currently fails on x86
> > trunk (gcc version 9.0.0 20180712 (experimental) (GCC))
> > on -O1 and above.
>
>
> I don't see where the FUD comes in here; either this builtin has a defined
> semantics across targets and they are adhered to, or the builtin doesn't have
> well defined semantics, or the targets fail to implement those semantics.

The problem comes from the fact the builtins are not documented at all.
See PR59039 for the issue on them not being documented.

Thanks,
Andrew


>
> I think this should go in as is. If other targets are unhappy with the
> failing test they should fix their target or skip the test if it is not
> appropriate.
>
> You may want to CC some of the maintainers of platforms you know to fail as
> a courtesy on the PR (add your testcase, and add failing targets and their
> maintainers to that PR) before committing so it doesn't come as a complete
> surprise.
>
> This is OK with some attempt to get target maintainers involved in the
> conversation before commit.
>
> Thanks,
> James
>
> > diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> > index f284e74..9792d28 100644
> > --- a/gcc/config/aarch64/aarch64.h
> > +++ b/gcc/config/aarch64/aarch64.h
> > @@ -473,7 +473,9 @@ extern unsigned aarch64_architecture_version;
> >  #define EH_RETURN_STACKADJ_RTX       gen_rtx_REG (Pmode, R4_REGNUM)
> >  #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
> >
> > -/* Don't use __builtin_setjmp until we've defined it.  */
> > +/* Don't use __builtin_setjmp until we've defined it.
> > +   CAUTION: This macro is only used during exception unwinding.
> > +   Don't fall for its name.  */
> >  #undef DONT_USE_BUILTIN_SETJMP
> >  #define DONT_USE_BUILTIN_SETJMP 1
> >
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index 01f35f8..4266a3d 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -3998,7 +3998,7 @@ static bool
> >  aarch64_needs_frame_chain (void)
> >  {
> >    /* Force a frame chain for EH returns so the return address is at FP+8.  */
> > -  if (frame_pointer_needed || crtl->calls_eh_return)
> > +  if (frame_pointer_needed || crtl->calls_eh_return || cfun->has_nonlocal_label)
> >      return true;
> >
> >    /* A leaf function cannot have calls or write LR.  */
> > @@ -12218,6 +12218,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
> >    expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
> >  }
> >
> > +/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
> > +static rtx
> > +aarch64_builtin_setjmp_frame_value (void)
> > +{
> > +  return hard_frame_pointer_rtx;
> > +}
> > +
> >  /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
> >
> >  static tree
> > @@ -17744,6 +17751,9 @@ aarch64_run_selftests (void)
> >  #undef TARGET_FOLD_BUILTIN
> >  #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
> >
> > +#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
> > +#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
> > +
> >  #undef TARGET_FUNCTION_ARG
> >  #define TARGET_FUNCTION_ARG aarch64_function_arg
> >
> > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> > index a014a01..d5f33d8 100644
> > --- a/gcc/config/aarch64/aarch64.md
> > +++ b/gcc/config/aarch64/aarch64.md
> > @@ -6087,6 +6087,30 @@
> >    DONE;
> >  })
> >
> > +;; This is broadly similar to the builtins.c except that it uses
> > +;; temporaries to load the incoming SP and FP.
> > +(define_expand "nonlocal_goto"
> > +  [(use (match_operand 0 "general_operand"))
> > +   (use (match_operand 1 "general_operand"))
> > +   (use (match_operand 2 "general_operand"))
> > +   (use (match_operand 3 "general_operand"))]
> > +  ""
> > +{
> > +    rtx label_in = copy_to_reg (operands[1]);
> > +    rtx fp_in = copy_to_reg (operands[3]);
> > +    rtx sp_in = copy_to_reg (operands[2]);
> > +
> > +    emit_move_insn (hard_frame_pointer_rtx, fp_in);
> > +    emit_stack_restore (SAVE_NONLOCAL, sp_in);
> > +
> > +    emit_use (hard_frame_pointer_rtx);
> > +    emit_use (stack_pointer_rtx);
> > +
> > +    emit_indirect_jump (label_in);
> > +
> > +    DONE;
> > +})
> > +
> >  ;; Helper for aarch64.c code.
> >  (define_expand "set_clobber_cc"
> >    [(parallel [(set (match_operand 0)
> > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> > new file mode 100644
> > index 0000000..564ef14
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> > @@ -0,0 +1,53 @@
> > +/* { dg-require-effective-target indirect_jumps } */
> > +
> > +#include <string.h>
> > +#include <alloca.h>
> > +#include <setjmp.h>
> > +
> > +jmp_buf buf;
> > +
> > +int uses_longjmp (void)
> > +{
> > +  jmp_buf buf2;
> > +  memcpy (buf2, buf, sizeof (buf));
> > +  __builtin_longjmp (buf2, 1);
> > +}
> > +
> > +int gl;
> > +void after_longjmp (void)
> > +{
> > +  gl = 5;
> > +}
> > +
> > +int
> > +test_1 (int n)
> > +{
> > +  volatile int *p = alloca (n);
> > +  if (__builtin_setjmp (buf))
> > +    {
> > +      after_longjmp ();
> > +    }
> > +  else
> > +    {
> > +      uses_longjmp ();
> > +    }
> > +
> > +  return 0;
> > +}
> > +
> > +int
> > +test_2 (int n)
> > +{
> > +  int i;
> > +  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
> > +  for (i = 0; i < n; i++)
> > +    ptr[i] = i;
> > +  test_1 (n);
> > +  return 0;
> > +}
> > +
> > +int main (int argc, const char **argv)
> > +{
> > +  __builtin_memset (&buf, 0xaf, sizeof (buf));
> > +  test_2 (100);
> > +}
>

Re: [PATCH][AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp

[Sudakshina Das]

Hi

On 31/07/18 22:48, Andrew Pinski wrote:
> On Tue, Jul 31, 2018 at 2:43 PM James Greenhalgh
> <james.greenhalgh@arm.com> wrote:
>>
>> On Thu, Jul 12, 2018 at 12:01:09PM -0500, Sudakshina Das wrote:
>>> Hi Eric
>>>
>>> On 27/06/18 12:22, Wilco Dijkstra wrote:
>>>> Eric Botcazou wrote:
>>>>
>>>>>> This test can easily be changed not to use optimize since it doesn't look
>>>>>> like it needs it. We really need to tests these builtins properly,
>>>>>> otherwise they will continue to fail on most targets.
>>>>>
>>>>> As far as I can see PR target/84521 has been reported only for Aarch64 so I'd
>>>>> just leave the other targets alone (and avoid propagating FUD if possible).
>>>>
>>>> It's quite obvious from PR84521 that this is an issue affecting all targets.
>>>> Adding better generic tests for __builtin_setjmp can only be a good thing.
>>>>
>>>> Wilco
>>>>
>>>
>>> This conversation seems to have died down and I would like to
>>> start it again. I would agree with Wilco's suggestion about
>>> keeping the test in the generic folder. I have removed the
>>> optimize attribute and the effect is still the same. It passes
>>> on AArch64 with this patch and it currently fails on x86
>>> trunk (gcc version 9.0.0 20180712 (experimental) (GCC))
>>> on -O1 and above.
>>
>>
>> I don't see where the FUD comes in here; either this builtin has a defined
>> semantics across targets and they are adhered to, or the builtin doesn't have
>> well defined semantics, or the targets fail to implement those semantics.
> 
> The problem comes from the fact the builtins are not documented at all.
> See PR59039 for the issue on them not being documented.
> 

Thanks @James for bringing this up again.
I tried to revive the conversation on PR59039 while working on this as
well but that conversation mainly focused on documenting if we are
allowed to use __builtin_setjmp and __builtin_longjmp on the same
function and with the same jmp buffer or not. This patch and this test
case however does not involve that issue. There are other holes in the
documentation/implementation of these builtins. For now as advised by
James, I have posted the test case on the PR. I personally don't see why
this test case should go on the AArch64 tests when it clearly fails on
other targets as well. But if we can not come to an agreement on that, I
am willing to move it to AArch64 tests and maybe open a new bug report
which is not marked as "target" with the same test case.

Thanks
Sudi

> Thanks,
> Andrew
> 
> 
>>
>> I think this should go in as is. If other targets are unhappy with the
>> failing test they should fix their target or skip the test if it is not
>> appropriate.
>>
>> You may want to CC some of the maintainers of platforms you know to fail as
>> a courtesy on the PR (add your testcase, and add failing targets and their
>> maintainers to that PR) before committing so it doesn't come as a complete
>> surprise.
>>
>> This is OK with some attempt to get target maintainers involved in the
>> conversation before commit.
>>
>> Thanks,
>> James
>>
>>> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
>>> index f284e74..9792d28 100644
>>> --- a/gcc/config/aarch64/aarch64.h
>>> +++ b/gcc/config/aarch64/aarch64.h
>>> @@ -473,7 +473,9 @@ extern unsigned aarch64_architecture_version;
>>>   #define EH_RETURN_STACKADJ_RTX       gen_rtx_REG (Pmode, R4_REGNUM)
>>>   #define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
>>>
>>> -/* Don't use __builtin_setjmp until we've defined it.  */
>>> +/* Don't use __builtin_setjmp until we've defined it.
>>> +   CAUTION: This macro is only used during exception unwinding.
>>> +   Don't fall for its name.  */
>>>   #undef DONT_USE_BUILTIN_SETJMP
>>>   #define DONT_USE_BUILTIN_SETJMP 1
>>>
>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>>> index 01f35f8..4266a3d 100644
>>> --- a/gcc/config/aarch64/aarch64.c
>>> +++ b/gcc/config/aarch64/aarch64.c
>>> @@ -3998,7 +3998,7 @@ static bool
>>>   aarch64_needs_frame_chain (void)
>>>   {
>>>     /* Force a frame chain for EH returns so the return address is at FP+8.  */
>>> -  if (frame_pointer_needed || crtl->calls_eh_return)
>>> +  if (frame_pointer_needed || crtl->calls_eh_return || cfun->has_nonlocal_label)
>>>       return true;
>>>
>>>     /* A leaf function cannot have calls or write LR.  */
>>> @@ -12218,6 +12218,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx nextarg ATTRIBUTE_UNUSED)
>>>     expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
>>>   }
>>>
>>> +/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE.  */
>>> +static rtx
>>> +aarch64_builtin_setjmp_frame_value (void)
>>> +{
>>> +  return hard_frame_pointer_rtx;
>>> +}
>>> +
>>>   /* Implement TARGET_GIMPLIFY_VA_ARG_EXPR.  */
>>>
>>>   static tree
>>> @@ -17744,6 +17751,9 @@ aarch64_run_selftests (void)
>>>   #undef TARGET_FOLD_BUILTIN
>>>   #define TARGET_FOLD_BUILTIN aarch64_fold_builtin
>>>
>>> +#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
>>> +#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
>>> +
>>>   #undef TARGET_FUNCTION_ARG
>>>   #define TARGET_FUNCTION_ARG aarch64_function_arg
>>>
>>> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
>>> index a014a01..d5f33d8 100644
>>> --- a/gcc/config/aarch64/aarch64.md
>>> +++ b/gcc/config/aarch64/aarch64.md
>>> @@ -6087,6 +6087,30 @@
>>>     DONE;
>>>   })
>>>
>>> +;; This is broadly similar to the builtins.c except that it uses
>>> +;; temporaries to load the incoming SP and FP.
>>> +(define_expand "nonlocal_goto"
>>> +  [(use (match_operand 0 "general_operand"))
>>> +   (use (match_operand 1 "general_operand"))
>>> +   (use (match_operand 2 "general_operand"))
>>> +   (use (match_operand 3 "general_operand"))]
>>> +  ""
>>> +{
>>> +    rtx label_in = copy_to_reg (operands[1]);
>>> +    rtx fp_in = copy_to_reg (operands[3]);
>>> +    rtx sp_in = copy_to_reg (operands[2]);
>>> +
>>> +    emit_move_insn (hard_frame_pointer_rtx, fp_in);
>>> +    emit_stack_restore (SAVE_NONLOCAL, sp_in);
>>> +
>>> +    emit_use (hard_frame_pointer_rtx);
>>> +    emit_use (stack_pointer_rtx);
>>> +
>>> +    emit_indirect_jump (label_in);
>>> +
>>> +    DONE;
>>> +})
>>> +
>>>   ;; Helper for aarch64.c code.
>>>   (define_expand "set_clobber_cc"
>>>     [(parallel [(set (match_operand 0)
>>> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
>>> new file mode 100644
>>> index 0000000..564ef14
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
>>> @@ -0,0 +1,53 @@
>>> +/* { dg-require-effective-target indirect_jumps } */
>>> +
>>> +#include <string.h>
>>> +#include <alloca.h>
>>> +#include <setjmp.h>
>>> +
>>> +jmp_buf buf;
>>> +
>>> +int uses_longjmp (void)
>>> +{
>>> +  jmp_buf buf2;
>>> +  memcpy (buf2, buf, sizeof (buf));
>>> +  __builtin_longjmp (buf2, 1);
>>> +}
>>> +
>>> +int gl;
>>> +void after_longjmp (void)
>>> +{
>>> +  gl = 5;
>>> +}
>>> +
>>> +int
>>> +test_1 (int n)
>>> +{
>>> +  volatile int *p = alloca (n);
>>> +  if (__builtin_setjmp (buf))
>>> +    {
>>> +      after_longjmp ();
>>> +    }
>>> +  else
>>> +    {
>>> +      uses_longjmp ();
>>> +    }
>>> +
>>> +  return 0;
>>> +}
>>> +
>>> +int
>>> +test_2 (int n)
>>> +{
>>> +  int i;
>>> +  int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
>>> +  for (i = 0; i < n; i++)
>>> +    ptr[i] = i;
>>> +  test_1 (n);
>>> +  return 0;
>>> +}
>>> +
>>> +int main (int argc, const char **argv)
>>> +{
>>> +  __builtin_memset (&buf, 0xaf, sizeof (buf));
>>> +  test_2 (100);
>>> +}
>>