[PATCH 1/3] gdbserver: replace event_queue with QUEUE

[PATCH 1/3] gdbserver: replace event_queue with QUEUE

[Yao Qi]

Hi,
We recently add a new data structure QUEUE (in common/queue.h), and we
can use it more widely.  This patch is to rewrite 'event_queue' in
event-loop.c using QUEUE.

Regression tested on x86_64-linux with boardfile
{unix, native-gdbserver} x {sync, async}.

gdb/gdbserver:

2013-01-24  Yao Qi  <yao@codesourcery.com>

	* event-loop.c: Include "queue.h".
	(gdb_event_p): New typedef.
	(struct gdb_event) <next_event>: Remove.
	(event_queue): Change to QUEUE(gdb_event_p).
	(async_queue_event): Remove.
	(process_event): Use API from QUEUE.
	(wait_for_event): Likewise.
	(gdb_event_xfree): New.
	(start_event_loop): Initialize "event_queue".
---
 gdb/gdbserver/event-loop.c |  118 ++++++++++++--------------------------------
 1 files changed, 31 insertions(+), 87 deletions(-)

diff --git a/gdb/gdbserver/event-loop.c b/gdb/gdbserver/event-loop.c
index 87487e0..53442e6 100644
--- a/gdb/gdbserver/event-loop.c
+++ b/gdb/gdbserver/event-loop.c
@@ -19,6 +19,7 @@
 /* Based on src/gdb/event-loop.c.  */
 
 #include "server.h"
+#include "queue.h"
 
 #include <sys/types.h>
 #include <string.h>
@@ -46,7 +47,8 @@ typedef int (event_handler_func) (gdb_fildes_t);
 #define GDB_WRITABLE	(1<<2)
 #define GDB_EXCEPTION	(1<<3)
 
-/* Events are queued by calling async_queue_event and serviced later
+/* Events are queued by calling 'QUEUE_enque (gdb_event_p, event_queue,
+   file_event_ptr)' and serviced later
    on by do_one_event.  An event can be, for instance, a file
    descriptor becoming ready to be read.  Servicing an event simply
    means that the procedure PROC will be called.  We have 2 queues,
@@ -57,17 +59,14 @@ typedef int (event_handler_func) (gdb_fildes_t);
    descriptor whose state change generated the event, plus doing other
    cleanups and such.  */
 
-struct gdb_event
+typedef struct gdb_event
   {
     /* Procedure to call to service this event.  */
     event_handler_func *proc;
 
     /* File descriptor that is ready.  */
     gdb_fildes_t fd;
-
-    /* Next in list of events or NULL.  */
-    struct gdb_event *next_event;
-  };
+  } *gdb_event_p;
 
 /* Information about each file descriptor we register with the event
    loop.  */
@@ -97,25 +96,9 @@ typedef struct file_handler
   }
 file_handler;
 
-/* Event queue:
-
-   Events can be inserted at the front of the queue or at the end of
-   the queue.  Events will be extracted from the queue for processing
-   starting from the head.  Therefore, events inserted at the head of
-   the queue will be processed in a last in first out fashion, while
-   those inserted at the tail of the queue will be processed in a
-   first in first out manner.  All the fields are NULL if the queue is
-   empty.  */
-
-static struct
-  {
-    /* The first pending event.  */
-    gdb_event *first_event;
-
-    /* The last pending event.  */
-    gdb_event *last_event;
-  }
-event_queue;
+DECLARE_QUEUE_P(gdb_event_p);
+static QUEUE(gdb_event_p) *event_queue = NULL;
+DEFINE_QUEUE_P(gdb_event_p);
 
 /* Gdb_notifier is just a list of file descriptors gdb is interested
    in.  These are the input file descriptor, and the target file
@@ -170,27 +153,6 @@ static struct
   }
 callback_list;
 
-/* Insert an event object into the gdb event queue.
-
-   EVENT_PTR points to the event to be inserted into the queue.  The
-   caller must allocate memory for the event.  It is freed after the
-   event has ben handled.  Events in the queue will be processed head
-   to tail, therefore, events will be processed first in first
-   out.  */
-
-static void
-async_queue_event (gdb_event *event_ptr)
-{
-  /* The event will become the new last_event.  */
-
-  event_ptr->next_event = NULL;
-  if (event_queue.first_event == NULL)
-    event_queue.first_event = event_ptr;
-  else
-    event_queue.last_event->next_event = event_ptr;
-  event_queue.last_event = event_ptr;
-}
-
 /* Process one event.  If an event was processed, 1 is returned
    otherwise 0 is returned.  Scan the queue from head to tail,
    processing therefore the high priority events first, by invoking
@@ -199,46 +161,18 @@ async_queue_event (gdb_event *event_ptr)
 static int
 process_event (void)
 {
-  gdb_event *event_ptr, *prev_ptr;
-  event_handler_func *proc;
-  gdb_fildes_t fd;
-
-  /* Look in the event queue to find an event that is ready
-     to be processed.  */
-
-  for (event_ptr = event_queue.first_event;
-       event_ptr != NULL;
-       event_ptr = event_ptr->next_event)
+  /* Let's get rid of the event from the event queue.  We need to
+     do this now because while processing the event, since the
+     proc function could end up jumping out to the caller of this
+     function.  In that case, we would have on the event queue an
+     event which has been processed, but not deleted.  */
+  if (!QUEUE_is_empty (gdb_event_p, event_queue))
     {
-      /* Call the handler for the event.  */
-
-      proc = event_ptr->proc;
-      fd = event_ptr->fd;
-
-      /* Let's get rid of the event from the event queue.  We need to
-         do this now because while processing the event, since the
-         proc function could end up jumping out to the caller of this
-         function.  In that case, we would have on the event queue an
-         event which has been processed, but not deleted.  */
-
-      if (event_queue.first_event == event_ptr)
-	{
-	  event_queue.first_event = event_ptr->next_event;
-	  if (event_ptr->next_event == NULL)
-	    event_queue.last_event = NULL;
-	}
-      else
-	{
-	  prev_ptr = event_queue.first_event;
-	  while (prev_ptr->next_event != event_ptr)
-	    prev_ptr = prev_ptr->next_event;
-
-	  prev_ptr->next_event = event_ptr->next_event;
-	  if (event_ptr->next_event == NULL)
-	    event_queue.last_event = prev_ptr;
-	}
-      free (event_ptr);
+      gdb_event *event_ptr = QUEUE_deque (gdb_event_p, event_queue);
+      event_handler_func *proc = event_ptr->proc;
+      gdb_fildes_t fd = event_ptr->fd;
 
+      xfree (event_ptr);
       /* Now call the procedure associated with the event.  */
       if ((*proc) (fd))
 	return -1;
@@ -522,7 +456,6 @@ static int
 wait_for_event (void)
 {
   file_handler *file_ptr;
-  gdb_event *file_event_ptr;
   int num_found = 0;
 
   /* Make sure all output is done before getting another event.  */
@@ -580,8 +513,9 @@ wait_for_event (void)
 
       if (file_ptr->ready_mask == 0)
 	{
-	  file_event_ptr = create_file_event (file_ptr->fd);
-	  async_queue_event (file_event_ptr);
+	  gdb_event *file_event_ptr = create_file_event (file_ptr->fd);
+
+	  QUEUE_enque (gdb_event_p, event_queue, file_event_ptr);
 	}
       file_ptr->ready_mask = mask;
     }
@@ -589,12 +523,22 @@ wait_for_event (void)
   return 0;
 }
 
