On 3/21/25 06:02, Philippe Mathieu-Daudé wrote:
On 20/3/25 21:16, Pierrick Bouvier wrote:
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.
We'll need to eventually remove target_words_bigendian(), so that'd just
be postponing that.
It seemed to me that one of the goal of current shared work on single
binary is to be able to check those kind of things at runtime. So I
don't see how we can plan to remove target_words_bigendian(), or an
equivalent.
If you mean we'll replace this call with another name/api, I don't see
it as a problem to use it for now. It's upstream and works.
