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)
-- 
2.39.5

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)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

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.

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.

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.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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.

Hi Alex,

On 19/3/25 19: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


> +/**
> + * gdb_get_register_value() - get register value for gdb
> + * mo: size and endian MemOp

Since we pass the size, ...

> + * buf: GByteArray to store in target order
> + * val: pointer to value in host order

we don't know the type of the value, ...

> + *
> + * 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);

so I'd rather use a 'void *' type here (also const),

int gdb_get_register_value(MemOp op, GByteArray *gbuf, const void *ptr);

...

> +/*
> + * 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)
> +{

using uint8_t type being one implementation choice for this method:

int gdb_get_register_value(MemOp op, GByteArray *gbuf, const void *ptr)
{
        const uint8_t *buf = ptr;

> +    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;
> +}

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.

There is an extra whitespace between "(" and "int".

Regards,
Akihiko Odaki

> +            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)

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).