From 4e63d0ac896b6036edd8e2c09a4dac7aa16a46d1 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 15 May 2015 16:26:53 +0100 Subject: 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-15 Pedro Alves Simon Marchi * event-loop.c (gdb_notifier) : 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/ChangeLog | 11 +++++ gdb/event-loop.c | 120 ++++++++++++++++++++++++++++++++++++++++--------------- 2 files changed, 99 insertions(+), 32 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 05c0b3f0851..b7694c769c0 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,14 @@ +2015-05-15 Pedro Alves + Simon Marchi + + * event-loop.c (gdb_notifier) : 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. + 2015-05-15 Pedro Alves * linux-tdep.c (linux_find_memory_regions_full): Rename local diff --git a/gdb/event-loop.c b/gdb/event-loop.c index 79e41fd549e..9ac49086f0c 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; } -- cgit v1.2.1