Re: GNU make jobserver redux

[Paul Eggert]

> From: "Paul D. Smith" <address@hidden>
> Date: Fri, 4 May 2001 01:58:24 -0400
> 
> The trick is we need the read() to be interruptible on signals,
> otherwise we'll never reap any children until we get a token... which
> could deadlock us.

Won't SA_RESTART let you implement that trick without putting EINTR
checks all over the place?

SA_RESTART lets the read() be interruptible, but it causes an
interrupted read() to be restarted after the signal handler returns.
Therefore, read() never fails with errno==EINTR; it either succeeds,
or it fails with errno!=EINTR (typically errno==EBADF).  This is the
behavior that you want.

Perhaps this SA_RESTART idea wasn't used before, either because the
SA_RESTART feature wasn't available long ago when you first worked on
this, or because some older Unix systems didn't have SA_RESTART.
However, SA_RESTART support is becoming standard.  It's supported on
modern versions of GNU/Linux, BSD, and Solaris (the systems I have
easy access to).  POSIX 1003.1-200x Draft 6 specifies SA_RESTART, so
it's becoming the consensus standard.  I think it's OK for "make" to
not support the jobserver feature on older systems that lack
SA_RESTART, given that SA_RESTART makes the code much simpler and more
reliable.

Here is a patch that implements this suggestion; it assumes the
patches I sent in for GNU make 3.79.1 in July.  This patch also fixes
an EINTR-related bug in arscan.c that I discovered while removing EINTR.

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

        Restart all system calls that are interrupted,
        instead of worrying about EINTR.

        * 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.)
        (main, FATAL_SIG, HANDLE_SIG): Use bsd_signal instead of signal.

        * make.h (EINTR_SET): Remove.

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

===================================================================
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.1
diff -pu -r3.79.1.0 -r3.79.1.1
--- job.c       2000/06/23 15:54:56     3.79.1.0
+++ job.c       2001/05/05 00:08:04     3.79.1.1
@@ -500,9 +500,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 +526,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 +785,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));
@@ -1489,8 +1481,8 @@ new_job (file)
           break;
 
         /* 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
+          If we already got a SIGCHLD, or get a SIGCHLD during the read,
+          the read will return with EBADF because
            the signal handler closes the dup'd file descriptor.  */
 
         if (read (job_rfd, &token, 1) == 1)
@@ -1500,7 +1492,7 @@ new_job (file)
             break;
           }
 
-        if (errno != EINTR && errno != EBADF)
+        if (errno != EBADF)
           pfatal_with_name (_("read jobs pipe"));
 
         /* Re-dup the read side of the pipe, so the signal handler can
===================================================================
RCS file: main.c,v
retrieving revision 3.79.1.0
retrieving revision 3.79.1.1
diff -pu -r3.79.1.0 -r3.79.1.1
--- main.c      2000/06/13 14:24:45     3.79.1.0
+++ main.c      2001/05/05 00:08:04     3.79.1.1
@@ -448,6 +448,27 @@ int fatal_signal_mask;
 #endif
 #endif
 
+#if !defined HAVE_BSD_SIGNAL && !defined bsd_signal
+# if !defined HAVE_SIGACTION || !defined HAVE_SA_RESTART
+#  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
 
@@ -1351,13 +1372,12 @@ int main (int argc, char ** argv)
 
     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;
+#  if defined HAVE_SA_RESTART
+    sa.sa_flags = SA_RESTART;
 #  endif
 #  define HANDLESIG(s) sigaction (s, &sa, NULL)
 # else
-#  define HANDLESIG(s) signal (s, child_handler)
+#  define HANDLESIG(s) bsd_signal (s, child_handler)
 # endif
 
     /* OK, now actually install the handlers.  */
@@ -1372,7 +1392,7 @@ int main (int argc, char ** argv)
 
   /* 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 +1547,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.1
diff -pu -r3.79.1.0 -r3.79.1.1
--- make.h      2000/06/15 05:25:37     3.79.1.0
+++ make.h      2001/05/05 00:08:04     3.79.1.1
@@ -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
===================================================================
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);
 }