Matheus Tavares Bernardino <quic_mathb...@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 still issues
> unsolicited stop messages through gdb_vm_state_change().
>
> Although this behavior doesn't seem to cause problems with GDB itself
> (the messages are just ignored), it can impact other debuggers that
> implement the GDB remote serial protocol, like hexagon-lldb. Let's
> change the gdbstub to send stop messages only as a response to a
> previous GDB command that accepts such a reply.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathb...@quicinc.com>
> ---
> gdbstub/internals.h | 5 +++++
> gdbstub/gdbstub.c | 37 ++++++++++++++++++++++++++++---------
> gdbstub/softmmu.c | 13 +++++++++++--
> gdbstub/user.c | 17 +++++++++++------
> 4 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index 94ddff4495..33d21d6488 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -65,6 +65,11 @@ typedef struct GDBState {
> GByteArray *mem_buf;
> int sstep_flags;
> int supported_sstep_flags;
> + /*
> + * Whether we are allowed to send a stop reply packet at this moment.
> + * Must be set off after sending the stop reply itself.
> + */
> + bool allow_stop_reply;
> } GDBState;
>
> /* lives in main gdbstub.c */
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 0760d78685..be18568d0a 100644
<snip>
> /*
> @@ -139,6 +140,10 @@ static void gdb_vm_state_change(void *opaque, bool
> running, RunState state)
> return;
> }
>
> + if (!gdbserver_state.allow_stop_reply) {
> + return;
> + }
> +
> gdb_append_thread_id(cpu, tid);
>
> switch (state) {
> @@ -205,6 +210,7 @@ static void gdb_vm_state_change(void *opaque, bool
> running, RunState state)
>
> send_packet:
> gdb_put_packet(buf->str);
> + gdbserver_state.allow_stop_reply = false;
>
> /* disable single step if it was enabled */
> cpu_single_step(cpu, 0);
> @@ -422,8 +428,11 @@ void gdb_exit(int code)
>
> trace_gdbstub_op_exiting((uint8_t)code);
>
> - snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
> - gdb_put_packet(buf);
> + if (gdbserver_state.allow_stop_reply) {
> + snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
> + gdb_put_packet(buf);
> + gdbserver_state.allow_stop_reply = false;
> + }
>
> qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
> }
> diff --git a/gdbstub/user.c b/gdbstub/user.c
> index 80488b6bb9..bb03622c83 100644
> --- a/gdbstub/user.c
> +++ b/gdbstub/user.c
> @@ -127,11 +127,14 @@ int gdb_handlesig(CPUState *cpu, int sig)
>
> if (sig != 0) {
> gdb_set_stop_cpu(cpu);
> - g_string_printf(gdbserver_state.str_buf,
> - "T%02xthread:", gdb_target_signal_to_gdb(sig));
> - gdb_append_thread_id(cpu, gdbserver_state.str_buf);
> - g_string_append_c(gdbserver_state.str_buf, ';');
> - gdb_put_strbuf();
> + if (gdbserver_state.allow_stop_reply) {
> + g_string_printf(gdbserver_state.str_buf,
> + "T%02xthread:", gdb_target_signal_to_gdb(sig));
> + gdb_append_thread_id(cpu, gdbserver_state.str_buf);
> + g_string_append_c(gdbserver_state.str_buf, ';');
> + gdb_put_strbuf();
> + gdbserver_state.allow_stop_reply = false;
> + }
> }
> /*
> * gdb_put_packet() might have detected that the peer terminated the
> @@ -174,12 +177,14 @@ void gdb_signalled(CPUArchState *env, int sig)
> {
> char buf[4];
>
> - if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
> + if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
> + !gdbserver_state.allow_stop_reply) {
> return;
> }
>
> snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
> gdb_put_packet(buf);
> + gdbserver_state.allow_stop_reply = false;
Did I miss an equivalent for softmmu mode here?
Anyway:
Acked-by: Alex Bennée <alex.ben...@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