+/* Free EVENT.  */
+
+static void
+gdb_event_xfree (struct gdb_event *event)
+{
+  xfree (event);
+}
+
 /* Start up the event loop.  This is the entry point to the event
    loop.  */
 
 void
 start_event_loop (void)
 {
+  event_queue = QUEUE_alloc (gdb_event_p, gdb_event_xfree);
+
   /* Loop until there is nothing to do.  This is the entry point to
      the event loop engine.  If nothing is ready at this time, wait
      for something to happen (via wait_for_event), then process it.
-- 
1.7.7.6

[PATCH 2/3] Remove queue_position.

[Yao Qi]

enum HEAD is never used and AFAICS, we don't need to insert element to
event_queue in different places of the queue,  so 'enum
queue_position' is not needed.  This patch is to remove it and
simplify 'async_queue_event'.

gdb:

2013-01-24  Yao Qi  <yao@codesourcery.com>

	* event-loop.c (async_queue_event): Remove one parameter
	'position'.  Remove code handling 'position' == TAIL.
	(gdb_wait_for_event): Caller update.
	(check_async_event_handlers): Caller update.
	(poll_timers): Caller update.
	* event-loop.h (enum queue_position): Remove.
---
 gdb/event-loop.c |   40 +++++++++++++---------------------------
 gdb/event-loop.h |   12 ------------
 2 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index be485c9..aea3b5e 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -274,9 +274,7 @@ static int gdb_wait_for_event (int);
 static void poll_timers (void);
 \f
 
-/* Insert an event object into the gdb event queue at 
-   the specified position.
-   POSITION can be head or tail, with values TAIL, HEAD.
+/* Insert an event object into the gdb event queue.
    EVENT_PTR points to the event to be inserted into the queue.
    The caller must allocate memory for the event.  It is freed
    after the event has ben handled.
@@ -285,28 +283,16 @@ static void poll_timers (void);
    as last in first out.  Event appended at the tail of the queue
    will be processed first in first out.  */
 static void
-async_queue_event (gdb_event * event_ptr, queue_position position)
+async_queue_event (gdb_event * event_ptr)
 {
-  if (position == TAIL)
-    {
-      /* The event will become the new last_event.  */
+  /* The event will become the new last_event.  */
 
-      event_ptr->next_event = NULL;
-      if (event_queue.first_event == NULL)
-	event_queue.first_event = event_ptr;
-      else
-	event_queue.last_event->next_event = event_ptr;
-      event_queue.last_event = event_ptr;
-    }
-  else if (position == HEAD)
-    {
-      /* The event becomes the new first_event.  */
-
-      event_ptr->next_event = event_queue.first_event;
-      if (event_queue.first_event == NULL)
-	event_queue.last_event = event_ptr;
-      event_queue.first_event = event_ptr;
-    }
+  event_ptr->next_event = NULL;
+  if (event_queue.first_event == NULL)
+    event_queue.first_event = event_ptr;
+  else
+    event_queue.last_event->next_event = event_ptr;
+  event_queue.last_event = event_ptr;
 }
 
 /* Create a generic event, to be enqueued in the event queue for
@@ -936,7 +922,7 @@ gdb_wait_for_event (int block)
 	      if (file_ptr->ready_mask == 0)
 		{
 		  file_event_ptr = create_file_event (file_ptr->fd);
-		  async_queue_event (file_event_ptr, TAIL);
+		  async_queue_event (file_event_ptr);
 		}
 	      file_ptr->ready_mask = (gdb_notifier.poll_fds + i)->revents;
 	    }
@@ -972,7 +958,7 @@ gdb_wait_for_event (int block)
 	  if (file_ptr->ready_mask == 0)
 	    {
 	      file_event_ptr = create_file_event (file_ptr->fd);
-	      async_queue_event (file_event_ptr, TAIL);
+	      async_queue_event (file_event_ptr);
 	    }
 	  file_ptr->ready_mask = mask;
 	}
@@ -1158,7 +1144,7 @@ check_async_event_handlers (void)
 	  data.ptr = hdata;
 
 	  event_ptr = create_event (invoke_async_event_handler, data);
-	  async_queue_event (event_ptr, TAIL);
+	  async_queue_event (event_ptr);
 	}
     }
 }
@@ -1365,7 +1351,7 @@ poll_timers (void)
 	  event_ptr = (gdb_event *) xmalloc (sizeof (gdb_event));
 	  event_ptr->proc = handle_timer_event;
 	  event_ptr->data.integer = timer_list.first_timer->timer_id;
-	  async_queue_event (event_ptr, TAIL);
+	  async_queue_event (event_ptr);
 	}
 
       /* Now we need to update the timeout for select/ poll, because
diff --git a/gdb/event-loop.h b/gdb/event-loop.h
index 16c2286..fc95cf1 100644
--- a/gdb/event-loop.h
+++ b/gdb/event-loop.h
@@ -75,18 +75,6 @@ typedef void (sig_handler_func) (gdb_client_data);
 typedef void (async_event_handler_func) (gdb_client_data);
 typedef void (timer_handler_func) (gdb_client_data);
 
-/* Where to add an event onto the event queue, by queue_event.  */
-typedef enum
-  {
-    /* Add at tail of queue.  It will be processed in first in first
-       out order.  */
-    TAIL,
-    /* Add at head of queue.  It will be processed in last in first
-       out order.  */
-    HEAD
-  }
-queue_position;
-
 /* Exported functions from event-loop.c */
 
 extern void start_event_loop (void);
-- 
1.7.7.6

Re: [PATCH 2/3] Remove queue_position.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2013-01-24  Yao Qi  <yao@codesourcery.com>
Yao> 	* event-loop.c (async_queue_event): Remove one parameter
Yao> 	'position'.  Remove code handling 'position' == TAIL.
Yao> 	(gdb_wait_for_event): Caller update.
Yao> 	(check_async_event_handlers): Caller update.
Yao> 	(poll_timers): Caller update.
Yao> 	* event-loop.h (enum queue_position): Remove.

Ok.  Thanks for cleaning this up.

Tom

Re: [PATCH 2/3] Remove queue_position.

[Pedro Alves]

On 01/24/2013 01:13 AM, Yao Qi wrote:
> enum HEAD is never used and AFAICS, we don't need to insert element to
> event_queue in different places of the queue,  so 'enum
> queue_position' is not needed.  This patch is to remove it and
> simplify 'async_queue_event'.

Yeah.  Thanks.  This was one of the things I simplified
when I forked gdbserver's event-loop.c, but never got
to cleanup on the GDB side.  Nowadays, I think that forking
was a mistake, and we should make gdb and gdbserver share
the same code.

> 
> gdb:
> 
> 2013-01-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* event-loop.c (async_queue_event): Remove one parameter
> 	'position'.  Remove code handling 'position' == TAIL.
> 	(gdb_wait_for_event): Caller update.
> 	(check_async_event_handlers): Caller update.
> 	(poll_timers): Caller update.
> 	* event-loop.h (enum queue_position): Remove.

Okay, thanks.

-- 
Pedro Alves

[PATCH 3/3] gdb: replace event_queue with QUEUE

[Yao Qi]

Hi,
This patch rewrites 'event_queue' with QUEUE API in common/queue.h.
This patch is a little different from the GDBserver one, because the
event queue is used before the event loop is started.  So I add a new
function init_event_queue to initialize event queue earlier.

Regression tested on x86_64-linux with board file {unix,
native-gdbserver} x {sync, async}.

gdb:

2013-01-24  Yao Qi  <yao@codesourcery.com>

	* event-loop.c: Include "queue.h".
	(gdb_event_p): New typedef.
	(typedef struct async_event_handler):
	(DECLARE_QUEUE_P): Use.
	(DEFINE_QUEUE_P): Use.
	(async_queue_event): Remove.
	(static int gdb_wait_for_event):
	(process_event): Use QUEUE macros.
	(event_queue): Remove.
	(gdb_wait_for_event): Caller update.
	(check_async_event_handlers): Likewise.
	(poll_timers): Likewise.
	(gdb_event_xfree): New.
	(init_event_queue): New.
	* event-loop.h: Declare it.
	* main.c (captured_main): Call init_event_queue.
---
 gdb/event-loop.c |  122 ++++++++++++++++-------------------------------------
 gdb/event-loop.h |    1 +
 gdb/main.c       |    4 ++
 3 files changed, 42 insertions(+), 85 deletions(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index aea3b5e..05a4598 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -20,6 +20,7 @@
 #include "defs.h"
 #include "event-loop.h"
 #include "event-top.h"
+#include "queue.h"
 
 #ifdef HAVE_POLL
 #if defined (HAVE_POLL_H)
@@ -68,17 +69,14 @@ typedef void (event_handler_func) (event_data);
    case of async signal handlers, it is
    invoke_async_signal_handler.  */
 
-struct gdb_event
+typedef struct gdb_event
   {
     /* Procedure to call to service this event.  */
     event_handler_func *proc;
 
     /* Data to pass to the event handler.  */
     event_data data;
-
-    /* Next in list of events or NULL.  */
-    struct gdb_event *next_event;
-  };
+  } *gdb_event_p;
 
 /* Information about each file descriptor we register with the event
    loop.  */
@@ -140,26 +138,9 @@ typedef struct async_event_handler
   }
 async_event_handler;
 
