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