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

[Andrew Pinski <pinskia@gmail.com>]

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