-
-/* Event queue:  
-   - the first event in the queue is the head of the queue.
-   It will be the next to be serviced.
-   - the last event in the queue 
-
-   Events can be inserted at the front of the queue or at the end of
-   the queue.  Events will be extracted from the queue for processing
-   starting from the head.  Therefore, events inserted at the head of
-   the queue will be processed in a last in first out fashion, while
-   those inserted at the tail of the queue will be processed in a first
-   in first out manner.  All the fields are NULL if the queue is
-   empty.  */
-
-static struct
-  {
-    gdb_event *first_event;	/* First pending event.  */
-    gdb_event *last_event;	/* Last pending event.  */
-  }
-event_queue;
+DECLARE_QUEUE_P (gdb_event_p);
+DEFINE_QUEUE_P (gdb_event_p);
+static QUEUE(gdb_event_p) *event_queue = NULL;
 
 /* Gdb_notifier is just a list of file descriptors gdb is interested in.
    These are the input file descriptor, and the target file
@@ -274,27 +255,6 @@ static int gdb_wait_for_event (int);
 static void poll_timers (void);
 \f
 
-/* Insert an event object into the gdb event queue.
-   EVENT_PTR points to the event to be inserted into the queue.
-   The caller must allocate memory for the event.  It is freed
-   after the event has ben handled.
-   Events in the queue will be processed head to tail, therefore,
-   events inserted at the head of the queue will be processed
-   as last in first out.  Event appended at the tail of the queue
-   will be processed first in first out.  */
-static void
-async_queue_event (gdb_event * event_ptr)
-{
-  /* The event will become the new last_event.  */
-
-  event_ptr->next_event = NULL;
-  if (event_queue.first_event == NULL)
-    event_queue.first_event = event_ptr;
-  else
-    event_queue.last_event->next_event = event_ptr;
-  event_queue.last_event = event_ptr;
-}
-
 /* Create a generic event, to be enqueued in the event queue for
    processing.  PROC is the procedure associated to the event.  DATA
    is passed to PROC upon PROC invocation.  */
