[PATCH v2] gdb: specify sh pointer register types

[PATCH v2] gdb: specify sh pointer register types

[Sébastien Michelland]

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?
-- 
2.44.0

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

[Luis Machado]

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?

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

[Sébastien Michelland]

Hi Luis,

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

I'm not sure I understand; the only GPR that I make a data pointer is 
r15 (the stack pointer). The others remain signed int.

If you mean gbr then its only use (besides reading/writing it directly) 
is as the base for the so-called global addressing modes:

mov.{b,w,l} @(disp, gbr), r0    # r0 = *(gbr + disp)
mov.{b,w,l} r0, @(disp, gbr)    # *(gbr + disp) = r0

As the displacements have a very limited range, this requires gbr to be 
a pointer. My understanding is that it's intended for global pointers 
such as the thread pointer or GOT (for which there is no reserved 
register on SH).

Sébastien

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

[Luis Machado]

On 4/10/24 08:03, Sébastien Michelland wrote:
> Hi Luis,
> 
>> 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?
> 
> I'm not sure I understand; the only GPR that I make a data pointer is r15 (the stack pointer). The others remain signed int.

Ah, sorry. I misread the code as handling all GPR's until 15. So you can ignore my comment.

> 
> If you mean gbr then its only use (besides reading/writing it directly) is as the base for the so-called global addressing modes:
> 
> mov.{b,w,l} @(disp, gbr), r0    # r0 = *(gbr + disp)
> mov.{b,w,l} r0, @(disp, gbr)    # *(gbr + disp) = r0
> 
> As the displacements have a very limited range, this requires gbr to be a pointer. My understanding is that it's intended for global pointers such as the thread pointer or GOT (for which there is no reserved register on SH).

Indeed.

> 
> Sébastien

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

[Sébastien Michelland]

Hi everyone, kindly pinging for process for this patch.

Simon approved the approach on a previous version but this one still 
needs validation, and ultimately a push if accepted.

I understand there is little maintenance going on for the sh target. Is 
there additional testing I should do to help with this patch?

On 2024-04-01 11: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?

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

[Simon Marchi]

On 2024-04-26 04:13, Sébastien Michelland wrote:
> Hi everyone, kindly pinging for process for this patch.
> 
> Simon approved the approach on a previous version but this one still needs validation, and ultimately a push if accepted.
> 
> I understand there is little maintenance going on for the sh target. Is there additional testing I should do to help with this patch?

Hi Sébastien,

I went ahead and pushed the patch.  You don't seem to have on the
copyright assignment on file for GDB, but I considered that the patch
was small enough for it to be ok (hopefully, we can start accepting
patches with Signed-Off-By and no copyright assignment soon).

In terms of testing, this architecture is relatively uncommon, so I'll
just take your word for it that you did your best :).  If you want to go
further to improve the SH port, you can always try to run the testsuite
and see what fails.  It might not be trivial depending on your setup, be
we can always help.

Simon