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

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

On 20/3/25 07:19, Akihiko Odaki wrote:
> On 2025/03/20 3: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>
> 
> The overall idea looks good to me. I have a few nitpicks:
> 
>> ---
>>   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--) {
> 
> memop_size() returns unsigned, but bytes is size_t and i is int, and 
> this function returns int. Let's keep them consistent.

I wasn't sure why this method returns any information at all, but
apparently the next patch shows some uses. Indeed as Akihiko pointed,
if we return something, let it be unsigned (possibly size_t).