Re: [Qemu-devel] [PATCH] gdbserver: Don't send a GDB syscall until the system CPU is stopped

[Peter Maydell]

Ping^3...

-- PMM

On 15 March 2012 17:49, Peter Maydell <address@hidden> wrote:
> From: Meador Inge <address@hidden>
>
> Fix an issue where the GDB server implementation was sending GDB syscall
> requests while the system CPU was still running.  Syscall requests must
> be sent while the CPU is stopped otherwise replies from the GDB client
> might get dropped and the GDB server might be incorrectly transitioned
> into a 'RUN_STATE_PAUSED' state.
>
> Signed-off-by: Meador Inge <address@hidden>
> [PMM: trivial rebase, reinstated comma after last item in RSState enum]
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> This patch got (trivially) busted by Andreas' commits changing
> CPUState to CPUArchState so I've rebased and resent it. I've also
> made the trivial style nit fix of not deleting the final comma in
> the RSState enum, based on conversation with Andreas in IRC.
>
> This patch has sitting on the list for about a month reviewed but
> unapplied (http://patchwork.ozlabs.org/patch/141867/) -- can
> somebody with commit rights apply it please?
>
>  gdbstub.c |   42 +++++++++++++++++++++++++++---------------
>  1 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index f4e97f7..6a7e2c4 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -284,7 +284,6 @@ enum RSState {
>     RS_GETLINE,
>     RS_CHKSUM1,
>     RS_CHKSUM2,
> -    RS_SYSCALL,
>  };
>  typedef struct GDBState {
>     CPUArchState *c_cpu; /* current CPU for step/continue ops */
> @@ -304,6 +303,8 @@ typedef struct GDBState {
>     CharDriverState *chr;
>     CharDriverState *mon_chr;
>  #endif
> +    char syscall_buf[256];
> +    gdb_syscall_complete_cb current_syscall_cb;
>  } GDBState;
>
>  /* By default use no IRQs and no timers while single stepping so as to
> @@ -346,8 +347,6 @@ static int get_char(GDBState *s)
>  }
>  #endif
>
> -static gdb_syscall_complete_cb gdb_current_syscall_cb;
> -
>  static enum {
>     GDB_SYS_UNKNOWN,
>     GDB_SYS_ENABLED,
> @@ -2097,8 +2096,10 @@ static int gdb_handle_packet(GDBState *s, const char 
> *line_buf)
>             if (*p == ',')
>                 p++;
>             type = *p;
> -            if (gdb_current_syscall_cb)
> -                gdb_current_syscall_cb(s->c_cpu, ret, err);
> +            if (s->current_syscall_cb) {
> +                s->current_syscall_cb(s->c_cpu, ret, err);
> +                s->current_syscall_cb = NULL;
> +            }
>             if (type == 'C') {
>                 put_packet(s, "T02");
>             } else {
> @@ -2398,7 +2399,12 @@ static void gdb_vm_state_change(void *opaque, int 
> running, RunState state)
>     const char *type;
>     int ret;
>
> -    if (running || s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
> +    if (running || s->state == RS_INACTIVE) {
> +        return;
> +    }
> +    /* Is there a GDB syscall waiting to be sent?  */
> +    if (s->current_syscall_cb) {
> +        put_packet(s, s->syscall_buf);
>         return;
>     }
>     switch (state) {
> @@ -2468,8 +2474,8 @@ send_packet:
>  void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
>  {
>     va_list va;
> -    char buf[256];
>     char *p;
> +    char *p_end;
>     target_ulong addr;
>     uint64_t i64;
>     GDBState *s;
> @@ -2477,14 +2483,13 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const 
> char *fmt, ...)
>     s = gdbserver_state;
>     if (!s)
>         return;
> -    gdb_current_syscall_cb = cb;
> -    s->state = RS_SYSCALL;
> +    s->current_syscall_cb = cb;
>  #ifndef CONFIG_USER_ONLY
>     vm_stop(RUN_STATE_DEBUG);
>  #endif
> -    s->state = RS_IDLE;
>     va_start(va, fmt);
> -    p = buf;
> +    p = s->syscall_buf;
> +    p_end = &s->syscall_buf[sizeof(s->syscall_buf)];
>     *(p++) = 'F';
>     while (*fmt) {
>         if (*fmt == '%') {
> @@ -2492,17 +2497,17 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const 
> char *fmt, ...)
>             switch (*fmt++) {
>             case 'x':
>                 addr = va_arg(va, target_ulong);
> -                p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx, addr);
> +                p += snprintf(p, p_end - p, TARGET_FMT_lx, addr);
>                 break;
>             case 'l':
>                 if (*(fmt++) != 'x')
>                     goto bad_format;
>                 i64 = va_arg(va, uint64_t);
> -                p += snprintf(p, &buf[sizeof(buf)] - p, "%" PRIx64, i64);
> +                p += snprintf(p, p_end - p, "%" PRIx64, i64);
>                 break;
>             case 's':
>                 addr = va_arg(va, target_ulong);
> -                p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx "/%x",
> +                p += snprintf(p, p_end - p, TARGET_FMT_lx "/%x",
>                               addr, va_arg(va, int));
>                 break;
>             default:
> @@ -2517,10 +2522,16 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const 
> char *fmt, ...)
>     }
>     *p = 0;
>     va_end(va);
> -    put_packet(s, buf);
>  #ifdef CONFIG_USER_ONLY
> +    put_packet(s, s->syscall_buf);
>     gdb_handlesig(s->c_cpu, 0);
>  #else
> +    /* In this case wait to send the syscall packet until notification that
> +       the CPU has stopped.  This must be done because if the packet is sent
> +       now the reply from the syscall request could be received while the CPU
> +       is still in the running state, which can cause packets to be dropped
> +       and state transition 'T' packets to be sent while the syscall is still
> +       being processed.  */
>     cpu_exit(s->c_cpu);
>  #endif
>  }
> @@ -2919,6 +2930,7 @@ int gdbserver_start(const char *device)
>     s->chr = chr;
>     s->state = chr ? RS_IDLE : RS_INACTIVE;
>     s->mon_chr = mon_chr;
> +    s->current_syscall_cb = NULL;
>
>     return 0;
>  }
> --
> 1.7.1