[09/30] Introduce interruptible_select

Message ID 1458328714-4938-10-git-send-email-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves March 18, 2016, 7:18 p.m. UTC
  We have places where we call a blocking gdb_select expecting that a
Ctrl-C will unblock it.  However, if the Ctrl-C is pressed just before
gdb_select, the SIGINT handler runs before gdb_select, and thus
gdb_select won't return.

For example gdb_readline_no_editing:

       QUIT;

       /* Wait until at least one byte of data is available.  Control-C
          can interrupt gdb_select, but not fgetc.  */
       FD_ZERO (&readfds);
       FD_SET (fd, &readfds);
       if (gdb_select (fd + 1, &readfds, NULL, NULL, NULL) == -1)

and stdio_file_read:

     /* For the benefit of Windows, call gdb_select before reading from
	the file.  Wait until at least one byte of data is available.
	Control-C can interrupt gdb_select, but not read.  */
     {
       fd_set readfds;
       FD_ZERO (&readfds);
       FD_SET (stdio->fd, &readfds);
       if (gdb_select (stdio->fd + 1, &readfds, NULL, NULL, NULL) == -1)
	 return -1;
     }
     return read (stdio->fd, buf, length_buf);


This is a race classically fixed with either the self-pipe trick, or
by blocking SIGINT and then using pselect instead of select.

Blocking SIGINT most of the time would mean that check_quit_flag (and
thus QUIT) would need to do a syscall every time it is called, which
sounds best avoided, since QUIT is called in many loops.  Thus we take
the self-pipe trick route (wrapped in a serial event).

Instead of having all places that need this manually add an extra file
descriptor to the set of gdb_select's watched file descriptors, we
introduce a wrapper, interruptible_select, that does that.

The Windows version of gdb_select actually does not suffer from this,
because mingw-hdep.c:gdb_call_async_signal_handler sets a Windows
event that gdb_select always waits on.  So this patch can be seen as
generalization of that technique.  We can't remove that extra event
from mingw-hdep.c until we get rid of immediate_quit though.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* defs.h: Extend QUIT-related comments to mention
	interruptible_select.
	(quit_serial_event_set, quit_serial_event_clear): Declare.
	* event-top.c: Include "ser-event.h" and "gdb_select.h".
	(quit_serial_event): New global.
	(async_init_signals): Make quit_serial_event.
	(quit_serial_event_set, quit_serial_event_clear)
	(quit_serial_event_fd, interruptible_select): New functions.
	* extension.c (set_quit_flag): Set the quit serial event.
	(check_quit_flag): Clear the quit serial event.
	* gdb_select.h (interruptible_select): New declaration.
	* guile/scm-ports.c (ioscm_input_waiting): Use
	interruptible_select instead of gdb_select.
	* top.c (gdb_readline_no_editing): Likewise.
	* ui-file.c (stdio_file_read): Likewise.
---
 gdb/defs.h            | 11 ++++++++
 gdb/event-top.c       | 75 +++++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/extension.c       | 15 ++++++++++-
 gdb/gdb_select.h      | 15 +++++++++++
 gdb/guile/scm-ports.c |  4 ++-
 gdb/top.c             |  4 +--
 gdb/ui-file.c         |  8 +++---
 7 files changed, 122 insertions(+), 10 deletions(-)
  

Comments

