[Emacs-diffs] /srv/bzr/emacs/emacs-24 r110950: Revert recent change for Bug#8855.

[Paul Eggert]

------------------------------------------------------------
revno: 110950
committer: Paul Eggert <address@hidden>
branch nick: emacs-24
timestamp: Sat 2012-11-24 00:24:11 -0800
message:
  Revert recent change for Bug#8855.
  
  As reported by Harald Hanche-Olsen in
  <http://lists.gnu.org/archive/html/emacs-devel/2012-11/msg00445.html>
  the change introduces a further bug, of creating lots of zombie
  processes in some cases.  Further work is needed to come up with a
  better fix for Bug#8855.
removed:
  nt/inc/sys/wait.h
modified:
  nt/ChangeLog
  nt/config.nt
  nt/inc/ms-w32.h
  src/ChangeLog
  src/process.c
  src/process.h
  src/sysdep.c
  src/w32proc.c

=== modified file 'nt/ChangeLog'
--- a/nt/ChangeLog      2012-11-24 06:24:13 +0000
+++ b/nt/ChangeLog      2012-11-24 08:24:11 +0000
@@ -1,3 +1,7 @@
+2012-11-24  Paul Eggert  <address@hidden>
+
+       Revert recent change for Bug#8855; see ../src/ChangeLog.
+
 2012-11-23  Eli Zaretskii  <address@hidden>
 
        Fix a race condition with glib (Bug#8855).

=== modified file 'nt/config.nt'
--- a/nt/config.nt      2012-11-23 22:20:31 +0000
+++ b/nt/config.nt      2012-11-24 08:24:11 +0000
@@ -963,7 +963,7 @@
 #undef HAVE_SYS_VLIMIT_H
 
 /* Define to 1 if you have <sys/wait.h> that is POSIX.1 compatible. */
-#define HAVE_SYS_WAIT_H 1
+#undef HAVE_SYS_WAIT_H
 
 /* 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-23 22:20:31 +0000
+++ b/nt/inc/ms-w32.h   2012-11-24 08:24:11 +0000
@@ -185,12 +185,15 @@
 
 /* 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

=== removed file 'nt/inc/sys/wait.h'
--- a/nt/inc/sys/wait.h 2012-11-23 22:20:31 +0000
+++ b/nt/inc/sys/wait.h 1970-01-01 00:00:00 +0000
@@ -1,33 +0,0 @@
-/* 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-24 07:25:52 +0000
+++ b/src/ChangeLog     2012-11-24 08:24:11 +0000
@@ -1,3 +1,12 @@
+2012-11-24  Paul Eggert  <address@hidden>
+
+       Revert recent change for Bug#8855.
+       As reported by Harald Hanche-Olsen in
+       <http://lists.gnu.org/archive/html/emacs-devel/2012-11/msg00445.html>
+       the change introduces a further bug, of creating lots of zombie
+       processes in some cases.  Further work is needed to come up with a
+       better fix for Bug#8855.
+
 2012-11-24  Eli Zaretskii  <address@hidden>
 
        * xdisp.c (draw_glyphs): Don't draw in mouse face if mouse

=== modified file 'src/process.c'
--- a/src/process.c     2012-11-23 22:20:31 +0000
+++ b/src/process.c     2012-11-24 08:24:11 +0000
@@ -130,6 +130,14 @@
                       EMACS_TIME *, void *);
 #endif
 
+#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.  */
@@ -787,8 +795,9 @@
 #ifdef SIGCHLD
 /* Fdelete_process promises to immediately forget about the process, but in
    reality, Emacs needs to remember those processes until they have been
-   treated by the SIGCHLD handler and waitpid has been invoked on them;
-   otherwise they might fill up the kernel's process table.  */
+   treated by the SIGCHLD handler; otherwise this handler would consider the
+   process as being synchronous and say that the synchronous process is
+   dead.  */
 static Lisp_Object deleted_pid_list;
 #endif
 
@@ -1695,7 +1704,16 @@
   if (inchannel > max_process_desc)
     max_process_desc = inchannel;
 
-  /* This may signal an error. */
+  /* Until we store the proper pid, enable the SIGCHLD handler
+     to recognize an unknown pid as standing for this process.
+     It is very important not to let this `marker' value stay
+     in the table after this function has returned; if it does
+     it might cause call-process to hang and subsequent asynchronous
+     processes to get their return values scrambled.  */
+  XPROCESS (process)->pid = -1;
+
+  /* This must be called after the above line because it may signal an
+     error. */
   setup_process_coding_systems (process);
 
   encoded_current_dir = ENCODE_FILE (current_dir);
@@ -1862,8 +1880,6 @@
 #endif
 
   XPROCESS (process)->pid = pid;
-  if (0 <= pid)
-    XPROCESS (process)->alive = 1;
 
   /* Stop blocking signals in the parent.  */
 #ifdef SIGCHLD
@@ -6257,35 +6273,9 @@
   return process;
 }
 
