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

[Pedro Alves <palves@redhat.com>]

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