Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true

[Segher Boessenkool]

Hi!

On Fri, Mar 27, 2020 at 09:34:00AM +0800, luoxhu wrote:
> On 2020/3/27 07:59, Segher Boessenkool wrote:
> > On Wed, Mar 25, 2020 at 11:15:22PM -0500, luo...@linux.ibm.com wrote:
> >> frame_pointer_needed is set to true in reload pass setup_can_eliminate,
> >> but regs_ever_live[31] is false, so pro_and_epilogue doesn't save/restore
> >> r31 even it is used actually, causing CPU2006 465.tonto segment fault of
> >> loading from invalid addresses.
> > 
> > If df_regs_ever_live_p(31) is false there is no hard frame pointer
> > anywhere in the program.  How can it be used then?
> 
> There is a piece of code emit move instruction to r31 even 
> df_regs_ever_live_p(31) is false
> in pro_and_epilogue.

Can you point out where (in rs6000-logue.c ot similar)?  We should fix
*that*.

> As frame_point_needed is true and frame_pointer_needed is widely
> used in this function, so I propose to save r31 in save_reg_p instead of check
> (frame_pointer_needed && df_regs_ever_live_p(31), I haven't verify whether 
> this works yet).
> Is this reasonable?  Thanks.

frame_pointer_needed is often true when the backend can figure out we do
not actually need it.

> rs6000-logue.c
> void
> rs6000_emit_prologue (void)
> {
> ...
> bbd21807fdf6 (geoffk         2000-03-16 03:16:41 +0000 26840)   /* Set frame 
> pointer, if needed.  */
> bbd21807fdf6 (geoffk         2000-03-16 03:16:41 +0000 26841)   if 
> (frame_pointer_needed)
> bbd21807fdf6 (geoffk         2000-03-16 03:16:41 +0000 26842)     {
> 0d6c02bf24e4 (jakub          2005-06-30 14:26:32 +0000 26843)       insn = 
> emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
> bbd21807fdf6 (geoffk         2000-03-16 03:16:41 +0000 26844)                 
>        sp_reg_rtx);
> bbd21807fdf6 (geoffk         2000-03-16 03:16:41 +0000 26845)       
> RTX_FRAME_RELATED_P (insn) = 1;
> 6b02f2a5c61e (meissner       1995-11-30 20:02:16 +0000 26846)     }
> d1bd513ed578 (kenner         1992-02-09 19:26:21 +0000 26847)
> ...
> }

Ah, so this you mean.  I see.  It looks like if you change this to

  if (frame_pointer_needed && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
    {
      insn = emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
                             sp_reg_rtx);
      RTX_FRAME_RELATED_P (insn) = 1;
    }

(so just that "if" clause changes), it'll all be fine.  Could you test
that please?

Thanks,


Segher