[Emacs-diffs] /srv/bzr/emacs/trunk r110922: Fix bug #12829 with aborts on MS-Windows when several child processes die.

[Eli Zaretskii]

------------------------------------------------------------
revno: 110922
fixes bug: http://debbugs.gnu.org/12829
committer: Eli Zaretskii <address@hidden>
branch nick: trunk
timestamp: Sat 2012-11-17 18:46:45 +0200
message:
  Fix bug #12829 with aborts on MS-Windows when several child processes die.
  
   nt/inc/sys/wait.h: New file, with prototype of waitpid and
   definitions of macros it needs.
   nt/inc/ms-w32.h (wait): Don't define, 'wait' is not used anymore.
   (sys_wait): Remove prototype.
   nt/config.nt (HAVE_SYS_WAIT_H): Define to 1.
  
   src/w32proc.c (create_child): Don't clip the PID of the child
   process to fit into an Emacs integer, as this is no longer a
   restriction.
   (waitpid): Rename from sys_wait.  Emulate a Posix 'waitpid' by
   reaping only the process specified by PID argument, if that is
   positive.  Use PID instead of dead_child to know which process to
   reap.  Wait for the child to die only if WNOHANG is not in
   OPTIONS.
   (sys_select): Don't set dead_child.
   src/sysdep.c (wait_for_termination_1): Remove the WINDOWSNT portion,
   as it is no longer needed.
   src/process.c (waitpid, WUNTRACED) [!WNOHANG]: Remove definitions,
   no longer needed.
   (record_child_status_change): Remove the setting of
   record_at_most_one_child for the !WNOHANG case.
added:
  nt/inc/sys/wait.h
modified:
  nt/ChangeLog
  nt/config.nt
  nt/inc/ms-w32.h
  src/ChangeLog
  src/process.c
  src/sysdep.c
  src/w32proc.c

=== modified file 'nt/ChangeLog'
--- a/nt/ChangeLog      2012-11-17 08:55:07 +0000
+++ b/nt/ChangeLog      2012-11-17 16:46:45 +0000
@@ -1,3 +1,13 @@
+2012-11-17  Eli Zaretskii  <address@hidden>
+
+       * inc/sys/wait.h: New file, with prototype of waitpid and
+       definitions of macros it needs.
+
+       * inc/ms-w32.h (wait): Don't define, 'wait' is not used anymore.
+       (sys_wait): Remove prototype.
+
+       * config.nt (HAVE_SYS_WAIT_H): Define to 1.
+
 2012-11-17  Dani Moncayo  <address@hidden>
 
        * zipdist.bat (ZIP_CHECK): Remove unused label.  When invoking 7z

=== modified file 'nt/config.nt'
--- a/nt/config.nt      2012-11-15 14:47:31 +0000
+++ b/nt/config.nt      2012-11-17 16:46:45 +0000
@@ -986,7 +986,7 @@
 #undef HAVE_SYS_VLIMIT_H
 
 /* Define to 1 if you have <sys/wait.h> that is POSIX.1 compatible. */
-#undef HAVE_SYS_WAIT_H
+#define HAVE_SYS_WAIT_H 1
 
 /* Define to 1 if you have the <term.h> header file. */
 #undef HAVE_TERM_H

=== modified file 'nt/inc/ms-w32.h'
--- a/nt/inc/ms-w32.h   2012-11-14 17:22:55 +0000
+++ b/nt/inc/ms-w32.h   2012-11-17 16:46:45 +0000
@@ -183,15 +183,12 @@
 
 /* Subprocess calls that are emulated.  */
 #define spawnve sys_spawnve
-#define wait    sys_wait
 #define kill    sys_kill
 #define signal  sys_signal
 
 /* Internal signals.  */
 #define emacs_raise(sig) emacs_abort()
 
-extern int sys_wait (int *);
-
 /* termcap.c calls that are emulated.  */
 #define tputs   sys_tputs
 #define tgetstr sys_tgetstr

=== added file 'nt/inc/sys/wait.h'
--- a/nt/inc/sys/wait.h 1970-01-01 00:00:00 +0000
+++ b/nt/inc/sys/wait.h 2012-11-17 16:46:45 +0000
@@ -0,0 +1,33 @@
+/* A limited emulation of sys/wait.h on Posix systems.
+
+Copyright (C) 2012  Free Software Foundation, Inc.
+
+This file is part of GNU Emacs.
+
+GNU Emacs is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+GNU Emacs is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef INC_SYS_WAIT_H_
+#define INC_SYS_WAIT_H_
+
+#define WNOHANG    1
+#define WUNTRACED  2
+#define WSTOPPED   2   /* same as WUNTRACED */
+#define WEXITED    4
+#define WCONTINUED 8
+
+/* The various WIF* macros are defined in src/syswait.h.  */
+
+extern pid_t waitpid (pid_t, int *, int);
+
+#endif /* INC_SYS_WAIT_H_ */