@@ -336,10 +296,6 @@ create_file_event (int fd)
 static int
 process_event (void)
 {
-  gdb_event *event_ptr, *prev_ptr;
-  event_handler_func *proc;
-  event_data data;
-
   /* First let's see if there are any asynchronous event handlers that
      are ready.  These would be the result of invoking any of the
      signal handlers.  */
@@ -350,46 +306,28 @@ process_event (void)
   /* Look in the event queue to find an event that is ready
      to be processed.  */
 
-  for (event_ptr = event_queue.first_event; event_ptr != NULL;
-       event_ptr = event_ptr->next_event)
+  if (QUEUE_is_empty (gdb_event_p, event_queue))
+    /* This is the case if there are no event on the event queue.  */
+    return 0;
+  else
     {
-      /* Call the handler for the event.  */
-
-      proc = event_ptr->proc;
-      data = event_ptr->data;
-
       /* Let's get rid of the event from the event queue.  We need to
-         do this now because while processing the event, the proc
-         function could end up calling 'error' and therefore jump out
-         to the caller of this function, gdb_do_one_event.  In that
-         case, we would have on the event queue an event wich has been
-         processed, but not deleted.  */
-
-      if (event_queue.first_event == event_ptr)
-	{
-	  event_queue.first_event = event_ptr->next_event;
-	  if (event_ptr->next_event == NULL)
-	    event_queue.last_event = NULL;
-	}
-      else
-	{
-	  prev_ptr = event_queue.first_event;
-	  while (prev_ptr->next_event != event_ptr)
-	    prev_ptr = prev_ptr->next_event;
+	 do this now because while processing the event, the proc
+	 function could end up calling 'error' and therefore jump out
+	 to the caller of this function, gdb_do_one_event.  In that
+	 case, we would have on the event queue an event wich has been
+	 processed, but not deleted.  */
+      gdb_event *event_ptr = QUEUE_deque (gdb_event_p, event_queue);
+      /* Call the handler for the event.  */
+      event_handler_func *proc = event_ptr->proc;
+      event_data data = event_ptr->data;
 
-	  prev_ptr->next_event = event_ptr->next_event;
-	  if (event_ptr->next_event == NULL)
-	    event_queue.last_event = prev_ptr;
-	}
       xfree (event_ptr);
 
       /* Now call the procedure associated with the event.  */
       (*proc) (data);
       return 1;
     }
-
-  /* This is the case if there are no event on the event queue.  */
-  return 0;
 }
 
 /* Process one high level event.  If nothing is ready at this time,
@@ -456,6 +394,20 @@ gdb_do_one_event (void)
   return 1;
 }
 
+/* Free EVENT.  */
+
+static void
+gdb_event_xfree (struct gdb_event *event)
+{
+  xfree (event);
+}
+
+void
+init_event_queue (void)
+{
+  event_queue = QUEUE_alloc (gdb_event_p, gdb_event_xfree);
+}
+
 /* Start up the event loop.  This is the entry point to the event loop
    from the command loop.  */
 
