[RFA] Fix gdbserver queued packet handling

[RFA] Fix gdbserver queued packet handling

[Doug Evans]

Hi.

gdbserver can read more than it needs to handle the current packet,
and then go to sleep in select waiting for the next packet.  Oops.

gdb handles this by using the timer support in the event loop to
queue more callbacks to finish processing buffered data.

e.g. ser-base.c:

	      next_state = create_timer (0, push_event, scb);

This isn't really a timer event, the delta is zero.
[btw, AFAICT, this is the only use of the timer support in the event loop]

This patch solves the problem in a similar way but doesn't bring over
the timer support from gdb's event-loop.c.
Timer support is a good thing to have in an event loop, but we don't need it,
at least not yet.

For reference sake, here's a patch to trigger the bug consistently:

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.402
diff -u -p -r1.402 remote.c
--- remote.c	16 Apr 2010 07:49:35 -0000	1.402
+++ remote.c	30 Apr 2010 16:37:59 -0000
@@ -6490,7 +6490,9 @@ putpkt_binary (char *buf, int cnt)
   int i;
   unsigned char csum = 0;
   char *buf2 = alloca (cnt + 6);
-
+  int is_noack_switch = strncmp (buf, "QStartNoAckMode",
+				 sizeof ("QStartNoAckMode")) == 0;
+  int timeout_test = ! rs->noack_mode && ! is_noack_switch;
   int ch;
   int tcount = 0;
   char *p;
@@ -6554,7 +6556,15 @@ putpkt_binary (char *buf, int cnt)
 	 Handle any notification that arrives in the mean time.  */
       while (1)
 	{
-	  ch = readchar (remote_timeout);
+	  if (timeout_test)
+	    {
+	      ch = SERIAL_TIMEOUT;
+	      timeout_test = 0;
+	    }
+	  else
+	    {
+	      ch = readchar (remote_timeout);
+	    }
 
 	  if (remote_debug)
 	    {


2010-04-30  Doug Evans  <dje@google.com>

	* server.h (queue_file_read_event): Declare.
	(reschedule_remote): Declare.
	* event-loop.c (queue_file_read_event): New function.
	* remote-utils.c (reschedule_remote): New function.
	(readchar_buf, readchar_bufcnt, readchar_bufp): New static globals,
	moved out of readchar.
	* server.c (handle_serial_event): Call reschedule_remote.

Index: event-loop.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/event-loop.c,v
retrieving revision 1.5
diff -u -p -r1.5 event-loop.c
--- event-loop.c	11 Apr 2010 16:33:55 -0000	1.5
+++ event-loop.c	30 Apr 2010 16:45:37 -0000
@@ -407,6 +407,37 @@ create_file_event (int fd)
   return file_event_ptr;
 }
 
+/* Queue a read event for file descriptor FD.
+   A handler must already be registered for FD.  */
+
+void
+queue_file_read_event (int fd)
+{
+  file_handler *file_ptr;
+  gdb_event *file_event_ptr;
+  int found = 0;
+
+  for (file_ptr = gdb_notifier.first_file_handler;
+       file_ptr != NULL;
+       file_ptr = file_ptr->next_file)
+    {
+      if (file_ptr->fd == fd)
+	{
+	  /* Enqueue an event only if there is no existing event for FD.  */
+	  if (file_ptr->ready_mask == 0)
+	    {
+	      file_event_ptr = create_file_event (file_ptr->fd);
+	      async_queue_event (file_event_ptr);
+	    }
+	  file_ptr->ready_mask |= GDB_READABLE;
+	  found = 1;
+	  break;
+	}
+    }
+
+  gdb_assert (found);
+}
+
 /* Called by do_one_event to wait for new events on the monitored file
    descriptors.  Queue file events as they are detected by the poll.
    If there are no events, this function will block in the call to
Index: remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.74
diff -u -p -r1.74 remote-utils.c
--- remote-utils.c	26 Apr 2010 17:38:07 -0000	1.74
+++ remote-utils.c	30 Apr 2010 16:45:37 -0000
@@ -926,23 +926,27 @@ initialize_async_io (void)
   unblock_async_io ();
 }
 
+/* Internal buffer used by readchar.
+   These are global to readchar because reschedule_remote needs to be
+   able to tell whether the buffer is empty.  */
+
+static unsigned char readchar_buf[BUFSIZ];
+static int readchar_bufcnt = 0;
+static unsigned char *readchar_bufp;
+
 /* Returns next char from remote GDB.  -1 if error.  */
 
 static int
 readchar (void)
 {
-  static unsigned char buf[BUFSIZ];
-  static int bufcnt = 0;
-  static unsigned char *bufp;
-
-  if (bufcnt-- > 0)
-    return *bufp++;
+  if (readchar_bufcnt-- > 0)
+    return *readchar_bufp++;
 
-  bufcnt = read (remote_desc, buf, sizeof (buf));
+  readchar_bufcnt = read (remote_desc, readchar_buf, sizeof (readchar_buf));
 
-  if (bufcnt <= 0)
+  if (readchar_bufcnt <= 0)
     {
-      if (bufcnt == 0)
+      if (readchar_bufcnt == 0)
 	fprintf (stderr, "readchar: Got EOF\n");
       else
 	perror ("readchar");
@@ -950,9 +954,19 @@ readchar (void)
       return -1;
     }
 
-  bufp = buf;
-  bufcnt--;
-  return *bufp++;
+  readchar_bufp = readchar_buf;
+  readchar_bufcnt--;
+  return *readchar_bufp++;
+}
+
+/* If there is still data in the buffer, queue another event to process it,
+   we can't sleep in select yet.  */
+
+void
+reschedule_remote (void)
+{
+  if (readchar_bufcnt > 0)
+    queue_file_read_event (remote_desc);
 }
 
 /* Read a packet from the remote machine, with error checking,
Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.118
diff -u -p -r1.118 server.c
--- server.c	26 Apr 2010 22:02:33 -0000	1.118
+++ server.c	30 Apr 2010 16:45:37 -0000
@@ -3009,6 +3009,10 @@ handle_serial_event (int err, gdb_client
      Important in the non-stop mode asynchronous protocol.  */
   set_desired_inferior (1);
 
+  /* Give the packet reader a chance to schedule more work before
+     we go to sleep.  */
+  reschedule_remote ();
+
   return 0;
 }
 
