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

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

[Yao Qi]

As reported in PR 19998, after type ctrl-c, GDB hang there and does
not send interrupt.  It causes a fail in gdb.base/interrupt.exp.
All targets support remote fileio should be affected.

When we type ctrc-c, SIGINT is handled by remote_fileio_sig_set,
as shown below,

 #0  remote_fileio_sig_set (sigint_func=0x4495d0 <remote_fileio_ctrl_c_signal_handler(int)>) at /home/yao/SourceCode/gnu/gdb/git/gdb/remote-fileio.c:325
 #1  0x00000000004495de in remote_fileio_ctrl_c_signal_handler (signo=<optimised out>) at /home/yao/SourceCode/gnu/gdb/git/gdb/remote-fileio.c:349
 #2  <signal handler called>
 #3  0x00007ffff647ed83 in __select_nocancel () at ../sysdeps/unix/syscall-template.S:81
 #4  0x00000000005530ce in interruptible_select (n=10, readfds=readfds@entry=0x7fffffffd730, writefds=writefds@entry=0x0, exceptfds=exceptfds@entry=0x0,
    timeout=timeout@entry=0x0) at /home/yao/SourceCode/gnu/gdb/git/gdb/event-top.c:1017
 #5  0x000000000061ab20 in stdio_file_read (file=<optimised out>, buf=0x12d02e0 "\n\022-\001", length_buf=16383)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/ui-file.c:577
 #6  0x000000000044a4dc in remote_fileio_func_read (buf=0x12c0360 "") at /home/yao/SourceCode/gnu/gdb/git/gdb/remote-fileio.c:583
 #7  0x0000000000449598 in do_remote_fileio_request (uiout=<optimised out>, buf_arg=buf_arg@entry=0x12c0340)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/remote-fileio.c:1179

we don't set quit_serial_event,

  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;

we can't go out of the loop above, and that is why GDB can't send
interrupt.

Recently, we stop throwing exception from SIGINT handler
(remote_fileio_ctrl_c_signal_handler)
https://sourceware.org/ml/gdb-patches/2016-03/msg00372.html, which
is correct, because gdb_select is interruptible.  However, in the
same patch series, we add interruptible_select later as a wrapper
to gdb_select, https://sourceware.org/ml/gdb-patches/2016-03/msg00375.html
and it is not interruptible (because of the loop in it) unless
select/poll-able file descriptors are marked.

This fix in this patch is to call quit_serial_event_set, so that we can
go out of the loop above, return -1 and set errno to EINTR.

I don't have the test env to run testsuite against targets support
remote fileio.

2016-05-19  Yao Qi  <yao.qi@linaro.org>

	PR remote/19998
	* remote-fileio.c (remote_fileio_ctrl_c_signal_handler): Call
	quit_serial_event_set.
---
 gdb/remote-fileio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 44817df..29c5ca3 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -348,6 +348,8 @@ 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 ();
 }
 
 static void
-- 
1.9.1

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

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

> 2016-05-19  Yao Qi  <yao.qi@linaro.org>
>
> 	PR remote/19998
> 	* remote-fileio.c (remote_fileio_ctrl_c_signal_handler): Call
> 	quit_serial_event_set.

I pushed it in.

-- 
Yao (齐尧)

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

[Pedro Alves]

On 06/01/2016 09:35 AM, Yao Qi wrote:
> Yao Qi <qiyaoltc@gmail.com> writes:
> 
>> 2016-05-19  Yao Qi  <yao.qi@linaro.org>
>>
>> 	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 <palves@redhat.com>
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;
 }
 \f
 
-- 
2.5.5

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

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> 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.
>

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?

-- 
Yao (齐尧)

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

[Pedro Alves]

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;
 }
 \f
 
-- 
2.5.5

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

[Pedro Alves]

On 06/01/2016 03:34 PM, Yao Qi wrote:

> That looks good to me.

Pushed.

Thanks,
Pedro Alves