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

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

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.

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

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,

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