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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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.