On 3/21/25 04:46, Alex Bennée wrote:
Pierrick Bouvier <pierrick.bouv...@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.
Defining MemOp and passing is ok, but loosing static typing guarantees
is wrong in my opinion. C is already on the weak side regarding typing,
we don't need to "void*"ize things more instead of replacing the calls
with correct variants.
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.
Adding proper functions definition instead of macros, and eliminating
ifdefs is not really boilerplate.
Adding casts to loosen type system is not a win versus that.
- 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
