Re: [PATCH][AArch64 - v4] Simplify eh_return implementation

[Wilco Dijkstra <Wilco dot Dijkstra at arm dot com>]

Here is the updated version:

This patch simplifies the handling of the EH return value.  We force the use of the
frame pointer so the return location is always at FP + 8.  This means we can emit
a simple volatile access in EH_RETURN_HANDLER_RTX without needing md
patterns, splitters and frame offset calculations.  The new implementation also
fixes various bugs in aarch64_final_eh_return_addr, which does not work with
-fomit-frame-pointer, alloca or outgoing arguments.

Bootstrap OK, GCC Regression OK, OK for trunk? Would it be useful to backport
this to GCC6.x?

ChangeLog:

2017-01-16  Wilco Dijkstra  <wdijkstr@arm.com>

        PR77455
gcc/
        * config/aarch64/aarch64.md (eh_return): Remove pattern and splitter.
        * config/aarch64/aarch64.h (AARCH64_EH_STACKADJ_REGNUM): Remove.
        (EH_RETURN_HANDLER_RTX): New define.
        * config/aarch64/aarch64.c (aarch64_frame_pointer_required):
        Force frame pointer in EH return functions.
        (aarch64_expand_epilogue): Add barrier for eh_return.
        (aarch64_final_eh_return_addr): Remove.
        (aarch64_eh_return_handler_rtx): New function.
        * config/aarch64/aarch64-protos.h (aarch64_final_eh_return_addr):
        Remove.
        (aarch64_eh_return_handler_rtx): New prototype.

testsuite/
        * gcc.target/aarch64/eh_return.c: New test.
--

diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 29a3bd71151aa4fb7c6728f0fb52e2f3f233f41d..602f54f934a9b62e782154e7f19cafe3b1bf0481 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -358,7 +358,7 @@ int aarch64_hard_regno_mode_ok (unsigned, machine_mode);
 int aarch64_hard_regno_nregs (unsigned, machine_mode);
 int aarch64_uxt_size (int, HOST_WIDE_INT);
 int aarch64_vec_fpconst_pow_of_2 (rtx);
-rtx aarch64_final_eh_return_addr (void);
+rtx aarch64_eh_return_handler_rtx (void);
 rtx aarch64_mask_from_zextract_ops (rtx, rtx);
 const char *aarch64_output_move_struct (rtx *operands);
 rtx aarch64_return_addr (int, rtx);
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 5aee9eb5b42d200343484ac4d18d650d26027078..924376223f52e9f614ea2b4bc12bd24d2ab2720a 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -400,9 +400,9 @@ extern unsigned aarch64_architecture_version;
 #define ASM_DECLARE_FUNCTION_NAME(STR, NAME, DECL)	\
   aarch64_declare_function_name (STR, NAME, DECL)
 
-/* The register that holds the return address in exception handlers.  */
-#define AARCH64_EH_STACKADJ_REGNUM	(R0_REGNUM + 4)
-#define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, AARCH64_EH_STACKADJ_REGNUM)
+/* For EH returns X4 contains the stack adjustment.  */
+#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.  */
 #undef DONT_USE_BUILTIN_SETJMP
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 4e5fad912ba24ffeca36e2d601b67e09c005bb54..d2585dc6eb14fc82c952ca8dbeda445d249fdb9b 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -2762,6 +2762,10 @@ aarch64_frame_pointer_required (void)
       && (!crtl->is_leaf || df_regs_ever_live_p (LR_REGNUM)))
     return true;
 
+  /* Force a frame pointer for EH returns so the return address is at FP+8.  */
+  if (crtl->calls_eh_return)
+    return true;
+
   return false;
 }
 
@@ -3623,7 +3627,8 @@ aarch64_expand_epilogue (bool for_sibcall)
 			 + cfun->machine->frame.saved_varargs_size) != 0;
 
   /* Emit a barrier to prevent loads from a deallocated stack.  */
-  if (final_adjust > crtl->outgoing_args_size || cfun->calls_alloca)
+  if (final_adjust > crtl->outgoing_args_size || cfun->calls_alloca
+      || crtl->calls_eh_return)
     {
       emit_insn (gen_stack_tie (stack_pointer_rtx, stack_pointer_rtx));
       need_barrier_p = false;
@@ -3691,52 +3696,40 @@ aarch64_expand_epilogue (bool for_sibcall)
     emit_jump_insn (ret_rtx);
 }
 