-/* If the status of the process DESIRED has changed, return true and
-   set *STATUS to its exit status; otherwise, return false.
-   If HAVE is nonnegative, assume that HAVE = waitpid (HAVE, STATUS, ...)
-   has already been invoked, and do not invoke waitpid again.  */
-
-static bool
-process_status_retrieved (pid_t desired, pid_t have, int *status)
-{
-  if (have < 0)
-    {
-      /* Invoke waitpid only with a known process ID; do not invoke
-        waitpid with a nonpositive argument.  Otherwise, Emacs might
-        reap an unwanted process by mistake.  For example, invoking
-        waitpid (-1, ...) can mess up glib by reaping glib's subprocesses,
-        so that another thread running glib won't find them.  */
-      do
-       have = waitpid (desired, status, WNOHANG | WUNTRACED);
-      while (have < 0 && errno == EINTR);
-    }
-
-  return have == desired;
-}
-
-/* If PID is nonnegative, the child process PID with wait status W has
-   changed its status; record this and return true.
-
-   If PID is negative, ignore W, and look for known child processes
-   of Emacs whose status have changed.  For each one found, record its new
-   status.
+/* On receipt of a signal that a child status has changed, loop asking
+   about children with changed statuses until the system says there
+   are no more.
 
    All we do is change the status; we do not run sentinels or print
    notifications.  That is saved for the next time keyboard input is
@@ -6308,15 +6298,13 @@
    ** Malloc WARNING: This should never call malloc either directly or
    indirectly; if it does, that is a bug  */
 
+/* Record the changed status of the child process PID with wait status W.  */
 void
 record_child_status_change (pid_t pid, int w)
 {
 #ifdef SIGCHLD
-
-  /* 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;
-
+  Lisp_Object proc;
+  struct Lisp_Process *p;
   Lisp_Object tail;
 
   /* Find the process that signaled us, and record its status.  */
@@ -6324,69 +6312,68 @@
   /* The process can have been deleted by Fdelete_process.  */
   for (tail = deleted_pid_list; CONSP (tail); tail = XCDR (tail))
     {
-      bool all_pids_are_fixnums
-       = (MOST_NEGATIVE_FIXNUM <= TYPE_MINIMUM (pid_t)
-          && TYPE_MAXIMUM (pid_t) <= MOST_POSITIVE_FIXNUM);
       Lisp_Object xpid = XCAR (tail);
-      if (all_pids_are_fixnums ? INTEGERP (xpid) : NUMBERP (xpid))
+      if ((INTEGERP (xpid) && pid == XINT (xpid))
+         || (FLOATP (xpid) && pid == XFLOAT_DATA (xpid)))
        {
-         pid_t deleted_pid;
-         if (INTEGERP (xpid))
-           deleted_pid = XINT (xpid);
-         else
-           deleted_pid = XFLOAT_DATA (xpid);
-         if (process_status_retrieved (deleted_pid, pid, &w))
-           {
-             XSETCAR (tail, Qnil);
-             if (record_at_most_one_child)
-               return;
-           }
+         XSETCAR (tail, Qnil);
+         return;
        }
     }
 
   /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
+  p = 0;
   for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail))
     {
-      Lisp_Object proc = XCDR (XCAR (tail));
-      struct Lisp_Process *p = XPROCESS (proc);
-      if (p->alive && process_status_retrieved (p->pid, pid, &w))
+      proc = XCDR (XCAR (tail));
+      p = XPROCESS (proc);
+      if (EQ (p->type, Qreal) && p->pid == pid)
+       break;
+      p = 0;
+    }
+
+  /* Look for an asynchronous process whose pid hasn't been filled
+     in yet.  */
+  if (! p)
+    for (tail = Vprocess_alist; CONSP (tail); tail = XCDR (tail))
+      {
+       proc = XCDR (XCAR (tail));
+       p = XPROCESS (proc);
+       if (p->pid == -1)
+         break;
+       p = 0;
+      }
+
+  /* Change the status of the process that was found.  */
+  if (p)
+    {
+      int clear_desc_flag = 0;
+
+      p->tick = ++process_tick;
+      p->raw_status = w;
+      p->raw_status_new = 1;
+
+      /* If process has terminated, stop waiting for its output.  */
+      if ((WIFSIGNALED (w) || WIFEXITED (w))
+         && p->infd >= 0)
+       clear_desc_flag = 1;
+
+      /* We use clear_desc_flag to avoid a compiler bug in Microsoft C.  */
+      if (clear_desc_flag)
        {
-         /* Change the status of the process that was found.  */
-         p->tick = ++process_tick;
-         p->raw_status = w;
-         p->raw_status_new = 1;
-
-         /* If process has terminated, stop waiting for its output.  */
-         if (WIFSIGNALED (w) || WIFEXITED (w))
-           {
-             int clear_desc_flag = 0;
-             p->alive = 0;
-             if (p->infd >= 0)
-               clear_desc_flag = 1;
-
-             /* clear_desc_flag avoids a compiler bug in Microsoft C.  */
-             if (clear_desc_flag)
-               {
-                 FD_CLR (p->infd, &input_wait_mask);
-                 FD_CLR (p->infd, &non_keyboard_wait_mask);
-               }
-           }
-
-         /* Tell wait_reading_process_output that it needs to wake up and
-            look around.  */
-         if (input_available_clear_time)
-           *input_available_clear_time = make_emacs_time (0, 0);
-
-         if (record_at_most_one_child)
-           return;
+         FD_CLR (p->infd, &input_wait_mask);
+         FD_CLR (p->infd, &non_keyboard_wait_mask);
        }
+
+      /* Tell wait_reading_process_output that it needs to wake up and
+        look around.  */
+      if (input_available_clear_time)
+       *input_available_clear_time = make_emacs_time (0, 0);
     }
