Re: ppc-linux-user GDB stub broken

["Alex Bennée" <alex.bennee@linaro.org>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 27/2/26 10:58, Philippe Mathieu-Daudé wrote:
>> Hi Florian,
>
>
>>> on current master (d8a9d97317d03190b34498741f98f22e2a9afe3e), the basic
>>> gdb stub test fails for ppc-linux-user when running "make check-tcg".
>
>
>>> I've debugged around a bit but I don't know my way around the gdbstub
>>> internals enough to propose a patch right away, so I'll just summarize
>>> what I figured out so far.
>>> 1. Due to cc->gdb_num_core_regs not being set explicitly anymore, it is
>>>     set to 0 at the end of gdbstub/gdbstub.c:gdb_init_cpu(). In the same
>>>     function, cpu->gdb_num_regs gets set to 70.
>>> 2. When the test tries to read a register in gdbstub/gdbstub.c:gdb_read_register(),
>>>     the first condition of "reg < cc->gdb_num_core_regs" is always false.
>>>     Also, the register number for fpscr passed to the function is is 103,
>>>     and cpu->gdb_num_regs is also 103. If the register number is supposed
>>>     to be an index (as I understand it), this would indicate an
>>>     off-by-one error somewhere.
>>>
>>> If you have some pointers on where to look / what to check out for
>>> fixing this, I'll happily try to work on a patch. But I suppose that
>>> somebody more experienced with this could probably fix this much faster
>>> than I can.
>> I'll have a look.
>
> Short term, this seems to fix it:
>
> -- >8 --
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 90f4b95135b..0b0a5d1e044 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -610,6 +610,10 @@ void gdb_register_coprocessor(CPUState *cpu,
>      guint i;
>      int base_reg = cpu->gdb_num_regs;
>
> +    if (g_pos && g_pos != base_reg) {
> +        base_reg = g_pos;
> +    }
> +
>      for (i = 0; i < cpu->gdb_regs->len; i++) {
>          /* Check for duplicates.  */
>          s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> @@ -622,14 +626,7 @@ void gdb_register_coprocessor(CPUState *cpu,
>
>      /* Add to end of list.  */
>      cpu->gdb_num_regs += feature->num_regs;
> -    if (g_pos) {
> -        if (g_pos != base_reg) {
> -            error_report("Error: Bad gdb register numbering for '%s', "
> -                         "expected %d got %d", feature->xml, g_pos,
>                           base_reg);
> -        } else {
> -            cpu->gdb_num_g_regs = cpu->gdb_num_regs;
> -        }
> -    }
> +    cpu->gdb_num_g_regs = cpu->gdb_num_regs;
>  }
>
>  void gdb_unregister_coprocessor_all(CPUState *cpu)
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index e0aae9c9eaf..51b14b95b9f 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -502,7 +502,7 @@ void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *pcc)
>  {
>      if (pcc->insns_flags & PPC_FLOAT) {
>          gdb_register_coprocessor(cs, gdb_get_float_reg, gdb_set_float_reg,
> - gdb_find_static_feature("power-fpu.xml"), 0);
> + gdb_find_static_feature("power-fpu.xml"), 71);
>      }
>      if (pcc->insns_flags & PPC_ALTIVEC) {
>          gdb_register_coprocessor(cs, gdb_get_avr_reg, gdb_set_avr_reg,
>
> ---

Tested-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro