From patchwork Fri Mar 18 19:18:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 11413 Received: (qmail 118716 invoked by alias); 18 Mar 2016 19:37:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 118626 invoked by uid 89); 18 Mar 2016 19:37:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Languages, 2019, 2017, 388 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 18 Mar 2016 19:37:50 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 956D564466 for ; Fri, 18 Mar 2016 19:18:43 +0000 (UTC) Received: from cascais.lan (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2IJIYk7028091 for ; Fri, 18 Mar 2016 15:18:42 -0400 From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 09/30] Introduce interruptible_select Date: Fri, 18 Mar 2016 19:18:13 +0000 Message-Id: <1458328714-4938-10-git-send-email-palves@redhat.com> In-Reply-To: <1458328714-4938-1-git-send-email-palves@redhat.com> References: <1458328714-4938-1-git-send-email-palves@redhat.com> 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 * 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(-) 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; }