Pierrick Bouvier <[email protected]> writes:
> 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?
It did seem natural that if you were defining a MemOp you would use all
of it rather than only its endian definition. But you are right we could
introduce the same helpers with a bool flag for endianess.
Maybe we should have fully formed mops and just assert in the helper:
gdb_get_reg32(MemOp op, GByteArray *buf, uint32_t val) {
g_assert(op & MO_SIZE == MO_32);
gdb_get_register_value(op, buf, &val);
}
I was also trying to avoid over boilerplating the code.
>
>> - 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
>>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