-
-  if (0 <= pid)
+  /* There was no asynchronous process found for that pid: we have
+     a synchronous process.  */
+  else
     {
-      /* The caller successfully waited for a pid but no asynchronous
-        process was found for it, so this is a synchronous process.  */
-
       synch_process_alive = 0;
 
       /* Report the status of the synchronous process.  */
@@ -6405,10 +6392,38 @@
 
 #ifdef SIGCHLD
 
+/* On some systems, the SIGCHLD handler must return right away.  If
+   any more processes want to signal us, we will get another signal.
+   Otherwise, loop around to use up all the processes that have
+   something to tell us.  */
+#if (defined WINDOWSNT \
+     || (defined USG && !defined GNU_LINUX \
+        && !(defined HPUX && defined WNOHANG)))
+enum { CAN_HANDLE_MULTIPLE_CHILDREN = 0 };
+#else
+enum { CAN_HANDLE_MULTIPLE_CHILDREN = 1 };
+#endif
+
 static void
 handle_child_signal (int sig)
 {
-  record_child_status_change (-1, 0);
+  do
+    {
+      pid_t pid;
+      int status;
+
+      do
+       pid = waitpid (-1, &status, WNOHANG | WUNTRACED);
+      while (pid < 0 && errno == EINTR);
+
+      /* PID == 0 means no processes found, PID == -1 means a real failure.
+        Either way, we have done all our job.  */
+      if (pid <= 0)
+       break;
+
+      record_child_status_change (pid, status);
+    }
+  while (CAN_HANDLE_MULTIPLE_CHILDREN);
 }
 
 static void

=== modified file 'src/process.h'
--- a/src/process.h     2012-11-23 22:20:31 +0000
+++ b/src/process.h     2012-11-24 08:24:11 +0000
@@ -142,9 +142,6 @@
     /* Flag to set coding-system of the process buffer from the
        coding_system used to decode process output.  */
     unsigned int inherit_coding_system_flag : 1;
-    /* Whether the process is alive, i.e., can be waited for.  Running
-       processes can be waited for, but exited and fake processes cannot.  */
-    unsigned int alive : 1;
     /* Record the process status in the raw form in which it comes from `wait'.
        This is to avoid consing in a signal handler.  The `raw_status_new'
        flag indicates that `raw_status' contains a new status that still

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

=== modified file 'src/w32proc.c'
--- a/src/w32proc.c     2012-11-23 22:20:31 +0000
+++ b/src/w32proc.c     2012-11-24 08:24:11 +0000
@@ -783,6 +783,7 @@
 /* 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);
 
@@ -1035,6 +1036,9 @@
   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;
@@ -1110,110 +1114,55 @@
     delete_child (cp);
 }
 
-/* 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.  */
+/* 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.  */
 
-pid_t
-waitpid (pid_t pid, int *status, int options)
+int
+sys_wait (int *status)
 {
   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;
-  /* 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. */
+  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;
+    }
 
   do
     {
+      /* Check for quit about once a second. */
       QUIT;
-      active = WaitForMultipleObjects (nh, wait_hnd, FALSE, timeout_ms);
+      active = WaitForMultipleObjects (nh, wait_hnd, FALSE, 1000);
     } while (active == WAIT_TIMEOUT);
 
   if (active == WAIT_FAILED)
@@ -1243,10 +1192,8 @@
     }
   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;
     }
@@ -1260,8 +1207,6 @@
   else
     retval <<= 8;
 
-  if (pid > 0 && active != 0)
-    emacs_abort ();
   cp = cps[active];
   pid = cp->pid;
 #ifdef FULL_DEBUG
@@ -2050,7 +1995,9 @@
              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)