[ARC] Reimplement exception handling support.

This is patch which solves the ARC issues with exception handling support.

Ok to apply?
Claudiu

2016-06-09  Claudiu Zissulescu  <claziss@synopsys.com>
	    Andrew Burgess  <andrew.burgess@embecosm.com>

	* config/arc/arc-protos.h (arc_compute_frame_size): Delete
	declaration.
	(arc_return_slot_offset): Likewise.
	(arc_eh_return_address_location): New declaration.
	* config/arc/arc.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
	(MUST_SAVE_REGISTER): Add exception handler case.
	(MUST_SAVE_RETURN_ADDR): Likewise.
	(arc_frame_pointer_required): Likewise.
	(arc_frame_pointer_needed): New function.
	(arc_compute_frame_size): Changed.
	(arc_expand_prologue): Likewise.
	(arc_expand_epilogue): Likewise.
	(arc_initial_elimination_offset): Likewise.
	(arc_return_slot_offset): Delete.
	(arc_eh_return_address_location): New function.
	(arc_builtin_setjmp_frame_value): Likewise.
	* config/arc/arc.h (EH_RETURN_DATA_REGNO): Use 2 registers.
	(EH_RETURN_STACKADJ_RTX): Define.
	(EH_RETURN_HANDLER_RTX): Likewise.
	* config/arc/arc.md (eh_return): Delete.
---
 gcc/config/arc/arc-protos.h |   2 +-
 gcc/config/arc/arc.c        | 202 +++++++++++++++++++++++++++++++++++---------
 gcc/config/arc/arc.h        |   7 +-
 gcc/config/arc/arc.md       |  33 --------
 4 files changed, 166 insertions(+), 78 deletions(-)

* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2017-11-02 13:00:09 +0100]:

> This is patch which solves the ARC issues with exception handling support.
> 
> Ok to apply?

Mostly OK, my only comment is about the comments!  There's a couple of
places where the comments are phrased in terms of "this commit" and
"see changes".  Comments in the code should (I think) be phrased in
terms of how the code is right now (or at least after the changes is
merged), so:

    (see changes to arc_frame_pointer_required)

becomes

    (see arc_frame_pointer_required)

while:

    This issue is fixed in this commit too.

can probably be deleted completely.

Otherwise, I'm happy.

Thanks,
Andrew




