[RFA] Fix gdbserver queued packet handling
Doug Evans
dje@google.com
Mon May 3 19:53:00 GMT 2010
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.
-------------- next part --------------
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);
More information about the Gdb-patches
mailing list