Re: [PATCH] Make GDB wait for events after handling target File-I/O

[Luis Machado <lgustavo@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 5198 bytes --]

On 09/10/2015 09:59 AM, Pedro Alves wrote:
> On 08/28/2015 01:58 AM, Luis Machado wrote:
>> GDB was not behaving correctly with qemu-system debugging:
>>
>> _ftext () at arm-vector.S:25
>> 25              ldr pc, [pc, #24] @ reset
>> (gdb) load
>> Loading section .text, size 0xc01c lma 0x0
>> Loading section .eh_frame, size 0x48 lma 0xc01c
>> Loading section .ARM.exidx, size 0x8 lma 0xc064
>> Loading section .rodata, size 0x398 lma 0xc070
>> Loading section .data, size 0x8e0 lma 0xc408
>> Start address 0x40, load size 52452
>> Transfer rate: 17074 KB/sec, 1748 bytes/write.
>> (gdb) c
>> Continuing.
>> infrun: clear_proceed_status_thread (Thread 1)
>> infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT)
>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [Thread 1] at 0x40
>> Sending packet: $vCont?#49...Ack
>> Packet received:
>> Packet vCont (verbose-resume) is NOT supported
>> Sending packet: $Hc0#db...Ack
>> Packet received: OK
>> Sending packet: $c#63...Ack
>> infrun: infrun_async(1)
>> infrun: prepare_to_wait
>> infrun: target_wait (-1.0.0, status) =
>> infrun:   -1.0.0 [Thread 0],
>> infrun:   status->kind = ignore
>> infrun: TARGET_WAITKIND_IGNORE
>> infrun: prepare_to_wait
>> Packet received: Ffstat,00000001,07fffdb0
>> Sending packet: $M7fffdb0,40:000000000000000000002080000000010000c336000001180000000000000000000000000000000000000200000000000000000055dfb11b55dfb11b55dfb11b#5a...Ack
>> Packet received: OK
>> Sending packet: $F0#76...Ack
>> infrun: target_wait (-1.0.0, status) =
>> infrun:   -1.0.0 [Thread 0],
>> infrun:   status->kind = ignore
>> infrun: TARGET_WAITKIND_IGNORE
>> infrun: prepare_to_wait
>> infrun: target_wait (-1.0.0, status) =
>> infrun:   -1.0.0 [Thread 0],
>> infrun:   status->kind = no-resumed
>> infrun: TARGET_WAITKIND_NO_RESUMED
>> infrun: stop_waiting
>> infrun: clear_step_over_info
>> Sending packet: $qfThreadInfo#bb...Ack
>> Packet received: m1
>> Sending packet: $qsThreadInfo#c8...Ack
>> Packet received: l
>> No unwaited-for children left.
>> infrun: infrun_async(0)
>> (gdb) c
>> Continuing.
>> Cannot execute this command while the selected thread is running.
>> (gdb)
>> Continuing.
>> Cannot execute this command while the selected thread is running.
>>
>> This behavior shows up whenever GDB is in all-stop mode and is handling
>> target-initiated File-I/O requests, in the middle of, say, a continue
>> request.
>>
>> When GDB is done handling the File-I/O request, it doesn't set
>> rs->waiting_for_stop_reply back to 1, meaning GDB should wait for
>> further target events.
>>
>> This seems to be a latent bug, because in the past this didn't really
>> cause any issues. But it seems to have been uncovered by commit
>> 567420d10895611e03d5ee65e6b24c16a69a6e99, which explicitly checks
>> for rs->waiting_for_stop_reply == 0, triggering the failures above.
>>
>> The following patch fixes this by setting rs->waiting_for_stop_reply
>> back to 1 after handling the target-initiated File-I/O request.
>>
>> infrun: prepare_to_wait
>> Packet received: Ffstat,00000001,07fffdb0
>> Sending packet: $M7fffdb0,40:000000000000000000002080000000010000c336000001180000000000000000000000000000000000000200000000000000000055dfb19e55dfb19e55dfb19e#7b...Ack
>> Packet received: OK
>> Sending packet: $F0#76...Ack
>> infrun: target_wait (-1.0.0, status) =
>> infrun:   -1.0.0 [Thread 0],
>> infrun:   status->kind = ignore
>> infrun: TARGET_WAITKIND_IGNORE
>> infrun: prepare_to_wait
>> Packet received: Fisatty,00000001
>> Sending packet: $F1#77...Ack
>> infrun: target_wait (-1.0.0, status) =
>> infrun:   -1.0.0 [Thread 0],
>> infrun:   status->kind = ignore
>> infrun: TARGET_WAITKIND_IGNORE
>> infrun: prepare_to_wait
>> Packet received: Fwrite,00000001,0000d098,00000004
>> Sending packet: $md098,4#d2...Ack
>> Packet received: 3732300a
>> 720
>> Sending packet: $F4#7a...Ack
>> infrun: target_wait (-1.0.0, status) =
>> infrun:   -1.0.0 [Thread 0],
>> infrun:   status->kind = ignore
>> infrun: TARGET_WAITKIND_IGNORE
>> infrun: prepare_to_wait
>> Packet received: Fwrite,00000001,07ffffac,00000011
>> Sending packet: $m7ffffac,11#8e...Ack
>> Packet received: 0a2a2a2a204558495420636f646520300a
>>
>> *** EXIT code 0
>>
>> Regression-tested on Ubuntu x86-64 and qemu-system-based debugging
>> for arm eabi.
>>
>> OK?
>
> This is OK.  Though I'd prefer if we removed the:
>
>    /* We got something.  */
>    rs->waiting_for_stop_reply = 0;
>
> line, and then cleared waiting_for_stop_reply in both the
>
>   case 'E':
>   case 'T': case 'S': case 'X': case 'W':
>
> cases.  That'd make the check for waiting_for_stop_reply in
> interrupt_query work correctly while inside remote_console_output too:
>
>      case 'O':		/* Console output.  */
>        remote_console_output (buf + 1);
>
>        /* The target didn't really stop; keep waiting.  */
>        rs->waiting_for_stop_reply = 1;
>
> Thanks,
> Pedro Alves
>
[now cc-ing the list]