> Claudiu
> 
> 2016-06-09  Claudiu Zissulescu  <claziss@synopsys.com>
> 	    Andrew Burgess  <andrew.burgess@embecosm.com>
> 
> 	* config/arc/arc-protos.h (arc_compute_frame_size): Delete
> 	declaration.
> 	(arc_return_slot_offset): Likewise.
> 	(arc_eh_return_address_location): New declaration.
> 	* config/arc/arc.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Define.
> 	(MUST_SAVE_REGISTER): Add exception handler case.
> 	(MUST_SAVE_RETURN_ADDR): Likewise.
> 	(arc_frame_pointer_required): Likewise.
> 	(arc_frame_pointer_needed): New function.
> 	(arc_compute_frame_size): Changed.
> 	(arc_expand_prologue): Likewise.
> 	(arc_expand_epilogue): Likewise.
> 	(arc_initial_elimination_offset): Likewise.
> 	(arc_return_slot_offset): Delete.
> 	(arc_eh_return_address_location): New function.
> 	(arc_builtin_setjmp_frame_value): Likewise.
> 	* config/arc/arc.h (EH_RETURN_DATA_REGNO): Use 2 registers.
> 	(EH_RETURN_STACKADJ_RTX): Define.
> 	(EH_RETURN_HANDLER_RTX): Likewise.
> 	* config/arc/arc.md (eh_return): Delete.
> ---
>  gcc/config/arc/arc-protos.h |   2 +-
>  gcc/config/arc/arc.c        | 202 +++++++++++++++++++++++++++++++++++---------
>  gcc/config/arc/arc.h        |   7 +-
>  gcc/config/arc/arc.md       |  33 --------
>  4 files changed, 166 insertions(+), 78 deletions(-)
> 
> diff --git a/gcc/config/arc/arc-protos.h b/gcc/config/arc/arc-protos.h
> index 1c7031c..6e7239f 100644
> --- a/gcc/config/arc/arc-protos.h
> +++ b/gcc/config/arc/arc-protos.h
> @@ -111,8 +111,8 @@ extern bool arc_epilogue_uses (int regno);
>  extern bool arc_eh_uses (int regno);
>  /* insn-attrtab.c doesn't include reload.h, which declares regno_clobbered_p. */
>  extern int regno_clobbered_p (unsigned int, rtx_insn *, machine_mode, int);
> -extern int arc_return_slot_offset (void);
>  extern bool arc_legitimize_reload_address (rtx *, machine_mode, int, int);
>  extern void arc_secondary_reload_conv (rtx, rtx, rtx, bool);
>  extern void arc_cpu_cpp_builtins (cpp_reader *);
>  extern bool arc_store_addr_hazard_p (rtx_insn *, rtx_insn *);
> +extern rtx arc_eh_return_address_location (void);
> diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
> index a0b66758..75d35cd 100644
> --- a/gcc/config/arc/arc.c
> +++ b/gcc/config/arc/arc.c
> @@ -597,6 +597,8 @@ static void arc_finalize_pic (void);
>  
>  #undef TARGET_MODES_TIEABLE_P
>  #define TARGET_MODES_TIEABLE_P arc_modes_tieable_p
> +#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
> +#define TARGET_BUILTIN_SETJMP_FRAME_VALUE arc_builtin_setjmp_frame_value
>  
>  /* Try to keep the (mov:DF _, reg) as early as possible so
>     that the d<add/sub/mul>h-lr insns appear together and can
> @@ -2556,8 +2558,7 @@ arc_compute_function_type (struct function *fun)
>     Addition for pic: The gp register needs to be saved if the current
>     function changes it to access gotoff variables.
>     FIXME: This will not be needed if we used some arbitrary register
> -   instead of r26.
> -*/
> +   instead of r26.  */
>  
>  static bool
>  arc_must_save_register (int regno, struct function *func)
> @@ -2620,14 +2621,51 @@ arc_must_save_return_addr (struct function *func)
>  /* Helper function to wrap FRAME_POINTER_NEEDED.  We do this as
>     FRAME_POINTER_NEEDED will not be true until the IRA (Integrated
>     Register Allocator) pass, while we want to get the frame size
> -   correct earlier than the IRA pass.  */
> +   correct earlier than the IRA pass.
> +
> +   When a function uses eh_return we must ensure that the fp register
> +   is saved and then restored so that the unwinder can restore the
> +   correct value for the frame we are going to jump to.
> +
> +   To do this we force all frames that call eh_return to require a
> +   frame pointer (see changes to arc_frame_pointer_required), this
> +   will ensure that the previous frame pointer is stored on entry to
> +   the function, and will then be reloaded at function exit.
> +
> +   As the frame pointer is handled as a special case in our prologue
> +   and epilogue code it must not be saved and restored using the
> +   MUST_SAVE_REGISTER mechanism otherwise we run into issues where GCC
> +   believes that the function is not using a frame pointer and that
> +   the value in the fp register is the frame pointer, while the
> +   prologue and epilogue are busy saving and restoring the fp
> +   register.  This issue is fixed in this commit too.
> +
> +   During compilation of a function the frame size is evaluated
> +   multiple times, it is not until the reload pass is complete the the
> +   frame size is considered fixed (it is at this point that space for
> +   all spills has been allocated).  However the frame_pointer_needed
> +   variable is not set true until the register allocation pass, as a
> +   result in the early stages the frame size does not include space
> +   for the frame pointer to be spilled.
> +
> +   The problem that this causes, that I have not yet tracked down, is
> +   that the rtl generated for EH_RETURN_HANDLER_RTX uses the details
> +   of the frame size to compute the offset from the frame pointer at
> +   which the return address lives.  However, in early passes GCC has
> +   not yet realised we need a frame pointer, and so has not included
> +   space for the frame pointer in the frame size, and so gets the
> +   offset of the return address wrong.  This should not be an issue as
> +   in later passes GCC has realised that the frame pointer needs to be
> +   spilled, and has increased the frame size.  However, the rtl for
> +   the EH_RETURN_HANDLER_RTX is not regenerated to use the newer,
> +   larger offset, and the wrong smaller offset is used.  */
> +
>  static bool
>  arc_frame_pointer_needed (void)
>  {
> -  return (frame_pointer_needed);
> +  return (frame_pointer_needed || crtl->calls_eh_return);
>  }
>  
> -
>  /* Return non-zero if there are registers to be saved or loaded using
>     millicode thunks.  We can only use consecutive sequences starting
>     with r13, and not going beyond r25.
> @@ -2658,26 +2696,32 @@ arc_compute_millicode_save_restore_regs (unsigned int gmask,
>    return 0;
>  }
>  
> -/* Return the bytes needed to compute the frame pointer from the current
> -   stack pointer.
> +/* Return the bytes needed to compute the frame pointer from the
> +   current stack pointer.  */
>  
> -   SIZE is the size needed for local variables.  */
> -
> -unsigned int
> -arc_compute_frame_size (int size)	/* size = # of var. bytes allocated.  */
> +static unsigned int
> +arc_compute_frame_size (void)
>  {
>    int regno;
>    unsigned int total_size, var_size, args_size, pretend_size, extra_size;
>    unsigned int reg_size, reg_offset;
>    unsigned int gmask;
> -  struct arc_frame_info *frame_info = &cfun->machine->frame_info;
> +  enum arc_function_type fn_type;
> +  int interrupt_p;
> +  struct arc_frame_info *frame_info;
> +  int size;
> +
> +  /* The answer might already be known.  */
> +  if (cfun->machine->frame_info.initialized)
> +    return cfun->machine->frame_info.total_size;
>  
> -  size = ARC_STACK_ALIGN (size);
> +  frame_info = &cfun->machine->frame_info;
> +  size = ARC_STACK_ALIGN (get_frame_size ());
>  
> -  /* 1) Size of locals and temporaries */
> +  /* 1) Size of locals and temporaries.  */
>    var_size	= size;
>  
> -  /* 2) Size of outgoing arguments */
> +  /* 2) Size of outgoing arguments.  */
>    args_size	= crtl->outgoing_args_size;
>  
>    /* 3) Calculate space needed for saved registers.
> @@ -2698,12 +2742,29 @@ arc_compute_frame_size (int size)	/* size = # of var. bytes allocated.  */
>  	}
>      }
>  
> +  /* In a frame that calls __builtin_eh_return two data registers are
> +     used to pass values back to the exception handler.
> +
> +     Ensure that these registers are spilled to the stack so that the
> +     exception throw code can find them, and update the saved values.
> +     The handling code will then consume these reloaded values to
> +     handle the exception.  */
> +  if (crtl->calls_eh_return)
> +    for (regno = 0; EH_RETURN_DATA_REGNO (regno) != INVALID_REGNUM; regno++)
> +      {
> +	reg_size += UNITS_PER_WORD;
> +	gmask |= 1 << regno;
> +      }
> +
>    /* 4) Space for back trace data structure.
>  	<return addr reg size> (if required) + <fp size> (if required).  */
>    frame_info->save_return_addr
> -    = (!crtl->is_leaf || df_regs_ever_live_p (RETURN_ADDR_REGNUM));
> +    = (!crtl->is_leaf || df_regs_ever_live_p (RETURN_ADDR_REGNUM)
> +       || crtl->calls_eh_return);
>    /* Saving blink reg in case of leaf function for millicode thunk calls.  */
> -  if (optimize_size && !TARGET_NO_MILLICODE_THUNK_SET)
> +  if (optimize_size
> +      && !TARGET_NO_MILLICODE_THUNK_SET
> +      && !crtl->calls_eh_return)
>      {
>        if (arc_compute_millicode_save_restore_regs (gmask, frame_info))
>  	frame_info->save_return_addr = true;
> @@ -2731,7 +2792,11 @@ arc_compute_frame_size (int size)	/* size = # of var. bytes allocated.  */
>    /* Compute total frame size.  */
>    total_size = var_size + args_size + extra_size + pretend_size + reg_size;
>  
> -  total_size = ARC_STACK_ALIGN (total_size);
> +  /* It used to be the case that the alignment was forced at this
> +     point.  However, that is dangerous, calculations based on
> +     total_size would be wrong.  Given that this has never cropped up
> +     as an issue I've changed this to an assert for now.  */
> +  gcc_assert (total_size == ARC_STACK_ALIGN (total_size));
>  
>    /* Compute offset of register save area from stack pointer:
>       Frame: pretend_size <blink> reg_size <fp> var_size args_size <--sp
> @@ -2999,7 +3064,7 @@ arc_dwarf_emit_irq_save_regs (void)
>  void
>  arc_expand_prologue (void)
>  {
> -  int size = get_frame_size ();
> +  int size;
>    unsigned int gmask = cfun->machine->frame_info.gmask;
>    /*  unsigned int frame_pointer_offset;*/
>    unsigned int frame_size_to_allocate;
> @@ -3013,12 +3078,8 @@ arc_expand_prologue (void)
>    if (ARC_NAKED_P (fn_type))
>      return;
>  
> -  size = ARC_STACK_ALIGN (size);
> -
> -  /* Compute/get total frame size.  */
> -  size = (!cfun->machine->frame_info.initialized
> -	   ? arc_compute_frame_size (size)
> -	   : cfun->machine->frame_info.total_size);
> +  /* Compute total frame size.  */
> +  size = arc_compute_frame_size ();
>  
>    if (flag_stack_usage_info)
>      current_function_static_stack_size = size;
> @@ -3121,13 +3182,10 @@ arc_expand_prologue (void)
>  void
>  arc_expand_epilogue (int sibcall_p)
>  {
> -  int size = get_frame_size ();
> +  int size;
>    unsigned int fn_type = arc_compute_function_type (cfun);
>  
> -  size = ARC_STACK_ALIGN (size);
> -  size = (!cfun->machine->frame_info.initialized
> -	   ? arc_compute_frame_size (size)
> -	   : cfun->machine->frame_info.total_size);
> +  size = arc_compute_frame_size ();
>  
>    unsigned int pretend_size = cfun->machine->frame_info.pretend_size;
>    unsigned int frame_size;
> @@ -3295,21 +3353,59 @@ arc_expand_epilogue (int sibcall_p)
>    if (size > restored)
>      frame_stack_add (size - restored);
>  
> +  /* For frames that use __builtin_eh_return, the register defined by
> +     EH_RETURN_STACKADJ_RTX is set to 0 for all standard return paths.
> +     On eh_return paths however, the register is set to the value that
> +     should be added to the stack pointer in order to restore the
> +     correct stack pointer for the exception handling frame.
> +
> +     For ARC we are going to use r2 for EH_RETURN_STACKADJ_RTX, add
> +     this onto the stack for eh_return frames.  */
> +  if (crtl->calls_eh_return)
> +    emit_insn (gen_add2_insn (stack_pointer_rtx,
> +			      EH_RETURN_STACKADJ_RTX));
> +
>    /* Emit the return instruction.  */
>    if (sibcall_p == FALSE)
>      emit_jump_insn (gen_simple_return ());
>  }
>  
> -/* Return the offset relative to the stack pointer where the return address
> -   is stored, or -1 if it is not stored.  */
> -
> -int
> -arc_return_slot_offset ()
> -{
> -  struct arc_frame_info *afi = &cfun->machine->frame_info;
> +/* Return rtx for the location of the return address on the stack,
> +   suitable for use in __builtin_eh_return.  The new return address
> +   will be written to this location in order to redirect the return to
> +   the exception handler.  */
>  
> -  return (afi->save_return_addr
> -	  ? afi->total_size - afi->pretend_size - afi->extra_size : -1);
> +rtx
> +arc_eh_return_address_location (void)
> +{
> +  rtx mem;
> +  int offset;
> +  struct arc_frame_info *afi;
> +
> +  arc_compute_frame_size ();
> +  afi = &cfun->machine->frame_info;
> +
> +  gcc_assert (crtl->calls_eh_return);
> +  gcc_assert (afi->save_return_addr);
> +  gcc_assert (afi->extra_size >= 4);
> +
> +  /* The '-4' removes the size of the return address, which is
> +     included in the 'extra_size' field.  */
> +  offset = afi->reg_size + afi->extra_size - 4;
> +  mem = gen_frame_mem (Pmode,
> +		       plus_constant (Pmode, frame_pointer_rtx, offset));
> +
> +  /* The following should not be needed, and is, really a hack.  The
> +     issue being worked around here is that the DSE (Dead Store
> +     Elimination) pass will remove this write to the stack as it sees
> +     a single store and no corresponding read.  The read however
> +     occurs in the epilogue code, which is not added into the function
> +     rtl until a later pass.  So, at the time of DSE, the decision to
> +     remove this store seems perfectly sensible.  Marking the memory
> +     address as volatile obviously has the effect of preventing DSE
> +     from removing the store.  */
> +  MEM_VOLATILE_P (mem) = 1;
> +  return mem;
>  }
>  
>  /* PIC */
> @@ -4807,8 +4903,8 @@ arc_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
>  int
>  arc_initial_elimination_offset (int from, int to)
>  {
> -  if (! cfun->machine->frame_info.initialized)
> -     arc_compute_frame_size (get_frame_size ());
> +  if (!cfun->machine->frame_info.initialized)
> +    arc_compute_frame_size ();
>  
>    if (from == ARG_POINTER_REGNUM && to == FRAME_POINTER_REGNUM)
>      {
> @@ -4836,7 +4932,7 @@ arc_initial_elimination_offset (int from, int to)
>  static bool
>  arc_frame_pointer_required (void)
>  {
> - return cfun->calls_alloca;
> + return cfun->calls_alloca || crtl->calls_eh_return;
>  }
>  
>  
> @@ -10633,6 +10729,28 @@ compact_memory_operand_p (rtx op, machine_mode mode,
>    return false;
>  }
>  
> +/* Return the frame pointer value to be backed up in the setjmp buffer.  */
> +
> +static rtx
> +arc_builtin_setjmp_frame_value (void)
> +{
> +  /* We always want to preserve whatever value is currently in the frame
> +     pointer register.  For frames that are using the frame pointer the new
> +     value of the frame pointer register will have already been computed
> +     (as part of the prologue).  For frames that are not using the frame
> +     pointer it is important that we backup whatever value is in the frame
> +     pointer register, as earlier (more outer) frames may have placed a
> +     value into the frame pointer register.  It might be tempting to try
> +     and use `frame_pointer_rtx` here, however, this is not what we want.
> +     For frames that are using the frame pointer this will give the
> +     correct value.  However, for frames that are not using the frame
> +     pointer this will still give the value that _would_ have been the
> +     frame pointer value for this frame (if the use of the frame pointer
> +     had not been removed).  We really do want the raw frame pointer
> +     register value.  */
> +  return gen_raw_REG (Pmode, FRAME_POINTER_REGNUM);
> +}
> +
>  /* Implement TARGET_USE_ANCHORS_FOR_SYMBOL_P.  We don't want to use
>     anchors for small data: the GP register acts as an anchor in that
>     case.  We also don't want to use them for PC-relative accesses,
> diff --git a/gcc/config/arc/arc.h b/gcc/config/arc/arc.h
> index 1d9063a..b1458dc 100644
> --- a/gcc/config/arc/arc.h
> +++ b/gcc/config/arc/arc.h
> @@ -1365,8 +1365,11 @@ do { \
>  
>  /* Frame info.  */
>  
> -#define EH_RETURN_DATA_REGNO(N)	\
> -  ((N) < 4 ? (N) : INVALID_REGNUM)
> +#define EH_RETURN_DATA_REGNO(N)  ((N) < 2 ? (N) : INVALID_REGNUM)
> +
> +#define EH_RETURN_STACKADJ_RTX   gen_rtx_REG (Pmode, 2)
> +
> +#define EH_RETURN_HANDLER_RTX    arc_eh_return_address_location ()
>  
>  /* Turn off splitting of long stabs.  */
>  #define DBX_CONTIN_LENGTH 0
> diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
> index c766306..faf6698 100644
> --- a/gcc/config/arc/arc.md
> +++ b/gcc/config/arc/arc.md
> @@ -4935,39 +4935,6 @@
>  	       (const_int 4)]
>  	      (const_int 2)))])
>  
> -(define_insn_and_split "eh_return"
> -  [(eh_return)
> -   (use (match_operand:SI 0 "move_src_operand" "rC32,mCalCpc"))
> -   (clobber (match_scratch:SI 1  "=X,r"))
> -   (clobber (match_scratch:SI 2 "=&r,r"))]
> -  ""
> -  "#"
> -  "reload_completed"
> -  [(set (match_dup 2) (match_dup 0))]
> -{
> -  int offs = arc_return_slot_offset ();
> -
> -  if (offs < 0)
> -    operands[2] = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
> -  else
> -    {
> -      if (!register_operand (operands[0], Pmode)
> -	  && !satisfies_constraint_C32 (operands[0]))
> -	{
> -	  emit_move_insn (operands[1], operands[0]);
> -	  operands[0] = operands[1];
> -	}
> -      rtx addr = plus_constant (Pmode, stack_pointer_rtx, offs);
> -      if (!strict_memory_address_p (Pmode, addr))
> -	{
> -	  emit_move_insn (operands[2], addr);
> -	  addr = operands[2];
> -	}
> -      operands[2] = gen_frame_mem (Pmode, addr);
> -    }
> -}
> -  [(set_attr "length" "12")])
> -
>  ;; ??? #ifdefs in function.c require the presence of this pattern, with a
>  ;; non-constant predicate.
>  (define_expand "return"
> -- 
> 1.9.1
>

> Mostly OK, my only comment is about the comments!  There's a couple of
> places where the comments are phrased in terms of "this commit" and
> "see changes".  Comments in the code should (I think) be phrased in
> terms of how the code is right now (or at least after the changes is
> merged), so:
> 
>     (see changes to arc_frame_pointer_required)
> 
> becomes
> 
>     (see arc_frame_pointer_required)
> 
> while:
> 
>     This issue is fixed in this commit too.
> 
> can probably be deleted completely.
> 

Committed with the additional suggestions. Thank you for your review,
Claudiu