> Alex Bennée <alex.ben...@linaro.org> wrote: > > Matheus Tavares Bernardino <quic_mathb...@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