Simon Marchi March 21, 2016, 5:59 p.m. UTC | #1
On 16-03-18 03:18 PM, Pedro Alves wrote:
> @@ -749,6 +757,8 @@ async_init_signals (void)
>  {
>    initialize_async_signal_handlers ();
>  
> +  quit_serial_event = make_serial_event ();
> +

Just above these line is this comment:

/* NOTE: 1999-04-30 This is the asynchronous version of init_signals.
   init_signals will become obsolete as we move to have to event loop
   as the default for gdb.  */


Is is still relevant?
  
Pedro Alves March 21, 2016, 6:33 p.m. UTC | #2
On 03/21/2016 05:59 PM, Simon Marchi wrote:
> On 16-03-18 03:18 PM, Pedro Alves wrote:
>> @@ -749,6 +757,8 @@ async_init_signals (void)
>>   {
>>     initialize_async_signal_handlers ();
>>   
>> +  quit_serial_event = make_serial_event ();
>> +
> 
> Just above these line is this comment:
> 
> /* NOTE: 1999-04-30 This is the asynchronous version of init_signals.
>     init_signals will become obsolete as we move to have to event loop
>     as the default for gdb.  */
> 
> 
> Is is still relevant?

That comment is stale since (I think):

2004-09-13  Andrew Cagney  <cagney@gnu.org>
...
        Eliminate event_loop_p, always has the value 1.
        * defs.h (event_loop_p): Delete macro.
        * breakpoint.c (until_break_command): Simplify.
        * utils.c (prompt_for_continue): Simplify.
        * tracepoint.c (read_actions): Simplify.
        * top.c (throw_exception, execute_command, gdb_readline_wrapper) 
        (gdb_rl_operate_and_get_next, command_line_input, get_prompt) 
        (set_prompt, init_main): Simplify.
        (init_signals, disconnect): Delete, unused.

That is, init_signals is long gone, and we always have an event loop
running.

Thanks,
Pedro Alves
  
Simon Marchi March 21, 2016, 7:48 p.m. UTC | #3
On 16-03-18 03:18 PM, Pedro Alves wrote:
> +int
> +interruptible_select (int n,
> +		      fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> +		      struct timeval *timeout)
> +{
> +  fd_set my_readfds;
> +  int fd;

Just a nit.  For clarity, you could name this variable "quit_fd" instead of "fd".
  
Pedro Alves March 21, 2016, 7:49 p.m. UTC | #4
On 03/21/2016 07:48 PM, Simon Marchi wrote:
> Just a nit.  For clarity, you could name this variable "quit_fd" instead of "fd".

I'll do that.  Thanks!

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/defs.h b/gdb/defs.h
index b94df30..ad9b259 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -131,6 +131,11 @@  extern char *debug_file_directory;
    take a long time, and which ought to be interruptible, checks this
    flag using the QUIT macro.
 
+   In addition to setting a flag, the SIGINT handler also marks a
+   select/poll-able file descriptor as read-ready.  That is used by
+   interruptible_select in order to support interrupting blocking I/O
+   in a race-free manner.
+
    These functions use the extension_language_ops API to allow extension
    language(s) and GDB SIGINT handling to coexist seamlessly.  */
 
@@ -159,6 +164,12 @@  extern void maybe_quit (void);
    connection.  */
 #define QUIT maybe_quit ()
 
+/* Set the serial event associated with the quit flag.  */
+extern void quit_serial_event_set (void);
+
+/* Clear the serial event associated with the quit flag.  */
+extern void quit_serial_event_clear (void);
+
 /* * Languages represented in the symbol table and elsewhere.
    This should probably be in language.h, but since enum's can't
    be forward declared to satisfy opaque references before their
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 9fff2be..da72b1d 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -38,6 +38,8 @@ 
 #include "annotate.h"
 #include "maint.h"
 #include "buffer.h"
+#include "ser-event.h"
+#include "gdb_select.h"
 
 /* readline include files.  */
 #include "readline/readline.h"
@@ -732,6 +734,12 @@  gdb_readline_no_editing_callback (gdb_client_data client_data)
 }
 
 
+/* The serial event associated with the QUIT flag.  set_quit_flag sets
+   this, and check_quit_flag clears it.  Used by interruptible_select
+   to be able to do interruptible I/O with no race with the SIGINT
+   handler.  */
+static struct serial_event *quit_serial_event;
+
 /* Initialization of signal handlers and tokens.  There is a function
    handle_sig* for each of the signals GDB cares about.  Specifically:
    SIGINT, SIGFPE, SIGQUIT, SIGTSTP, SIGHUP, SIGWINCH.  These
@@ -749,6 +757,8 @@  async_init_signals (void)
 {
   initialize_async_signal_handlers ();
 
+  quit_serial_event = make_serial_event ();
+
   signal (SIGINT, handle_sigint);
   sigint_token =
     create_async_signal_handler (async_request_quit, NULL);
@@ -793,8 +803,33 @@  async_init_signals (void)
 #endif
 }
 
-/* Tell the event loop what to do if SIGINT is received.
-   See event-signal.c.  */
+/* See defs.h.  */
+
+void
+quit_serial_event_set (void)
+{
+  serial_event_set (quit_serial_event);
+}
+
+/* See defs.h.  */
+
+void
+quit_serial_event_clear (void)
+{
+  serial_event_clear (quit_serial_event);
+}
+
+/* Return the selectable file descriptor of the serial event
+   associated with the quit flag.  */
+
+static int
+quit_serial_event_fd (void)
+{
+  return serial_event_fd (quit_serial_event);
+}
+
+/* Handle a SIGINT.  */
+
 void
 handle_sigint (int sig)
 {
@@ -818,6 +853,42 @@  handle_sigint (int sig)
   gdb_call_async_signal_handler (sigint_token, immediate_quit);
 }
 
+/* See gdb_select.h.  */
+
+int
+interruptible_select (int n,
+		      fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
+		      struct timeval *timeout)
+{
+  fd_set my_readfds;
+  int fd;
+  int res;
+
+  if (readfds == NULL)
+    {
+      readfds = &my_readfds;
+      FD_ZERO (&my_readfds);
+    }
+
+  fd = quit_serial_event_fd ();
+  FD_SET (fd, readfds);
+  if (n <= fd)
+    n = fd + 1;
+
+  do
+    {
+      res = gdb_select (n, readfds, writefds, exceptfds, timeout);
+    }
+  while (res == -1 && errno == EINTR);
+
+  if (res == 1 && FD_ISSET (fd, readfds))
+    {
+      errno = EINTR;
+      return -1;
+    }
+  return res;
+}
+
 /* Handle GDB exit upon receiving SIGTERM if target_can_async_p ().  */
 
 static void
diff --git a/gdb/extension.c b/gdb/extension.c
index d2c5669..c00db47 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -828,7 +828,16 @@  set_quit_flag (void)
       && active_ext_lang->ops->set_quit_flag != NULL)
     active_ext_lang->ops->set_quit_flag (active_ext_lang);
   else
-    quit_flag = 1;
+    {
+      quit_flag = 1;
+
+      /* Now wake up the event loop, or any interruptible_select.  Do
+	 this after setting the flag, because signals on Windows
+	 actually run on a separate thread, and thus otherwise the
+	 main code could be woken up and find quit_flag still
+	 clear.  */
+      quit_serial_event_set ();
+    }
 }
 
 /* Return true if the quit flag has been set, false otherwise.
@@ -852,6 +861,10 @@  check_quit_flag (void)
   /* This is written in a particular way to avoid races.  */
   if (quit_flag)
     {
+      /* No longer need to wake up the event loop or any
+	 interruptible_select.  The caller handles the quit
+	 request.  */
+      quit_serial_event_clear ();
       quit_flag = 0;
       result = 1;
     }
