Re: [Qemu-devel] [PATCH v1 0/1] Fix GDB semihosting

[Meador Inge <meadori@codesourcery.com>]

On 02/15/2012 02:14 PM, Peter Maydell wrote:

> Here's tracing when it goes wrong:
> gdb_do_syscall: vm_stop(RUN_STATE_DEBUG)
> reply='Fread,00000003,04000188,00000200'
> gdb_chr_receive bytes 1042
> # got the data back before the state change
> Got ACK
> dropping char $, vm_stop(RUN_STATE_PAUSED)
> # ...so gdb_read_byte drops this $ and sends a spurious state change:
> gdb_vm_state_change: to 0
> <5:M><1:4><1:0><1:0><1:0><1:1><1:8><1:8><1:,><1:2><1:0><1:0><1::><1:3><1:6><1:3><1:3><1:3><1:9><1:0><1:9><1:3><1:1><1:3><1:3><1:3><1:8><1:3><1:9><1:3><1:0><1:3><1:7><1:3><1:9><1:3>
> # and we end up ignoring the whole packet
> <1:1><1:2>gdb_chr_receive bytes 1041
> # gdb got bored and retransmitted
> <1:$><2:M><2:4><2:0><2:0>
> # snip again; this time we got it, though.
> <2:#><3:1><4:2>command='M4000188,200:3633390931333839303739333432093533353238363134310a31363432363633313938093437343432363309313538323438323433370a313033333230363230320938343431363939333909313135333236333539300a3139393238363531323809323836373931363331093138313232363531330a313635303939343537310931343835353131383034093938363437383235370a323132343839383133380938343839333436383309313133313335323334360a313534313431373534300939343331393034393509313134353135313232350a33303338373232360938373730363839373209313234353033363432310a313339303836353732350939353631333431353809313630383334303633340a38333230373736343509313733313139303935320936353132303335360a37333637333333390931313839393334343609313232303538353437320a3738343137363033093137303134373538383309313636333536383131310a3932323538373534320937303732353538323509313135383734373636310a31323039333739313734093838383438323333390934343437303231360a353437343037333330093138373439363035393609323033373333353334340a3133393633343230313309383538383239323934
09313534303834363236370a3139323034383836300932303033393830353139'
> # etc

Ah, I see.  In my current patch a reply to the syscall request can still come
in while the CPU is in the running state.  Thank you very much for the analysis.

> I think the right way to deal with both the problem you were seeing
> and this related issue is simply not to try to send the syscall
> request until we have really stopped the CPU. That is, when not
> in CONFIG_USER_ONLY we should send the syscall request from
> gdb_vm_state_change().

I agree.  I am doing some more testing and will send an official v2 patch
later, but just to make sure I am on the right track something like (this
worked in the basic testing I have done so far and avoids the pitfall pointed
out above):

diff --git a/gdbstub.c b/gdbstub.c
index 7d470b6..66c3760 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -347,6 +347,7 @@ static int get_char(GDBState *s)
 #endif

 static gdb_syscall_complete_cb gdb_current_syscall_cb;
+static char gdb_syscall_buf[256];

 static enum {
     GDB_SYS_UNKNOWN,
@@ -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;
+    }
+    if (s->state == RS_SYSCALL) {
+        put_packet(s, gdb_syscall_buf);
+        s->state = RS_IDLE;
         return;
     }
     switch (state) {
@@ -2466,7 +2472,6 @@ send_packet:
 void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
 {
     va_list va;
-    char buf[256];
     char *p;
     target_ulong addr;
     uint64_t i64;
@@ -2480,9 +2485,8 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char
*fmt, ...)
 #ifndef CONFIG_USER_ONLY
     vm_stop(RUN_STATE_DEBUG);
 #endif
-    s->state = RS_IDLE;
     va_start(va, fmt);
-    p = buf;
+    p = gdb_syscall_buf;
     *(p++) = 'F';
     while (*fmt) {
         if (*fmt == '%') {
@@ -2490,18 +2494,20 @@ 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, &gdb_syscall_buf[sizeof(gdb_syscall_buf)] - 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, &gdb_syscall_buf[sizeof(gdb_syscall_buf)] - p,
+                              "%" PRIx64, i64);
                 break;
             case 's':
                 addr = va_arg(va, target_ulong);
-                p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx "/%x",
-                              addr, va_arg(va, int));
+                p += snprintf(p, &gdb_syscall_buf[sizeof(gdb_syscall_buf)] - p,
+                              TARGET_FMT_lx "/%x", addr, va_arg(va, int));
                 break;
             default:
             bad_format:
@@ -2515,10 +2521,17 @@ 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
+    s->state = RS_IDLE;
+    put_packet(s, gdb_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
 }

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software