-/* Return the place to copy the exception unwinding return address to.
-   This will probably be a stack slot, but could (in theory be the
-   return register).  */
-rtx
-aarch64_final_eh_return_addr (void)
-{
-  HOST_WIDE_INT fp_offset;
-
-  aarch64_layout_frame ();
+/* Implement EH_RETURN_HANDLER_RTX.  EH returns need to either return
+   normally or return to a previous frame after unwinding.
 
-  fp_offset = cfun->machine->frame.frame_size
-	      - cfun->machine->frame.hard_fp_offset;
+   An EH return uses a single shared return sequence.  The epilogue is
+   exactly like a normal epilogue except that it has an extra input
+   register (EH_RETURN_STACKADJ_RTX) which contains the stack adjustment
+   that must be applied after the frame has been destroyed.  An extra label
+   is inserted before the epilogue which initializes this register to zero,
+   and this is the entry point for a normal return.
 
-  if (cfun->machine->frame.reg_offset[LR_REGNUM] < 0)
-    return gen_rtx_REG (DImode, LR_REGNUM);
+   An actual EH return updates the return address, initializes the stack
+   adjustment and jumps directly into the epilogue (bypassing the zeroing
+   of the adjustment).  Since the return address is typically saved on the
+   stack when a function makes a call, the saved LR must be updated outside
+   the epilogue.
 
-  /* DSE and CSELIB do not detect an alias between sp+k1 and fp+k2.  This can
-     result in a store to save LR introduced by builtin_eh_return () being
-     incorrectly deleted because the alias is not detected.
-     So in the calculation of the address to copy the exception unwinding
-     return address to, we note 2 cases.
-     If FP is needed and the fp_offset is 0, it means that SP = FP and hence
-     we return a SP-relative location since all the addresses are SP-relative
-     in this case.  This prevents the store from being optimized away.
-     If the fp_offset is not 0, then the addresses will be FP-relative and
-     therefore we return a FP-relative location.  */
+   This poses problems as the store is generated well before the epilogue,
+   so the offset of LR is not known yet.  Also optimizations will remove the
+   store as it appears dead, even after the epilogue is generated (as the
+   base or offset for loading LR is different in many cases).
 
-  if (frame_pointer_needed)
-    {
-      if (fp_offset)
-        return gen_frame_mem (DImode,
-			      plus_constant (Pmode, hard_frame_pointer_rtx, UNITS_PER_WORD));
-      else
-        return gen_frame_mem (DImode,
-			      plus_constant (Pmode, stack_pointer_rtx, UNITS_PER_WORD));
-    }
-
-  /* If FP is not needed, we calculate the location of LR, which would be
-     at the top of the saved registers block.  */
+   To avoid these problems this implementation forces the frame pointer
+   in eh_return functions so that the location of LR is fixed and known early.
+   It also marks the store volatile, so no optimization is permitted to
+   remove the store.  */
+rtx
+aarch64_eh_return_handler_rtx (void)
+{
+  rtx tmp = gen_frame_mem (Pmode,
+    plus_constant (Pmode, hard_frame_pointer_rtx, UNITS_PER_WORD));
 
-  return gen_frame_mem (DImode,
-			plus_constant (Pmode,
-				       stack_pointer_rtx,
-				       fp_offset
-				       + cfun->machine->frame.saved_regs_size
-				       - 2 * UNITS_PER_WORD));
+  /* Mark the store volatile, so no optimization is permitted to remove it.  */
+  MEM_VOLATILE_P (tmp) = true;
+  return tmp;
 }
 
 /* Output code to add DELTA to the first argument, and then jump
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index bde42317f1bfd91a9d38a4cfa94d4cedd5246003..b3be106ac6ff1fb77bdc05cb4b0e57102787b87c 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -592,25 +592,6 @@ (define_insn "simple_return"
   [(set_attr "type" "branch")]
 )
 
-(define_insn "eh_return"
-  [(unspec_volatile [(match_operand:DI 0 "register_operand" "r")]
-    UNSPECV_EH_RETURN)]
-  ""
-  "#"
-  [(set_attr "type" "branch")]
-
-)
-
-(define_split
-  [(unspec_volatile [(match_operand:DI 0 "register_operand" "")]
-    UNSPECV_EH_RETURN)]
-  "reload_completed"
-  [(set (match_dup 1) (match_dup 0))]
-  {
-    operands[1] = aarch64_final_eh_return_addr ();
-  }
-)
-
 (define_insn "*cb<optab><mode>1"
   [(set (pc) (if_then_else (EQL (match_operand:GPI 0 "register_operand" "r")
 				(const_int 0))
diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return.c b/gcc/testsuite/gcc.target/aarch64/eh_return.c
new file mode 100644
index 0000000000000000000000000000000000000000..32179488085ed4c84aa77007565c875d09c4197c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/eh_return.c
@@ -0,0 +1,82 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-inline" } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+int val, test, failed;
+
+int main (void);
+
+void
+eh0 (void *p)
+{
+  val = (int)(long)p & 7;
+  if (val)
+    abort ();
+}
+
+void
+eh1 (void *p, int x)
+{
+  void *q = __builtin_alloca (x);
+  eh0 (q);
+  __builtin_eh_return (0, p);
+}
+
+void
+eh2a (int a,int b,int c,int d,int e,int f,int g,int h, void *p)
+{
+  val = a + b + c + d + e + f + g + h +  (int)(long)p & 7;
+}
+
+void
+eh2 (void *p)
+{
+  eh2a (val, val, val, val, val, val, val, val, p);
+  __builtin_eh_return (0, p);
+}
+
+
+void
+continuation (void)
+{
+  test++;
+  main ();
+}
+
+void
+fail (void)
+{
+  failed = 1;
+  printf ("failed\n");
+  continuation ();
+}
+
+void
+do_test1 (void)
+{
+  if (!val)
+    eh1 (continuation, 100);
+  fail ();
+}
+
+void
+do_test2 (void)
+{
+  if (!val)
+    eh2 (continuation);
+  fail ();
+}
+
+int
+main (void)
+{
+  if (test == 0)
+    do_test1 ();
+  if (test == 1)
+    do_test2 ();
+  if (failed || test != 2)
+    exit (1);
+  exit (0);
+}