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

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

Pierrick Bouvier <pierrick.bouvier@linaro.org> 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