Re: [PATCH v2] gdb: specify sh pointer register types

[Luis Machado <luis.machado@arm.com>]

Hi,


On 4/1/24 10:55, Sébastien Michelland wrote:
> This patch fixes a pretty funny issue on sh targets that occurred
> because $pc (and similar registers) were typed as int. When $pc is in
> the upper half of the address space (i.e. kernel code on sh), `x/i $pc'
> would resolve to a negative value. At least in the case of a remote
> target with an Xfer memory map, this leads to a spurious "cannot access
> memory" error as negative addresses are out of bounds.
> 
> (gdb) x/i $pc
>     0x8c202c04:    Cannot access memory at address 0x8c202c04
> (gdb) x/i 0x8c202c04
> => 0x8c202c04 <gintctl_gint_gdb+304>:    mov.l    @r1,r10
> 
> The issue is fixed by specifying pointer types for pc and other pointer
> registers. Code pointer registers on sh include pc, pr (return address
> of a call), vbr (interrupt handler) and spc (return address after
> interrupt). Data pointers include r15 (stack pointer) and gbr (base
> register for a few specific addressing modes).
> ---
> 
> Compared to v1, this patch applies to all sh architectures. It also adds
> r15 (stack pointer) as a data pointer and sets gbr to a data rather than
> code pointer (which was a mistake).
> 
>  gdb/sh-tdep.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
> index 1c67ea42..b76efa3a 100644
> --- a/gdb/sh-tdep.c
> +++ b/gdb/sh-tdep.c
> @@ -1400,6 +1400,11 @@ sh_sh2a_register_type (struct gdbarch *gdbarch, int reg_nr)
>      return builtin_type (gdbarch)->builtin_float;
>    else if (reg_nr >= DR0_REGNUM && reg_nr <= DR_LAST_REGNUM)
>      return builtin_type (gdbarch)->builtin_double;
> +  else if (reg_nr == PC_REGNUM || reg_nr == PR_REGNUM || reg_nr == VBR_REGNUM
> +      || reg_nr == SPC_REGNUM)
> +    return builtin_type (gdbarch)->builtin_func_ptr;
> +  else if (reg_nr == R0_REGNUM+15 || reg_nr == GBR_REGNUM)
> +    return builtin_type (gdbarch)->builtin_data_ptr;
>    else
>      return builtin_type (gdbarch)->builtin_int;
>  }
> @@ -1412,6 +1417,11 @@ sh_sh3e_register_type (struct gdbarch *gdbarch, int reg_nr)
>    if ((reg_nr >= gdbarch_fp0_regnum (gdbarch)
>         && (reg_nr <= FP_LAST_REGNUM)) || (reg_nr == FPUL_REGNUM))
>      return builtin_type (gdbarch)->builtin_float;
> +  else if (reg_nr == PC_REGNUM || reg_nr == PR_REGNUM || reg_nr == VBR_REGNUM
> +      || reg_nr == SPC_REGNUM)
> +    return builtin_type (gdbarch)->builtin_func_ptr;
> +  else if (reg_nr == R0_REGNUM+15 || reg_nr == GBR_REGNUM)
> +    return builtin_type (gdbarch)->builtin_data_ptr;
>    else
>      return builtin_type (gdbarch)->builtin_int;
>  }
> @@ -1433,6 +1443,11 @@ sh_sh4_register_type (struct gdbarch *gdbarch, int reg_nr)
>      return builtin_type (gdbarch)->builtin_double;
>    else if (reg_nr >= FV0_REGNUM && reg_nr <= FV_LAST_REGNUM)
>      return sh_sh4_build_float_register_type (gdbarch, 3);
> +  else if (reg_nr == PC_REGNUM || reg_nr == PR_REGNUM || reg_nr == VBR_REGNUM
> +      || reg_nr == SPC_REGNUM)
> +    return builtin_type (gdbarch)->builtin_func_ptr;
> +  else if (reg_nr == R0_REGNUM+15 || reg_nr == GBR_REGNUM)
> +    return builtin_type (gdbarch)->builtin_data_ptr;
>    else
>      return builtin_type (gdbarch)->builtin_int;
>  }
> @@ -1440,7 +1455,13 @@ sh_sh4_register_type (struct gdbarch *gdbarch, int reg_nr)
>  static struct type *
>  sh_default_register_type (struct gdbarch *gdbarch, int reg_nr)
>  {
> -  return builtin_type (gdbarch)->builtin_int;
> +  if (reg_nr == PC_REGNUM || reg_nr == PR_REGNUM || reg_nr == VBR_REGNUM
> +      || reg_nr == SPC_REGNUM)
> +    return builtin_type (gdbarch)->builtin_func_ptr;
> +  else if (reg_nr == R0_REGNUM+15 || reg_nr == GBR_REGNUM)
> +    return builtin_type (gdbarch)->builtin_data_ptr;
> +  else
> +    return builtin_type (gdbarch)->builtin_int;
>  }
>  
>  /* Is a register in a reggroup?

This looks OK to me. Unfortunately it doesn't look like we have
an active maintainer for SH, so testing might be a bit limited.

Sounds like Simon had already OK-ed your approach. In any case,
are you sure all the GPR's should also be typed as data pointer?

Should those be unsigned integers maybe?