[PATCH 2/3] When disabling target async, remove all target event sources from the event loop

[PATCH 2/3] When disabling target async, remove all target event sources from the event loop

[Pedro Alves]

The sigall-reverse.exp test occasionally fails with something like this:

 (gdb) PASS: gdb.reverse/sigall-reverse.exp: send signal TERM
 continue
 Continuing.
 The next instruction is syscall exit_group.  It will make the program exit.  Do you want to stop the program?([y] or n) FAIL: gdb.reverse/sigall-reverse.exp: continue to signal exit (timeout)
 FAIL: gdb.reverse/sigall-reverse.exp: reverse to handler of TERM (timeout)
 FAIL: gdb.reverse/sigall-reverse.exp: reverse to gen_TERM (timeout)

This is another event-loop/async related problem exposed by the patch
that made 'query' use gdb_readline_wrapper (588dcc3edbde19f9).

The problem is that even though gdb_readline_wrapper disables
target-async while the secondary prompt is in progress, the record
target's async event source is left marked.  So when
gdb_readline_wrapper nests an event loop to process input, it may
happen that that event loop ends up processing a target event while
GDB is not really ready for it.  Here's the relevant part of the
backtrace showing the root issue in action:

...
 #14 0x000000000061cb48 in fetch_inferior_event (client_data=0x0) at src/gdb/infrun.c:4158
 #15 0x0000000000642917 in inferior_event_handler (event_type=INF_REG_EVENT, client_data=0x0) at src/gdb/inf-loop.c:57
 #16 0x000000000077ca5c in record_full_async_inferior_event_handler (data=0x0) at src/gdb/record-full.c:791
 #17 0x0000000000640fdf in invoke_async_event_handler (data=...) at src/gdb/event-loop.c:1067
 #18 0x000000000063fb01 in process_event () at src/gdb/event-loop.c:339
 #19 0x000000000063fb2a in gdb_do_one_event () at src/gdb/event-loop.c:360
 #20 0x000000000074d607 in gdb_readline_wrapper (prompt=0x3588f40 "The next instruction is syscall exit_group.  It will make the program exit.  Do you want to stop the program?([y] or n) ") at src/gdb/top.c:842
 #21 0x0000000000750bd9 in defaulted_query (ctlstr=0x8c6588 "The next instruction is syscall exit_group.  It will make the program exit.  Do you want to stop the program?", defchar=121 'y', args=0x7fff70524410) at src/gdb/utils.c:1279
 #22 0x0000000000750e4c in yquery (ctlstr=0x8c6588 "The next instruction is syscall exit_group.  It will make the program exit.  Do you want to stop the program?") at src/gdb/utils.c:1358
 #23 0x00000000004b020e in record_linux_system_call (syscall=gdb_sys_exit_group, regcache=0x3529450, tdep=0xd6c840 <amd64_linux_record_tdep>) at src/gdb/linux-record.c:1933

With my all-stop-on-top-of-non-stop series, I'm also seeing
gdb.server/ext-attach.exp fail occasionally due to the same issue.

The first part of the fix is for target_async implementations to make
sure to remove/unmark all target-related event sources from the event
loop.

Tested on x86_64 Fedora 20, native and gdbserver.

gdb/
2015-01-26  Pedro Alves  <palves@redhat.com>

	* event-loop.c (clear_async_event_handler): New function.
	* event-loop.h (clear_async_event_handler): New declaration.
	* record-btrace.c (record_btrace_async): New function.
	(init_record_btrace_ops): Install record_btrace_async.
	* record-full.c (record_full_async): New function.
	(record_full_resume): Don't mark the async event source here.
	(init_record_full_ops): Install record_full_async.
	(record_full_core_resume): Don't mark the async event source here.
	(init_record_full_core_ops): Install record_full_async.
	* remote.c (remote_async): Mark and clear the async stop reply
	queue event-loop token as appropriate.
---
 gdb/event-loop.c    |  8 ++++++++
 gdb/event-loop.h    |  4 ++++
 gdb/record-btrace.c | 17 +++++++++++++++++
 gdb/record-full.c   | 32 ++++++++++++++++++++------------
 gdb/remote.c        | 10 +++++++++-
 5 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index cfdd9a6..e4de572 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -1050,6 +1050,14 @@ mark_async_event_handler (async_event_handler *async_handler_ptr)
   async_handler_ptr->ready = 1;
 }
 
+/* See event-loop.h.  */
+
+void
+clear_async_event_handler (async_event_handler *async_handler_ptr)
+{
+  async_handler_ptr->ready = 0;
+}
+
 struct async_event_handler_data
 {
   async_event_handler_func* proc;
diff --git a/gdb/event-loop.h b/gdb/event-loop.h
index feb5ea0..b9338a2 100644
--- a/gdb/event-loop.h
+++ b/gdb/event-loop.h
@@ -131,3 +131,7 @@ extern void
 /* Call the handler from HANDLER the next time through the event
    loop.  */
 extern void mark_async_event_handler (struct async_event_handler *handler);
+
+/* Mark the handler (ASYNC_HANDLER_PTR) as NOT ready.  */
+
+extern void clear_async_event_handler (struct async_event_handler *handler);
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3b7ef5c..7af549f 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -266,6 +266,22 @@ record_btrace_close (struct target_ops *self)
     btrace_teardown (tp);
 }
 
