From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55123 invoked by alias); 1 Jun 2016 14:15:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 55084 invoked by uid 89); 1 Jun 2016 14:15:30 -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=served, restoring, H*M:ab03 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 14:15:29 +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 D280A85540; Wed, 1 Jun 2016 14:15:27 +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 u51EFQ61028366; Wed, 1 Jun 2016 10:15:27 -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> <2102e16d-b52c-82da-6714-c536092dcb01@redhat.com> <864m9dxbxt.fsf@gmail.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <85362c56-ab03-1ea1-3171-a7d75bededc8@redhat.com> Date: Wed, 01 Jun 2016 14:15:00 -0000 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: <864m9dxbxt.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-06/txt/msg00019.txt.bz2 On 06/01/2016 02:42 PM, Yao Qi wrote: > Pedro Alves writes: > Yes, that is completely the right way to go. > >> + >> + /* If the user hit C-c before, pretend that it was hit right >> + here. */ >> + QUIT; > > Why do we need this? I think I was thinking of the user pressing C-c between remote_fileio_reply and restoring the quit handler. The C-c is only served the next time QUIT is called. Thinking we don't know when that might be, calling QUIT here would take care of it right on the spot. However, in this case, since we're getting back to the event loop, the event loop will take care of it quite soon through async_request_quit, so it's not really necessary. Here's the patch without that, and with proper commit log / ChangeLog. >From a458561ef6e4e51dd16a42b1ee3013b5a36ae4dd Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 1 Jun 2016 15:13:05 +0100 Subject: [PATCH] gdb/remote-fileio.c: Eliminate custom SIGINT signal handler ... and fix Ctrl-C races. The current remote-fileio.c SIGINT/EINTR code can lose Ctrl-C -- there's a period where SIG_IGN is installed as signal handler, for example. Since: - 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 principle, except that instead of setting the quit flag, it sets a separate flag. I think we should be able to completely remove the remote-fileio.c SIGINT handler, and centralize on the quit flag, thus fixing the Ctrl-C race. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * remote-fileio.c (remote_fio_ctrl_c_flag, remote_fio_sa) (remote_fio_osa) (remote_fio_ofunc, remote_fileio_sig_init, remote_fileio_sig_set) (remote_fileio_sig_exit, remote_fileio_ctrl_c_signal_handler): Delete. (remote_fileio_o_quit_handler): New global. (remote_fileio_quit_handler): New function. (remote_fileio_reply): Check the quit flag instead of the custom 'remote_fio_ctrl_c_flag' flag. Restore the quit handler instead of changing the SIGINT handler. (do_remote_fileio_request): Override the quit handler instead of changing the SIGINT handler. --- gdb/remote-fileio.c | 80 ++++++++++++++--------------------------------------- 1 file changed, 20 insertions(+), 60 deletions(-) diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c index 29c5ca3..93121aa 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 +/* 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_init (void) +remote_fileio_quit_handler (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 -} - -static void -remote_fileio_sig_exit (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,7 @@ remote_fileio_request (char *buf, int ctrlc_pending_p) } } - remote_fileio_sig_exit (); + quit_handler = remote_fileio_o_quit_handler; } -- 2.5.5