[PATCH] gdb: specify sh4al-dsp register types

[PATCH] gdb: specify sh4al-dsp register types

[Sébastien Michelland]

This avoids $pc and similar registers being interpreted as negative when
in the upper half of the address space (e.g. by x/i), which doesn't
interact well with Xfer memory maps.
---

Hi,

This patch fixes a pretty funny issue for the sh4al-dsp resulting from
$pc being typed as an int. When $pc is in the upper half of the address
space, `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

Affected registers are pc, pr (return address of a call), gbr (base
register for some specific addressing modes), vbr (interrupt handler)
and spc (return address after interrupt).

It's not immediately clear to me why existing sh variants don't also do
that. Maybe it should be considered an issue with the memory map, or
maybe it's just a rare enough condition to not care.

 gdb/sh-tdep.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index 1c67ea42..c1845111 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -1437,6 +1437,16 @@ sh_sh4_register_type (struct gdbarch *gdbarch, int reg_nr)
     return builtin_type (gdbarch)->builtin_int;
 }
 
+static struct type *
+sh_sh4al_dsp_register_type (struct gdbarch *gdbarch, int reg_nr)
+{
+  if (reg_nr == PC_REGNUM || reg_nr == PR_REGNUM || reg_nr == GBR_REGNUM
+      || reg_nr == VBR_REGNUM || reg_nr == SPC_REGNUM)
+    return builtin_type (gdbarch)->builtin_func_ptr;
+  else
+    return builtin_type (gdbarch)->builtin_int;
+}
+
 static struct type *
 sh_default_register_type (struct gdbarch *gdbarch, int reg_nr)
 {
@@ -2353,6 +2363,7 @@ sh_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
     case bfd_mach_sh4al_dsp:
       set_gdbarch_register_name (gdbarch, sh_sh4al_dsp_register_name);
+      set_gdbarch_register_type (gdbarch, sh_sh4al_dsp_register_type);
       set_gdbarch_register_sim_regno (gdbarch, sh_dsp_register_sim_regno);
       break;
 
-- 
2.44.0

Re: [PATCH] gdb: specify sh4al-dsp register types

[Simon Marchi]

On 2024-03-31 18:26, Sébastien Michelland wrote:
> This avoids $pc and similar registers being interpreted as negative when
> in the upper half of the address space (e.g. by x/i), which doesn't
> interact well with Xfer memory maps.
> ---
> 
> Hi,
> 
> This patch fixes a pretty funny issue for the sh4al-dsp resulting from
> $pc being typed as an int. When $pc is in the upper half of the address
> space, `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
> 
> Affected registers are pc, pr (return address of a call), gbr (base
> register for some specific addressing modes), vbr (interrupt handler)
> and spc (return address after interrupt).
> 
> It's not immediately clear to me why existing sh variants don't also do
> that. Maybe it should be considered an issue with the memory map, or
> maybe it's just a rare enough condition to not care.

Hi Sébastien,

I would suggest including all that info in the commit message proper.
If I were to do some git-blaming and fall on that commit, I would
appreciate having all that info right there.

I checked a few other arches, and it seems customary for arches to
indicate that the PC register and some others should be typed as a
builtin_func_ptr (while the SP register and some others should be typed
as a builtin_data_ptr).  The fact that the sh arch doesn't do it seems
like an oversight.

Another effect of having the pc typed as an int is that printing it will
show it as a decimal, without symbol resolution.

For instance, on amd64, it typically looks like this:

    (gdb) p $pc
    $1 = (void (*)()) 0x555555555122 <main+9>

If I hack it to consider `rip` as an int64, it now looks like this:

    (gdb) p $pc
    $1 = 93824992235810

I suppose that what you see on sh is similar to the latter (obviously
not as nice).

If you want to try to fix the behavior on all sh arches, feel free to do
so.  But otherwise, the patch LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Let me know if you don't have push access and would like me to push it
for you.

Simon

Re: [PATCH] gdb: specify sh4al-dsp register types

[Sébastien Michelland]

On 2024-04-01 05:25, Simon Marchi wrote:
> On 2024-03-31 18:26, Sébastien Michelland wrote:
>> This avoids $pc and similar registers being interpreted as negative when
>> in the upper half of the address space (e.g. by x/i), which doesn't
>> interact well with Xfer memory maps.
>> ---
>>
>> Hi,
>>
>> This patch fixes a pretty funny issue for the sh4al-dsp resulting from
>> $pc being typed as an int. When $pc is in the upper half of the address
>> space, `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
>>
>> Affected registers are pc, pr (return address of a call), gbr (base
>> register for some specific addressing modes), vbr (interrupt handler)
>> and spc (return address after interrupt).
>>
>> It's not immediately clear to me why existing sh variants don't also do
>> that. Maybe it should be considered an issue with the memory map, or
>> maybe it's just a rare enough condition to not care.
> 
> Hi Sébastien,
> 
> I would suggest including all that info in the commit message proper.
> If I were to do some git-blaming and fall on that commit, I would
> appreciate having all that info right there.
> 
> I checked a few other arches, and it seems customary for arches to
> indicate that the PC register and some others should be typed as a
> builtin_func_ptr (while the SP register and some others should be typed
> as a builtin_data_ptr).  The fact that the sh arch doesn't do it seems
> like an oversight.
> 
> Another effect of having the pc typed as an int is that printing it will
> show it as a decimal, without symbol resolution.

Hi Simon, thanks for checking this patch.

I didn't think much of it but I do also get the decimal printing.

> If you want to try to fix the behavior on all sh arches, feel free to do
> so.  But otherwise, the patch LGTM:
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Let me know if you don't have push access and would like me to push it
> for you.

I'll submit a v2 to get that same behavior on all sh arches--it feels 
better to have a consistent interpretation.

I indeed don't have push access, thanks for your help.

Sébastien