Re: [PATCH] Wake up interruptible_select in remote_fileio ctrl-c handler

[Pedro Alves <palves at redhat dot com>]

On 06/01/2016 02:42 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> 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 <palves@redhat.com>
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  <palves@redhat.com>

	* 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