=== modified file 'src/ChangeLog'
--- a/src/ChangeLog     2012-11-17 15:15:49 +0000
+++ b/src/ChangeLog     2012-11-17 16:46:45 +0000
@@ -1,3 +1,23 @@
+2012-11-17  Eli Zaretskii  <address@hidden>
+
+       * w32proc.c (create_child): Don't clip the PID of the child
+       process to fit into an Emacs integer, as this is no longer a
+       restriction.
+       (waitpid): Rename from sys_wait.  Emulate a Posix 'waitpid' by
+       reaping only the process specified by PID argument, if that is
+       positive.  Use PID instead of dead_child to know which process to
+       reap.  Wait for the child to die only if WNOHANG is not in
+       OPTIONS.
+       (sys_select): Don't set dead_child.
+
+       * sysdep.c (wait_for_termination_1): Remove the WINDOWSNT portion,
+       as it is no longer needed.
+
+       * process.c (waitpid, WUNTRACED) [!WNOHANG]: Remove definitions,
+       no longer needed.
+       (record_child_status_change): Remove the setting of
+       record_at_most_one_child for the !WNOHANG case.
+
 2012-11-17  Paul Eggert  <address@hidden>
 
        Fix problems in ns port found by static checking.

=== modified file 'src/process.c'
--- a/src/process.c     2012-11-16 15:29:22 +0000
+++ b/src/process.c     2012-11-17 16:46:45 +0000
@@ -130,18 +130,6 @@
                       EMACS_TIME *, void *);
 #endif
 
-/* This is for DOS_NT ports.  FIXME: Remove this old portability cruft
-   by having DOS_NT ports implement waitpid instead of wait.  Nowadays
-   POSIXish hosts all define waitpid, WNOHANG, and WUNTRACED, as these
-   have been standard since POSIX.1-1988.  */
-#ifndef WNOHANG
-# undef waitpid
-# define waitpid(pid, status, options) wait (status)
-#endif
-#ifndef WUNTRACED
-# define WUNTRACED 0
-#endif
-
 /* Work around GCC 4.7.0 bug with strict overflow checking; see
    <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52904>.
    These lines can be removed once the GCC bug is fixed.  */
@@ -6295,17 +6283,9 @@
 {
 #ifdef SIGCHLD
 
-# ifdef WNOHANG
   /* On POSIXish hosts, record at most one child only if we already
      know one child that has exited.  */
   bool record_at_most_one_child = 0 <= pid;
-# else
-  /* On DOS_NT (the only porting target that lacks WNOHANG),
-     record the status of at most one child process, since the SIGCHLD
-     handler must return right away.  If any more processes want to
-     signal us, we will get another signal.  */
-  bool record_at_most_one_child = 1;
-# endif
 
   Lisp_Object tail;
 

=== modified file 'src/sysdep.c'
--- a/src/sysdep.c      2012-11-14 04:55:41 +0000
+++ b/src/sysdep.c      2012-11-17 16:46:45 +0000
@@ -289,10 +289,6 @@
 {
   while (1)
     {
-#ifdef WINDOWSNT
-      wait (0);
-      break;
-#else /* not WINDOWSNT */
       int status;
       int wait_result = waitpid (pid, &status, 0);
       if (wait_result < 0)
@@ -306,7 +302,8 @@
          break;
        }
 
-#endif /* not WINDOWSNT */
+      /* Note: the MS-Windows emulation of waitpid calls QUIT
+        internally.  */
       if (interruptible)
        QUIT;
     }

=== modified file 'src/w32proc.c'
--- a/src/w32proc.c     2012-11-16 17:20:23 +0000
+++ b/src/w32proc.c     2012-11-17 16:46:45 +0000
@@ -789,7 +789,6 @@
 /* Child process management list.  */
 int child_proc_count = 0;
 child_process child_procs[ MAX_CHILDREN ];
-child_process *dead_child = NULL;
 
 static DWORD WINAPI reader_thread (void *arg);
 
@@ -1042,9 +1041,6 @@
   if (cp->pid < 0)
     cp->pid = -cp->pid;
 
-  /* pid must fit in a Lisp_Int */
-  cp->pid = cp->pid & INTMASK;
-
   *pPid = cp->pid;
 
   return TRUE;
@@ -1120,55 +1116,110 @@
     delete_child (cp);
 }
 
-/* Wait for any of our existing child processes to die
-   When it does, close its handle
-   Return the pid and fill in the status if non-NULL.  */
+/* Wait for a child process specified by PID, or for any of our
+   existing child processes (if PID is nonpositive) to die.  When it
+   does, close its handle.  Return the pid of the process that died
+   and fill in STATUS if non-NULL.  */
 
