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

[Qemu-devel] [PATCH v1 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.

As an example, consider this processing of the 'isatty' syscall.
'gdb_do_syscall' gets executed while 'current_run_state == RUN_STATE_RUNNING'.
It then changes the VM and GDB server state with:

    s->state = RS_SYSCALL;
    vm_stop(RUN_STATE_DEBUG);
    s->state = RS_IDLE;

which leaves the GDB server state in 'RS_IDLE', but since the syscall
happens while in the TCG thread the 'RUN_STATE_DEBUG' stop does not
happen immediately.  This leads to the following course of events:

1. 'gdb_do_syscall' finishes up by sending a 'Fisatty,00000001' packet to the
   GDB client.
2. 'RUN_STATE_DEBUG' get pulled from the event queue.
3. 'gdb_vm_state_change' is invoked thus causing a 'T05thread:01;' reply
   to be sent back to the GDB client.
4. A payload of '+$F1#77+' comes in containing the syscall reply and the
   'T05' ACK.
5. '+$F1#77' is processed and 'gdb_handle_packet' transitions the
   state back 'RUN_STATE_RUNNING'.
6. 'gdb_read_byte' starts processing the remaining payload of '+',
   but the current state is 'RUN_STATE_RUNNING' so 'gdb_read_byte' issues
   a 'vm_stop(RUN_STATE_PAUSED)' which takes effect immediately.
7. 'gdb_vm_state_change' is invoked thus causing a 'T02thread:01;' reply
   to be sent back to the GDB client.
8. The GDB client interrupts and breaks the semihosting flow.

This patch fixes the problem be staying in the 'RS_SYSCALL' state until next
packet read comes in.  Therefore keeping any 'T' statuses from being sent
back to the GDB client while the syscall is still being processed.

Meador Inge (1):
  gdbserver: Keep VM state status replies from happening during a
    syscall

 gdbstub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH v1 1/1] gdbserver: Keep VM state status replies from happening during a syscall

[Meador Inge]

