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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 21/3/25 18:31, Pierrick Bouvier wrote:
> On 3/21/25 04:46, Alex Bennée wrote:
>> 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.
>>
> 
> 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.

In another thread Richard said for these cases we should use _Generic()
more.

> Adding casts to loosen type system is not a win versus that.

Agreed.