Re: GNU make jobserver redux

[Paul Eggert]

> Date: Sat, 5 May 2001 15:55:28 -0400
> From: "Paul D. Smith" <address@hidden>
> 
> Yes, this is what I was suggesting.  That instead of installing the
> SIGCHLD handler at the start of the make process and leaving it there,
> we only install it when we're going to do the read, then after we get
> the token we put back SIG_DFL for SIGCHLD.

Mightn't SIG_DFL cause the read() to misbehave on the Hurd, for
reasons similar to why SA_RESTART misbehaves?

If the current code is working in the field, perhaps it would be
better to disable SA_RESTART instead; that is closer to what is
currently occurring.  Here's a proposed patch to implement that idea;
it replaces my earlier patch on this subject.  (This patch also uses
bsd_signal in a couple more places than the earlier patch, to simplify
the code.)

2001-05-06  Paul Eggert  <address@hidden>

        Restart almost all system calls that are interrupted, instead
        of worrying about EINTR.  The lone exception is the read() for
        job tokens.

        * configure.in (HAVE_SA_RESTART): New macro.
        (MAKE_JOBSERVER): Define to 1 only if HAVE_SA_RESTART.
        * main.c (main): Use SA_RESTART instead of the old,
        nonstandard SA_INTERRUPT.

        * configure.in (AC_CHECK_FUNCS): Add bsd_signal.
        * main.c (bsd_signal): New function or macro,
        if the implementation doesn't supply it.
        (The bsd_signal function will be in POSIX 1003.1-200x.)
        (HANDLESIG): Remove.
        (main, FATAL_SIG): Use bsd_signal instead of signal or HANDLESIG.

        * make.h (EINTR_SET): Remove.
        (SA_RESTART): New macro.

        * arscan.c (ar_member_touch): Don't worry about EINTR.
        * function.c (func_shell): Likewise.
        * job.c (reap_children, free_child, new_job): Likewise.
        * main.c (main): Likewise.
        * remake.c (touch_file, name_mtime): Likewise.

        * arscan.c (ar_member_touch): Fix bug uncovered by EINTR removal;
        if fstat failed with errno!=EINTR, the error was ignored.

        * job.c (set_child_handler_action_flags): New function.
        (new_job): Use it to temporarily clear the SIGCHLD action flags
        while reading the token.

===================================================================
RCS file: arscan.c,v
retrieving revision 3.79.1.0
retrieving revision 3.79.1.1
diff -pu -r3.79.1.0 -r3.79.1.1
--- arscan.c    2000/04/18 14:40:59     3.79.1.0
+++ arscan.c    2001/05/05 00:08:04     3.79.1.1
@@ -793,8 +793,8 @@ ar_member_touch (arname, memname)
   if (AR_HDR_SIZE != write (fd, (char *) &ar_hdr, AR_HDR_SIZE))
     goto lose;
   /* The file's mtime is the time we we want.  */
-  while (fstat (fd, &statbuf) < 0 && EINTR_SET)
-    ;
+  if (fstat (fd, &statbuf) < 0)
+    goto lose;
 #if defined(ARFMAG) || defined(ARFZMAG) || defined(AIAMAG) || 
defined(WINDOWS32)
   /* Advance member's time to that time */
   for (i = 0; i < sizeof ar_hdr.ar_date; i++)
===================================================================
RCS file: configure.in,v
retrieving revision 3.79.1.3
retrieving revision 3.79.1.4
diff -pu -r3.79.1.3 -r3.79.1.4
--- configure.in        2000/07/24 06:44:47     3.79.1.3
+++ configure.in        2001/05/05 00:08:04     3.79.1.4
@@ -113,6 +113,7 @@ if test $ac_cv_func_gettimeofday = yes; 
 fi
 
 AC_CHECK_FUNCS(        memmove memcpy strchr strdup psignal mkstemp mktemp 
fdopen \
+               bsd_signal \
                dup2 getcwd sigsetmask sigaction getgroups setlinebuf \
                seteuid setegid setreuid setregid pipe strerror strsignal)
 
@@ -196,9 +197,20 @@ case "$ac_cv_func_waitpid/$ac_cv_func_wa
   no/no) has_wait_nohang=no ;;
 esac
 
