| Message ID | 4ee92fe8-3070-0129-59ad-40cbe0207822@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [AARCH64] PR target/84521 Fix frame pointer corruption with -fomit-frame-pointer with __builtin_setjmp | expand |
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); > +}
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); >> +} >
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
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 > >
> 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.
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. 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); +}
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.
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. >
> 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 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
> 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 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
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 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); +}
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); > +}
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); > > +} >
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); >>> +} >>
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); +}