Patchwork gdbserver: Don't send a GDB syscall until the system CPU is stopped

login
register
mail settings
Submitter Peter Maydell
Date March 15, 2012, 5:49 p.m.
Message ID <1331833785-9377-1-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/147052/
State New
Headers show

Comments

Peter Maydell - March 15, 2012, 5:49 p.m.
From: Meador Inge <meadori@codesourcery.com>

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 <meadori@codesourcery.com>
[PMM: trivial rebase, reinstated comma after last item in RSState enum]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
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(-)
Andreas Färber - March 15, 2012, 5:54 p.m.
Am 15.03.2012 18:49, schrieb Peter Maydell:
> From: Meador Inge <meadori@codesourcery.com>
> 
> 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 <meadori@codesourcery.com>
> [PMM: trivial rebase, reinstated comma after last item in RSState enum]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

> ---
> 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.

Thanks, Peter!

Andreas

> 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(-)
Peter Maydell - March 23, 2012, 11:46 a.m.
Ping^3...

-- PMM

On 15 March 2012 17:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> From: Meador Inge <meadori@codesourcery.com>
>
> 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 <meadori@codesourcery.com>
> [PMM: trivial rebase, reinstated comma after last item in RSState enum]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> 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
Blue Swirl - March 24, 2012, 4:13 p.m.
Thanks, applied.

On Thu, Mar 15, 2012 at 17:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> From: Meador Inge <meadori@codesourcery.com>
>
> 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 <meadori@codesourcery.com>
> [PMM: trivial rebase, reinstated comma after last item in RSState enum]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> 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
>
>

Patch

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;
 }