Index: server.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
retrieving revision 1.66
diff -u -p -r1.66 server.h
--- server.h	12 Apr 2010 13:51:22 -0000	1.66
+++ server.h	30 Apr 2010 16:45:37 -0000
@@ -341,6 +341,7 @@ typedef int (handler_func) (int, gdb_cli
 extern void delete_file_handler (int fd);
 extern void add_file_handler (int fd, handler_func *proc,
 			      gdb_client_data client_data);
+extern void queue_file_read_event (int fd);
 
 extern void start_event_loop (void);
 
@@ -372,6 +373,7 @@ int putpkt (char *buf);
 int putpkt_binary (char *buf, int len);
 int putpkt_notif (char *buf);
 int getpkt (char *buf);
+void reschedule_remote (void);
 void remote_open (char *name);
 void remote_close (void);
 void write_ok (char *buf);

Re: [RFA] Fix gdbserver queued packet handling

[Pedro Alves]

On Friday 30 April 2010 18:17:31, Doug Evans wrote:

> 2010-04-30  Doug Evans  <dje@google.com>
> 
> 	* server.h (queue_file_read_event): Declare.
> 	(reschedule_remote): Declare.
> 	* event-loop.c (queue_file_read_event): New function.
> 	* remote-utils.c (reschedule_remote): New function.
> 	(readchar_buf, readchar_bufcnt, readchar_bufp): New static globals,
> 	moved out of readchar.
> 	* server.c (handle_serial_event): Call reschedule_remote.

This looks good, thanks.  Please apply.

> @@ -3009,6 +3009,10 @@ handle_serial_event (int err, gdb_client
>       Important in the non-stop mode asynchronous protocol.  */
>    set_desired_inferior (1);
>  
> +  /* Give the packet reader a chance to schedule more work before
> +     we go to sleep.  */
> +  reschedule_remote ();
> +

This seems to break the abstraction a bit.  GDB attempts
a reschedule on every `readchar', and avoids unnecessary calls
into the event loop by maintaining a state machine, so
everything is nicelly hidden within the serial handling code.
But I'm fine with this simpler mechanism.  I can't think
of any reads from the socket outside of handle_serial_event;
we'd be still in trouble this way if there were.  In non-stop, we
push stop notifications to gdb when handling target events, but
asynchronous RSP notifications are a special kind of packet
that is never acked, even if no-ack is off, so not even those
cause reads that could cause data being pulled to the buffer.

-- 
Pedro Alves

Re: [RFA] Fix gdbserver queued packet handling

[Doug Evans]

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

On Fri, Apr 30, 2010 at 10:55 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> This seems to break the abstraction a bit.  GDB attempts
> a reschedule on every `readchar', and avoids unnecessary calls
> into the event loop by maintaining a state machine, so
> everything is nicelly hidden within the serial handling code.

Blech.  I wish I had gone with my original thought.

[btw, IWBN if one didn't have to have the state machine - e.g. if one
could query whether an event was scheduled.  One could register timer
callbacks first, get a stable handle to use in the query, and then
schedule them, for example.]

I like this patch better.
[Essentially all it does is the same thing gdb does, except that it
doesn't bring over the complexity of timer handling, which isn't
needed.]

P.S. IWBN if one didn't have to continually keep bringing stuff over
from gdb.  Wouldn't it be nice if one could just include an
event-loop.h (or some such) and use it.  I'm developing the feeling
that this is another instance where IWBN if gdb and gdbserver could
share code.

2010-05-03  Doug Evans  <dje@google.com>

        * event-loop.c (struct callback_event): New struct.
        (callback_list): New global.
        (append_callback_event, delete_callback_event): New functions.
        (process_callback): New function.
        (start_event_loop): Call it.
        * remote-utils.c (NOT_SCHEDULED): Define.
        (readchar_buf, readchar_bufcnt, readchar_bufp): New static globals,
        moved out of readchar.
        (readchar): Rewrite.  Call reschedule before returning.
        (reset_readchar): New function.
        (remote_close): Call it.
        (process_remaining, reschedule): New functions.
        * server.h (callback_handler_func): New typedef.
        (append_callback_event, delete_callback_event): Declare.

[-- Attachment #2: gdb-100503-gdbserver-queue-2.patch.txt --]
[-- Type: text/plain, Size: 8795 bytes --]

2010-05-03  Doug Evans  <dje@google.com>

	* event-loop.c (struct callback_event): New struct.
	(callback_list): New global.
	(append_callback_event, delete_callback_event): New functions.
	(process_callback): New function.
	(start_event_loop): Call it.
	* remote-utils.c (NOT_SCHEDULED): Define.
	(readchar_buf, readchar_bufcnt, readchar_bufp): New static globals,
	moved out of readchar.
	(readchar): Rewrite.  Call reschedule before returning.
	(reset_readchar): New function.
	(remote_close): Call it.
	(process_remaining, reschedule): New functions.
	* server.h (callback_handler_func): New typedef.
	(append_callback_event, delete_callback_event): Declare.

Index: event-loop.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/event-loop.c,v
retrieving revision 1.5
diff -u -p -r1.5 event-loop.c
--- event-loop.c	11 Apr 2010 16:33:55 -0000	1.5
+++ event-loop.c	3 May 2010 19:26:48 -0000
@@ -141,6 +141,36 @@ static struct
   }
 gdb_notifier;
 
+/* Callbacks are just routines that are executed before waiting for the
+   next event.  In GDB this is struct gdb_timer.  We don't need timers
+   so rather than copy all that complexity in gdbserver, we provide what
+   we need, but we do so in a way that if/when the day comes that we need
+   that complexity, it'll be easier to add - replace callbacks with timers
+   and use a delta of zero (which is all gdb currently uses timers for anyway).
+
+   PROC will be executed before gdbserver goes to sleep to wait for the
+   next event.  */
+
+struct callback_event
+  {
+    int id;
+    callback_handler_func *proc;
+    gdb_client_data *data;
+    struct callback_event *next;
+  };
+
+/* Table of registered callbacks.  */
+
+static struct
+  {
+    struct callback_event *first;
+    struct callback_event *last;
+
+    /* Id of the last callback created.  */
+    int num_callbacks;
+  }
+callback_list;
+
 /* Insert an event object into the gdb event queue.
 
    EVENT_PTR points to the event to be inserted into the queue.  The
@@ -220,6 +250,81 @@ process_event (void)
   return 0;
 }
 
+/* Append PROC to the callback list.
+   The result is the "id" of the callback that can be passed back to
+   delete_callback_event.  */
+
+int
+append_callback_event (callback_handler_func *proc, gdb_client_data data)
+{
+  struct callback_event *event_ptr;
+
+  event_ptr = xmalloc (sizeof (*event_ptr));
+  event_ptr->id = callback_list.num_callbacks++;
+  event_ptr->proc = proc;
+  event_ptr->data = data;
+  event_ptr->next = NULL;
+  if (callback_list.first == NULL)
+    callback_list.first = event_ptr;
+  if (callback_list.last != NULL)
+    callback_list.last->next = event_ptr;
+  callback_list.last = event_ptr;
+  return event_ptr->id;
+}
+
+/* Delete callback ID.
+   It is not an error callback ID doesn't exist.  */
+
+void
+delete_callback_event (int id)
+{
+  struct callback_event **p;
+
+  for (p = &callback_list.first; *p != NULL; p = &(*p)->next)
+    {
+      struct callback_event *event_ptr = *p;
+
+      if (event_ptr->id == id)
+	{
+	  *p = event_ptr->next;
+	  if (event_ptr == callback_list.last)
+	    callback_list.last = NULL;
+	  free (event_ptr);
+	  break;
+	}
+    }
+}
+
+/* Run the next callback.
+   The result is 1 if a callback was called and event processing
+   should continue, -1 if the callback wants the event loop to exit,
+   and 0 if there are no more callbacks.  */
+
+static int
+process_callback (void)
+{
+  struct callback_event *event_ptr;
+
+  event_ptr = callback_list.first;
+  if (event_ptr != NULL)
+    {
+      callback_handler_func *proc = event_ptr->proc;
+      gdb_client_data *data = event_ptr->data;
+
+      /* Remove the event before calling PROC,
+	 more events may get added by PROC.  */
+      callback_list.first = event_ptr->next;
+      if (callback_list.first == NULL)
+	callback_list.last = NULL;
+      free  (event_ptr);
+      if ((*proc) (data))
+	return -1;
+      return 1;
+    }
+
+  return 0;
+}
+
 /* Add a file handler/descriptor to the list of descriptors we are
    interested in.  FD is the file descriptor for the file/stream to be
    listened to.  MASK is a combination of READABLE, WRITABLE,
@@ -507,6 +612,16 @@ start_event_loop (void)
       if (res)
 	continue;
 
+      /* Process any queued callbacks before we go to sleep.  */
+      res = process_callback ();
+
+      /* Did the callback want the event loop to stop?  */
+      if (res == -1)
+	return;
+
+      if (res)
+	continue;
+
       /* Wait for a new event.  If wait_for_event returns -1, we
 	 should get out because this means that there are no event
 	 sources left.  This will make the event loop stop, and the
Index: remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.74
diff -u -p -r1.74 remote-utils.c
--- remote-utils.c	26 Apr 2010 17:38:07 -0000	1.74
+++ remote-utils.c	3 May 2010 19:26:48 -0000
@@ -80,7 +80,19 @@ typedef int socklen_t;
 # define INVALID_DESCRIPTOR -1
 #endif
 
+/* Extra value for readchar_callback.  */
+enum {
+  /* The callback is currently not scheduled.  */
+  NOT_SCHEDULED = -1
+};
+
+/* Status of the readchar callback.
+   Either NOT_SCHEDULED or the callback id.  */
+static int readchar_callback = NOT_SCHEDULED;
+
 static int readchar (void);
+static void reset_readchar (void);
+static void reschedule (void);
 
 /* A cache entry for a successfully looked-up symbol.  */
 struct sym_cache
@@ -341,6 +353,8 @@ remote_close (void)
   close (remote_desc);
 #endif
   remote_desc = INVALID_DESCRIPTOR;
+
+  reset_readchar ();
 }
 
 /* Convert hex digit A to a number.  */
@@ -926,33 +940,86 @@ initialize_async_io (void)
   unblock_async_io ();
 }
 
