gdbstub: only send stop-reply upon request

Commit Message

Matheus Tavares Bernardino Aug. 23, 2022, 1:39 p.m. UTC

The GDB remote serial protocol[1] specifies that the communication
between GDB and the stub is driven by GDB:

    The host (GDB) sends commands, and the target (the debugging
    stub incorporated in your program) sends a response.

This is further emphasized by Embecosm's "Howto: GDB Remote Serial
Protocol" document[2], which says:

    This is the only circumstance under which the server sends a
    packet: in reply to a packet from the client requiring a
    response.

However, QEMU may send stop-reply packets even when not solicited by
GDB, as these are currently issued whenever the vm stops. 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 when it receives an
unexpected stop-reply from QEMU as attaching to it.

Instead, let's make QEMU send stop-reply messages only as a response to
a previous GDB command, and only to commands that accept a stop-reply
packet. According to [3], these are: 'C', 'c', 'S', 's', '?', 'vCont',
'vAttach', 'vRun', and 'vStopped'. The ones starting with 'v' are not
implemented by gdbstup.c, so we only have to handle the single-letter
ones.

[1]: https://sourceware.org/gdb/download/onlinedocs/gdb/Overview.html#Overview
[2]: https://www.embecosm.com/appnotes/ean4/embecosm-howto-rsp-server-ean4-issue-2.html
[3]: https://sourceware.org/gdb/download/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets

Signed-off-by: Matheus Tavares Bernardino <[e-mail]>
---
 gdbstub.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Comments

Matheus Tavares Bernardino Aug. 23, 2022, 3:37 p.m. UTC | #1

On Thu, Aug 32, 2022 at 1:39 PM Matheus Tavares Bernardino wrote:
>
> ---
>  gdbstub.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)

Please ignore this patch. This version is broken, I'm working on a new
one. Sorry for the noise.

Peter Maydell Aug. 23, 2022, 3:56 p.m. UTC | #2

On Tue, 23 Aug 2022 at 16:07, Matheus Tavares Bernardino
<[e-mail]> wrote:
>
> The GDB remote serial protocol[1] specifies that the communication
> between GDB and the stub is driven by GDB:
>
>     The host (GDB) sends commands, and the target (the debugging
>     stub incorporated in your program) sends a response.
>
> This is further emphasized by Embecosm's "Howto: GDB Remote Serial
> Protocol" document[2], which says:
>
>     This is the only circumstance under which the server sends a
>     packet: in reply to a packet from the client requiring a
>     response.

That document is 14 years old and is not a reliable reference
for the protocol today...

In particular,
https://sourceware.org/gdb/download/onlinedocs/gdb/Notification-Packets.html
says:
"The GDB remote serial protocol includes notifications, packets that
require no acknowledgment. Both the GDB and the stub may send
notifications (although the only notifications defined at present
are sent by the stub)."

That said, the thing we're sending here isn't a notification, so
it's not like we're in compliance with the protocol. So I guess
I'm just saying the commit message could be improved.

(The notification mechanism does allow notification of "we just
stopped" if the stub implements non-stop mode, but we don't and
it would be a real pain to try, so that's a bit of a red herring.)

thanks
-- PMM

Patch

diff --git a/gdbstub.c b/gdbstub.c
index cf869b10e3..b8fc0ce07b 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -339,6 +339,7 @@  enum RSState {
     RS_GETLINE_RLE,
     RS_CHKSUM1,
     RS_CHKSUM2,
+    RS_HANDLING_CMD,
 };
 typedef struct GDBState {
     bool init;       /* have we been initialised? */
@@ -2562,6 +2563,7 @@  static int gdb_handle_packet(const char *line_buf)
 {
     const GdbCmdParseEntry *cmd_parser = NULL;
 
+    gdbserver_state.state = RS_HANDLING_CMD;
     trace_gdbstub_io_command(line_buf);
 
     switch (line_buf[0]) {
@@ -2821,6 +2823,23 @@  void gdb_set_stop_cpu(CPUState *cpu)
 }
 
 #ifndef CONFIG_USER_ONLY
+static bool cmd_allows_stop_reply(const char *cmd)
+{
+    if (strlen(cmd) != 1) {
+        return false;
+    }
+    switch (cmd[0]) {
+    case 'C':
+    case 'c':
+    case 'S':
+    case 's':
+    case '?':
+        return true;
+    default:
+        return false;
+    }
+}
+
 static void gdb_vm_state_change(void *opaque, bool running, RunState state)
 {
     CPUState *cpu = gdbserver_state.c_cpu;
@@ -2829,7 +2848,8 @@  static void gdb_vm_state_change(void *opaque, bool running, RunState state)
     const char *type;
     int ret;
 
-    if (running || gdbserver_state.state == RS_INACTIVE) {
+    if (running || gdbserver_state.state != RS_HANDLING_CMD ||
+        !cmd_allows_stop_reply(gdbserver_state.line_buf)) {
         return;
     }
     /* Is there a GDB syscall waiting to be sent?  */