From patchwork Wed Jun 1 11:52:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 12675 Received: (qmail 89781 invoked by alias); 1 Jun 2016 11:52:48 -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 89762 invoked by uid 89); 1 Jun 2016 11:52:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=translated, restored, QUIT 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; Wed, 01 Jun 2016 11:52:37 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3E572D7A4F; Wed, 1 Jun 2016 11:52:36 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u51BqYhh005643; Wed, 1 Jun 2016 07:52:35 -0400 Subject: Re: [PATCH] Wake up interruptible_select in remote_fileio ctrl-c handler To: Yao Qi References: <1463668660-9205-1-git-send-email-yao.qi@linaro.org> <86lh2pxq6a.fsf@gmail.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <2102e16d-b52c-82da-6714-c536092dcb01@redhat.com> Date: Wed, 1 Jun 2016 12:52:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <86lh2pxq6a.fsf@gmail.com> On 06/01/2016 09:35 AM, Yao Qi wrote: > Yao Qi writes: > >> 2016-05-19 Yao Qi >> >> PR remote/19998 >> * remote-fileio.c (remote_fileio_ctrl_c_signal_handler): Call >> quit_serial_event_set. > > I pushed it in. Thanks Yao. Sorry for not noticing / forgetting this patch. When I wrote the Ctrl-C rework series, I wrote the patch below too, but in the end didn't include it in the series, because I didn't have a way to test fileio. Missed that without the patch below we'd need to wake up interrupt_select. So thanks much for the fix. Basically, my thinking for the patch was: - remote.c no longer installs a custom SIGINT handler. - The current remote-fileio.c SIGINT handler is basically the same as the default SIGINT handler (event-top.c:handle_sigint), in priciple, except that instead of setting the quit flag, it sets a separate flag. - The current code can lose Ctrl-C -- there's a period where SIG_IGN is installed as signal handler, for example. I think we should be able to completely remove the remote-fileio.c SIGINT handler, and fix these corner cases, with the patch below. WDYT? From 5fa9ddb69be0c54a1dfb60f60efa6ccd53b08120 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 22 Feb 2016 13:00:11 +0000 Subject: [PATCH] remote-fileio.c: Eliminate SIGINT signal handler ... and plug ctrl-c races. --- gdb/remote-fileio.c | 84 +++++++++++++++-------------------------------------- 1 file changed, 24 insertions(+), 60 deletions(-) diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c index 29c5ca3..b70bb54 100644 --- a/gdb/remote-fileio.c +++ b/gdb/remote-fileio.c @@ -298,66 +298,25 @@ remote_fileio_to_fio_timeval (struct timeval *tv, struct fio_timeval *ftv) remote_fileio_to_fio_long (tv->tv_usec, ftv->ftv_usec); } -static int remote_fio_ctrl_c_flag = 0; +/* The quit handler originally installed. */ +static quit_handler_ftype *remote_fileio_o_quit_handler; -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) -static struct sigaction remote_fio_sa; -static struct sigaction remote_fio_osa; -#else -static void (*remote_fio_ofunc)(int); -#endif - -static void -remote_fileio_sig_init (void) -{ -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) - remote_fio_sa.sa_handler = SIG_IGN; - sigemptyset (&remote_fio_sa.sa_mask); - remote_fio_sa.sa_flags = 0; - sigaction (SIGINT, &remote_fio_sa, &remote_fio_osa); -#else - remote_fio_ofunc = signal (SIGINT, SIG_IGN); -#endif -} - -static void -remote_fileio_sig_set (void (*sigint_func)(int)) -{ -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) - remote_fio_sa.sa_handler = sigint_func; - sigemptyset (&remote_fio_sa.sa_mask); - remote_fio_sa.sa_flags = 0; - sigaction (SIGINT, &remote_fio_sa, NULL); -#else - signal (SIGINT, sigint_func); -#endif -} +/* What to do on a QUIT call while handling a file I/O request. We + throw a quit exception, which is caught by remote_fileio_request + and translated to an EINTR reply back to the target. */ static void -remote_fileio_sig_exit (void) +remote_fileio_quit_handler (void) { -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) - sigaction (SIGINT, &remote_fio_osa, NULL); -#else - signal (SIGINT, remote_fio_ofunc); -#endif -} - -static void -remote_fileio_ctrl_c_signal_handler (int signo) -{ - remote_fileio_sig_set (remote_fileio_ctrl_c_signal_handler); - remote_fio_ctrl_c_flag = 1; - /* Wake up interruptible_select. */ - quit_serial_event_set (); + quit (); } static void remote_fileio_reply (int retcode, int error) { char buf[32]; + int ctrl_c = check_quit_flag (); - remote_fileio_sig_set (SIG_IGN); strcpy (buf, "F"); if (retcode < 0) { @@ -365,9 +324,9 @@ remote_fileio_reply (int retcode, int error) retcode = -retcode; } sprintf (buf + strlen (buf), "%x", retcode); - if (error || remote_fio_ctrl_c_flag) + if (error || ctrl_c) { - if (error && remote_fio_ctrl_c_flag) + if (error && ctrl_c) error = FILEIO_EINTR; if (error < 0) { @@ -375,11 +334,10 @@ remote_fileio_reply (int retcode, int error) error = -error; } sprintf (buf + strlen (buf), ",%x", error); - if (remote_fio_ctrl_c_flag) + if (ctrl_c) strcat (buf, ",C"); } - remote_fio_ctrl_c_flag = 0; - remote_fileio_sig_set (remote_fileio_ctrl_c_signal_handler); + quit_handler = remote_fileio_o_quit_handler; putpkt (buf); } @@ -1166,7 +1124,7 @@ do_remote_fileio_request (struct ui_out *uiout, void *buf_arg) char *c; int idx; - remote_fileio_sig_set (remote_fileio_ctrl_c_signal_handler); + quit_handler = remote_fileio_quit_handler; c = strchr (++buf, ','); if (c) @@ -1213,20 +1171,22 @@ remote_fileio_request (char *buf, int ctrlc_pending_p) { int ex; - remote_fileio_sig_init (); + /* Save the previous quit handler, so we can restore it. No need + for a cleanup since we catch all exceptions below. Note that the + quit handler is also restored by remote_fileio_reply just before + pushing a packet. */ + remote_fileio_o_quit_handler = quit_handler; if (ctrlc_pending_p) { /* If the target hasn't responded to the Ctrl-C sent asynchronously earlier, take this opportunity to send the Ctrl-C synchronously. */ - remote_fio_ctrl_c_flag = 1; + set_quit_flag (); remote_fileio_reply (-1, FILEIO_EINTR); } else { - remote_fio_ctrl_c_flag = 0; - ex = catch_exceptions (current_uiout, do_remote_fileio_request, (void *)buf, RETURN_MASK_ALL); @@ -1243,7 +1203,11 @@ remote_fileio_request (char *buf, int ctrlc_pending_p) } } - remote_fileio_sig_exit (); + quit_handler = remote_fileio_o_quit_handler; + + /* If the user hit C-c before, pretend that it was hit right + here. */ + QUIT; }