On 17/10/14 09:21, Eric Botcazou wrote:
> Hi,
>
> some OSes, for example VxWorks 6, still use DWARF unwinding on the ARM, which
> means that they use __builtin_eh_return (EABI unwinding doesn't). The
> builtin
> is implemented by means of {arm|thumb}_set_return_address, which can generate
> a store if LR has been stored on function entry. The problem is that, if
> this
> store is FP-based, it is not seen by the RTL DSE pass as being consumed by
> the
> SP-based load at the same address on function exit.
>
> That's by design in the RTL DSE pass: FP and SP are never substituted for
> each
> other by cselib, see for example this comment:
>
> /* The only thing that we are not willing to do (this
> is requirement of dse and if others potential uses
> need this function we should add a parm to control
> it) is that we will not substitute the
> STACK_POINTER_REGNUM, FRAME_POINTER or the
> HARD_FRAME_POINTER.
>
> These expansions confuses the code that notices that
> stores into the frame go dead at the end of the
> function and that the frame is not effected by calls
> to subroutines. If you allow the
> STACK_POINTER_REGNUM substitution, then dse will
> think that parameter pushing also goes dead which is
> wrong. If you allow the FRAME_POINTER or the
> HARD_FRAME_POINTER then you lose the opportunity to
> make the frame assumptions. */
> if (regno == STACK_POINTER_REGNUM
> || regno == FRAME_POINTER_REGNUM
> || regno == HARD_FRAME_POINTER_REGNUM
> || regno == cfa_base_preserved_regno)
> return orig;
>
> so a FP-based store and a SP-based load are never seen as a RAW dependency.
>
> This nevertheless used to work because the blockage insn emitted by the RTL
> epilogue was acting as a "wild load" but this got broken by Richard's patch:
>
> 2014-03-11 Richard Sandiford <[email protected]>
>
> * builtins.c (expand_builtin_setjmp_receiver): Use and clobber
> hard_frame_pointer_rtx.
> * cse.c (cse_insn): Remove volatile check.
> * cselib.c (cselib_process_insn): Likewise.
> * dse.c (scan_insn): Likewise.
>
> which removed the "wild load" trick. This is visible at -O2 for:
>
> void
> foo (void *c1, void *t1, void *ra)
> {
> long offset = uw_install_context_1 (c1, t1);
> void *handler = __builtin_frob_return_addr (ra);
> __builtin_unwind_init ();
> __builtin_eh_return (offset, handler);
> }
>
> The attached patch fixes the breakage by marking the stores as frame related.
>
> Tested on ARM/VxWorks, OK for mainline and 4.9 branch?
>
>
> 2014-10-17 Eric Botcazou <[email protected]>
>
> * config/arm/arm.c (arm_set_return_address): Mark the store as frame
> related, if any.
> (thumb_set_return_address): Likewise.
>
>
OK.
R.
>
> arm_eh_return-2.diff
>
>
> Index: config/arm/arm.c
> ===================================================================
> --- config/arm/arm.c (revision 216252)
> +++ config/arm/arm.c (working copy)
> @@ -28952,7 +28952,11 @@ arm_set_return_address (rtx source, rtx
>
> addr = plus_constant (Pmode, addr, delta);
> }
> - emit_move_insn (gen_frame_mem (Pmode, addr), source);
> + /* The store needs to be marked as frame related in order to prevent
> + DSE from deleting it as dead if it is based on fp. */
> + rtx insn = emit_move_insn (gen_frame_mem (Pmode, addr), source);
> + RTX_FRAME_RELATED_P (insn) = 1;
> + add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (Pmode, LR_REGNUM));
> }
> }
>
> @@ -29004,7 +29008,11 @@ thumb_set_return_address (rtx source, rt
> else
> addr = plus_constant (Pmode, addr, delta);
>
> - emit_move_insn (gen_frame_mem (Pmode, addr), source);
> + /* The store needs to be marked as frame related in order to prevent
> + DSE from deleting it as dead if it is based on fp. */
> + rtx insn = emit_move_insn (gen_frame_mem (Pmode, addr), source);
> + RTX_FRAME_RELATED_P (insn) = 1;
> + add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (Pmode, LR_REGNUM));
> }
> else
> emit_move_insn (gen_rtx_REG (Pmode, LR_REGNUM), source);
>
