[RFC PATCH v2] gdbstub: only send stop-reply packets when allowed to

[Matheus Tavares Bernardino]

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 may still issue
unsolicited stop messages through gdb_vm_state_change().

Although this behavior doesn't seem to cause problems with GDB itself,
it does with other debuggers that implement the GDB remote serial
protocol, like hexagon-lldb. In this case, the debugger fails upon an
unexpected stop-reply message from QEMU when lldb attaches to it.

Instead, let's change gdb_set_stop_cpu() to send stop messages only as a
response to a previous GDB command, also making sure to check that the
command accepts such a reply.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---

Thanks Peter for pointing out about the notification packets at v1.

Changes in this version include: improvements in the commit message;
proper handling of the idle state (so that stop-replies can be sent
later, e.g. when the program is stopped due to a watchpoint); and
inclusion of other implemented GDB cmds that accept stop-reply as a
response.

There are three additional places that I think may send stop-reply
packages asynchronously, but I haven't touched those as I'm not sure if
that is really needed:

- gdb_exit() sends a "W" reply.
- gdb_signalled() sends "X".
- gdb_handlesig() sends "T".

Should we also restrict the message sending at these functions with the
same rules added to gdb_vm_state_change()?

 gdbstub.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index cf869b10e3..23507f21ca 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -350,6 +350,7 @@ typedef struct GDBState {
     int line_buf_index;
     int line_sum; /* running checksum */
     int line_csum; /* checksum at the end of the packet */
+    char last_cmd[MAX_PACKET_LENGTH];
     GByteArray *last_packet;
     int signal;
 #ifdef CONFIG_USER_ONLY
@@ -412,6 +413,7 @@ static void reset_gdbserver_state(void)
     g_free(gdbserver_state.processes);
     gdbserver_state.processes = NULL;
     gdbserver_state.process_num = 0;
+    gdbserver_state.last_cmd[0] = '\0';
 }
 #endif
 
@@ -2558,7 +2560,7 @@ static void handle_target_halt(GArray *params, void 
*user_ctx)
     gdb_breakpoint_remove_all();
 }
 
-static int gdb_handle_packet(const char *line_buf)
+static void gdb_handle_packet(const char *line_buf)
 {
     const GdbCmdParseEntry *cmd_parser = NULL;
 
@@ -2800,8 +2802,6 @@ static int gdb_handle_packet(const char *line_buf)
     if (cmd_parser) {
         run_cmd_parser(line_buf, cmd_parser);
     }
-
-    return RS_IDLE;
 }
 
 void gdb_set_stop_cpu(CPUState *cpu)
@@ -2821,8 +2821,14 @@ void gdb_set_stop_cpu(CPUState *cpu)
 }
 
 #ifndef CONFIG_USER_ONLY
+static inline bool char_in(char c, const char *str)
+{
+    return strchr(str, c) != NULL;
+}
+
 static void gdb_vm_state_change(void *opaque, bool running, RunState state)
 {
+    const char *cmd = gdbserver_state.last_cmd;
     CPUState *cpu = gdbserver_state.c_cpu;
     g_autoptr(GString) buf = g_string_new(NULL);
     g_autoptr(GString) tid = g_string_new(NULL);
@@ -2843,6 +2849,18 @@ static void gdb_vm_state_change(void *opaque, bool 
running, RunState state)
         return;
     }
 
+    /*
+     * We don't implement notification packets, so we should only send a
+     * stop-reply in response to a previous GDB command. Commands that accept
+     * stop-reply packages are: C, c, S, s, ?, vCont, vAttach, vRun, and
+     * vStopped. We don't implement vRun, and vStopped. For vAttach and ?, the
+     * stop-reply is already sent from their respective cmd handler functions.
+     */
+    if (gdbserver_state.state != RS_IDLE || /* still parsing the cmd */
+        !(startswith(cmd, "vCont;") || (strlen(cmd) == 1 && char_in(cmd[0], 
"cCsS")))) {
+        return;
+    }
+
     gdb_append_thread_id(cpu, tid);
 
     switch (state) {
@@ -3130,11 +3148,14 @@ static void gdb_read_byte(uint8_t ch)
                 reply = '-';
                 put_buffer(&reply, 1);
                 gdbserver_state.state = RS_IDLE;
+                gdbserver_state.last_cmd[0] = '\0';
             } else {
                 /* send ACK reply */
                 reply = '+';
                 put_buffer(&reply, 1);
-                gdbserver_state.state = 
gdb_handle_packet(gdbserver_state.line_buf);
+                strcpy(gdbserver_state.last_cmd, gdbserver_state.line_buf);
+                gdbserver_state.state = RS_IDLE;
+                gdb_handle_packet(gdbserver_state.line_buf);
             }
             break;
         default:
-- 
2.25.1