-case 
"$ac_cv_func_pipe/$ac_cv_func_sigaction/$has_wait_nohang/$make_cv_job_server" in
-  yes/yes/yes/yes) AC_DEFINE(MAKE_JOBSERVER, 1,
-                     [Define this to enable job server support in GNU make.]);;
+AC_CACHE_CHECK(for SA_RESTART, make_cv_sa_restart, [
+  AC_TRY_COMPILE([#include <signal.h>],
+      [return SA_RESTART;],
+      make_cv_sa_restart=yes,
+      make_cv_sa_restart=no)])
+if test "$make_cv_sa_restart" != no; then
+  AC_DEFINE(HAVE_SA_RESTART, 1,
+     [Define if <signal.h> defines the SA_RESTART constant.])
+fi
+
+case 
"$ac_cv_func_pipe/$ac_cv_func_sigaction/$make_cv_sa_restart/$has_wait_nohang/$make_cv_job_server"
 in
+  yes/yes/yes/yes/yes)
+    AC_DEFINE(MAKE_JOBSERVER, 1,
+              [Define this to enable job server support in GNU make.]);;
 esac
 
 dnl Allow building with dmalloc
===================================================================
RCS file: function.c,v
retrieving revision 3.79.1.0
retrieving revision 3.79.1.1
diff -pu -r3.79.1.0 -r3.79.1.1
--- function.c  2000/06/20 14:00:16     3.79.1.0
+++ function.c  2001/05/05 00:08:04     3.79.1.1
@@ -1435,8 +1435,7 @@ func_shell (o, argv, funcname)
       buffer = (char *) xmalloc (maxlen + 1);
 
       /* Read from the pipe until it gets EOF.  */
-      i = 0;
-      do
+      for (i = 0; ; i += cc)
        {
          if (i == maxlen)
            {
@@ -1444,12 +1443,10 @@ func_shell (o, argv, funcname)
              buffer = (char *) xrealloc (buffer, maxlen + 1);
            }
 
-         errno = 0;
          cc = read (pipedes[0], &buffer[i], maxlen - i);
-         if (cc > 0)
-           i += cc;
+         if (cc <= 0)
+           break;
        }
-      while (cc > 0 || EINTR_SET);
 
       /* Close the read side of the pipe.  */
 #ifdef  __MSDOS__