-int
-sys_wait (int *status)
+pid_t
+waitpid (pid_t pid, int *status, int options)
 {
   DWORD active, retval;
   int nh;
-  int pid;
   child_process *cp, *cps[MAX_CHILDREN];
   HANDLE wait_hnd[MAX_CHILDREN];
+  DWORD timeout_ms;
+  int dont_wait = (options & WNOHANG) != 0;
 
   nh = 0;
-  if (dead_child != NULL)
-    {
-      /* We want to wait for a specific child */
-      wait_hnd[nh] = dead_child->procinfo.hProcess;
-      cps[nh] = dead_child;
-      if (!wait_hnd[nh]) emacs_abort ();
-      nh++;
-      active = 0;
-      goto get_result;
-    }
-  else
-    {
-      for (cp = child_procs + (child_proc_count-1); cp >= child_procs; cp--)
-       /* some child_procs might be sockets; ignore them */
-       if (CHILD_ACTIVE (cp) && cp->procinfo.hProcess
-           && (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0))
-         {
-           wait_hnd[nh] = cp->procinfo.hProcess;
-           cps[nh] = cp;
-           nh++;
-         }
-    }
-
-  if (nh == 0)
-    {
-      /* Nothing to wait on, so fail */
-      errno = ECHILD;
-      return -1;
-    }
+  /* According to Posix:
+
+     PID = -1 means status is requested for any child process.
+
+     PID > 0 means status is requested for a single child process
+     whose pid is PID.
+
+     PID = 0 means status is requested for any child process whose
+     process group ID is equal to that of the calling process.  But
+     since Windows has only a limited support for process groups (only
+     for console processes and only for the purposes of passing
+     Ctrl-BREAK signal to them), and since we have no documented way
+     of determining whether a given process belongs to our group, we
+     treat 0 as -1.
+
+     PID < -1 means status is requested for any child process whose
+     process group ID is equal to the absolute value of PID.  Again,
+     since we don't support process groups, we treat that as -1.  */
+  if (pid > 0)
+    {
+      int our_child = 0;
+
+      /* We are requested to wait for a specific child.  */
+      for (cp = child_procs + (child_proc_count-1); cp >= child_procs; cp--)
+       {
+         /* Some child_procs might be sockets; ignore them.  Also
+            ignore subprocesses whose output is not yet completely
+            read.  */
+         if (CHILD_ACTIVE (cp)
+             && cp->procinfo.hProcess
+             && cp->pid == pid)
+           {
+             our_child = 1;
+             break;
+           }
+       }
+      if (our_child)
+       {
+         if (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0)
+           {
+             wait_hnd[nh] = cp->procinfo.hProcess;
+             cps[nh] = cp;
+             nh++;
+           }
+         else if (dont_wait)
+           {
+             /* PID specifies our subprocess, but its status is not
+                yet available.  */
+             return 0;
+           }
+       }
+      if (nh == 0)
+       {
+         /* No such child process, or nothing to wait for, so fail.  */
+         errno = ECHILD;
+         return -1;
+       }
+    }
+  else
+    {
+      for (cp = child_procs + (child_proc_count-1); cp >= child_procs; cp--)
+       {
+         if (CHILD_ACTIVE (cp)
+             && cp->procinfo.hProcess
+             && (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0))
+           {
+             wait_hnd[nh] = cp->procinfo.hProcess;
+             cps[nh] = cp;
+             nh++;
+           }
+       }
+      if (nh == 0)
+       {
+         /* Nothing to wait on, so fail.  */
+         errno = ECHILD;
+         return -1;
+       }
+    }
+
+  if (dont_wait)
+    timeout_ms = 0;
+  else
+    timeout_ms = 1000; /* check for quit about once a second. */
 
   do
     {
-      /* Check for quit about once a second. */
       QUIT;
-      active = WaitForMultipleObjects (nh, wait_hnd, FALSE, 1000);
+      active = WaitForMultipleObjects (nh, wait_hnd, FALSE, timeout_ms);
     } while (active == WAIT_TIMEOUT);
 
   if (active == WAIT_FAILED)
@@ -1198,8 +1249,10 @@
     }
   if (retval == STILL_ACTIVE)
     {
-      /* Should never happen */
+      /* Should never happen.  */
       DebPrint (("Wait.WaitForMultipleObjects returned an active process\n"));
+      if (pid > 0 && dont_wait)
+       return 0;
       errno = EINVAL;
       return -1;
     }
@@ -1213,6 +1266,8 @@
   else
     retval <<= 8;
 
+  if (pid > 0 && active != 0)
+    emacs_abort ();
   cp = cps[active];
   pid = cp->pid;
 #ifdef FULL_DEBUG
@@ -2001,9 +2056,7 @@
              DebPrint (("select calling SIGCHLD handler for pid %d\n",
                         cp->pid));
 #endif
-             dead_child = cp;
              sig_handlers[SIGCHLD] (SIGCHLD);
-             dead_child = NULL;
            }
        }
       else if (fdindex[active] == -1)