PING Re: [PATCH] rs6000, unwind-on-each-instruction fix.

[Carl Love <cel@linux.ibm.com>]

Ping,

-------------------------------------------------------------------

On 1/25/24 13:35, Carl Love wrote:
> GDB maintainers:
> 
> The unwind-on-each-instruction test currently has 10 failures on Power.
> The test fails when executing the various tests for the instructions in
> the epilogue part of a function.  The function 
> rs6000_epilogue_frame_cache is called when executing the instructions
> in the epilogue.  The function assumes thatthe stack and the general 
> purpose registers (gpr) and link register (LR) have all been restored
> to their values on entry to the function.  The assumption is not correct.
> Only the stack pointer (SP), stored in gpr1, has been restored. The LR
> and the other gprs, specifically gpr31 which is also used to store the
> SP, may not have been restored. 
> 
> This patch adds code to update the rs6000_frame_cache in function 
> rs6000_epilogue_frame_cache with the rules to properly unroll the gprs
> and LR to their values on entry to the function thus fixing the various
> test failures.
> 
> The patch fixes all 10 regression failures for the test.  The patch
> has been tested on Power 9 and Power 10 with no additional regression
> test failures.
> 
> Please let me know if this patch is acceptable to GDB mainline.
> 
>                         Carl 
> --------------------------------------------------
> 
> rs6000, unwind-on-each-instruction fix.
> 
> The function rs6000_epilogue_frame_cache assumes the LR and gprs have been
> restored.  In fact register r31 and the link register, lr, may not have
> been restored yet.  This patch adds support to store the lr and gpr
> register unrolling rules in the cache.  The LR and GPR values can now be
> unrolled correctly.
> 
> Patch fixes all 10 regresion test failures for the unwind-on-each-insn.exp.
> ---
>  gdb/rs6000-tdep.c | 53 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index cc91250d5fe..e831c3748b5 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -3858,6 +3858,8 @@ rs6000_epilogue_frame_cache (frame_info_ptr this_frame, void **this_cache)
>    struct rs6000_frame_cache *cache;
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
> +  struct rs6000_framedata fdata;
> +  int wordsize = tdep->wordsize;
>  
>    if (*this_cache)
>      return (struct rs6000_frame_cache *) *this_cache;
> @@ -3868,17 +3870,56 @@ rs6000_epilogue_frame_cache (frame_info_ptr this_frame, void **this_cache)
>  
>    try
>      {
> -      /* At this point the stack looks as if we just entered the
> -	 function, and the return address is stored in LR.  */
> -      CORE_ADDR sp, lr;
> +      /* At this point the stack looks as if we just entered the function.
> +	 The SP (r1) has been restored but the LR and r31 may not have been
> +	 restored yet.  Need to update the register unrolling information in
> +	 the cache for the LR and the saved gprs.  */
> +      CORE_ADDR sp;
> +      CORE_ADDR func = 0, pc = 0;
>  
> -      sp = get_frame_register_unsigned (this_frame, gdbarch_sp_regnum (gdbarch));
> -      lr = get_frame_register_unsigned (this_frame, tdep->ppc_lr_regnum);
> +      func = get_frame_func (this_frame);
> +      cache->pc = func;
> +      pc = get_frame_pc (this_frame);
> +      skip_prologue (gdbarch, func, pc, &fdata);
> +
> +      /* SP is in r1 and it has been restored.  Get the current value.  */
> +      sp = get_frame_register_unsigned (this_frame,
> +					gdbarch_sp_regnum (gdbarch));
>  
>        cache->base = sp;
>        cache->initial_sp = sp;
>  
> -      cache->saved_regs[gdbarch_pc_regnum (gdbarch)].set_value (lr);
> +      /* Store the unwinding rules for the gpr registers that have not been
> +	 restored yet, specifically r31.
> +
> +	 if != -1, fdata.saved_gpr is the smallest number of saved_gpr.
> +	 All gpr's from saved_gpr to gpr31 are saved (except during the
> +	 prologue).  */
> +
> +      if (fdata.saved_gpr >= 0)
> +	{
> +	  int i;
> +	  CORE_ADDR gpr_addr = cache->base + fdata.gpr_offset;
> +
> +	  for(i = fdata.saved_gpr; i < ppc_num_gprs; i++)
> +	    {
> +	      if (fdata.gpr_mask & (1U << i))
> +		cache->saved_regs[tdep->ppc_gp0_regnum + i].set_addr (gpr_addr);
> +	      gpr_addr += wordsize;
> +	    }
> +	}
> +
> +      /* Store the lr unwinding rules.  */
> +      if (fdata.lr_offset != 0)
> +	cache->saved_regs[tdep->ppc_lr_regnum].set_addr (cache->base
> +							 + fdata.lr_offset);
> +
> +      else if (fdata.lr_register != -1)
> +	cache->saved_regs[tdep->ppc_lr_regnum].set_realreg (fdata.lr_register);
> +
> +      /* The PC is found in the link register.  */
> +      cache->saved_regs[gdbarch_pc_regnum (gdbarch)] =
> +	cache->saved_regs[tdep->ppc_lr_regnum];
>      }
>    catch (const gdb_exception_error &ex)
>      {