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