How does the attached update look? I did not move this statement:

rs->stop_reason = TARGET_STOPPED_BY_NO_REASON;

... since it only makes sense with the right value of 
rs->waiting_for_stop_reply, but it could also be set within each case 
statement.

[-- Attachment #2: fileio.diff --]
[-- Type: text/x-patch, Size: 1845 bytes --]

2015-10-16  Luis Machado  <lgustavo@codesourcery.com>

	* remote.c (remote_wait_as): Reset rs->waiting_for_stop_reply
	when handling 'E', 'T', 'S', 'X' and 'W' packets.
	Set rs->waiting_for_stop_reply to 1 after handling 'F' packets.

diff --git a/gdb/remote.c b/gdb/remote.c
index f40f791..87b0b58 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6635,9 +6635,6 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
 
   rs->stop_reason = TARGET_STOPPED_BY_NO_REASON;
 
-  /* We got something.  */
-  rs->waiting_for_stop_reply = 0;
-
   /* Assume that the target has acknowledged Ctrl-C unless we receive
      an 'F' or 'O' packet.  */
   if (buf[0] != 'F' && buf[0] != 'O')
@@ -6648,6 +6645,8 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
     case 'E':		/* Error of some sort.	*/
       /* We're out of sync with the target now.  Did it continue or
 	 not?  Not is more likely, so report a stop.  */
+      rs->waiting_for_stop_reply = 0;
+
       warning (_("Remote failure reply: %s"), buf);
       status->kind = TARGET_WAITKIND_STOPPED;
       status->value.sig = GDB_SIGNAL_0;
@@ -6655,10 +6654,19 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
     case 'F':		/* File-I/O request.  */
       remote_fileio_request (buf, rs->ctrlc_pending_p);
       rs->ctrlc_pending_p = 0;
+
+      /* GDB handled the File-I/O request, but the target is running
+	 again.  Keep waiting for events.  */
+      rs->waiting_for_stop_reply = 1;
       break;
     case 'T': case 'S': case 'X': case 'W':
       {
-	struct stop_reply *stop_reply
+	struct stop_reply *stop_reply;
+
+	/* There is a stop reply to handle.  */
+	rs->waiting_for_stop_reply = 0;
+
+	stop_reply
 	  = (struct stop_reply *) remote_notif_parse (&notif_client_stop,
 						      rs->buf);