@@ -922,7 +874,7 @@ gdb_wait_for_event (int block)
 	      if (file_ptr->ready_mask == 0)
 		{
 		  file_event_ptr = create_file_event (file_ptr->fd);
-		  async_queue_event (file_event_ptr);
+		  QUEUE_enque (gdb_event_p, event_queue, file_event_ptr);
 		}
 	      file_ptr->ready_mask = (gdb_notifier.poll_fds + i)->revents;
 	    }
@@ -958,7 +910,7 @@ gdb_wait_for_event (int block)
 	  if (file_ptr->ready_mask == 0)
 	    {
 	      file_event_ptr = create_file_event (file_ptr->fd);
-	      async_queue_event (file_event_ptr);
+	      QUEUE_enque (gdb_event_p, event_queue, file_event_ptr);
 	    }
 	  file_ptr->ready_mask = mask;
 	}
@@ -1144,7 +1096,7 @@ check_async_event_handlers (void)
 	  data.ptr = hdata;
 
 	  event_ptr = create_event (invoke_async_event_handler, data);
-	  async_queue_event (event_ptr);
+	  QUEUE_enque (gdb_event_p, event_queue, event_ptr);
 	}
     }
 }
@@ -1351,7 +1303,7 @@ poll_timers (void)
 	  event_ptr = (gdb_event *) xmalloc (sizeof (gdb_event));
 	  event_ptr->proc = handle_timer_event;
 	  event_ptr->data.integer = timer_list.first_timer->timer_id;
-	  async_queue_event (event_ptr);
+	  QUEUE_enque (gdb_event_p, event_queue, event_ptr);
 	}
 
       /* Now we need to update the timeout for select/ poll, because
diff --git a/gdb/event-loop.h b/gdb/event-loop.h
index fc95cf1..fd7efc0 100644
--- a/gdb/event-loop.h
+++ b/gdb/event-loop.h
@@ -77,6 +77,7 @@ typedef void (timer_handler_func) (gdb_client_data);
 
 /* Exported functions from event-loop.c */
 
+extern void init_event_queue (void);
 extern void start_event_loop (void);
 extern int gdb_do_one_event (void);
 extern void delete_file_handler (int fd);
diff --git a/gdb/main.c b/gdb/main.c
index 6ed014f..689b2d8 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -997,6 +997,10 @@ captured_main (void *data)
   ALL_OBJFILES (objfile)
     load_auto_scripts_for_objfile (objfile);
 
+  /* Initialize event queue here because execution commands below
+     will push events to the queue.  */
+  init_event_queue ();
+
   /* Process '-x' and '-ex' options.  */
   for (i = 0; VEC_iterate (cmdarg_s, cmdarg_vec, i, cmdarg_p); i++)
     switch (cmdarg_p->type)
-- 
1.7.7.6

Re: [PATCH 3/3] gdb: replace event_queue with QUEUE

[Pedro Alves]

On 01/24/2013 01:14 AM, Yao Qi wrote:
> Hi,
> This patch rewrites 'event_queue' with QUEUE API in common/queue.h.
> This patch is a little different from the GDBserver one, because the
> event queue is used before the event loop is started.  So I add a new
> function init_event_queue to initialize event queue earlier.

Ah, yeah, please do the same in gdbserver.

> 2013-01-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* event-loop.c: Include "queue.h".
> 	(gdb_event_p): New typedef.
> 	(typedef struct async_event_handler):
> 	(DECLARE_QUEUE_P): Use.
> 	(DEFINE_QUEUE_P): Use.
> 	(async_queue_event): Remove.
> 	(static int gdb_wait_for_event):

Doesn't look finished.

> 	(process_event): Use QUEUE macros.
> 	(event_queue): Remove.
> 	(gdb_wait_for_event): Caller update.
> 	(check_async_event_handlers): Likewise.
> 	(poll_timers): Likewise.
> 	(gdb_event_xfree): New.
> 	(init_event_queue): New.
> 	* event-loop.h: Declare it.

What's "it"?

> 	* main.c (captured_main): Call init_event_queue.

> +DECLARE_QUEUE_P (gdb_event_p);
> +DEFINE_QUEUE_P (gdb_event_p);

I noticed that the gdbserver patch didn't put spaces
in these.


