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

[Pierrick Bouvier]

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