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.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use unsigned consistently
  - fix some rouge whitespace
  - add typed reg32/64 wrappers
  - pass void * to underlying helper to avoid casting
---
 include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
 gdbstub/gdbstub.c           | 23 ++++++++++++++++
 2 files changed, 78 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..2220f58efe
--- /dev/null
+++ b/include/gdbstub/registers.h
@@ -0,0 +1,55 @@
+/*
+ * 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.
+ *
+ * There are wrapper functions for the common sizes you can use to
+ * keep type checking.
+ *
+ * Returns the number of bytes written to the array.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
+
+/**
+ * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
+    g_assert((op & MO_SIZE) == MO_32);
+    return gdb_get_register_value(op, buf, val);
+}
+
+/**
+ * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
+    g_assert((op & MO_SIZE) == MO_64);
+    return gdb_get_register_value(op, buf, val);
+}
+
+#endif /* GDB_REGISTERS_H */
+
+
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
+{
+    unsigned bytes = memop_size(op);
+
+    if (op & MO_BSWAP) {
+        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
+        for (unsigned i = bytes; i > 0; i--) {
+            g_byte_array_append(buf, ptr--, 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 2025/03/24 19:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +

Nitpick: extra newlines here.

> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 1);
> +        };
> +    } else {
> +        g_byte_array_append(buf, val, bytes);
> +    }
> +
> +    return bytes;
> +}
> +
> +

An extra blank line here too; each of the other functions in this file 
has only one blank line following.

>   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 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +

After reading the whole series, I think I am missing the point of 
introduce op here.
If what we really want is just the target endianness, why not add the 
"if target_words_bigendian()" in the wrapper here?

Are there cases where we want another endianness than target one?

> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 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 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +

Maybe we could simply expect endian information from MemOp, and add 
required size here.
It clutters call sites to have gdb_get_regX_value(MO_X | ...).
We can eventually assert no size was set, so it's not added by mistake 
from callers.

> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 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 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +

Could it be made static, so it's hidden from the public interface?
You'll need to un-inline gdb_get_reg*_value functions. I doubt it's 
performance critical and need to be inline.

> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 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 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}

Passing the value by address is crazy.

> @@ -551,6 +553,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 1);
> +        };

I think you're still wed to this supposedly generic way to handle values, and this method 
of bswap is really silly.

Better, I think is

int gdb_get_reg32_value(MemOp endian, GByteArray *buf, uint32_t val)
{
     /* We only expect MO_BE or MO_LE, one of which is 0 depending on host. */
     if (endian) {
         assert(endian == MO_BSWAP);
         val = bswap32(val);
     }

     g_byte_array_append(buf, &val, sizeof(val));
     return sizeof(val);
}


I'll also mention that the return value is now mostly useless: I think you should just 
rely on buf->len.  This is something that we could have cleaned up when converting from 
uint8_t *buf to GByteArray in the first place.  But changing the interface of all of the 
gdb_read_register hooks is a larger job.


r~

Alex Bennée <alex.bennee@linaro.org> writes:

> 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.
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - use unsigned consistently
>   - fix some rouge whitespace
>   - add typed reg32/64 wrappers
>   - pass void * to underlying helper to avoid casting
> ---
>  include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>  gdbstub/gdbstub.c           | 23 ++++++++++++++++
>  2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 1);
> +        };

I forgot to fix this up. Hopefully the following seems a little less
like pointer abuse:

/*
 * 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, void *val)
{
    unsigned bytes = memop_size(op);
    uint8_t *data = val;

    if (op & MO_BSWAP) {
        uint8_t *ptr = &data[bytes - 1];
        do {
            g_byte_array_append(buf, ptr--, 1);
        } while (ptr >= data);
    } else {
        g_byte_array_append(buf, val, bytes);
    }

    return bytes;
}


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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

On 2025/03/24 19:32, Alex Bennée wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
>> 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.
>>
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>    - use unsigned consistently
>>    - fix some rouge whitespace
>>    - add typed reg32/64 wrappers
>>    - pass void * to underlying helper to avoid casting
>> ---
>>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>>   2 files changed, 78 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..2220f58efe
>> --- /dev/null
>> +++ b/include/gdbstub/registers.h
>> @@ -0,0 +1,55 @@
>> +/*
>> + * 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.
>> + *
>> + * There are wrapper functions for the common sizes you can use to
>> + * keep type checking.
>> + *
>> + * Returns the number of bytes written to the array.
>> + */
>> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
>> +
>> +/**
>> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
>> + * mo: size and endian MemOp
>> + * buf: GByteArray to store in target order
>> + * val: pointer to uint32_t value in host order
>> + */
>> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
>> +    g_assert((op & MO_SIZE) == MO_32);
>> +    return gdb_get_register_value(op, buf, val);
>> +}
>> +
>> +/**
>> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
>> + * mo: size and endian MemOp
>> + * buf: GByteArray to store in target order
>> + * val: pointer to uint32_t value in host order
>> + */
>> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
>> +    g_assert((op & MO_SIZE) == MO_64);
>> +    return gdb_get_register_value(op, buf, val);
>> +}
>> +
>> +#endif /* GDB_REGISTERS_H */
>> +
>> +
>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
>> +{
>> +    unsigned bytes = memop_size(op);
>> +
>> +    if (op & MO_BSWAP) {
>> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
>> +        for (unsigned i = bytes; i > 0; i--) {
>> +            g_byte_array_append(buf, ptr--, 1);
>> +        };
> 
> I forgot to fix this up. Hopefully the following seems a little less
> like pointer abuse:
> 
> /*
>   * 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, void *val)
> {
>      unsigned bytes = memop_size(op);

Nitpick: I would name it "size" instead of "bytes" for consistency.

>      uint8_t *data = val;

It's unclear what "data" means. This can be named "bytes", "val8", 
"octets", etc to differentiate from "val".

> 
>      if (op & MO_BSWAP) {
>          uint8_t *ptr = &data[bytes - 1];
>          do {
>              g_byte_array_append(buf, ptr--, 1);
>          } while (ptr >= data);

It may be better to initialize ptr to point the position after the last 
element:

uint8_t *ptr = &data[bytes];

while (ptr > data) {
   g_byte_array_append(buf, --ptr, 1);
}

Strictly speaking, this is well-defined when bytes == 0, which is not in 
the original version, and you don't need to write decrements at two 
places (initialization and update).

>      } else {
>          g_byte_array_append(buf, val, bytes);
>      }
> 
>      return bytes;
> }
> 
> 
>> +    } 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)
>