Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Pierrick Bouvier]

On 3/20/25 12:52, Pierrick Bouvier wrote:
> On 3/19/25 11:22, Alex Bennée wrote:
> > The aim of this work is to get rid of the endian aware helpers in
> > gdbstub/helpers.h which due to their use of tswap() mean target
> > gdbstubs need to be built multiple times. While this series doesn't
> > actually build each stub once it introduces a new helper -
> > gdb_get_register_value() which takes a MemOp which can describe the
> > current endian state of the system. This will be a lot easier to
> > dynamically feed from a helper function.
> >
> > The most complex example is PPC which has a helper called
> > ppc_maybe_bswap_register() which was doing this.
> >
> > This is still an RFC so I'm interested in feedback:
> >
> >     - is the API sane
> >     - can we avoid lots of (uint8_t *) casting?
>
> Even though the series has a good intent, the fact we make everything
> "generic" makes that we lose all guarantees we could get by relying on
> static typing, and that we had possibility of mistakes when passing size
> (which happened in patch 4 if I'm correct). And explicit casting comes
> as a *strong* warning about that.
>
> By patch 7, I was really feeling it's not a win vs explicit functions
> per size.
>
> If the goal of the series is to get rid of endian aware helpers, well,
> this can be fixed in the helpers themselves, without needing to
> introduce a "generic" size helper. Maybe we are trying to solve two
> different problems here?
>
> >     - should we have a reverse helper for setting registers
> >
> > If this seems like the right approach I can have a go at more of the
> > frontends later.

Looking at include/gdbstub/helpers.h, gdb_get_reg128 can be solved by 
using target_words_bigendian() instead of TARGET_BIG_ENDIAN, which is 
already what tswap primitives are doing.

For gdb_get_regl, I would get rid of it completely, by editing all 
targets gdbstub.c, and replacing with gdb_get_reg32 or gdb_get_reg64 
explicit calls.
ppc is a very naughty boy, because registers are defined as 
target_ulong, while other arch use fixed types. The solution might be as 
simple as changing ppc registers definition to uint64_t.
If it's too complicated, you can postpone the problem by leaving 
gdb_get_regl defined only in ppc gdbstub, and clean up all other arch.
Thanks to static typing, it will be easy to spot a wrong gdb_get_regl 
conversion, so it's a no-risk operaton.

For ldtul_p, ldtul_le_p, and ldtul_be_p, it's a similar game. It's 
harder because only return type will differ, and you might miss occurences.
A safe way could be to replace ldtul_p by call to a function taking 
return value through pointer in parameter. This way, you can replace 
easily with l and q variants, without any risk off implicit conversion.
And for the one left depending on target_ulong/target_long, leave that 
for now, and move ldtul*_p to a target-helper.h included only for archs 
needing this.

> > There are a few other misc clean-ups I did on the way which might be
> > worth cherry picking for 10.0 but I'll leave that up to maintainers.
> >
> > Alex.
> >
> > Alex Bennée (10):
> >     include/gdbstub: fix include guard in commands.h
> >     gdbstub: introduce target independent gdb register helper
> >     target/arm: convert 32 bit gdbstub to new helper
> >     target/arm: convert 64 bit gdbstub to new helper
> >     target/ppc: expand comment on FP/VMX/VSX access functions
> >     target/ppc: make ppc_maybe_bswap_register static
> >     target/ppc: convert gdbstub to new helper (!hacky)
> >     gdbstub: assert earlier in handle_read_all_regs
> >     include/exec: fix assert in size_memop
> >     target/microblaze: convert gdbstub to new helper
> >
> >    include/exec/memop.h        |   4 +-
> >    include/gdbstub/commands.h  |   2 +-
> >    include/gdbstub/registers.h |  30 ++++++
> >    target/ppc/cpu.h            |   8 +-
> >    gdbstub/gdbstub.c           |  24 ++++-
> >    target/arm/gdbstub.c        |  57 +++++++----
> >    target/arm/gdbstub64.c      |  53 ++++++----
> >    target/microblaze/gdbstub.c |  44 ++++----
> >    target/ppc/gdbstub.c        | 194 ++++++++++++++++++++----------------
> >    9 files changed, 257 insertions(+), 159 deletions(-)
> >    create mode 100644 include/gdbstub/registers.h