+/* The to_async method of target record-btrace.  */
+
+static void
+record_btrace_async (struct target_ops *ops,
+		     void (*callback) (enum inferior_event_type event_type,
+				       void *context),
+		     void *context)
+{
+  if (callback != NULL)
+    mark_async_event_handler (record_btrace_async_inferior_event_handler);
+  else
+    clear_async_event_handler (record_btrace_async_inferior_event_handler);
+
+  ops->beneath->to_async (ops->beneath, callback, context);
+}
+
 /* The to_info_record method of target record-btrace.  */
 
 static void
@@ -1940,6 +1956,7 @@ init_record_btrace_ops (void)
   ops->to_doc = "Collect control-flow trace and provide the execution history.";
   ops->to_open = record_btrace_open;
   ops->to_close = record_btrace_close;
+  ops->to_async = record_btrace_async;
   ops->to_detach = record_detach;
   ops->to_disconnect = record_disconnect;
   ops->to_mourn_inferior = record_mourn_inferior;
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 6db16d6..c660743 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -910,6 +910,22 @@ record_full_close (struct target_ops *self)
     delete_async_event_handler (&record_full_async_inferior_event_token);
 }
 
+/* "to_async" target method.  */
+
+static void
+record_full_async (struct target_ops *ops,
+		   void (*callback) (enum inferior_event_type event_type,
+				     void *context),
+		   void *context)
+{
+  if (callback != NULL)
+    mark_async_event_handler (record_full_async_inferior_event_token);
+  else
+    clear_async_event_handler (record_full_async_inferior_event_token);
+
+  ops->beneath->to_async (ops->beneath, callback, context);
+}
+
 static int record_full_resume_step = 0;
 
 /* True if we've been resumed, and so each record_full_wait call should
@@ -989,12 +1005,7 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step,
   /* We are about to start executing the inferior (or simulate it),
      let's register it with the event loop.  */
   if (target_can_async_p ())
-    {
-      target_async (inferior_event_handler, 0);
-      /* Notify the event loop there's an event to wait for.  We do
-	 most of the work in record_full_wait.  */
-      mark_async_event_handler (record_full_async_inferior_event_token);
-    }
+    target_async (inferior_event_handler, 0);
 }
 
 static int record_full_get_sig = 0;
@@ -1902,6 +1913,7 @@ init_record_full_ops (void)
     "Log program while executing and replay execution from log.";
   record_full_ops.to_open = record_full_open;
   record_full_ops.to_close = record_full_close;
+  record_full_ops.to_async = record_full_async;
   record_full_ops.to_resume = record_full_resume;
   record_full_ops.to_wait = record_full_wait;
   record_full_ops.to_disconnect = record_disconnect;
@@ -1943,12 +1955,7 @@ record_full_core_resume (struct target_ops *ops, ptid_t ptid, int step,
   /* We are about to start executing the inferior (or simulate it),
      let's register it with the event loop.  */
   if (target_can_async_p ())
-    {
-      target_async (inferior_event_handler, 0);
-
-      /* Notify the event loop there's an event to wait for.  */
-      mark_async_event_handler (record_full_async_inferior_event_token);
-    }
+    target_async (inferior_event_handler, 0);
 }
 
 /* "to_kill" method for prec over corefile.  */
@@ -2141,6 +2148,7 @@ init_record_full_core_ops (void)
     "Log program while executing and replay execution from log.";
   record_full_core_ops.to_open = record_full_open;
   record_full_core_ops.to_close = record_full_close;
+  record_full_core_ops.to_async = record_full_async;
   record_full_core_ops.to_resume = record_full_core_resume;
   record_full_core_ops.to_wait = record_full_wait;
   record_full_core_ops.to_kill = record_full_core_kill;
diff --git a/gdb/remote.c b/gdb/remote.c
index 9be15cb..e971a29 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -11691,9 +11691,17 @@ remote_async (struct target_ops *ops,
       serial_async (rs->remote_desc, remote_async_serial_handler, rs);
       rs->async_client_callback = callback;
       rs->async_client_context = context;
+
+      /* If there are pending events in the stop reply queue tell the
+	 event loop to process them.  */
+      if (!QUEUE_is_empty (stop_reply_p, stop_reply_queue))
+	mark_async_event_handler (remote_async_inferior_event_token);
     }
   else
-    serial_async (rs->remote_desc, NULL, NULL);
+    {
+      serial_async (rs->remote_desc, NULL, NULL);
+      clear_async_event_handler (remote_async_inferior_event_token);
+    }
 }
 
 static void
-- 
1.9.3

[PATCH 0/3] Fix racy FAILs of sigall-reverse.exp (and more)

[Pedro Alves]

As can be seen in the buildbot results sent to the gdb-testers@ list,
sigall-reverse.exp is occasionally failing.

With my WIP all-stop-on-stop-of-non-stop series some other tests were
failing in a similar manner, which was what prompted me to look at
this in the first place.

This series fixes the root problems.  sigall-reverse.exp seems to be
robust for me now.

Tested on x86_64 Fedora 20, native and gdbserver.

Given the 'query' change that exposed these bugs is in 7.9, I'd like
to see this fixed there too.  My idea would be to push this into
master, give it a few days of buildbot exposure, and if all goes well,
push it to the branch.

Pedro Alves (3):
  Fix up some target is-async vs can-async confusions
  When disabling target async, remove all target event sources from the
    event loop
  Simplify event-loop core, remove two-step event processing

 gdb/event-loop.c    | 336 +++++++++++++++-------------------------------------
 gdb/event-loop.h    |   5 +-
 gdb/linux-nat.c     |  16 +--
 gdb/record-btrace.c |  17 +++
 gdb/record-full.c   |  32 +++--
 gdb/remote.c        |  16 ++-
 gdb/top.c           |   1 -
 7 files changed, 157 insertions(+), 266 deletions(-)

-- 
1.9.3

[PATCH 1/3] Fix up some target is-async vs can-async confusions

[Pedro Alves]

In all these cases we're interested in whether the target is currently
async, with its event sources installed in the event loop, not whether
it can async if needed.  Also, I'm not seeing the point of the
target_async call from within linux_nat_wait.  That's normally done on
resume instead, which this target already does.

Tested on x86_64 Fedora 20, native and gdbserver.

gdb/
2015-01-26  Pedro Alves  <palves@redhat.com>

	* linux-nat.c (linux_child_follow_fork, linux_nat_wait_1): Use
	target_is_async_p instead of target_can_async.
	(linux_nat_wait): Use target_is_async_p instead of
	target_can_async.  Don't enable async here.
	* remote.c (interrupt_query, remote_wait, putpkt_binary): Use
	target_is_async_p instead of target_can_async.
---
 gdb/linux-nat.c | 16 ++++++----------
 gdb/remote.c    |  6 +++---
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b49cd57..b4893d44 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -520,7 +520,7 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
 
 	      /* If we're in async mode, need to tell the event loop
 		 there's something here to process.  */
-	      if (target_can_async_p ())
+	      if (target_is_async_p ())
 		async_file_mark ();
 	    }
 	}
@@ -3222,7 +3222,7 @@ linux_nat_wait_1 (struct target_ops *ops,
 			    target_pid_to_str (lp->ptid));
     }
 
-  if (!target_can_async_p ())
+  if (!target_is_async_p ())
     {
       /* Causes SIGINT to be passed on to the attached process.  */
       set_sigint_trap ();
@@ -3293,7 +3293,7 @@ linux_nat_wait_1 (struct target_ops *ops,
 
 	  ourstatus->kind = TARGET_WAITKIND_NO_RESUMED;
 
-	  if (!target_can_async_p ())
+	  if (!target_is_async_p ())
 	    clear_sigint_trap ();
 
 	  restore_child_signals_mask (&prev_mask);
@@ -3321,7 +3321,7 @@ linux_nat_wait_1 (struct target_ops *ops,
       sigsuspend (&suspend_mask);
     }
 
-  if (!target_can_async_p ())
+  if (!target_is_async_p ())
     clear_sigint_trap ();
 
   gdb_assert (lp);
@@ -3479,7 +3479,7 @@ linux_nat_wait (struct target_ops *ops,
     }
 
   /* Flush the async file first.  */
-  if (target_can_async_p ())
+  if (target_is_async_p ())
     async_file_flush ();
 
   /* Resume LWPs that are currently stopped without any pending status
@@ -3497,16 +3497,12 @@ linux_nat_wait (struct target_ops *ops,
   /* If we requested any event, and something came out, assume there
      may be more.  If we requested a specific lwp or process, also
      assume there may be more.  */
-  if (target_can_async_p ()
+  if (target_is_async_p ()
       && ((ourstatus->kind != TARGET_WAITKIND_IGNORE
 	   && ourstatus->kind != TARGET_WAITKIND_NO_RESUMED)
 	  || !ptid_equal (ptid, minus_one_ptid)))
     async_file_mark ();
 
-  /* Get ready for the next event.  */
-  if (target_can_async_p ())
-    target_async (inferior_event_handler, 0);
-
   return event_ptid;
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index c4d09ba..9be15cb 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5079,7 +5079,7 @@ interrupt_query (void)
 {
   target_terminal_ours ();
 
-  if (target_can_async_p ())
+  if (target_is_async_p ())
     {
       signal (SIGINT, handle_sigint);
       quit ();
@@ -6036,7 +6036,7 @@ remote_wait (struct target_ops *ops,
   else
     event_ptid = remote_wait_as (ptid, status, options);
 
-  if (target_can_async_p ())
+  if (target_is_async_p ())
     {
       /* If there are are events left in the queue tell the event loop
 	 to return here.  */
@@ -7226,7 +7226,7 @@ putpkt_binary (const char *buf, int cnt)
      case it's not possible to issue a command while the target is
      running.  This is not a problem in non-stop mode, because in that
      case, the stub is always ready to process serial input.  */
-  if (!non_stop && target_can_async_p () && rs->waiting_for_stop_reply)
+  if (!non_stop && target_is_async_p () && rs->waiting_for_stop_reply)
     {
       error (_("Cannot execute this command while the target is running.\n"
 	       "Use the \"interrupt\" command to stop the target\n"
-- 
1.9.3

[PATCH 3/3] Simplify event-loop core, remove two-step event processing

[Pedro Alves]

Even with the previous patch installed, we'll still see
sigall-reverse.exp occasionally fail.  The problem is that the event
loop's event handling processing is done in two steps:

 #1 - poll all event sources, and push new event objects to the event
  queue, until all event sources are drained.

 #2 - go through the event queue, processing each event object at a
  time.  For each event, call the associated callback, and deletes the
  event object from the queue.

and then bad things happen if between #1 and #2 something decides that
events from an event source that has already queued events shouldn't
be processed yet.  To do that, we either remove the event source from
the list of event sources, or clear its "have events" flag.  However,
if an event for that source has meanwhile already been pushed in the
event queue, #2 will still process it and call the associated
callback...

One way to fix it that I considered was to do something to the event
objects already in the event queue when an event source is no longer
interesting.  But then I couldn't find any good reason for the
two-step process in the first place.  It's much simpler (and less
code) to call the event source callbacks as we poll the sources and
find events.

Tested on x86-64 Fedora 20, native and gdbserver.

gdb/
2015-01-26  Pedro Alves  <palves@redhat.com>

	* event-loop.c: Don't declare nor define a queue type for
	gdb_event_p.
	(event_queue): Delete.
	(create_event, create_file_event, gdb_event_xfree)
	(initialize_event_loop, process_event): Delete.
	(gdb_do_one_event): Return as soon as one event is handled.
	(handle_file_event): Change prototype.  Used the passed in
	file_handler pointer and ready_mask instead of looping over all
	file handlers.
	(gdb_wait_for_event): Update the poll/select timeouts before
	blocking.  Run event handlers directly instead of queueing events.
	Return as soon as one event is handled.
	(struct async_event_handler_data): Delete.
	(invoke_async_event_handler): Delete.
	(check_async_event_handlers): Change return type to int.  Run
	event handlers directly instead of queueing events.  Return as
	soon as one event is handled.
	(handle_timer_event): Delete.
	(update_wait_timeout): New function, factored out from
	poll_timers.
	(poll_timers): Reimplement.
	* event-loop.h (initialize_event_loop): Delete declaration.
	* top.c (gdb_init): Don't call initialize_event_loop.
---
 gdb/event-loop.c | 334 ++++++++++++++++---------------------------------------
 gdb/event-loop.h |   1 -
 gdb/top.c        |   1 -
 3 files changed, 93 insertions(+), 243 deletions(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index e4de572..7425b3a 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -135,10 +135,6 @@ typedef struct async_event_handler
   }
 async_event_handler;
 
-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
    descriptor.  We have two flavors of the notifier, one for platforms
@@ -246,104 +242,12 @@ async_event_handler_list;
 static int invoke_async_signal_handlers (void);
 static void create_file_handler (int fd, int mask, handler_func *proc,
 				 gdb_client_data client_data);
-static void handle_file_event (event_data data);
-static void check_async_event_handlers (void);
+static int check_async_event_handlers (void);
 static int gdb_wait_for_event (int);
-static void poll_timers (void);
+static int update_wait_timeout (void);
+static int poll_timers (void);
 \f
 
-/* 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.  */
-
-static gdb_event *
-create_event (event_handler_func proc, event_data data)
-{
-  gdb_event *event;
-
-  event = xmalloc (sizeof (*event));
-  event->proc = proc;
-  event->data = data;
-
-  return event;
-}
-
-/* Create a file event, to be enqueued in the event queue for
-   processing.  The procedure associated to this event is always
-   handle_file_event, which will in turn invoke the one that was
-   associated to FD when it was registered with the event loop.  */
-static gdb_event *
-create_file_event (int fd)
-{
-  event_data data;
-
-  data.integer = 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
-   the reception of a signal.
-   If an event was processed (either way), 1 is returned otherwise
-   0 is returned.
-   Scan the queue from head to tail, processing therefore the high
-   priority events first, by invoking the associated event handler
-   procedure.  */
-static int
-process_event (void)
-{
-  /* 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.  */
-
-  if (invoke_async_signal_handlers ())
-    return 1;
-
-  /* Look in the event queue to find an event that is ready
-     to be processed.  */
-
-  if (!QUEUE_is_empty (gdb_event_p, event_queue))
-    {
-      /* 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.  */
-      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;
-
-      gdb_event_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,
    wait for something to happen (via gdb_wait_for_event), then process
    it.  Returns >0 if something was done otherwise returns <0 (this
@@ -356,40 +260,42 @@ gdb_do_one_event (void)
   const int number_of_sources = 3;
   int current = 0;
 
-  /* Any events already waiting in the queue?  */
-  if (process_event ())
+  /* First let's see if there are any asynchronous signal handlers
+     that are ready.  These would be the result of invoking any of the
+     signal handlers.  */
+  if (invoke_async_signal_handlers ())
     return 1;
 
   /* To level the fairness across event sources, we poll them in a
      round-robin fashion.  */
   for (current = 0; current < number_of_sources; current++)
     {
+      int res;
+
       switch (event_source_head)
 	{
 	case 0:
-	  /* Are any timers that are ready? If so, put an event on the
-	     queue.  */
-	  poll_timers ();
+	  /* Are any timers that are ready?  */
+	  res = poll_timers ();
 	  break;
 	case 1:
 	  /* Are there events already waiting to be collected on the
 	     monitored file descriptors?  */
-	  gdb_wait_for_event (0);
+	  res = gdb_wait_for_event (0);
 	  break;
 	case 2:
 	  /* Are there any asynchronous event handlers ready?  */
-	  check_async_event_handlers ();
+	  res = check_async_event_handlers ();
 	  break;
 	}
 
       event_source_head++;
       if (event_source_head == number_of_sources)
 	event_source_head = 0;
-    }
 
-  /* Handle any new events collected.  */
-  if (process_event ())
-    return 1;
+      if (res > 0)
+	return 1;
+    }
 
   /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
      we should get out because this means that there are no event
@@ -399,10 +305,6 @@ gdb_do_one_event (void)
   if (gdb_wait_for_event (1) < 0)
     return -1;
 
-  /* Handle any new events occurred while waiting.  */
-  if (process_event ())
-    return 1;
-
   /* If gdb_wait_for_event has returned 1, it means that one event has
      been handled.  We break out of the loop.  */
   return 1;
@@ -684,25 +586,17 @@ delete_file_handler (int fd)
 }
 
 /* Handle the given event by calling the procedure associated to the
-   corresponding file handler.  Called by process_event indirectly,
-   through event_ptr->proc.  EVENT_FILE_DESC is file descriptor of the
-   event in the front of the event queue.  */
+   corresponding file handler.  */
+
 static void
-handle_file_event (event_data data)
+handle_file_event (file_handler *file_ptr, int ready_mask)
 {
-  file_handler *file_ptr;
   int mask;
 #ifdef HAVE_POLL
   int error_mask;
 #endif
-  int event_file_desc = data.integer;
 
-  /* Search the file handler list to find one that matches the fd in
-     the event.  */
-  for (file_ptr = gdb_notifier.first_file_handler; file_ptr != NULL;
-       file_ptr = file_ptr->next_file)
     {
-      if (file_ptr->fd == event_file_desc)
 	{
 	  /* With poll, the ready_mask could have any of three events
 	     set to 1: POLLHUP, POLLERR, POLLNVAL.  These events
@@ -720,7 +614,7 @@ handle_file_event (event_data data)
 	      /* POLLHUP means EOF, but can be combined with POLLIN to
 		 signal more data to read.  */
 	      error_mask = POLLHUP | POLLERR | POLLNVAL;
-	      mask = file_ptr->ready_mask & (file_ptr->mask | error_mask);
+	      mask = ready_mask & (file_ptr->mask | error_mask);
 
 	      if ((mask & (POLLERR | POLLNVAL)) != 0)
 		{
@@ -743,7 +637,7 @@ handle_file_event (event_data data)
 	    }
 	  else
 	    {
-	      if (file_ptr->ready_mask & GDB_EXCEPTION)
+	      if (ready_mask & GDB_EXCEPTION)
 		{
 		  printf_unfiltered (_("Exception condition detected "
 				       "on fd %d\n"), file_ptr->fd);
@@ -751,30 +645,27 @@ handle_file_event (event_data data)
 		}
 	      else
 		file_ptr->error = 0;
-	      mask = file_ptr->ready_mask & file_ptr->mask;
+	      mask = ready_mask & file_ptr->mask;
 	    }
 
-	  /* Clear the received events for next time around.  */
-	  file_ptr->ready_mask = 0;
-
 	  /* If there was a match, then call the handler.  */
 	  if (mask != 0)
 	    (*file_ptr->proc) (file_ptr->error, file_ptr->client_data);
-	  break;
 	}
     }
 }
 
-/* Called by gdb_do_one_event to wait for new events on the monitored
-   file descriptors.  Queue file events as they are detected by the
-   poll.  If BLOCK and if there are no events, this function will
-   block in the call to poll.  Return -1 if there are no file
-   descriptors to monitor, otherwise return 0.  */
+/* Wait for new events on the monitored file descriptors.  Run the
+   event handler if the first descriptor that is detected by the poll.
+   If BLOCK and if there are no events, this function will block in
+   the call to poll.  Return 1 if an event was handled.  Return -1 if
+   there are no file descriptors to monitor.  Return 1 if an event was
+   handled, otherwise returns 0.  */
+
 static int
 gdb_wait_for_event (int block)
 {
   file_handler *file_ptr;
-  gdb_event *file_event_ptr;
   int num_found = 0;
   int i;
 
@@ -785,6 +676,9 @@ gdb_wait_for_event (int block)
   if (gdb_notifier.num_fds == 0)
     return -1;
 
+  if (block)
+    update_wait_timeout ();
+
   if (use_poll)
     {
 #ifdef HAVE_POLL
@@ -844,7 +738,10 @@ gdb_wait_for_event (int block)
 	}
     }
 
-  /* Enqueue all detected file events.  */
+  /* Run event handlers.  We always run just one handler and go back
+     to polling, in case a handler changes the notifier list.  Since
+     events for sources we haven't consumed yet wake poll/select
+     immediately, no event is lost.  */
 
   if (use_poll)
     {
@@ -866,14 +763,10 @@ gdb_wait_for_event (int block)
 
 	  if (file_ptr)
 	    {
-	      /* Enqueue an event only if this is still a new event for
-	         this fd.  */
-	      if (file_ptr->ready_mask == 0)
-		{
-		  file_event_ptr = create_file_event (file_ptr->fd);
-		  QUEUE_enque (gdb_event_p, event_queue, file_event_ptr);
-		}
-	      file_ptr->ready_mask = (gdb_notifier.poll_fds + i)->revents;
+	      int mask = (gdb_notifier.poll_fds + i)->revents;
+
+	      handle_file_event (file_ptr, mask);
+	      return 1;
 	    }
 	}
 #else
@@ -901,15 +794,8 @@ gdb_wait_for_event (int block)
 	  else
 	    num_found--;
 
-	  /* Enqueue an event only if this is still a new event for
-	     this fd.  */
-
-	  if (file_ptr->ready_mask == 0)
-	    {
-	      file_event_ptr = create_file_event (file_ptr->fd);
-	      QUEUE_enque (gdb_event_p, event_queue, file_event_ptr);
-	    }
-	  file_ptr->ready_mask = mask;
+	  handle_file_event (file_ptr, mask);
+	  return 1;
 	}
     }
   return 0;
@@ -1058,32 +944,13 @@ clear_async_event_handler (async_event_handler *async_handler_ptr)
   async_handler_ptr->ready = 0;
 }
 
-struct async_event_handler_data
-{
-  async_event_handler_func* proc;
-  gdb_client_data client_data;
-};
-
-static void
-invoke_async_event_handler (event_data data)
-{
-  struct async_event_handler_data *hdata = data.ptr;
-  async_event_handler_func* proc = hdata->proc;
-  gdb_client_data client_data = hdata->client_data;
+/* Check if asynchronous event handlers are ready, and call the
+   handler function for one that is.  */
 
-  xfree (hdata);
-  (*proc) (client_data);
-}
-
-/* Check if any asynchronous event handlers are ready, and queue
-   events in the ready queue for any that are.  */
-static void
+static int
 check_async_event_handlers (void)
 {
   async_event_handler *async_handler_ptr;
-  struct async_event_handler_data *hdata;
-  struct gdb_event *event_ptr;
-  event_data data;
 
   for (async_handler_ptr = async_event_handler_list.first_handler;
        async_handler_ptr != NULL;
@@ -1092,18 +959,12 @@ check_async_event_handlers (void)
       if (async_handler_ptr->ready)
 	{
 	  async_handler_ptr->ready = 0;
-
-	  hdata = xmalloc (sizeof (*hdata));
-
-	  hdata->proc = async_handler_ptr->proc;
-	  hdata->client_data = async_handler_ptr->client_data;
-
-	  data.ptr = hdata;
-
-	  event_ptr = create_event (invoke_async_event_handler, data);
-	  QUEUE_enque (gdb_event_p, event_queue, event_ptr);
+	  (*async_handler_ptr->proc) (async_handler_ptr->client_data);
+	  return 1;
 	}
     }
+
+  return 0;
 }
 
 /* Delete an asynchronous handler (ASYNC_HANDLER_PTR).
@@ -1236,49 +1097,13 @@ delete_timer (int id)
   gdb_notifier.timeout_valid = 0;
 }
 
-/* When a timer event is put on the event queue, it will be handled by
-   this function.  Just call the associated procedure and delete the
-   timer event from the event queue.  Repeat this for each timer that
-   has expired.  */
-static void
-handle_timer_event (event_data dummy)
-{
-  struct timeval time_now;
-  struct gdb_timer *timer_ptr, *saved_timer;
-
-  gettimeofday (&time_now, NULL);
-  timer_ptr = timer_list.first_timer;
-
-  while (timer_ptr != NULL)
-    {
-      if ((timer_ptr->when.tv_sec > time_now.tv_sec)
-	  || ((timer_ptr->when.tv_sec == time_now.tv_sec)
-	      && (timer_ptr->when.tv_usec > time_now.tv_usec)))
-	break;
-
-      /* Get rid of the timer from the beginning of the list.  */
-      timer_list.first_timer = timer_ptr->next;
-      saved_timer = timer_ptr;
-      timer_ptr = timer_ptr->next;
-      /* Call the procedure associated with that timer.  */
-      (*saved_timer->proc) (saved_timer->client_data);
-      xfree (saved_timer);
-    }
-
-  gdb_notifier.timeout_valid = 0;
-}
+/* Update the timeout for the select() or poll().  Returns true if the
+   timer has already expired, false otherwise.  */
 
-/* Check whether any timers in the timers queue are ready.  If at least
-   one timer is ready, stick an event onto the event queue.  Even in
-   case more than one timer is ready, one event is enough, because the
-   handle_timer_event() will go through the timers list and call the
-   procedures associated with all that have expired.l Update the
-   timeout for the select() or poll() as well.  */
-static void
-poll_timers (void)
+static int
+update_wait_timeout (void)
 {
   struct timeval time_now, delta;
-  gdb_event *event_ptr;
 
   if (timer_list.first_timer != NULL)
     {
@@ -1292,27 +1117,18 @@ poll_timers (void)
 	  delta.tv_usec += 1000000;
 	}
 
-      /* Oops it expired already.  Tell select / poll to return
-         immediately.  (Cannot simply test if delta.tv_sec is negative
-         because time_t might be unsigned.)  */
+      /* Cannot simply test if delta.tv_sec is negative because time_t
+         might be unsigned.  */
       if (timer_list.first_timer->when.tv_sec < time_now.tv_sec
 	  || (timer_list.first_timer->when.tv_sec == time_now.tv_sec
 	      && timer_list.first_timer->when.tv_usec < time_now.tv_usec))
 	{
+	  /* It expired already.  */
 	  delta.tv_sec = 0;
 	  delta.tv_usec = 0;
 	}
 
-      if (delta.tv_sec == 0 && delta.tv_usec == 0)
-	{
-	  event_ptr = (gdb_event *) xmalloc (sizeof (gdb_event));
-	  event_ptr->proc = handle_timer_event;
-	  event_ptr->data.integer = timer_list.first_timer->timer_id;
-	  QUEUE_enque (gdb_event_p, event_queue, event_ptr);
-	}
-
-      /* Now we need to update the timeout for select/ poll, because
-         we don't want to sit there while this timer is expiring.  */
+      /* Update the timeout for select/ poll.  */
       if (use_poll)
 	{
 #ifdef HAVE_POLL
@@ -1328,7 +1144,43 @@ poll_timers (void)
 	  gdb_notifier.select_timeout.tv_usec = delta.tv_usec;
 	}
       gdb_notifier.timeout_valid = 1;
+
+      if (delta.tv_sec == 0 && delta.tv_usec == 0)
+	return 1;
     }
   else
     gdb_notifier.timeout_valid = 0;
+
+  return 0;
+}
+
+/* Check whether a timer in the timers queue is ready.  If a timer is
+   ready, call its handler and return.  Update the timeout for the
+   select() or poll() as well.  Return 1 if an event was handled,
+   otherwise returns 0.*/
+
+static int
+poll_timers (void)
+{
+  if (update_wait_timeout ())
+    {
+      struct gdb_timer *timer_ptr = timer_list.first_timer;
+      timer_handler_func *proc = timer_ptr->proc;
+      gdb_client_data client_data = timer_ptr->client_data;
+
+      /* Get rid of the timer from the beginning of the list.  */
+      timer_list.first_timer = timer_ptr->next;
+
+      /* Delete the timer before calling the callback, not after, in
+	 case the callback itself decides to try deleting the timer
+	 too.  */
+      xfree (timer_ptr);
+
+      /* Call the procedure associated with that timer.  */
+      (proc) (client_data);
+
+      return 1;
+    }
+
+  return 0;
 }
diff --git a/gdb/event-loop.h b/gdb/event-loop.h
index b9338a2..6ae4013 100644
--- a/gdb/event-loop.h
+++ b/gdb/event-loop.h
@@ -77,7 +77,6 @@ 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 a1462a0..6b17fc4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1952,7 +1952,6 @@ 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.9.3

Re: [PATCH 0/3] Fix racy FAILs of sigall-reverse.exp (and more)

[Pedro Alves]

On 01/26/2015 06:19 PM, Pedro Alves wrote:
> As can be seen in the buildbot results sent to the gdb-testers@ list,
> sigall-reverse.exp is occasionally failing.
> 
> With my WIP all-stop-on-stop-of-non-stop series some other tests were
> failing in a similar manner, which was what prompted me to look at
> this in the first place.
> 
> This series fixes the root problems.  sigall-reverse.exp seems to be
> robust for me now.
> 
> Tested on x86_64 Fedora 20, native and gdbserver.
> 
> Given the 'query' change that exposed these bugs is in 7.9, I'd like
> to see this fixed there too.  My idea would be to push this into
> master, give it a few days of buildbot exposure, and if all goes well,
> push it to the branch.

I've pushed the series to master now.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Simplify event-loop core, remove two-step event processing

[Patrick Palka]

This patch causes a build failure when compiling GDB with GCC 4.9:

/home/patrick/code/binutils-gdb/gdb/event-loop.c: In function
‘gdb_do_one_event’:
/home/patrick/code/binutils-gdb/gdb/event-loop.c:296:10: error: ‘res’
may be used uninitialized in this function
[-Werror=maybe-uninitialized]
       if (res > 0)
          ^

Re: [PATCH 3/3] Simplify event-loop core, remove two-step event processing

[Sergio Durigan Junior]

On Monday, January 26 2015, Pedro Alves wrote:

> Tested on x86-64 Fedora 20, native and gdbserver.

Hey!

Patrick already mentioned it, but:

  <http://gdb-build.sergiodj.net/builders/Fedora-x86_64-m64/builds/137>

This is on Fedora 21, latest gcc.

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH 3/3] Simplify event-loop core, remove two-step event processing

[Pedro Alves]

On 02/03/2015 11:48 PM, Patrick Palka wrote:
> This patch causes a build failure when compiling GDB with GCC 4.9:
> 
> /home/patrick/code/binutils-gdb/gdb/event-loop.c: In function
> ‘gdb_do_one_event’:
> /home/patrick/code/binutils-gdb/gdb/event-loop.c:296:10: error: ‘res’
> may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>        if (res > 0)
>           ^
> 

Bah, -O0 vs -O2.  I've pushed the fix below.

-------
Subject: [PATCH] Fix build breakage due to event loop simplification

commit 70b66289 (Simplify event-loop core, remove two-step event
processing) causes a build failure when compiling GDB with gcc/-O2:

 gdb/event-loop.c: In function ‘gdb_do_one_event’:
 gdb/event-loop.c:296:10: error: ‘res’ may be used uninitialized in this function
 [-Werror=maybe-uninitialized]
	if (res > 0)
	   ^

GCC isn't realizing that event_source_head can never be > 2 and that
therefore 'res' is always initialized in all possible paths.  Adding a
default case that internal_error's makes GCC realize that.

Tested on x86_64 Fedora 20.

gdb/ChangeLog:
2015-02-04  Pedro Alves  <palves@redhat.com>

	Fix build breakage.
	* event-loop.c (gdb_do_one_event): Add default switch case.
---
 gdb/ChangeLog    | 5 +++++
 gdb/event-loop.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2266c11..1116853 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-04  Pedro Alves  <palves@redhat.com>
+
+	Fix build breakage.
+	* event-loop.c (gdb_do_one_event): Add default switch case.
+
 2015-02-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Filter out inferior gcc option -fpreprocessed.
diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index 7425b3a..a2b41a7 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -287,6 +287,10 @@ gdb_do_one_event (void)
 	  /* Are there any asynchronous event handlers ready?  */
 	  res = check_async_event_handlers ();
 	  break;
+	default:
+	  internal_error (__FILE__, __LINE__,
+			  "unexpected event_source_head %d",
+			  event_source_head);
 	}
 
       event_source_head++;
-- 
1.9.3

Re: Re: [PATCH 3/3] Simplify event-loop core, remove two-step event processing

[Alex Velenko]

On 03/02/15 22:48, Patrick Palka wrote:
> This patch causes a build failure when compiling GDB with GCC 4.9:
>
> /home/patrick/code/binutils-gdb/gdb/event-loop.c: In function
> ‘gdb_do_one_event’:
> /home/patrick/code/binutils-gdb/gdb/event-loop.c:296:10: error: ‘res’
> may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>         if (res > 0)
>            ^
>

Hi,
Event-loop.c "gdb_do_one_event" function did not initialize variable "res".
This caused gdb_binutils to fail building. This patch fixes it.

Builds with this patch, does not build without it.

Is patch ok? If ok, could you, please, commit (I can't)?

Kind regards,
Alex

gdb/

2015-02-04  Alex Velenko  <Alex.Velenko@arm.com>

   * event-loop.c (gdb_do_one_event): variable "res" initialized.
---
  gdb/event-loop.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index 7425b3a..1e2dc45 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -270,7 +270,7 @@ gdb_do_one_event (void)
       round-robin fashion.  */
    for (current = 0; current < number_of_sources; current++)
      {
-      int res;
+      int res = 0;

        switch (event_source_head)
  	{
-- 
1.8.1.2

Re: [PATCH 3/3] Simplify event-loop core, remove two-step event processing

[Pedro Alves]

On 02/04/2015 06:34 PM, Alex Velenko wrote:
> On 03/02/15 22:48, Patrick Palka wrote:
>> This patch causes a build failure when compiling GDB with GCC 4.9:
>>
>> /home/patrick/code/binutils-gdb/gdb/event-loop.c: In function
>> ‘gdb_do_one_event’:
>> /home/patrick/code/binutils-gdb/gdb/event-loop.c:296:10: error: ‘res’
>> may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>         if (res > 0)
>>            ^
>>
> 
> Hi,
> Event-loop.c "gdb_do_one_event" function did not initialize variable "res".
> This caused gdb_binutils to fail building. This patch fixes it.

Thanks!  However, this fixed it for me:

 https://sourceware.org/ml/gdb-patches/2015-02/msg00075.html

Can you make sure you have that in your tree?

Thanks,
Pedro Alves