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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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