diff mbox series

[v2,RESEND,1/7] gdbstub: only send stop-reply packets when allowed to

Message ID ec04a0d3cb1c1072703f776624e503ad6257dccd.1681993775.git.quic_mathbern@quicinc.com
State New
Headers show
Series Hexagon: add lldb support | expand

Commit Message

Matheus Tavares Bernardino April 20, 2023, 12:31 p.m. UTC 
GDB's remote serial protocol allows stop-reply messages to be sent by
the stub either as a notification packet or as a reply to a GDB command
(provided that the cmd accepts such a response). QEMU currently does not
implement notification packets, so it should only send stop-replies
synchronously and when requested. Nevertheless, it still issues
unsolicited stop messages through gdb_vm_state_change().

Although this behavior doesn't seem to cause problems with GDB itself
(the messages are just ignored), it can impact other debuggers that
implement the GDB remote serial protocol, like hexagon-lldb. Let's
change the gdbstub to send stop messages only as a response to a
previous GDB command that accepts such a reply.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 gdbstub/internals.h |  5 +++++
 gdbstub/gdbstub.c   | 37 ++++++++++++++++++++++++++++---------
 gdbstub/softmmu.c   | 13 +++++++++++--
 gdbstub/user.c      | 17 +++++++++++------
 4 files changed, 55 insertions(+), 17 deletions(-)

Comments

Alex Bennée April 20, 2023, 1:41 p.m. UTC | #1 
Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes:

