[PATCH 0/4] Minor gdbstub cleanups

[PATCH 0/4] Minor gdbstub cleanups

[Ilya Leoshkevich]

Hi,

These four patches are from the [1] series, which will probably take a
while. But IMHO they make sense on their own, so I'm sending them as a
separate series.

[1] https://lore.kernel.org/qemu-devel/20240923162208.90745-1-iii@linux.ibm.com/

Best regards,
Ilya

Ilya Leoshkevich (4):
  gdbstub: Make gdb_get_char() static
  gdbstub: Move phy_memory_mode to GDBSystemState
  gdbstub: Move gdb_syscall_mode to GDBSyscallState
  gdbstub: Factor out gdb_try_stop()

 gdbstub/gdbstub.c   | 15 +++++++++++----
 gdbstub/internals.h |  4 ++--
 gdbstub/syscalls.c  | 20 ++++++++++----------
 gdbstub/system.c    | 18 ++++++++----------
 gdbstub/user.c      | 13 +++++--------
 5 files changed, 36 insertions(+), 34 deletions(-)

-- 
2.46.2

[PATCH 1/4] gdbstub: Make gdb_get_char() static

[Ilya Leoshkevich]

It's user-only since commit a7e0f9bd2ace ("gdbstub: abstract target
specific details from gdb_put_packet_binary").

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/internals.h | 2 --
 gdbstub/user.c      | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index bf5a5c63029..5acc36846f3 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -142,8 +142,6 @@ void gdb_create_default_process(GDBState *s);
 int gdb_signal_to_target(int sig);
 int gdb_target_signal_to_gdb(int sig);
 
