qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alex Bennée
Subject: Re: [RFC PATCH v2] gdbstub: only send stop-reply packets when allowed to
Date: Fri, 16 Sep 2022 11:33:10 +0100
User-agent: mu4e 1.9.0; emacs 28.1.91

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,

-- 
Alex Bennée



reply via email to

[Prev in Thread] Current Thread [Next in Thread]