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

[Philippe Mathieu-Daudé]

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.