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

Pedro Alves
2013-01-24T19:08:44Z

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