-int gdb_get_char(void); /* user only */
-
 /**
  * gdb_continue() - handle continue in mode specific way.
  */
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 0b4bfa9c488..1fef5201f4e 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -103,7 +103,7 @@ typedef struct {
 
 static GDBUserState gdbserver_user_state;
 
-int gdb_get_char(void)
+static int gdb_get_char(void)
 {
     uint8_t ch;
     int ret;
-- 
2.46.2

[PATCH 2/4] gdbstub: Move phy_memory_mode to GDBSystemState

[Ilya Leoshkevich]

Follow the convention that all the pieces of the global stub state must
be inside a single struct. While at it, convert phy_memory_mode to
bool.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/system.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdbstub/system.c b/gdbstub/system.c
index c9f236e94f7..2e31e42f7dd 100644
--- a/gdbstub/system.c
+++ b/gdbstub/system.c
@@ -35,6 +35,7 @@
 typedef struct {
     CharBackend chr;
     Chardev *mon_chr;
+    bool phy_memory_mode;
 } GDBSystemState;
 
 GDBSystemState gdbserver_system_state;
@@ -445,14 +446,12 @@ void gdb_qemu_exit(int code)
 /*
  * Memory access
  */
-static int phy_memory_mode;
-
 int gdb_target_memory_rw_debug(CPUState *cpu, hwaddr addr,
                                uint8_t *buf, int len, bool is_write)
 {
     CPUClass *cc;
 
-    if (phy_memory_mode) {
+    if (gdbserver_system_state.phy_memory_mode) {
         if (is_write) {
             cpu_physical_memory_write(addr, buf, len);
         } else {
@@ -491,7 +490,8 @@ bool gdb_can_reverse(void)
 void gdb_handle_query_qemu_phy_mem_mode(GArray *params,
                                         void *ctx)
 {
-    g_string_printf(gdbserver_state.str_buf, "%d", phy_memory_mode);
+    g_string_printf(gdbserver_state.str_buf, "%d",
+                    gdbserver_system_state.phy_memory_mode);
     gdb_put_strbuf();
 }
 
@@ -503,9 +503,9 @@ void gdb_handle_set_qemu_phy_mem_mode(GArray *params, void *ctx)
     }
 
     if (!gdb_get_cmd_param(params, 0)->val_ul) {
-        phy_memory_mode = 0;
+        gdbserver_system_state.phy_memory_mode = false;
     } else {
-        phy_memory_mode = 1;
+        gdbserver_system_state.phy_memory_mode = true;
     }
     gdb_put_packet("OK");
 }
-- 
2.46.2

[PATCH 3/4] gdbstub: Move gdb_syscall_mode to GDBSyscallState

[Ilya Leoshkevich]

Follow the convention that all the pieces of the global stub state must
be inside a single struct.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/syscalls.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/gdbstub/syscalls.c b/gdbstub/syscalls.c
index 4ddd5cae067..bf6a6c01b4f 100644
--- a/gdbstub/syscalls.c
+++ b/gdbstub/syscalls.c
@@ -24,6 +24,11 @@
 typedef struct {
     char syscall_buf[256];
     gdb_syscall_complete_cb current_syscall_cb;
+    enum {
+        GDB_SYS_UNKNOWN,
+        GDB_SYS_ENABLED,
+        GDB_SYS_DISABLED,
+    } mode;
 } GDBSyscallState;
 
 static GDBSyscallState gdbserver_syscall_state;
@@ -37,12 +42,6 @@ static bool gdb_attached(void)
     return gdbserver_state.init && gdbserver_state.c_cpu;
 }
 
-static enum {
-    GDB_SYS_UNKNOWN,
-    GDB_SYS_ENABLED,
-    GDB_SYS_DISABLED,
-} gdb_syscall_mode;
-
 /* Decide if either remote gdb syscalls or native file IO should be used. */
 int use_gdb_syscalls(void)
 {
@@ -57,16 +56,17 @@ int use_gdb_syscalls(void)
 
     /* -semihosting-config target=auto */
     /* On the first call check if gdb is connected and remember. */
-    if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
-        gdb_syscall_mode = gdb_attached() ? GDB_SYS_ENABLED : GDB_SYS_DISABLED;
+    if (gdbserver_syscall_state.mode == GDB_SYS_UNKNOWN) {
+        gdbserver_syscall_state.mode = gdb_attached() ? GDB_SYS_ENABLED :
+                                                        GDB_SYS_DISABLED;
     }
-    return gdb_syscall_mode == GDB_SYS_ENABLED;
+    return gdbserver_syscall_state.mode == GDB_SYS_ENABLED;
 }
 
 /* called when the stub detaches */
 void gdb_disable_syscalls(void)
 {
-    gdb_syscall_mode = GDB_SYS_DISABLED;
+    gdbserver_syscall_state.mode = GDB_SYS_DISABLED;
 }
 
 void gdb_syscall_reset(void)
-- 
2.46.2

[PATCH 4/4] gdbstub: Factor out gdb_try_stop()

[Ilya Leoshkevich]

Move checking and setting allow_stop_reply into a function.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/gdbstub.c   | 15 +++++++++++----
 gdbstub/internals.h |  2 ++
 gdbstub/system.c    |  6 ++----
 gdbstub/user.c      | 11 ++++-------
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b1def7e71d2..c45934b52b8 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1422,11 +1422,10 @@ static void handle_v_attach(GArray *params, void *user_ctx)
     gdbserver_state.g_cpu = cpu;
     gdbserver_state.c_cpu = cpu;
 
-    if (gdbserver_state.allow_stop_reply) {
+    if (gdb_try_stop()) {
         g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
         gdb_append_thread_id(cpu, gdbserver_state.str_buf);
         g_string_append_c(gdbserver_state.str_buf, ';');
-        gdbserver_state.allow_stop_reply = false;
 cleanup:
         gdb_put_strbuf();
     }
@@ -2016,12 +2015,11 @@ static void handle_gen_set(GArray *params, void *user_ctx)
 
 static void handle_target_halt(GArray *params, void *user_ctx)
 {
-    if (gdbserver_state.allow_stop_reply) {
+    if (gdb_try_stop()) {
         g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
         gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
         g_string_append_c(gdbserver_state.str_buf, ';');
         gdb_put_strbuf();
-        gdbserver_state.allow_stop_reply = false;
     }
     /*
      * Remove all the breakpoints when this query is issued,
@@ -2493,3 +2491,12 @@ void gdb_create_default_process(GDBState *s)
     process->target_xml = NULL;
 }
 
+bool gdb_try_stop(void)
+{
+    if (!gdbserver_state.allow_stop_reply) {
+        return false;
+    }
+
+    gdbserver_state.allow_stop_reply = false;
+    return true;
+}
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 5acc36846f3..310861e581b 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -215,4 +215,6 @@ void gdb_breakpoint_remove_all(CPUState *cs);
 int gdb_target_memory_rw_debug(CPUState *cs, hwaddr addr,
                                uint8_t *buf, int len, bool is_write);
 
+bool gdb_try_stop(void);
+
 #endif /* GDBSTUB_INTERNALS_H */
diff --git a/gdbstub/system.c b/gdbstub/system.c
index 2e31e42f7dd..64cfcf25221 100644
--- a/gdbstub/system.c
+++ b/gdbstub/system.c
@@ -141,7 +141,7 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
         return;
     }
 
-    if (!gdbserver_state.allow_stop_reply) {
+    if (!gdb_try_stop()) {
         return;
     }
 
@@ -211,7 +211,6 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
 
 send_packet:
     gdb_put_packet(buf->str);
-    gdbserver_state.allow_stop_reply = false;
 
     /* disable single step if it was enabled */
     cpu_single_step(cpu, 0);
@@ -428,10 +427,9 @@ void gdb_exit(int code)
 
     trace_gdbstub_op_exiting((uint8_t)code);
 
-    if (gdbserver_state.allow_stop_reply) {
+    if (gdb_try_stop()) {
         snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
         gdb_put_packet(buf);
-        gdbserver_state.allow_stop_reply = false;
     }
 
     qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 1fef5201f4e..17bcb9d70d9 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -181,10 +181,9 @@ void gdb_exit(int code)
 
     trace_gdbstub_op_exiting((uint8_t)code);
 
-    if (gdbserver_state.allow_stop_reply) {
+    if (gdb_try_stop()) {
         snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
         gdb_put_packet(buf);
-        gdbserver_state.allow_stop_reply = false;
     }
 
 }
@@ -222,7 +221,7 @@ int gdb_handlesig(CPUState *cpu, int sig, const char *reason, void *siginfo,
 
     if (sig != 0) {
         gdb_set_stop_cpu(cpu);
-        if (gdbserver_state.allow_stop_reply) {
+        if (gdb_try_stop()) {
             g_string_printf(gdbserver_state.str_buf,
                             "T%02xthread:", gdb_target_signal_to_gdb(sig));
             gdb_append_thread_id(cpu, gdbserver_state.str_buf);
@@ -231,7 +230,6 @@ int gdb_handlesig(CPUState *cpu, int sig, const char *reason, void *siginfo,
                 g_string_append(gdbserver_state.str_buf, reason);
             }
             gdb_put_strbuf();
-            gdbserver_state.allow_stop_reply = false;
         }
     }
     /*
@@ -276,13 +274,12 @@ void gdb_signalled(CPUArchState *env, int sig)
     char buf[4];
 
     if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
-        !gdbserver_state.allow_stop_reply) {
+        !gdb_try_stop()) {
         return;
     }
 
     snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
     gdb_put_packet(buf);
-    gdbserver_state.allow_stop_reply = false;
 }
 
 static void gdb_accept_init(int fd)
@@ -502,7 +499,7 @@ void gdbserver_fork_end(CPUState *cpu, pid_t pid)
         gdbserver_user_state.fork_peer_pid = pid;
         gdbserver_user_state.fork_peer_tid = pid;
 
-        if (!gdbserver_state.allow_stop_reply) {
+        if (!gdb_try_stop()) {
             goto fail;
         }
         g_string_printf(gdbserver_state.str_buf,
-- 
2.46.2