===================================================================
RCS file: job.c,v
retrieving revision 3.79.1.0
retrieving revision 3.79.1.3
diff -pu -r3.79.1.0 -r3.79.1.3
--- job.c       2000/06/23 15:54:56     3.79.1.0
+++ job.c       2001/05/07 06:51:03     3.79.1.3
@@ -195,6 +195,7 @@ extern int remote_status PARAMS ((int *e
 
 RETSIGTYPE child_handler PARAMS ((int));
 static void free_child PARAMS ((struct child *));
+static void set_child_handler_action_flags PARAMS ((int));
 static void start_job_command PARAMS ((struct child *child));
 static int load_too_high PARAMS ((void));
 static int job_next_command PARAMS ((struct child *));
@@ -500,9 +501,6 @@ reap_children (block, err)
        {
           /* A remote status command failed miserably.  Punt.  */
        remote_status_lose:
-         if (EINTR_SET)
-           continue;
-
          pfatal_with_name ("remote_status");
        }
       else
@@ -529,10 +527,6 @@ reap_children (block, err)
 
          if (pid < 0)
            {
-              /* EINTR?  Try again. */
-             if (EINTR_SET)
-               goto local_wait;
-
               /* The wait*() failed miserably.  Punt.  */
              pfatal_with_name ("wait");
            }
@@ -792,9 +786,8 @@ free_child (child)
 
       /* Write a job token back to the pipe.  */
 
-      while (write (job_fds[1], &token, 1) != 1)
-        if (!EINTR_SET)
-          pfatal_with_name (_("write jobserver"));
+      if (write (job_fds[1], &token, 1) != 1)
+       pfatal_with_name (_("write jobserver"));
 
       DB (DB_JOBS, (_("Released token for child 0x%08lx (%s).\n"),
                     (unsigned long int) child, child->file->name));
@@ -848,6 +841,25 @@ unblock_sigs ()
 }
 #endif
 
+/* Set the child handler action flags to FLAGS.  */
+static void
+set_child_handler_action_flags (flags)
+     int flags;
+{
+#if defined HAVE_SIGACTION && defined HAVE_SA_RESTART
+  struct sigaction sa;
+  bzero ((char *) &sa, sizeof sa);
+  sa.sa_handler = child_handler;
+  sa.sa_flags = flags;
+# if defined SIGCHLD
+  sigaction (SIGCHLD, &sa, NULL);
+# endif
+# if defined SIGCLD && SIGCLD != SIGCHLD
+  sigaction (SIGCLD, &sa, NULL);
+# endif
+#endif
+}
+
 /* Start a job to run the commands specified in CHILD.
    CHILD is updated to reflect the commands and ID of the child process.
 
@@ -1483,6 +1495,8 @@ new_job (file)
     while (1)
       {
         char token;
+       int read_yield;
+       int read_errno;
 
         /* If we don't already have a job started, use our "free" token.  */
         if (!children)
@@ -1491,15 +1505,32 @@ new_job (file)
         /* Read a token.  As long as there's no token available we'll block.
            If we get a SIGCHLD we'll return with EINTR.  If one happened
            before we got here we'll return immediately with EBADF because
-           the signal handler closes the dup'd file descriptor.  */
+           the signal handler closes the dup'd file descriptor.
+
+          Temporarily adjust the SIGCHLD action so that read returns
+          with EINTR if interrupted with SIGCHLD; otherwise, on the
+          Hurd (and perhaps other systems) the read might hang
+          indefinitely, because on these systems SA_RESTART causes
+          interrupted system calls to restart from where they left
+          off, not from the beginning of the call.
+
+          It's a pain to check for EINTR after each system call, so
+          normally SA_RESTART is used; this `read' call is the only
+          exception.  */
+
+       set_child_handler_action_flags (0);
+       read_yield = read (job_rfd, &token, 1);
+       read_errno = errno;
+       set_child_handler_action_flags (SA_RESTART);
 
-        if (read (job_rfd, &token, 1) == 1)
+       if (read_yield == 1)
           {
             DB (DB_JOBS, (_("Obtained token for child 0x%08lx (%s).\n"),
                           (unsigned long int) c, c->file->name));
             break;
           }
 
+       errno = read_errno;
         if (errno != EINTR && errno != EBADF)
           pfatal_with_name (_("read jobs pipe"));
 
===================================================================
RCS file: main.c,v
retrieving revision 3.79.1.0
retrieving revision 3.79.1.2
diff -pu -r3.79.1.0 -r3.79.1.2
--- main.c      2000/06/13 14:24:45     3.79.1.0
+++ main.c      2001/05/07 06:37:51     3.79.1.2
@@ -448,6 +448,27 @@ int fatal_signal_mask;
 #endif
 #endif
 
+#if !defined HAVE_BSD_SIGNAL && !defined bsd_signal
+# if !defined HAVE_SIGACTION
+#  define bsd_signal signal
+# else
+static RETSIGTYPE
+(*bsd_signal) PARAMS ((int)) (sig, func)
+     int sig;
+     RETSIGTYPE (*func) PARAMS ((int));
+{
+  struct sigaction act, oact;
+  act.sa_handler = func;
+  act.sa_flags = SA_RESTART;
+  sigemptyset (&act.sa_mask);
+  sigaddset (&act.sa_mask, sig);
+  if (sigaction (sig, &act, &oact) != 0)
+    return SIG_ERR;
+  return oact.sa_handler;
+}
+# endif
+#endif
+
 static struct file *
 enter_command_line_file (name)
      char *name;
@@ -839,8 +860,8 @@ int main (int argc, char ** argv)
 #endif
 
 #define        FATAL_SIG(sig)                                                  
      \
-  if (signal ((sig), fatal_error_signal) == SIG_IGN)                         \
-    (void) signal ((sig), SIG_IGN);                                          \
+  if (bsd_signal (sig, fatal_error_signal) == SIG_IGN)                       \
+    bsd_signal (sig, SIG_IGN);                                               \
   else                                                                       \
     ADD_SIG (sig);
 
@@ -879,10 +900,10 @@ int main (int argc, char ** argv)
 
 #ifdef HAVE_WAIT_NOHANG
 # if defined SIGCHLD
-  (void) signal (SIGCHLD, SIG_DFL);
+  (void) bsd_signal (SIGCHLD, SIG_DFL);
 # endif
 # if defined SIGCLD && SIGCLD != SIGCHLD
-  (void) signal (SIGCLD, SIG_DFL);
+  (void) bsd_signal (SIGCLD, SIG_DFL);
 # endif
 #endif
 
@@ -1345,34 +1366,18 @@ int main (int argc, char ** argv)
      If none of these are true, we don't need a signal handler at all.  */
   {
     extern RETSIGTYPE child_handler PARAMS ((int sig));
-
-# if defined HAVE_SIGACTION
-    struct sigaction sa;
-
-    bzero ((char *)&sa, sizeof (struct sigaction));
-    sa.sa_handler = child_handler;
-#  if defined SA_INTERRUPT
-    /* This is supposed to be the default, but what the heck... */
-    sa.sa_flags = SA_INTERRUPT;
-#  endif
-#  define HANDLESIG(s) sigaction (s, &sa, NULL)
-# else
-#  define HANDLESIG(s) signal (s, child_handler)
-# endif
-
-    /* OK, now actually install the handlers.  */
 # if defined SIGCHLD
-    (void) HANDLESIG (SIGCHLD);
+    bsd_signal (SIGCHLD, child_handler);
 # endif
 # if defined SIGCLD && SIGCLD != SIGCHLD
-    (void) HANDLESIG (SIGCLD);
+    bsd_signal (SIGCLD, child_handler);
 # endif
   }
 #endif
 
   /* Let the user send us SIGUSR1 to toggle the -d flag during the run.  */
 #ifdef SIGUSR1
-  (void) signal (SIGUSR1, debug_signal_handler);
+  bsd_signal (SIGUSR1, debug_signal_handler);
 #endif
 
   /* Define the initial list of suffixes for old-style rules.  */
@@ -1527,9 +1532,8 @@ int main (int argc, char ** argv)
          want job_slots to be 0 to indicate we're using the jobserver.  */
 
       while (--job_slots)
-        while (write (job_fds[1], &c, 1) != 1)
-          if (!EINTR_SET)
-            pfatal_with_name (_("init jobserver pipe"));
+        if (write (job_fds[1], &c, 1) != 1)
+         pfatal_with_name (_("init jobserver pipe"));
 
       /* Fill in the jobserver_fds struct for our children.  */
 
===================================================================
RCS file: make.h,v
retrieving revision 3.79.1.0
retrieving revision 3.79.1.2
diff -pu -r3.79.1.0 -r3.79.1.2
--- make.h      2000/06/15 05:25:37     3.79.1.0
+++ make.h      2001/05/07 06:40:42     3.79.1.2
@@ -111,14 +111,6 @@ Boston, MA 02111-1307, USA.  */
 extern int errno;
 #endif
 
-/* A shortcut for EINTR checking.  Note you should be careful when negating
-   this!  That might not mean what you want if EINTR is not available.  */
-#ifdef EINTR
-# define EINTR_SET (errno == EINTR)
-#else
-# define EINTR_SET (0)
-#endif
-
 #ifndef isblank
 # define isblank(c)     ((c) == ' ' || (c) == '\t')
 #endif
@@ -149,6 +141,10 @@ extern int errno;
 # define sigmask(sig)   (1 << ((sig) - 1))
 #endif
 
+#ifndef HAVE_SA_RESTART
+# define SA_RESTART 0
+#endif
+
 #ifdef  HAVE_LIMITS_H
 # include <limits.h>
 #endif
===================================================================
RCS file: remake.c,v
retrieving revision 3.79.1.3
retrieving revision 3.79.1.4
diff -pu -r3.79.1.3 -r3.79.1.4
--- remake.c    2000/07/24 06:44:47     3.79.1.3
+++ remake.c    2001/05/05 00:08:04     3.79.1.4
@@ -933,13 +933,8 @@ touch_file (file)
        {
          struct stat statbuf;
          char buf;
-         int status;
 
-         do
-           status = fstat (fd, &statbuf);
-         while (status < 0 && EINTR_SET);
-
-         if (status < 0)
+         if (fstat (fd, &statbuf) < 0)
            TOUCH_ERROR ("touch: fstat: ");
          /* Rewrite character 0 same as it already is.  */
          if (read (fd, &buf, 1) < 0)
@@ -1243,13 +1238,12 @@ name_mtime (name)
 {
   struct stat st;
 
-  while (stat (name, &st) != 0)
-    if (errno != EINTR)
-      {
-       if (errno != ENOENT && errno != ENOTDIR)
-         perror_with_name ("stat:", name);
-       return NONEXISTENT_MTIME;
-      }
+  if (stat (name, &st) != 0)
+    {
+      if (errno != ENOENT && errno != ENOTDIR)
+       perror_with_name ("stat:", name);
+      return NONEXISTENT_MTIME;
+    }
 
   return FILE_TIMESTAMP_STAT_MODTIME (name, st);
 }