[RFA] Don't immediately SIGTERM the child of "target remote |".

[Doug Evans <dje@google.com>]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

On Sun, Nov 27, 2011 at 2:23 PM, Doug Evans <dje@google.com> wrote:
> On Sun, Nov 27, 2011 at 12:40 PM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
>> On Thu, 24 Nov 2011 01:00:51 +0100, Doug Evans wrote:
>> [...]
>>> -      int status;
>>> -      kill (state->pid, SIGTERM);
>>> +      int rc, status;
>>> +      void (*ofunc) ();      /* Previous SIGALRM handler.  */
>>                        ^int (or void)
>>> +
>>> +      /* Don't kill the task right away, give it a chance to shut down cleanly.
>>> +      But don't wait forever though.  */
>>> +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
>>> +      {
>>> +     struct sigaction sa, osa;
>>> +     sa.sa_handler = sigalrm_handler;
>>> +     sigemptyset (&sa.sa_mask);
>>> +     sa.sa_flags = 0;
>>> +     sigaction (SIGALRM, &sa, &osa);
>>> +     ofunc = osa.sa_handler;
>>> +      }
>>> +#else
>>> +      ofunc = (void (*)()) signal (SIGALRM, sigalrm_handler);
>>> +#endif
>> [...]
>>> +      alarm (0);
>>> +      signal (SIGALRM, ofunc);
>>
>> You should restore OSA, not just OFUNC.
>
> The code was borrowed from remote-sim.c.
> In this case I suspect it doesn't matter, but it's just as well to restore osa.
>

I will check this in on Monday if there are no objections.

2011-12-01  Doug Evans  <dje@google.com>

        * defs.h (wait_to_die_with_timeout): Declare.
        * utils.c: #include "gdb_wait.h".
        (sigalrm_handler, wait_to_die_with_timeout): New functions.
        * ser-pipe.c: Don't #include "gdb_wait.h".
        (pipe_close): Give child a chance to die on its own after closing
        its stdin before SIGTERM'ing it.

[-- Attachment #2: gdb-111201-ser-pipe-sigterm-2.patch.txt --]
[-- Type: text/plain, Size: 4446 bytes --]

2011-12-01  Doug Evans  <dje@google.com>

	* defs.h (wait_to_die_with_timeout): Declare.
	* utils.c: #include "gdb_wait.h".
	(sigalrm_handler, wait_to_die_with_timeout): New functions.
	* ser-pipe.c: Don't #include "gdb_wait.h".
	(pipe_close): Give child a chance to die on its own after closing
	its stdin before SIGTERM'ing it.

Index: defs.h
===================================================================
RCS file: /cvs/src/src/gdb/defs.h,v
retrieving revision 1.305
diff -u -p -r1.305 defs.h
--- defs.h	10 Nov 2011 20:21:27 -0000	1.305
+++ defs.h	1 Dec 2011 19:41:30 -0000
@@ -439,6 +439,10 @@ extern struct cleanup *make_bpstat_clear
 
 extern int producer_is_gcc_ge_4 (const char *producer);
 
+#ifdef HAVE_WAITPID
+extern int wait_to_die_with_timeout (int pid, int *status, int timeout);
+#endif
+
 \f
 /* Annotation stuff.  */
 
Index: ser-pipe.c
===================================================================
RCS file: /cvs/src/src/gdb/ser-pipe.c,v
retrieving revision 1.32
diff -u -p -r1.32 ser-pipe.c
--- ser-pipe.c	4 Mar 2011 19:23:42 -0000	1.32
+++ ser-pipe.c	1 Dec 2011 19:41:30 -0000
@@ -31,7 +31,6 @@
 #include <sys/time.h>
 #include <fcntl.h>
 #include "gdb_string.h"
-#include "gdb_wait.h"
 
 #include <signal.h>
 
@@ -163,14 +162,30 @@ pipe_close (struct serial *scb)
 
   if (state != NULL)
     {
-      int status;
-      kill (state->pid, SIGTERM);
-#ifdef HAVE_WAITPID
+      int wait_result, status;
+
+      /* Don't kill the task right away, give it a chance to shut down cleanly.
+	 But don't wait forever though.  */
+#define PIPE_CLOSE_TIMEOUT 5
+
       /* Assume the program will exit after SIGTERM.  Might be
 	 useful to print any remaining stderr output from
 	 scb->error_fd while waiting.  */
-      waitpid (state->pid, &status, 0);
+#define SIGTERM_TIMEOUT INT_MAX
+
+      wait_result = -1;
+#ifdef HAVE_WAITPID
+      wait_result = wait_to_die_with_timeout (state->pid, &status,
+					      PIPE_CLOSE_TIMEOUT);
 #endif
+      if (wait_result == -1)
+	{
+	  kill (state->pid, SIGTERM);
+#ifdef HAVE_WAITPID
+	  wait_to_die_with_timeout (state->pid, &status, SIGTERM_TIMEOUT);
+#endif
+	}
+
       if (scb->error_fd != -1)
 	close (scb->error_fd);
       scb->error_fd = -1;
Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.267
diff -u -p -r1.267 utils.c
--- utils.c	16 Nov 2011 18:14:52 -0000	1.267
+++ utils.c	1 Dec 2011 19:41:30 -0000
@@ -24,6 +24,7 @@
 #include "gdb_assert.h"
 #include <ctype.h>
 #include "gdb_string.h"
+#include "gdb_wait.h"
 #include "event-top.h"
 #include "exceptions.h"
 #include "gdbthread.h"
@@ -3773,6 +3774,78 @@ producer_is_gcc_ge_4 (const char *produc
   return minor;
 }
 
+#ifdef HAVE_WAITPID
+
+#ifdef SIGALRM
+
+/* SIGALRM handler for waitpid_with_timeout.  */
+
+static void
+sigalrm_handler (int signo)
+{
+  /* Nothing to do.  */
+}
+
+#endif
+
+/* Wrapper to wait for child PID to die with TIMEOUT.
+   TIMEOUT is the time to stop waiting in seconds.
+   If TIMEOUT is zero, pass WNOHANG to waitpid.
+   Returns PID if it was successfully waited for, otherwise -1.
+
+   Timeouts are currently implemented with alarm and SIGALRM.
+   If the host does not support them, this waits "forever".
+   It would be odd though for a host to have waitpid and not SIGALRM.  */
+
+int
+wait_to_die_with_timeout (int pid, int *status, int timeout)
+{
+  int waitpid_result;
+
+  gdb_assert (pid > 0);
+  gdb_assert (timeout >= 0);
+
+  if (timeout > 0)
+    {
+#ifdef SIGALRM
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+      struct sigaction sa, old_sa;
+
+      sa.sa_handler = sigalrm_handler;
+      sigemptyset (&sa.sa_mask);
+      sa.sa_flags = 0;
+      sigaction (SIGALRM, &sa, &old_sa);
+#else
+      void (*ofunc) ();
+
+      ofunc = (void (*)()) signal (SIGALRM, sigalrm_handler);
+#endif
+
+      alarm (timeout);
+#endif
+
+      waitpid_result = waitpid (pid, status, 0);
+
+#ifdef SIGALRM
+      alarm (0);
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+      sigaction (SIGALRM, &old_sa, NULL);
+#else
+      signal (SIGALRM, ofunc);
+#endif
+#endif
+    }
+  else
+    waitpid_result = waitpid (pid, status, WNOHANG);
+
+  if (waitpid_result == pid)
+    return pid;
+  else
+    return -1;
+}
+
+#endif /* HAVE_WAITPID */
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_utils;