Fix an issue where the GDB server implementation was allowing 'RUN_STATE_DEBUG'
transitions to send a signal trap status back to the GDB client while a syscall
is being processed.  This eventually resulted in sending a SIGINT to the GDB
client.

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 gdbstub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 7d470b6..34d2717 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2480,7 +2480,6 @@ 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++) = 'F';
@@ -2557,6 +2556,8 @@ static void gdb_read_byte(GDBState *s, int ch)
 #endif
     {
         switch(s->state) {
+        case RS_SYSCALL:
+            s->state = RS_IDLE;
         case RS_IDLE:
             if (ch == '$') {
                 s->line_buf_index = 0;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v1 1/1] gdbserver: Keep VM state status replies from happening during a syscall

[Blue Swirl]

On Wed, Feb 15, 2012 at 16:55, Meador Inge <meadori@codesourcery.com> wrote:
> Fix an issue where the GDB server implementation was allowing 'RUN_STATE_DEBUG'
> transitions to send a signal trap status back to the GDB client while a syscall
> is being processed.  This eventually resulted in sending a SIGINT to the GDB
> client.
>
> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> ---
>  gdbstub.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 7d470b6..34d2717 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2480,7 +2480,6 @@ 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++) = 'F';
> @@ -2557,6 +2556,8 @@ static void gdb_read_byte(GDBState *s, int ch)
>  #endif
>     {
>         switch(s->state) {
> +        case RS_SYSCALL:
> +            s->state = RS_IDLE;

Missing break statement or a comment about fallthrough.

>         case RS_IDLE:
>             if (ch == '$') {
>                 s->line_buf_index = 0;
> --
> 1.7.7.6
>
>

Re: [Qemu-devel] [PATCH v1 1/1] gdbserver: Keep VM state status replies from happening during a syscall

[Meador Inge]

On 02/15/2012 11:54 AM, Blue Swirl wrote:

> On Wed, Feb 15, 2012 at 16:55, Meador Inge <meadori@codesourcery.com> wrote:
>> Fix an issue where the GDB server implementation was allowing 'RUN_STATE_DEBUG'
>> transitions to send a signal trap status back to the GDB client while a syscall
>> is being processed.  This eventually resulted in sending a SIGINT to the GDB
>> client.
>>
>> Signed-off-by: Meador Inge <meadori@codesourcery.com>
>> ---
>>  gdbstub.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 7d470b6..34d2717 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -2480,7 +2480,6 @@ 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++) = 'F';
>> @@ -2557,6 +2556,8 @@ static void gdb_read_byte(GDBState *s, int ch)
>>  #endif
>>     {
>>         switch(s->state) {
>> +        case RS_SYSCALL:
>> +            s->state = RS_IDLE;
> 
> Missing break statement or a comment about fallthrough.

The fallthrough is intentional.  I will add a comment.


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

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

[Peter Maydell]

On 15 February 2012 16:55, Meador Inge <meadori@codesourcery.com> wrote:
> This patch fixes the problem be staying in the 'RS_SYSCALL' state until next
> packet read comes in.  Therefore keeping any 'T' statuses from being sent
> back to the GDB client while the syscall is still being processed.

Wouldn't it be more logical to stay in "we are processing a syscall"
state until we actually get the whole reply packet and process it
rather than merely until we get the first byte of the reply?
(this probably implies it being a separate flag rather than an RSState
state, but that's cleaner anyhow I think since the RSStates represent
the state machine as we work through parsing the packet, and RS_SYSCALL
isn't a distinct parsing state.)

This patch works (in that it fixes this problem with a test case I have
coincidentally received from another reporter this week), although I
notice that doing read/write syscalls via gdb is dreadfully slow
because there seems to be ~1second delay between gdb sending its response
to a syscall (Fwrite/Fread) packet and getting the ack back from qemu.
I'm guessing that's a different bug...

-- PMM

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

[Peter Maydell]

I wrote:
> This patch works (in that it fixes this problem with a test case I have
> coincidentally received from another reporter this week), although I
> notice that doing read/write syscalls via gdb is dreadfully slow
> because there seems to be ~1second delay between gdb sending its response
> to a syscall (Fwrite/Fread) packet and getting the ack back from qemu.
> I'm guessing that's a different bug...

In fact it's an extremely close relative of the bug you're trying to
fix, and it shows why the approach you're trying to take here is the
wrong one. Basically the stalls occur when gdb sends us the reply
to our syscall before we've got round to processing the vm_stop():
in this case gdb_read_byte() will drop the initial "$" on the floor
and then send (another) vm_stop(). After this we drop the whole of the
reply packet (because we're in RS_IDLE/RS_SYSCALL and the "$" was
dropped rather than handled). So we stall until gdb times out and
retransmits the packet.

Some debug tracing demonstrating this: here's a "good" syscall
(# comments mine):

gdb_do_syscall: vm_stop(RUN_STATE_DEBUG)
reply='Fread,00000003,04000188,00000200'
gdb_vm_state_change: to 0
# we got the vm_state_change first...
gdb_chr_receive bytes 1042
Got ACK
# so we take the "+" ACK and then handle the packet
<5:$><2:M><2:4><2:0><2:0><2:0><2:1><2:8><2:8><2:,><2:2><2:0><2:0><2::><2:3><2:4><2:3><2:5><2:3><2:2><2:3><2:2><2:0><2:9><2:3><2:1><2:3><2:4><2:3><2:4><2:3><2:3><2:3><2:8><2:3><2:6><2:3><2:2><2:3><2:3><2:3><2:2><2:3><2:7><2:0><2:9><2:3><2:3><2:3><2:5><2:3><2:9><2:3><2:7><2:3><2:7><2:3><2:2><2:3><2:2>
# this tracing is <s->state:char>; I've snipped the dull middle bit out of it
<2:7><2:#><3:0><4:3>command='M4000188,200:343532320931343433383632333237093335393737323233340a313332333532340939313030343533323909323639393834360a323037313036353035340933333438373030313909313232313039383630350a313433313031363434390933303239343136303409313037323631363436380a33363337323434300931353134373636363009313533393131343436320a333438313332353031093131383038383930373909313732333831303638360a32303033303836343639093133333831333634373609313537363139353139350a333838353638383330093136343832333036343209313331303035373931310a33373333343736343009363437373232363633093939303738343230370a313830323134383139340934363932383339313709313831333238343936370a3130393835323638373309383239303536313531093133393836333037380a3230303835373232303209383331373535393937093337343936373630300a3139353935383537330932303532383534363032093337363239313132350a34393839303031373809393737393837343232093239393837323533310a3635303337363833380933363936313832333609313733303838383938300a31383331323635393137093230393334323839323309313736373236313432300a3130323139313837343609313532323134303437'
# that was GDB writing the memory for our read()
reply='OK'
gdb_chr_receive bytes 9
Got ACK
<1:$><2:F><2:2><2:0><2:0><2:#><3:d><4:8>command='F200'
# and the final status from the read(), so go again:
gdb_continue: vm_start()
gdb_vm_state_change: to 1


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:3633390931333839303739333432093533353238363134310a31363432363633313938093437343432363309313538323438323433370a313033333230363230320938343431363939333909313135333236333539300a3139393238363531323809323836373931363331093138313232363531330a313635303939343537310931343835353131383034093938363437383235370a323132343839383133380938343839333436383309313133313335323334360a313534313431373534300939343331393034393509313134353135313232350a33303338373232360938373730363839373209313234353033363432310a313339303836353732350939353631333431353809313630383334303633340a38333230373736343509313733313139303935320936353132303335360a37333637333333390931313839393334343609313232303538353437320a3738343137363033093137303134373538383309313636333536383131310a3932323538373534320937303732353538323509313135383734373636310a31323039333739313734093838383438323333390934343437303231360a353437343037333330093138373439363035393609323033373333353334340a313339363334323031330938353838323932393409313534303834363236370a3139323034383836300932303033393830353139'
# etc


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

-- PMM

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

[Meador Inge]

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

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

[Peter Maydell]

On 16 February 2012 18:39, Meador Inge <meadori@codesourcery.com> wrote:
> On 02/15/2012 02:14 PM, Peter Maydell wrote:
>> 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):

That looks roughly OK, but:
 * shouldn't gdb_syscall_buf[] be in GDBState ?
 * I don't think the "are we stopping to do a syscall?" flag should be
   implemented as an RSState enum -- that enum is for the
parsing-incoming-packet
   state machine

Bonus extra semihosting bug: if you start with "-gdb none" rather than "-s" then
we segfault, because gdbserver_start() creates a GDBState with a NULL s->chr
but use_gdb_syscalls() only looks at whether gdbserver_state is non-NULL, not
whether s->state is RS_INACTIVE, so the first gdb_do_syscall() ends up
dereferencing that NULL pointer. (Watch out when fixing this that you don't
break semihosting in linux-user mode, because at the moment linux-user mode
doesn't set up s->state at all so it's always RS_INACTIVE... We may also
want to declare that mixing all of gdb, semihosting and fork() in a linux-user
guest is not supported ;-))

-- PMM

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

[Meador Inge]

On 02/16/2012 01:08 PM, Peter Maydell wrote:

> On 16 February 2012 18:39, Meador Inge <meadori@codesourcery.com> wrote:
>> On 02/15/2012 02:14 PM, Peter Maydell wrote:
>>> 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):
> 
> That looks roughly OK, but:
>  * shouldn't gdb_syscall_buf[] be in GDBState ?
>  * I don't think the "are we stopping to do a syscall?" flag should be
>    implemented as an RSState enum -- that enum is for the
> parsing-incoming-packet
>    state machine

I cleaned up these bits.  v2 patch coming up soon.

> Bonus extra semihosting bug: if you start with "-gdb none" rather than "-s" then
> we segfault, because gdbserver_start() creates a GDBState with a NULL s->chr
> but use_gdb_syscalls() only looks at whether gdbserver_state is non-NULL, not
> whether s->state is RS_INACTIVE, so the first gdb_do_syscall() ends up
> dereferencing that NULL pointer. (Watch out when fixing this that you don't
> break semihosting in linux-user mode, because at the moment linux-user mode
> doesn't set up s->state at all so it's always RS_INACTIVE... We may also
> want to declare that mixing all of gdb, semihosting and fork() in a linux-user
> guest is not supported ;-))

I will take a look at that one as a separate patch :-)

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