> GDB's remote serial protocol allows stop-reply messages to be sent by
> the stub either as a notification packet or as a reply to a GDB command
> (provided that the cmd accepts such a response). QEMU currently does not
> implement notification packets, so it should only send stop-replies
> synchronously and when requested. Nevertheless, it still issues
> unsolicited stop messages through gdb_vm_state_change().
>
> Although this behavior doesn't seem to cause problems with GDB itself
> (the messages are just ignored), it can impact other debuggers that
> implement the GDB remote serial protocol, like hexagon-lldb. Let's
> change the gdbstub to send stop messages only as a response to a
> previous GDB command that accepts such a reply.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  gdbstub/internals.h |  5 +++++
>  gdbstub/gdbstub.c   | 37 ++++++++++++++++++++++++++++---------
>  gdbstub/softmmu.c   | 13 +++++++++++--
>  gdbstub/user.c      | 17 +++++++++++------
>  4 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index 94ddff4495..33d21d6488 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -65,6 +65,11 @@ typedef struct GDBState {
>      GByteArray *mem_buf;
>      int sstep_flags;
>      int supported_sstep_flags;
> +    /*
> +     * Whether we are allowed to send a stop reply packet at this moment.
> +     * Must be set off after sending the stop reply itself.
> +     */
> +    bool allow_stop_reply;
>  } GDBState;
>  
>  /* lives in main gdbstub.c */
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 0760d78685..be18568d0a 100644
<snip>
>  /*
> @@ -139,6 +140,10 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
>          return;
>      }
>  
> +    if (!gdbserver_state.allow_stop_reply) {
> +        return;
> +    }
> +
>      gdb_append_thread_id(cpu, tid);
>  
>      switch (state) {
> @@ -205,6 +210,7 @@ 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);
> @@ -422,8 +428,11 @@ void gdb_exit(int code)
>  
>      trace_gdbstub_op_exiting((uint8_t)code);
>  
> -    snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
> -    gdb_put_packet(buf);
> +    if (gdbserver_state.allow_stop_reply) {
> +        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 80488b6bb9..bb03622c83 100644
> --- a/gdbstub/user.c
> +++ b/gdbstub/user.c
> @@ -127,11 +127,14 @@ int gdb_handlesig(CPUState *cpu, int sig)
>  
>      if (sig != 0) {
>          gdb_set_stop_cpu(cpu);
> -        g_string_printf(gdbserver_state.str_buf,
> -                        "T%02xthread:", gdb_target_signal_to_gdb(sig));
> -        gdb_append_thread_id(cpu, gdbserver_state.str_buf);
> -        g_string_append_c(gdbserver_state.str_buf, ';');
> -        gdb_put_strbuf();
> +        if (gdbserver_state.allow_stop_reply) {
> +            g_string_printf(gdbserver_state.str_buf,
> +                            "T%02xthread:", gdb_target_signal_to_gdb(sig));
> +            gdb_append_thread_id(cpu, gdbserver_state.str_buf);
> +            g_string_append_c(gdbserver_state.str_buf, ';');
> +            gdb_put_strbuf();
> +            gdbserver_state.allow_stop_reply = false;
> +        }
>      }
>      /*
>       * gdb_put_packet() might have detected that the peer terminated the
> @@ -174,12 +177,14 @@ void gdb_signalled(CPUArchState *env, int sig)
>  {
>      char buf[4];
>  
> -    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
> +        !gdbserver_state.allow_stop_reply) {
>          return;
>      }
>  
>      snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
>      gdb_put_packet(buf);
> +    gdbserver_state.allow_stop_reply = false;

Did I miss an equivalent for softmmu mode here?

Anyway:

Acked-by: Alex Bennée <alex.bennee@linaro.org>
Matheus Tavares Bernardino April 21, 2023, 11:31 a.m. UTC | #2 
Alex Bennée <alex.bennee@linaro.org> wrote:
>
> > Matheus Tavares <quic_mathbern@quicinc.com> wrote:
> >
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 80488b6bb9..bb03622c83 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -174,12 +177,14 @@ void gdb_signalled(CPUArchState *env, int sig)
> >  {
> >      char buf[4];
> >  
> > -    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
> > +    if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
> > +        !gdbserver_state.allow_stop_reply) {
> >          return;
> >      }
> >  
> >      snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
> >      gdb_put_packet(buf);
> > +    gdbserver_state.allow_stop_reply = false;
> 
> Did I miss an equivalent for softmmu mode here?

Hmm, there doesn't seem to be other "X aa" stop-replies sent from
softmmu in our gdbstub. However, I just realize I did miss another spot
of "W aa" at gdbstub/user.c:gdb_exit(). I'll add the allow_stop_reply
guard there for the next iteration.
diff mbox series

Patch

diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 94ddff4495..33d21d6488 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -65,6 +65,11 @@  typedef struct GDBState {
     GByteArray *mem_buf;
     int sstep_flags;
     int supported_sstep_flags;
+    /*
+     * Whether we are allowed to send a stop reply packet at this moment.
+     * Must be set off after sending the stop reply itself.
+     */
+    bool allow_stop_reply;
 } GDBState;
 
 /* lives in main gdbstub.c */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 0760d78685..be18568d0a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -777,6 +777,10 @@  typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
 /*
  * cmd_startswith -> cmd is compared using startswith
  *
+ * allow_stop_reply -> true iff the gdbstub can respond to this command with a
+ *   "stop reply" packet. The list of commands that accept such response is
+ *   defined at the GDB Remote Serial Protocol documentation. see:
+ *   https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
  *
  * schema definitions:
  * Each schema parameter entry consists of 2 chars,
@@ -802,6 +806,7 @@  typedef struct GdbCmdParseEntry {
     const char *cmd;
     bool cmd_startswith;
     const char *schema;
+    bool allow_stop_reply;
 } GdbCmdParseEntry;
 
 static inline int startswith(const char *string, const char *pattern)
@@ -835,6 +840,7 @@  static int process_string_cmd(void *user_ctx, const char *data,
             }
         }
 
+        gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
         cmd->handler(params, user_ctx);
         return 0;
     }
@@ -1283,11 +1289,14 @@  static void handle_v_attach(GArray *params, void *user_ctx)
     gdbserver_state.g_cpu = cpu;
     gdbserver_state.c_cpu = cpu;
 
-    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, ';');
+    if (gdbserver_state.allow_stop_reply) {
+        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();
+        gdb_put_strbuf();
+    }
 }
 
 static void handle_v_kill(GArray *params, void *user_ctx)
@@ -1310,12 +1319,14 @@  static const GdbCmdParseEntry gdb_v_commands_table[] = {
         .handler = handle_v_cont,
         .cmd = "Cont",
         .cmd_startswith = 1,
+        .allow_stop_reply = true,
         .schema = "s0"
     },
     {
         .handler = handle_v_attach,
         .cmd = "Attach;",
         .cmd_startswith = 1,
+        .allow_stop_reply = true,
         .schema = "l0"
     },
     {
@@ -1698,10 +1709,13 @@  static void handle_gen_set(GArray *params, void *user_ctx)
 
 static void handle_target_halt(GArray *params, void *user_ctx)
 {
-    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();
+    if (gdbserver_state.allow_stop_reply) {
+        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,
      * because gdb is doing an initial connect and the state
@@ -1725,7 +1739,8 @@  static int gdb_handle_packet(const char *line_buf)
             static const GdbCmdParseEntry target_halted_cmd_desc = {
                 .handler = handle_target_halt,
                 .cmd = "?",
-                .cmd_startswith = 1
+                .cmd_startswith = 1,
+                .allow_stop_reply = true,
             };
             cmd_parser = &target_halted_cmd_desc;
         }
@@ -1736,6 +1751,7 @@  static int gdb_handle_packet(const char *line_buf)
                 .handler = handle_continue,
                 .cmd = "c",
                 .cmd_startswith = 1,
+                .allow_stop_reply = true,
                 .schema = "L0"
             };
             cmd_parser = &continue_cmd_desc;
@@ -1747,6 +1763,7 @@  static int gdb_handle_packet(const char *line_buf)
                 .handler = handle_cont_with_sig,
                 .cmd = "C",
                 .cmd_startswith = 1,
+                .allow_stop_reply = true,
                 .schema = "l0"
             };
             cmd_parser = &cont_with_sig_cmd_desc;
@@ -1785,6 +1802,7 @@  static int gdb_handle_packet(const char *line_buf)
                 .handler = handle_step,
                 .cmd = "s",
                 .cmd_startswith = 1,
+                .allow_stop_reply = true,
                 .schema = "L0"
             };
             cmd_parser = &step_cmd_desc;
@@ -1976,6 +1994,7 @@  void gdb_read_byte(uint8_t ch)
 {
     uint8_t reply;
 
+    gdbserver_state.allow_stop_reply = false;
 #ifndef CONFIG_USER_ONLY
     if (gdbserver_state.last_packet->len) {
         /* Waiting for a response to the last packet.  If we see the start
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 22ecd09d04..99d994e6bf 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -43,6 +43,7 @@  static void reset_gdbserver_state(void)
     g_free(gdbserver_state.processes);
     gdbserver_state.processes = NULL;
     gdbserver_state.process_num = 0;
+    gdbserver_state.allow_stop_reply = false;
 }
 
 /*
@@ -139,6 +140,10 @@  static void gdb_vm_state_change(void *opaque, bool running, RunState state)
         return;
     }
 
+    if (!gdbserver_state.allow_stop_reply) {
+        return;
+    }
+
     gdb_append_thread_id(cpu, tid);
 
     switch (state) {
@@ -205,6 +210,7 @@  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);
@@ -422,8 +428,11 @@  void gdb_exit(int code)
 
     trace_gdbstub_op_exiting((uint8_t)code);
 
-    snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
-    gdb_put_packet(buf);
+    if (gdbserver_state.allow_stop_reply) {
+        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 80488b6bb9..bb03622c83 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -127,11 +127,14 @@  int gdb_handlesig(CPUState *cpu, int sig)
 
     if (sig != 0) {
         gdb_set_stop_cpu(cpu);
-        g_string_printf(gdbserver_state.str_buf,
-                        "T%02xthread:", gdb_target_signal_to_gdb(sig));
-        gdb_append_thread_id(cpu, gdbserver_state.str_buf);
-        g_string_append_c(gdbserver_state.str_buf, ';');
-        gdb_put_strbuf();
+        if (gdbserver_state.allow_stop_reply) {
+            g_string_printf(gdbserver_state.str_buf,
+                            "T%02xthread:", gdb_target_signal_to_gdb(sig));
+            gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+            g_string_append_c(gdbserver_state.str_buf, ';');
+            gdb_put_strbuf();
+            gdbserver_state.allow_stop_reply = false;
+        }
     }
     /*
      * gdb_put_packet() might have detected that the peer terminated the
@@ -174,12 +177,14 @@  void gdb_signalled(CPUArchState *env, int sig)
 {
     char buf[4];
 
-    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
+        !gdbserver_state.allow_stop_reply) {
         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)