diff --git a/gdb/gdb_select.h b/gdb/gdb_select.h
index e00a332..d346c60 100644
--- a/gdb/gdb_select.h
+++ b/gdb/gdb_select.h
@@ -33,4 +33,19 @@ 
 extern int gdb_select (int n, fd_set *readfds, fd_set *writefds,
 		       fd_set *exceptfds, struct timeval *timeout);
 
+/* Convenience wrapper around gdb_select that returns -1/EINTR if
+   set_quit_flag is set, either on entry or from a signal handler or
+   from a different thread while select is blocked.  The quit flag is
+   not cleared on exit -- the caller is responsible to check it with
+   check_quit_flag or QUIT.
+
+   Note this does NOT return -1/EINTR if any signal handler other than
+   SIGINT runs, nor if the current SIGINT handler does not call
+   set_quit_flag.  */
+extern int interruptible_select (int n,
+				 fd_set *readfds,
+				 fd_set *writefds,
+				 fd_set *exceptfds,
+				 struct timeval *timeout);
+
 #endif /* !defined(GDB_SELECT_H) */
diff --git a/gdb/guile/scm-ports.c b/gdb/guile/scm-ports.c
index a7d61af..b0f576e 100644
--- a/gdb/guile/scm-ports.c
+++ b/gdb/guile/scm-ports.c
@@ -201,7 +201,9 @@  ioscm_input_waiting (SCM port)
     FD_ZERO (&input_fds);
     FD_SET (fdes, &input_fds);
 
-    num_found = gdb_select (num_fds, &input_fds, NULL, NULL, &timeout);
+    num_found = interruptible_select (num_fds,
+				      &input_fds, NULL, NULL,
+				      &timeout);
     if (num_found < 0)
       {
 	/* Guile doesn't export SIGINT hooks like Python does.
diff --git a/gdb/top.c b/gdb/top.c
index 41ff6b2..f5ef718 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -613,10 +613,10 @@  gdb_readline_no_editing (const char *prompt)
       QUIT;
 
       /* Wait until at least one byte of data is available.  Control-C
-	 can interrupt gdb_select, but not fgetc.  */
+	 can interrupt interruptible_select, but not fgetc.  */
       FD_ZERO (&readfds);
       FD_SET (fd, &readfds);
-      if (gdb_select (fd + 1, &readfds, NULL, NULL, NULL) == -1)
+      if (interruptible_select (fd + 1, &readfds, NULL, NULL, NULL) == -1)
 	{
 	  if (errno == EINTR)
 	    {
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index c86994d..4260710 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -567,14 +567,14 @@  stdio_file_read (struct ui_file *file, char *buf, long length_buf)
     internal_error (__FILE__, __LINE__,
 		    _("stdio_file_read: bad magic number"));
 
-  /* For the benefit of Windows, call gdb_select before reading from
-     the file.  Wait until at least one byte of data is available.
-     Control-C can interrupt gdb_select, but not read.  */
+  /* Wait until at least one byte of data is available, or we get
+     interrupted with Control-C.  */
   {
     fd_set readfds;
+
     FD_ZERO (&readfds);
     FD_SET (stdio->fd, &readfds);
-    if (gdb_select (stdio->fd + 1, &readfds, NULL, NULL, NULL) == -1)
+    if (interruptible_select (stdio->fd + 1, &readfds, NULL, NULL, NULL) == -1)
       return -1;
   }