[Qemu-devel] [PATCH v2 0/1] Fix GDB semihosting

[Qemu-devel] [PATCH v2 0/1] Fix GDB semihosting

[Meador Inge]

Hi All,

GDB semihosting support is broken in the current trunk.  When debugging a
basic "Hello, World" application via the QEMU GDB stub:

   $ qemu-system-arm -s -S -M integratorcp -cpu any --semihosting
     --monitor null --serial null -kernel hello

GDB (7.2.50) receives an interrupt before anything is printed:

   Program received signal SIGINT, Interrupt.

The fundamental issue is that the GDB server implementation sends syscall
requests while the system CPU is still running and isn't prepared to handle
the replies.  This patch fixes the problem by delaying syscall request until
the system CPU has stopped.

* Changes from v1
 - At the suggestion of Peter Maydell I changed the implementation to delay
   sending syscall requests until the CPU has stopped instead of incorrectly
   attempting to just suppress the sending of 'T' status replies.

Meador Inge (1):
  gdbserver: Don't send a GDB syscall until the system CPU is stopped

 gdbstub.c |   44 ++++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 16 deletions(-)

-- 
1.7.7.6

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

[Meador Inge]

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>
---
 gdbstub.c |   44 ++++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 7d470b6..a08532f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -283,8 +283,7 @@ enum RSState {
     RS_IDLE,
     RS_GETLINE,
     RS_CHKSUM1,
-    RS_CHKSUM2,
-    RS_SYSCALL,
+    RS_CHKSUM2
 };
 typedef struct GDBState {
     CPUState *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,
@@ -2095,8 +2094,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 {
@@ -2396,7 +2397,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) {
@@ -2466,8 +2472,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;
@@ -2475,14 +2481,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 == '%') {
@@ -2490,17 +2495,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:
@@ -2515,10 +2520,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
 }
@@ -2917,6 +2928,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.7.6

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

[Peter Maydell]

On 17 February 2012 16:21, Meador Inge <meadori@codesourcery.com> wrote:
> 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>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I can confirm that this fixes the "read/write syscalls stall" issue
I was seeing with your previous patch.

-- PMM

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

[Peter Maydell]

Ping? Who commits gdbstub patches?

-- PMM

On 20 February 2012 16:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 February 2012 16:21, Meador Inge <meadori@codesourcery.com> wrote:
>> 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>
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I can confirm that this fixes the "read/write syscalls stall" issue
> I was seeing with your previous patch.
>
> -- PMM