On 3/21/25 04:36, Alex Bennée wrote:
Pierrick Bouvier <pierrick.bouv...@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.ben...@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.
