rs6000, unwind-on-each-instruction fix.

  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(-)
  

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)
>      {
  

>>>>> "Carl" == Carl Love <cel@linux.ibm.com> writes:

Carl> The patch fixes all 10 regression failures for the test.  The patch
Carl> has been tested on Power 9 and Power 10 with no additional regression
Carl> test failures.

Carl> Please let me know if this patch is acceptable to GDB mainline.

I think it would be best if someone familiar with PPC reviewed this.

FWIW it looks reasonable to me.

Carl> +      /* The PC is found in the link register.  */
Carl> +      cache->saved_regs[gdbarch_pc_regnum (gdbarch)] =
Carl> +	cache->saved_regs[tdep->ppc_lr_regnum];

Here the '=' should go on the 2nd line.

Tom
  

Tom:

On 2/8/24 10:55, Tom Tromey wrote:
>>>>>> "Carl" == Carl Love <cel@linux.ibm.com> writes:
> 
> Carl> The patch fixes all 10 regression failures for the test.  The patch
> Carl> has been tested on Power 9 and Power 10 with no additional regression
> Carl> test failures.
> 
> Carl> Please let me know if this patch is acceptable to GDB mainline.
> 
> I think it would be best if someone familiar with PPC reviewed this.
> 
> FWIW it looks reasonable to me.
> 
> Carl> +      /* The PC is found in the link register.  */
> Carl> +      cache->saved_regs[gdbarch_pc_regnum (gdbarch)] =
> Carl> +	cache->saved_regs[tdep->ppc_lr_regnum];
> 
> Here the '=' should go on the 2nd line.

Yes, the "=" is on the wrong line.  I fixed it locally.  Thanks for catching
that.  

Hopefully Ulrich will get a chance to look at the patch.  I think he
was out of the office for a few days.  I am thinking the patch got overlooked
with all of his other mail.  

                     Carl
  

Carl Love <cel@linux.ibm.com> wrote:

>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.

This is OK (with the cosmetic fix pointed out by Tom).

Thanks,
Ulrich