>  /* Create a generic event, to be enqueued in the event queue for
>     processing.  PROC is the procedure associated to the event.  DATA
>     is passed to PROC upon PROC invocation.  */
> @@ -336,10 +296,6 @@ create_file_event (int fd)
>  static int
>  process_event (void)
>  {
...
> +	 do this now because while processing the event, the proc
> +	 function could end up calling 'error' and therefore jump out
> +	 to the caller of this function, gdb_do_one_event.  In that
> +	 case, we would have on the event queue an event wich has been
> +	 processed, but not deleted.  */
> +      gdb_event *event_ptr = QUEUE_deque (gdb_event_p, event_queue);
> +      /* Call the handler for the event.  */
> +      event_handler_func *proc = event_ptr->proc;
> +      event_data data = event_ptr->data;
>  
> -	  prev_ptr->next_event = event_ptr->next_event;
> -	  if (event_ptr->next_event == NULL)
> -	    event_queue.last_event = prev_ptr;
> -	}
>        xfree (event_ptr);

Same comment with gdb_event_xfree.

> @@ -336,10 +296,6 @@ create_file_event (int fd)
>  static int
>  process_event (void)
>  {
> -  gdb_event *event_ptr, *prev_ptr;
> -  event_handler_func *proc;
> -  event_data data;
> -
>    /* First let's see if there are any asynchronous event handlers that
>       are ready.  These would be the result of invoking any of the
>       signal handlers.  */
> @@ -350,46 +306,28 @@ process_event (void)
>    /* Look in the event queue to find an event that is ready
>       to be processed.  */
>
> -  for (event_ptr = event_queue.first_event; event_ptr != NULL;
> -       event_ptr = event_ptr->next_event)
> +  if (QUEUE_is_empty (gdb_event_p, event_queue))
> +    /* This is the case if there are no event on the event queue.  */
> +    return 0;
> +  else
>      {

Any reason not to do the exact same the gdbserver patch did? :

   if (!QUEUE_is_empty (gdb_event_p, event_queue))

and then leave the return bit alone at the bottom.

> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -997,6 +997,10 @@ captured_main (void *data)
>    ALL_OBJFILES (objfile)
>      load_auto_scripts_for_objfile (objfile);
>
> +  /* Initialize event queue here because execution commands below
> +     will push events to the queue.  */
> +  init_event_queue ();
> +

Looks like this needs to be before scripts too, as those can
run execution commands too.  I think it's just best to put
this in gdb_init, close to other "manual" initialize_foo calls.

>    /* Process '-x' and '-ex' options.  */
>    for (i = 0; VEC_iterate (cmdarg_s, cmdarg_vec, i, cmdarg_p); i++)
>      switch (cmdarg_p->type)

-- 
Pedro Alves

Re: [PATCH 3/3] gdb: replace event_queue with QUEUE

[Yao Qi]

On 01/25/2013 03:08 AM, Pedro Alves wrote:
>> -  for (event_ptr = event_queue.first_event; event_ptr != NULL;
>> >-       event_ptr = event_ptr->next_event)
>> >+  if (QUEUE_is_empty (gdb_event_p, event_queue))
>> >+    /* This is the case if there are no event on the event queue.  */
>> >+    return 0;
>> >+  else
>> >      {
> Any reason not to do the exact same the gdbserver patch did? :
> 

No.  This patch is for refactor, so I don't consider the consistency
between GDB and GDBserver.

>     if (!QUEUE_is_empty (gdb_event_p, event_queue))
> 
> and then leave the return bit alone at the bottom.
> 
>> >--- a/gdb/main.c
>> >+++ b/gdb/main.c
>> >@@ -997,6 +997,10 @@ captured_main (void *data)
>> >    ALL_OBJFILES (objfile)
>> >      load_auto_scripts_for_objfile (objfile);
>> >
>> >+  /* Initialize event queue here because execution commands below
>> >+     will push events to the queue.  */
>> >+  init_event_queue ();
>> >+
> Looks like this needs to be before scripts too, as those can
> run execution commands too.  I think it's just best to put
> this in gdb_init, close to other "manual" initialize_foo calls.
> 

Add a function "initialized_event_loop" and call it in gdb_init.  The
patch below is tested on x86_64-linux.  Is it OK?

-- 
Yao (齐尧)

gdb:

2013-01-25  Yao Qi  <yao@codesourcery.com>

	* event-loop.c: Include "queue.h".
	(gdb_event_p): New typedef.
	(DECLARE_QUEUE_P): Use.
	(DEFINE_QUEUE_P): Use.
	(async_queue_event): Remove.
	(gdb_event_xfree): New.
	(initialize_event_loop): New.
	(process_event): Use QUEUE macros.
	(event_queue): Remove.
	(gdb_wait_for_event): Caller update.
	(check_async_event_handlers): Likewise.
	(poll_timers): Likewise.
	* event-loop.h (initialize_event_loop): Declare.
	* event-loop.c (gdb_event_xfree): New.
	* top.c (gdb_init): Call initialize_event_loop.
---
 gdb/event-loop.c |  121 +++++++++++++++++-------------------------------------
 gdb/event-loop.h |    1 +
 gdb/top.c        |    1 +
 3 files changed, 40 insertions(+), 83 deletions(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index aea3b5e..f34f153 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -20,6 +20,7 @@
 #include "defs.h"
 #include "event-loop.h"
 #include "event-top.h"
+#include "queue.h"
 
 #ifdef HAVE_POLL
 #if defined (HAVE_POLL_H)
@@ -68,17 +69,14 @@ typedef void (event_handler_func) (event_data);
    case of async signal handlers, it is
    invoke_async_signal_handler.  */
 
-struct gdb_event
+typedef struct gdb_event
   {
     /* Procedure to call to service this event.  */
     event_handler_func *proc;
 
     /* Data to pass to the event handler.  */
     event_data data;
-
-    /* Next in list of events or NULL.  */
-    struct gdb_event *next_event;
-  };
+  } *gdb_event_p;
 
 /* Information about each file descriptor we register with the event
    loop.  */
@@ -140,26 +138,9 @@ typedef struct async_event_handler
   }
 async_event_handler;
 
-
-/* Event queue:  
-   - the first event in the queue is the head of the queue.
-   It will be the next to be serviced.
-   - the last event in the queue 
-
-   Events can be inserted at the front of the queue or at the end of
-   the queue.  Events will be extracted from the queue for processing
-   starting from the head.  Therefore, events inserted at the head of
-   the queue will be processed in a last in first out fashion, while
-   those inserted at the tail of the queue will be processed in a first
-   in first out manner.  All the fields are NULL if the queue is
-   empty.  */
-
-static struct
-  {
-    gdb_event *first_event;	/* First pending event.  */
-    gdb_event *last_event;	/* Last pending event.  */
-  }
-event_queue;
+DECLARE_QUEUE_P(gdb_event_p);
+DEFINE_QUEUE_P(gdb_event_p);
+static QUEUE(gdb_event_p) *event_queue = NULL;
 
 /* Gdb_notifier is just a list of file descriptors gdb is interested in.
    These are the input file descriptor, and the target file
@@ -274,27 +255,6 @@ static int gdb_wait_for_event (int);
 static void poll_timers (void);
 \f
 
-/* Insert an event object into the gdb event queue.
-   EVENT_PTR points to the event to be inserted into the queue.
-   The caller must allocate memory for the event.  It is freed
-   after the event has ben handled.
-   Events in the queue will be processed head to tail, therefore,
-   events inserted at the head of the queue will be processed
-   as last in first out.  Event appended at the tail of the queue
-   will be processed first in first out.  */
-static void
-async_queue_event (gdb_event * event_ptr)
-{
-  /* The event will become the new last_event.  */
-
-  event_ptr->next_event = NULL;
-  if (event_queue.first_event == NULL)
-    event_queue.first_event = event_ptr;
-  else
-    event_queue.last_event->next_event = event_ptr;
-  event_queue.last_event = event_ptr;
-}
-
 /* Create a generic event, to be enqueued in the event queue for
    processing.  PROC is the procedure associated to the event.  DATA
    is passed to PROC upon PROC invocation.  */
@@ -324,6 +284,23 @@ create_file_event (int fd)
   return create_event (handle_file_event, data);
 }
 
+
+/* Free EVENT.  */
+
+static void
+gdb_event_xfree (struct gdb_event *event)
+{
+  xfree (event);
+}
+
+/* Initialize the event queue.  */
+
+void
+initialize_event_loop (void)
+{
+  event_queue = QUEUE_alloc (gdb_event_p, gdb_event_xfree);
+}
+
 /* Process one event.
    The event can be the next one to be serviced in the event queue,
    or an asynchronous event handler can be invoked in response to
@@ -336,10 +313,6 @@ create_file_event (int fd)
 static int
 process_event (void)
 {
-  gdb_event *event_ptr, *prev_ptr;
-  event_handler_func *proc;
-  event_data data;
-
   /* First let's see if there are any asynchronous event handlers that
      are ready.  These would be the result of invoking any of the
      signal handlers.  */
@@ -350,38 +323,20 @@ process_event (void)
   /* Look in the event queue to find an event that is ready
      to be processed.  */
 
-  for (event_ptr = event_queue.first_event; event_ptr != NULL;
-       event_ptr = event_ptr->next_event)
+  if (!QUEUE_is_empty (gdb_event_p, event_queue))
     {
-      /* Call the handler for the event.  */
-
-      proc = event_ptr->proc;
-      data = event_ptr->data;
-
       /* Let's get rid of the event from the event queue.  We need to
-         do this now because while processing the event, the proc
-         function could end up calling 'error' and therefore jump out
-         to the caller of this function, gdb_do_one_event.  In that
-         case, we would have on the event queue an event wich has been
-         processed, but not deleted.  */
-
-      if (event_queue.first_event == event_ptr)
-	{
-	  event_queue.first_event = event_ptr->next_event;
-	  if (event_ptr->next_event == NULL)
-	    event_queue.last_event = NULL;
-	}
-      else
-	{
-	  prev_ptr = event_queue.first_event;
-	  while (prev_ptr->next_event != event_ptr)
-	    prev_ptr = prev_ptr->next_event;
+	 do this now because while processing the event, the proc
+	 function could end up calling 'error' and therefore jump out
+	 to the caller of this function, gdb_do_one_event.  In that
+	 case, we would have on the event queue an event wich has been
+	 processed, but not deleted.  */
+      gdb_event *event_ptr = QUEUE_deque (gdb_event_p, event_queue);
+      /* Call the handler for the event.  */
+      event_handler_func *proc = event_ptr->proc;
+      event_data data = event_ptr->data;
 
-	  prev_ptr->next_event = event_ptr->next_event;
-	  if (event_ptr->next_event == NULL)
-	    event_queue.last_event = prev_ptr;
-	}
-      xfree (event_ptr);
+      gdb_event_xfree (event_ptr);
 
       /* Now call the procedure associated with the event.  */
       (*proc) (data);
@@ -922,7 +877,7 @@ gdb_wait_for_event (int block)
 	      if (file_ptr->ready_mask == 0)
 		{
 		  file_event_ptr = create_file_event (file_ptr->fd);
-		  async_queue_event (file_event_ptr);
+		  QUEUE_enque (gdb_event_p, event_queue, file_event_ptr);
 		}
 	      file_ptr->ready_mask = (gdb_notifier.poll_fds + i)->revents;
 	    }
@@ -958,7 +913,7 @@ gdb_wait_for_event (int block)
 	  if (file_ptr->ready_mask == 0)
 	    {
 	      file_event_ptr = create_file_event (file_ptr->fd);
-	      async_queue_event (file_event_ptr);
+	      QUEUE_enque (gdb_event_p, event_queue, file_event_ptr);
 	    }
 	  file_ptr->ready_mask = mask;
 	}