+/* Internal buffer used by readchar.
+   These are global to readchar because reschedule_remote needs to be
+   able to tell whether the buffer is empty.  */
+
+static unsigned char readchar_buf[BUFSIZ];
+static int readchar_bufcnt = 0;
+static unsigned char *readchar_bufp;
+
 /* Returns next char from remote GDB.  -1 if error.  */
 
 static int
 readchar (void)
 {
-  static unsigned char buf[BUFSIZ];
-  static int bufcnt = 0;
-  static unsigned char *bufp;
+  int ch;
 
-  if (bufcnt-- > 0)
-    return *bufp++;
+  if (readchar_bufcnt == 0)
+    {
+      readchar_bufcnt = read (remote_desc, readchar_buf, sizeof (readchar_buf));
 
-  bufcnt = read (remote_desc, buf, sizeof (buf));
+      if (readchar_bufcnt <= 0)
+	{
+	  if (readchar_bufcnt == 0)
+	    fprintf (stderr, "readchar: Got EOF\n");
+	  else
+	    perror ("readchar");
 
-  if (bufcnt <= 0)
-    {
-      if (bufcnt == 0)
-	fprintf (stderr, "readchar: Got EOF\n");
-      else
-	perror ("readchar");
+	  return -1;
+	}
 
-      return -1;
+      readchar_bufp = readchar_buf;
+    }
+
+  readchar_bufcnt--;
+  ch = *readchar_bufp++;
+  reschedule ();
+  return ch;
+}
+
+/* Reset the readchar state machine.  */
+
+static void
+reset_readchar (void)
+{
+  readchar_bufcnt = 0;
+  if (readchar_callback != NOT_SCHEDULED)
+    {
+      delete_callback_event (readchar_callback);
+      readchar_callback = NOT_SCHEDULED;
     }
+}
+
+/* Process remaining data in readchar_buf.  */
+
+static int
+process_remaining (void *context)
+{
+  int res;
+
+  /* This is a one-shot event.  */
+  readchar_callback = NOT_SCHEDULED;
 
-  bufp = buf;
-  bufcnt--;
-  return *bufp++;
+  if (readchar_bufcnt > 0)
+    res = handle_serial_event (0, NULL);
+  else
+    res = 0;
+
+  return res;
+}
+
+/* If there is still data in the buffer, queue another event to process it,
+   we can't sleep in select yet.
+   If there is no data left in the buffer, delete any pending callback.  */
+
+static void
+reschedule (void)
+{
+  if (readchar_bufcnt > 0 && readchar_callback == NOT_SCHEDULED)
+    {
+      readchar_callback = append_callback_event (process_remaining, NULL);
+    }
 }
 
 /* Read a packet from the remote machine, with error checking,
Index: server.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
retrieving revision 1.66
diff -u -p -r1.66 server.h
--- server.h	12 Apr 2010 13:51:22 -0000	1.66
+++ server.h	3 May 2010 19:26:48 -0000
@@ -337,10 +337,14 @@ extern int non_stop;
 /* Functions from event-loop.c.  */
 typedef void *gdb_client_data;
 typedef int (handler_func) (int, gdb_client_data);
+typedef int (callback_handler_func) (gdb_client_data);
 
 extern void delete_file_handler (int fd);
 extern void add_file_handler (int fd, handler_func *proc,
 			      gdb_client_data client_data);
+extern int append_callback_event (callback_handler_func *proc,
+				   gdb_client_data client_data);
+extern void delete_callback_event (int id);
 
 extern void start_event_loop (void);
 

Re: [RFA] Fix gdbserver queued packet handling

[Pedro Alves]

On Monday 03 May 2010 20:53:14, Doug Evans wrote:

> P.S. IWBN if one didn't have to continually keep bringing stuff over
> from gdb.  Wouldn't it be nice if one could just include an
> event-loop.h (or some such) and use it.  I'm developing the feeling
> that this is another instance where IWBN if gdb and gdbserver could
> share code.

Yes, of course I agree.

> 2010-05-03  Doug Evans  <dje@google.com>
> 
>         * event-loop.c (struct callback_event): New struct.
...

This is okay too.  Thank you for fixing this.

> +/* If there is still data in the buffer, queue another event to process it,
> +   we can't sleep in select yet.
> +   If there is no data left in the buffer, delete any pending callback.  */

This last sentence appears stale.

> +static void
> +reschedule (void)
> +{
> +  if (readchar_bufcnt > 0 && readchar_callback == NOT_SCHEDULED)
> +    {
> +      readchar_callback = append_callback_event (process_remaining, NULL);
> +    }
>  }

-- 
Pedro Alves