Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

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

On 3/21/25 04:36, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 3/20/25 12:30, Pierrick Bouvier wrote:
>>> On 3/19/25 11:22, Alex Bennée wrote:
>>>> The current helper.h functions rely on hard coded assumptions about
>>>> target endianess to use the tswap macros. We also end up double
>>>> swapping a bunch of values if the target can run in multiple endianess
>>>> modes. Avoid this by getting the target to pass the endianess and size
>>>> via a MemOp and fixing up appropriately.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> ---
>>>>     include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>>>>     gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>>>>     2 files changed, 52 insertions(+)
>>>>     create mode 100644 include/gdbstub/registers.h
>>>>
>>>> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
>>>> new file mode 100644
>>>> index 0000000000..4abc7a6ae7
>>>> --- /dev/null
>>>> +++ b/include/gdbstub/registers.h
>>>> @@ -0,0 +1,30 @@
>>>> +/*
>>>> + * GDB Common Register Helpers
>>>> + *
>>>> + * Copyright (c) 2025 Linaro Ltd
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + */
>>>> +
>>>> +#ifndef GDB_REGISTERS_H
>>>> +#define GDB_REGISTERS_H
>>>> +
>>>> +#include "exec/memop.h"
>>>> +
>>>> +/**
>>>> + * gdb_get_register_value() - get register value for gdb
>>>> + * mo: size and endian MemOp
>>>> + * buf: GByteArray to store in target order
>>>> + * val: pointer to value in host order
>>>> + *
>>>> + * This replaces the previous legacy read functions with a single
>>>> + * function to handle all sizes. Passing @mo allows the target mode to
>>>> + * be taken into account and avoids using hard coded tswap() macros.
>>>> + *
>>>> + * Returns the number of bytes written to the array.
>>>> + */
>>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
>>>> +
>>>> +#endif /* GDB_REGISTERS_H */
>>>> +
>>>> +
>>>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>>> index 282e13e163..3d7b1028e4 100644
>>>> --- a/gdbstub/gdbstub.c
>>>> +++ b/gdbstub/gdbstub.c
>>>> @@ -32,6 +32,7 @@
>>>>     #include "exec/gdbstub.h"
>>>>     #include "gdbstub/commands.h"
>>>>     #include "gdbstub/syscalls.h"
>>>> +#include "gdbstub/registers.h"
>>>>     #ifdef CONFIG_USER_ONLY
>>>>     #include "accel/tcg/vcpu-state.h"
>>>>     #include "gdbstub/user.h"
>>>> @@ -45,6 +46,7 @@
>>>>     #include "system/runstate.h"
>>>>     #include "exec/replay-core.h"
>>>>     #include "exec/hwaddr.h"
>>>> +#include "exec/memop.h"
>>>>        #include "internals.h"
>>>>     @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState
>>>> *cpu, uint8_t *mem_buf, int reg)
>>>>         return 0;
>>>>     }
>>>>     +/*
>>>> + * Target helper function to read value into GByteArray, target
>>>> + * supplies the size and target endianess via the MemOp.
>>>> + */
>>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
>>>> +{
>>>> +    size_t bytes = memop_size(op);
>>>> +
>>>> +    if (op & MO_BSWAP) {
>>>> +        for ( int i = bytes ; i > 0; i--) {
>>>> +            g_byte_array_append(buf, &val[i - 1], 1);
>>>> +        };
>>>> +    } else {
>>>> +        g_byte_array_append(buf, val, bytes);
>>>> +    }
>>>> +
>>>> +    return bytes;
>>>> +}
>>>> +
>>>> +
>>>>     static void gdb_register_feature(CPUState *cpu, int base_reg,
>>>>                                      gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>>>>                                      const GDBFeature *feature)
>>> It could be preferable to set buf with the value, instead of simply
>>> appending the value. This way, there is no need to return the size, as
>>> it's contained in buffer size itself.
>>> If we insist on returning the size, it's better to make it a
>>> parameter
>>> (and use a void parameter type), because at the moment, it gives the
>>> impression the function itself returns the value, which may be confusing.
>>
>> Seems like it's the existing convention through
>> gdb_set_reg_cb/gdb_get_reg_cb, so we have to follow this.
> 
> For the "g" packet we append multiple registers so the buffer size grows
> as we append each one.
> 

Thanks for sharing the rationale behind this.