@@ -1144,7 +1099,7 @@ check_async_event_handlers (void)
 	  data.ptr = hdata;
 
 	  event_ptr = create_event (invoke_async_event_handler, data);
-	  async_queue_event (event_ptr);
+	  QUEUE_enque (gdb_event_p, event_queue, event_ptr);
 	}
     }
 }
@@ -1351,7 +1306,7 @@ poll_timers (void)
 	  event_ptr = (gdb_event *) xmalloc (sizeof (gdb_event));
 	  event_ptr->proc = handle_timer_event;
 	  event_ptr->data.integer = timer_list.first_timer->timer_id;
-	  async_queue_event (event_ptr);
+	  QUEUE_enque (gdb_event_p, event_queue, event_ptr);
 	}
 
       /* Now we need to update the timeout for select/ poll, because
diff --git a/gdb/event-loop.h b/gdb/event-loop.h
index fc95cf1..e994fc1 100644
--- a/gdb/event-loop.h
+++ b/gdb/event-loop.h
@@ -77,6 +77,7 @@ typedef void (timer_handler_func) (gdb_client_data);
 
 /* Exported functions from event-loop.c */
 
+extern void initialize_event_loop (void);
 extern void start_event_loop (void);
 extern int gdb_do_one_event (void);
 extern void delete_file_handler (int fd);
