Re: [PATCH v2 RESEND 1/7] 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 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_mathbern@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.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro