[PATCH] Cleanup num_found usage in gdb_wait_for_event

[PATCH] Cleanup num_found usage in gdb_wait_for_event

[Simon Marchi]

Probably an artifact from the past, managing num_found in those loops is
not need.

Regtested on Ubuntu 14.04 x64, although only with use_poll == 1.

gdb/ChangeLog:

	* event-loop.c (gdb_wait_for_event): Cleanup uneeded usages of
	num_found.
---
 gdb/event-loop.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index 79e41fd..c7ad8b8 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -751,24 +751,22 @@ gdb_wait_for_event (int block)
   if (use_poll)
     {
 #ifdef HAVE_POLL
-      for (i = 0; (i < gdb_notifier.num_fds) && (num_found > 0); i++)
+      for (i = 0; i < gdb_notifier.num_fds; i++)
 	{
-	  if ((gdb_notifier.poll_fds + i)->revents)
-	    num_found--;
-	  else
+	  if (!gdb_notifier.poll_fds[i].revents)
 	    continue;
 
 	  for (file_ptr = gdb_notifier.first_file_handler;
 	       file_ptr != NULL;
 	       file_ptr = file_ptr->next_file)
 	    {
-	      if (file_ptr->fd == (gdb_notifier.poll_fds + i)->fd)
+	      if (file_ptr->fd == gdb_notifier.poll_fds[i].fd)
 		break;
 	    }
 
 	  if (file_ptr)
 	    {
-	      int mask = (gdb_notifier.poll_fds + i)->revents;
+	      int mask = gdb_notifier.poll_fds[i].revents;
 
 	      handle_file_event (file_ptr, mask);
 	      return 1;
@@ -782,7 +780,7 @@ gdb_wait_for_event (int block)
   else
     {
       for (file_ptr = gdb_notifier.first_file_handler;
-	   (file_ptr != NULL) && (num_found > 0);
+	   file_ptr != NULL;
 	   file_ptr = file_ptr->next_file)
 	{
 	  int mask = 0;
@@ -796,8 +794,6 @@ gdb_wait_for_event (int block)
 
 	  if (!mask)
 	    continue;
-	  else
-	    num_found--;
 
 	  handle_file_event (file_ptr, mask);
 	  return 1;
-- 
2.1.4

Re: [PATCH] Cleanup num_found usage in gdb_wait_for_event

[Pedro Alves]

On 04/29/2015 07:19 PM, Simon Marchi wrote:
> Probably an artifact from the past, managing num_found in those loops is
> not need.
> 
> Regtested on Ubuntu 14.04 x64, although only with use_poll == 1.
> 
> gdb/ChangeLog:
> 
> 	* event-loop.c (gdb_wait_for_event): Cleanup uneeded usages of
> 	num_found.

Hmm, I'm not sure whether the state of poll_fds[i].revents is defined
if poll returns timeout or -1/EINTR, and it seems that with this change
we'll walk the poll_fds array on -1/EINTR.

I was planning on sending this patch soon that touches this code, and
makes use of num_found:

 https://github.com/palves/gdb/commit/5652cc4487a1cc5e1d5ab85b64f325292fd059f2

Now I'm curious how you ran into this.

Thanks,
Pedro Alves

Re: [PATCH] Cleanup num_found usage in gdb_wait_for_event

[Simon Marchi]

On 15-04-29 03:19 PM, Pedro Alves wrote:
> On 04/29/2015 07:19 PM, Simon Marchi wrote:
>> Probably an artifact from the past, managing num_found in those loops is
>> not need.
>>
>> Regtested on Ubuntu 14.04 x64, although only with use_poll == 1.
>>
>> gdb/ChangeLog:
>>
>> 	* event-loop.c (gdb_wait_for_event): Cleanup uneeded usages of
>> 	num_found.
> 
> Hmm, I'm not sure whether the state of poll_fds[i].revents is defined
> if poll returns timeout or -1/EINTR, and it seems that with this change
> we'll walk the poll_fds array on -1/EINTR.
> 
> I was planning on sending this patch soon that touches this code, and
> makes use of num_found:
> 
>  https://github.com/palves/gdb/commit/5652cc4487a1cc5e1d5ab85b64f325292fd059f2
> 
> Now I'm curious how you ran into this.
> 
> Thanks,
> Pedro Alves

Oh well, I was investigating the exact same test failure (mi-nsmoribund.exp). It's
failing maybe 90% of the time on our x86 Jenkins boxes. I was actually writing a
patch for that at the moment.

For poll, I keep a static variable around that tells which pollfd structure to start
iterating from. So the first time we check 0, 1, 2, next time we'll check 1, 2, 0, then
2, 0, 1, etc. It's really simple. The static variable should be a field in gdb_notifier
though.

For select, I move the handled file_ptr at the end of the linked list, so that it has the
least priority the next time around. You solution to keep a pointer to the first file_ptr
to check next time is simpler though. Then you can just iterate on the list starting at
that item, making sure you handle correctly the wrap at the end of the list.

Here is my patch in raw form:
https://github.com/simark/binutils-gdb/commit/ec49aaec6963f0aa974d183b8fca669f6261274d

In any case, I don't see why interacting with num_found here is useful, given that we only
handle one event. If we did handle all the found events, then I would understand, since you
want to know when you are done.

[PATCH] Fix gdb.mi/mi-nsmoribund.exp timeouts (Re: [PATCH] Cleanup num_found usage in gdb_wait_for_event)

[Pedro Alves]

On 04/29/2015 08:40 PM, Simon Marchi wrote:

> Oh well, I was investigating the exact same test failure (mi-nsmoribund.exp). It's
> failing maybe 90% of the time on our x86 Jenkins boxes. I was actually writing a
> patch for that at the moment.
> 
> For poll, I keep a static variable around that tells which pollfd structure to start
> iterating from. So the first time we check 0, 1, 2, next time we'll check 1, 2, 0, then
> 2, 0, 1, etc. It's really simple. The static variable should be a field in gdb_notifier
> though.

That's _exactly_ what my patch does too.  :-)  That was this bit:

@@ -751,8 +794,20 @@ gdb_wait_for_event (int block)
   if (use_poll)
     {
 #ifdef HAVE_POLL
-      for (i = 0; (i < gdb_notifier.num_fds) && (num_found > 0); i++)
+      /* To level the fairness across event sources, we poll them in a
+        round-robin-like fashion.  The number and order of the polled
+        file descriptors may change between invocations, but this is
+        good enough.  */
+      static int last_notifier = -1;
+
+      while (num_found > 0)
        {
+         last_notifier++;
+         if (last_notifier >= gdb_notifier.num_fds)
+           last_notifier = 0;
+
+         i = last_notifier;
+
          if ((gdb_notifier.poll_fds + i)->revents)
            num_found--;
          else


> 
> Here is my patch in raw form:
> https://github.com/simark/binutils-gdb/commit/ec49aaec6963f0aa974d183b8fca669f6261274d
> 
> In any case, I don't see why interacting with num_found here is useful, given that we only
> handle one event. If we did handle all the found events, then I would understand, since you
> want to know when you are done.

The only reason I think is to avoid looking at the pollfds if poll
returned no events.  So, how about we combine and simplify things,
like this?

Thanks,
Pedro Alves

From 1dfb4a7a57179f633b18feb054211257050a22c2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 6 May 2015 18:34:32 +0100
Subject: [PATCH] Fix gdb.mi/mi-nsmoribund.exp timeouts

The PPC64 buildbot has been showing timeouts in mi-nsmoribund.exp,
like this:

 (...)
 -thread-info
 FAIL: gdb.mi/mi-nsmoribund.exp: thread state: all running except the breakpoint thread (timeout)

... and I can reproduce this on gcc110 (PPC64) on the gcc compile
farm.

That is, the test sends "-thread-info" to GDB, but GDB never replies
back.

The problem is that these machines are too fast for gdb.  :-)

That test has a few threads running the same tight loop, and
constantly hitting a thread-specific breakpoint that needs to be
stepped over.  If threads trip on breakpoints fast enough that
linux-nat.c's event pipe associated with SIGCHLD is constantly being
written to, even if the stdin file descriptor also has an event to
handle, gdb never gets to it. because linux-nat.c's pipe comes first
in the set of descriptors served by the poll/select code in the event
loop.

Fix this by having the event loop serve file event sources in
round-robin-like fashion, similarly to how its done in
gdb_do_one_event.

Unfortunately, the poll and the select variants each need their own
fixing.

Tested on x86_64 Fedora 20 (poll and select variants), and PPC64
Fedora 18.  Fixes the timeout in the PPC64 machine in the compile farm
that times out without this, and I won't be surprised if it fixes
other random timeouts in other tests.

(gdbserver's copy of the event-loop doesn't need this (yet), as it
still pushes all ready events to an event queue.  That is, it hasn't
had 70b66289 merged yet.  We should really merge both event-loop.c
copies into a single shared file, but that's for another day.)

gdb/ChangeLog:
2015-05-06  Pedro Alves  <palves@redhat.com>
	    Simon Marchi  <simon.marchi@ericsson.com>

	* event-loop.c (gdb_notifier) <next_file_handler,
	next_poll_fds_index>: New fields.
	(get_next_file_handler_to_handle_and_advance): New function.
	(delete_file_handler): If deleting the next file handler to
	handle, advance to the next file handler.
	(gdb_wait_for_event): Bail early if no event fired.  Poll file
	handlers in round-robin fashion.
---
 gdb/event-loop.c | 120 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 88 insertions(+), 32 deletions(-)

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index 79e41fd..9ac4908 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -165,10 +165,24 @@ static struct
     /* Ptr to head of file handler list.  */
     file_handler *first_file_handler;
 
+    /* Next file handler to handle, for the select variant.  To level
+       the fairness across event sources, we serve file handlers in a
+       round-robin-like fashion.  The number and order of the polled
+       file handlers may change between invocations, but this is good
+       enough.  */
+    file_handler *next_file_handler;
+
 #ifdef HAVE_POLL
     /* Ptr to array of pollfd structures.  */
     struct pollfd *poll_fds;
 
+    /* Next file descriptor to handle, for the poll variant.  To level
+       the fairness across event sources, we poll the file descriptors
+       in a round-robin-like fashion.  The number and order of the
+       polled file descriptors may change between invocations, but
+       this is good enough.  */
+    int next_poll_fds_index;
+
     /* Timeout in milliseconds for calls to poll().  */
     int poll_timeout;
 #endif
@@ -494,6 +508,31 @@ create_file_handler (int fd, int mask, handler_func * proc,
   file_ptr->mask = mask;
 }
 
+/* Return the next file handler to handle, and advance to the next
+   file handler, wrapping around if the end of the list is
+   reached.  */
+
+static file_handler *
+get_next_file_handler_to_handle_and_advance (void)
+{
+  file_handler *curr_next;
+
+  /* The first time around, this is still NULL.  */
+  if (gdb_notifier.next_file_handler == NULL)
+    gdb_notifier.next_file_handler = gdb_notifier.first_file_handler;
+
+  curr_next = gdb_notifier.next_file_handler;
+  gdb_assert (curr_next != NULL);
+
+  /* Advance.  */
+  gdb_notifier.next_file_handler = curr_next->next_file;
+  /* Wrap around, if necessary.  */
+  if (gdb_notifier.next_file_handler == NULL)
+    gdb_notifier.next_file_handler = gdb_notifier.first_file_handler;
+
+  return curr_next;
+}
+
 /* Remove the file descriptor FD from the list of monitored fd's: 
    i.e. we don't care anymore about events on the FD.  */
 void
@@ -576,6 +615,17 @@ delete_file_handler (int fd)
 
   file_ptr->mask = 0;
 
+  /* If this file handler was going to be the next one to be handled,
+     advance to the next's next, if any.  */
+  if (gdb_notifier.next_file_handler == file_ptr)
+    {
+      if (file_ptr->next_file == NULL
+	  && file_ptr == gdb_notifier.first_file_handler)
+	gdb_notifier.next_file_handler = NULL;
+      else
+	get_next_file_handler_to_handle_and_advance ();
+    }
+
   /* Get rid of the file handler in the file handler list.  */
   if (file_ptr == gdb_notifier.first_file_handler)
     gdb_notifier.first_file_handler = file_ptr->next_file;
@@ -672,7 +722,6 @@ gdb_wait_for_event (int block)
 {
   file_handler *file_ptr;
   int num_found = 0;
-  int i;
 
   /* Make sure all output is done before getting another event.  */
   gdb_flush (gdb_stdout);
@@ -743,37 +792,47 @@ gdb_wait_for_event (int block)
 	}
     }
 
+  /* Avoid looking at poll_fds[i]->revents if no event fired.  */
+  if (num_found <= 0)
+    return 0;
+
   /* 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.  */
 
+  /* To level the fairness across event descriptors, we handle them in
+     a round-robin-like fashion.  The number and order of descriptors
+     may change between invocations, but this is good enough.  */
   if (use_poll)
     {
 #ifdef HAVE_POLL
-      for (i = 0; (i < gdb_notifier.num_fds) && (num_found > 0); i++)
-	{
-	  if ((gdb_notifier.poll_fds + i)->revents)
-	    num_found--;
-	  else
-	    continue;
+      int i;
+      int mask;
 
-	  for (file_ptr = gdb_notifier.first_file_handler;
-	       file_ptr != NULL;
-	       file_ptr = file_ptr->next_file)
-	    {
-	      if (file_ptr->fd == (gdb_notifier.poll_fds + i)->fd)
-		break;
-	    }
+      while (1)
+	{
+	  if (gdb_notifier.next_poll_fds_index >= gdb_notifier.num_fds)
+	    gdb_notifier.next_poll_fds_index = 0;
+	  i = gdb_notifier.next_poll_fds_index++;
 
-	  if (file_ptr)
-	    {
-	      int mask = (gdb_notifier.poll_fds + i)->revents;
+	  gdb_assert (i < gdb_notifier.num_fds);
+	  if ((gdb_notifier.poll_fds + i)->revents)
+	    break;
+	}
 
-	      handle_file_event (file_ptr, mask);
-	      return 1;
-	    }
+      for (file_ptr = gdb_notifier.first_file_handler;
+	   file_ptr != NULL;
+	   file_ptr = file_ptr->next_file)
+	{
+	  if (file_ptr->fd == (gdb_notifier.poll_fds + i)->fd)
+	    break;
 	}
+      gdb_assert (file_ptr != NULL);
+
+      mask = (gdb_notifier.poll_fds + i)->revents;
+      handle_file_event (file_ptr, mask);
+      return 1;
 #else
       internal_error (__FILE__, __LINE__,
 		      _("use_poll without HAVE_POLL"));
@@ -781,11 +840,12 @@ gdb_wait_for_event (int block)
     }
   else
     {
-      for (file_ptr = gdb_notifier.first_file_handler;
-	   (file_ptr != NULL) && (num_found > 0);
-	   file_ptr = file_ptr->next_file)
+      /* See comment about even source fairness above.  */
+      int mask = 0;
+
+      do
 	{
-	  int mask = 0;
+	  file_ptr = get_next_file_handler_to_handle_and_advance ();
 
 	  if (FD_ISSET (file_ptr->fd, &gdb_notifier.ready_masks[0]))
 	    mask |= GDB_READABLE;
@@ -793,15 +853,11 @@ gdb_wait_for_event (int block)
 	    mask |= GDB_WRITABLE;
 	  if (FD_ISSET (file_ptr->fd, &gdb_notifier.ready_masks[2]))
 	    mask |= GDB_EXCEPTION;
-
-	  if (!mask)
-	    continue;
-	  else
-	    num_found--;
-
-	  handle_file_event (file_ptr, mask);
-	  return 1;
 	}
+      while (mask == 0);
+
+      handle_file_event (file_ptr, mask);
+      return 1;
     }
   return 0;
 }
-- 
1.9.3

Re: [PATCH] Fix gdb.mi/mi-nsmoribund.exp timeouts (Re: [PATCH] Cleanup num_found usage in gdb_wait_for_event)

[Pedro Alves]

On 05/06/2015 06:46 PM, Pedro Alves wrote:

> gdb/ChangeLog:
> 2015-05-06  Pedro Alves  <palves@redhat.com>
> 	    Simon Marchi  <simon.marchi@ericsson.com>
> 
> 	* event-loop.c (gdb_notifier) <next_file_handler,
> 	next_poll_fds_index>: New fields.
> 	(get_next_file_handler_to_handle_and_advance): New function.
> 	(delete_file_handler): If deleting the next file handler to
> 	handle, advance to the next file handler.
> 	(gdb_wait_for_event): Bail early if no event fired.  Poll file
> 	handlers in round-robin fashion.

I pushed this in now.

Thanks,
Pedro Alves