diff --git a/gdb/top.c b/gdb/top.c
index e9d6c1c..e9a40fc 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1713,6 +1713,7 @@ gdb_init (char *argv0)
   initialize_inferiors ();
   initialize_current_architecture ();
   init_cli_cmds();
+  initialize_event_loop ();
   init_main ();			/* But that omits this file!  Do it now.  */
 
   initialize_stdin_serial ();
-- 
1.7.7.6

Re: [PATCH 3/3] gdb: replace event_queue with QUEUE

[Pedro Alves]

On 01/25/2013 07:01 AM, Yao Qi wrote:
> 2013-01-25  Yao Qi  <yao@codesourcery.com>
> 
> 	* event-loop.c: Include "queue.h".
> 	(gdb_event_p): New typedef.
> 	(DECLARE_QUEUE_P): Use.
> 	(DEFINE_QUEUE_P): Use.
> 	(async_queue_event): Remove.
> 	(gdb_event_xfree): New.
> 	(initialize_event_loop): New.
> 	(process_event): Use QUEUE macros.
> 	(event_queue): Remove.
> 	(gdb_wait_for_event): Caller update.
> 	(check_async_event_handlers): Likewise.
> 	(poll_timers): Likewise.
> 	* event-loop.h (initialize_event_loop): Declare.
> 	* event-loop.c (gdb_event_xfree): New.
> 	* top.c (gdb_init): Call initialize_event_loop.

Thanks, this is OK.

-- 
Pedro Alves

Re: [PATCH 1/3] gdbserver: replace event_queue with QUEUE

[Pedro Alves]

On 01/24/2013 01:13 AM, Yao Qi wrote:

> -
>  /* Process one event.  If an event was processed, 1 is returned
>     otherwise 0 is returned.  Scan the queue from head to tail,
>     processing therefore the high priority events first, by invoking
> @@ -199,46 +161,18 @@ async_queue_event (gdb_event *event_ptr)
>  static int
>  process_event (void)
>  {
...

> -      free (event_ptr);
> +      gdb_event *event_ptr = QUEUE_deque (gdb_event_p, event_queue);
> +      event_handler_func *proc = event_ptr->proc;
> +      gdb_fildes_t fd = event_ptr->fd;
>  
> +      xfree (event_ptr);

I think here

   gdb_event_xfree (event_ptr);

would be better.

>  /* Start up the event loop.  This is the entry point to the event
>     loop.  */
>  
>  void
>  start_event_loop (void)
>  {
> +  event_queue = QUEUE_alloc (gdb_event_p, gdb_event_xfree);

This will go wrong if we ever nest event loops.
Can you put this in a initialize_event_loop function instead,
called from main, close to the initialize_low, etc. calls, please?

Okay with that change.

-- 
Pedro Alves