[PATCH] gdbstub: only send stop-reply upon request

[PATCH] gdbstub: only send stop-reply upon request

[Matheus Tavares Bernardino]

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 <quic_mathbern@quicinc.com>
---
 gdbstub.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

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?  */
-- 
2.25.1

Re: [PATCH] gdbstub: only send stop-reply upon request

[Matheus Tavares Bernardino]

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.

Re: [PATCH] gdbstub: only send stop-reply upon request

[Peter Maydell]

On Tue, 23 Aug 2022 at 16:07, Matheus Tavares Bernardino
<quic_mathbern@quicinc.com> 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

[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

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

[Matheus Tavares Bernardino]

On Wed, 24 Aug 2022 at 14:51, Matheus Tavares Bernardino <quic_mathbern@quicinc.com> wrote:
>
> 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>
> ---

Gentle ping :)

Any feedback will be highly appreciated.

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

[Brian Cain]

> -----Original Message-----
> From: Qemu-devel <qemu-devel-bounces+bcain=quicinc.com@nongnu.org>
> On Behalf Of Matheus Tavares Bernardino
...
> On Wed, 24 Aug 2022 at 14:51, Matheus Tavares Bernardino
> <quic_mathbern@quicinc.com> wrote:
> >
> > 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>
> > ---
> 
> Gentle ping :)

Alex, Peter - any thoughts on this change?

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

[Alex Bennée]

Brian Cain <bcain@quicinc.com> writes:

>> -----Original Message-----
>> From: Qemu-devel <qemu-devel-bounces+bcain=quicinc.com@nongnu.org>
>> On Behalf Of Matheus Tavares Bernardino
> ...
>> On Wed, 24 Aug 2022 at 14:51, Matheus Tavares Bernardino
>> <quic_mathbern@quicinc.com> wrote:
>> >
>> > 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>
>> > ---
>> 
>> Gentle ping :)
>
> Alex, Peter - any thoughts on this change?

Sorry I was super busy on the run up to KVM forum, I have given some
thoughts today.

-- 
Alex Bennée

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

[Alex Bennée]

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 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.

Does this mean we can't have a test case that exercises this behaviour
with gdb? I'm guessing it will be tricky to exercise anyway because we'd
need to trigger a vm state change.

> 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()?

Hmm probably - that is certainly the implication of:

  https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets

>
>  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_inde;
>      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';

I'm not super keen on adding another static buffer to the gdb state
especially as we've been slowly removing the others in favour of
GString's where appropriate. More over isn't this really a boolean state
we want, maybe .allow_stop_reply?

>  }
>  #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;
>  }

I guess this is changed to allow the later check against RS_IDLE. May I
suggest a better place would be to extend GdbCmdParseEntry to contain
the value of .allow_stop_reply which we could set on successful handling
of a packet in process_string_cmd, something like:

        cmd->handler(params, user_ctx);
        gdbserver_state.allow_stop_reply = cmd.allow_stop_reply;
        return 0;

And then just annotate the command table entries for commands that
explicitly allow it.

>  
>  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;
> +}
> +

We can then drop this.

>  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;
> +    }
> +

Then this becomes a simple test of

       if (!gdbserver_state.allow_stop_reply) {
          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:

Please post the next revision as a standalone patch rather than a reply
to previous versions. It tends to confuse tooling.

Thanks,

-- 
Alex Bennée

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

[Matheus Bernardino (QUIC)]

> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes:
> 
> >
> > 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.
> 
> Does this mean we can't have a test case that exercises this behaviour
> with gdb? I'm guessing it will be tricky to exercise anyway because we'd
> need to trigger a vm state change.

Hmm, I think we can test it by enabling debug information on gdb
(`set debug remote 1`) and then checking stderr for a "invalid reply"
message. Simply attaching to QEMU would trigger the vm state
change, I think. I will try it out and send a reroll with that soon-ish.

> > 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()?
> 
> Hmm probably - that is certainly the implication of:
> 
>   https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-
> Reply-Packets

OK, will do.

> >  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_inde;
> >      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';
> 
> I'm not super keen on adding another static buffer to the gdb state
> especially as we've been slowly removing the others in favour of
> GString's where appropriate. More over isn't this really a boolean state
> we want, maybe .allow_stop_reply?

Yeah, makes sense.

> >  }
> >  #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;
> >  }
> 
> I guess this is changed to allow the later check against RS_IDLE. May I
> suggest a better place would be to extend GdbCmdParseEntry to contain
> the value of .allow_stop_reply which we could set on successful handling
> of a packet in process_string_cmd, something like:
> 
>         cmd->handler(params, user_ctx);
>         gdbserver_state.allow_stop_reply = cmd.allow_stop_reply;
>         return 0;
> 
> And then just annotate the command table entries for commands that
> explicitly allow it.

Good call, will do.

> Please post the next revision as a standalone patch rather than a reply
> to previous versions. It tends to confuse tooling.

Oh, I see. Thanks for the tip and the comments above!

Best,
Matheus