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 <[email protected]>
Signed-off-by: Alex Bennée <[email protected]>
---
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~
