[PATCH] Use cygwin spawn functions

[PATCH] Use cygwin spawn functions

[Richard Henderson]

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

Emulating fork(2) on cygwin is a bit of a challenge; the hoop jumping is
a regular circus act.  Worse, fork has a habit of failing on Windows 7 64-bit,
as seen in the many "Bad address" and "Resource temporarily unavailable"
errors during testing
(e.g. http://gcc.gnu.org/ml/gcc-testresults/2010-10/msg00624.html).

Cygwin's own faq (sec 5.5) says that it's better to avoid using fork+exec
when possible and use the spawn family of calls instead.

With this, I managed an entire check-gcc run without a single spurious
fork failure for --host=i686-cygwin --target=x86_64-w64-mingw64.

Like so.  Ok to commit?


r~

[-- Attachment #2: d-spawn-2 --]
[-- Type: text/plain, Size: 5712 bytes --]

	* configure.ac (process.h): Test for it.
	(dup3, spawnvpe): Test for them.
	* configure, config.in: Rebuild.
	* pex-unix.c: Include process.h if available.
	(struct save_fd, save_and_install_fd, restore_fd): New.
	(pex_unix_exec_child): New implementation using spawnvpe.


diff --git a/libiberty/configure.ac b/libiberty/configure.ac
index 73ea6c9..34a1a7e 100644
--- a/libiberty/configure.ac
+++ b/libiberty/configure.ac
@@ -246,7 +246,7 @@ AC_SUBST_FILE(host_makefile_frag)
 # It's OK to check for header files.  Although the compiler may not be
 # able to link anything, it had better be able to at least compile
 # something.
-AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h)
+AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h process.h)
 AC_HEADER_SYS_WAIT
 AC_HEADER_TIME
 
@@ -359,14 +359,14 @@ vars="sys_errlist sys_nerr sys_siglist"
 
 checkfuncs="getrusage on_exit psignal strerror strsignal sysconf times sbrk gettimeofday"
 checkfuncs="$checkfuncs realpath canonicalize_file_name pstat_getstatic pstat_getdynamic sysmp"
-checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking"
+checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking dup3 spawnvpe"
 
 # These are neither executed nor required, but they help keep
 # autoheader happy without adding a bunch of text to acconfig.h.
 if test "x" = "y"; then
   AC_CHECK_FUNCS(asprintf atexit \
     basename bcmp bcopy bsearch bzero \
-    calloc canonicalize_file_name clock \
+    calloc canonicalize_file_name clock dup3 \
     ffs __fsetlocking \
     getcwd getpagesize getrusage getsysinfo gettimeofday \
     index insque \
@@ -374,10 +374,10 @@ if test "x" = "y"; then
     on_exit \
     psignal pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
-    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy strcasecmp strchr \
-    strdup \
+    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy \
+     strcasecmp strchr strdup \
      strerror strncasecmp strndup strrchr strsignal strstr strtod strtol \
-     strtoul strverscmp sysconf sysctl sysmp \
+     strtoul strverscmp sysconf sysctl sysmp spawnvpe \
     table times tmpnam \
     vasprintf vfprintf vprintf vsprintf \
     wait3 wait4 waitpid)
diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
index 85733a6..eaf9c3e 100644
--- a/libiberty/pex-unix.c
+++ b/libiberty/pex-unix.c
@@ -55,7 +55,9 @@ extern int errno;
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
-
+#ifdef HAVE_PROCESS_H
+#include <process.h>
+#endif
 
 #ifdef vfork /* Autoconf may define this to fork for us. */
 # define VFORK_STRING "fork"
@@ -387,6 +389,106 @@ pex_child_error (struct pex_obj *obj, const char *executable,
 
 extern char **environ;
 
+#ifdef HAVE_SPAWNVPE
+/* Helpers for saving file descriptors around spawning a child.  */
+
+struct save_fd
+{
+  int fd;
+  int flags;
+};
+
+static int
+save_and_install_fd(struct save_fd *s, int old_fd, int child_fd)
+{
+  s->fd = -1;
+  s->flags = 0;
+
+  if (old_fd == child_fd)
+    return;
+
+  s->fd = dup (old_fd);
+  if (s->fd >= 0)
+    {
+      s->flags = fcntl (old_fd, F_GETFD);
+      fcntl (s->fd, F_SETFD, FD_CLOEXEC);
+    }
+  else
+    return -1;
+
+  fcntl (child_fd, F_SETFD, FD_CLOEXEC);
+  return dup2 (child_fd, old_fd);
+}
+
+static void
+restore_fd(struct save_fd *s, int old_fd)
+{
+  if (s->fd < 0)
+    return;
+#ifdef HAVE_DUP3
+  else if (s->flags == FD_CLOEXEC)
+    dup3 (s->fd, old_fd, O_CLOEXEC);
+#endif
+  else
+    {
+      dup2 (s->fd, old_fd);
+      if (s->flags != 0)
+	fcntl (old_fd, F_SETFD, s->flags);
+    }
+  close (s->fd);
+}
+
+static pid_t
+pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
+		     char * const * argv, char * const * env,
+                     int in, int out, int errdes,
+		     int toclose, const char **errmsg, int *err)
+{
+  struct save_fd save_in, save_out, save_err;
+  int sleep_interval, retries;
+  pid_t pid;
+
+  if (save_and_install_fd (&save_in, STDIN_FILE_NO, in) < 0
+      || save_and_install_fd (&save_out, STDOUT_FILE_NO, out) < 0
+      || save_and_install_fd (&save_err, STDERR_FILE_NO, errdes) < 0)
+    {
+      *err = errno;
+      *errmsg = "dup2";
+      return -1;
+    }
+
+  if (env == NULL)
+    env = environ;
+
+  sleep_interval = 1;
+  pid = -1;
+  for (retries = 0; retries < 4; ++retries)
+    {
+      if (flags & PEX_SEARCH)
+	pid = spawnvpe (_P_NOWAITO, executable, argv, env);
+      else
+	pid = spawnve (_P_NOWAITO, executable, argv, env);
+
+      if (pid > 0)
+	{
+	  /* Success.  */
+	  restore_fd (&save_in, STDIN_FILE_NO);
+	  restore_fd (&save_out, STDOUT_FILE_NO);
+	  restore_fd (&save_err, STDERR_FILE_NO);
+	  return pid;
+	}
+
+      if (errno != EAGAIN)
+	break;
+      sleep (sleep_interval);
+      sleep_interval *= 2;
+    }
+
+  *err = errno;
+  *errmsg = "spawn";
+  return -1;
+}
+#else
 static pid_t
 pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
 		     char * const * argv, char * const * env,
@@ -521,6 +623,7 @@ pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
       return pid;
     }
 }
+#endif /* SPAWN */
 
 /* Wait for a child process to complete.  */
 

Re: [PATCH] Use cygwin spawn functions

[Dave Korn]

On 12/10/2010 01:41, Richard Henderson wrote:
> Emulating fork(2) on cygwin is a bit of a challenge; the hoop jumping is
> a regular circus act.  Worse, fork has a habit of failing on Windows 7 64-bit,
> as seen in the many "Bad address" and "Resource temporarily unavailable"
> errors during testing
> (e.g. http://gcc.gnu.org/ml/gcc-testresults/2010-10/msg00624.html).
> 
> Cygwin's own faq (sec 5.5) says that it's better to avoid using fork+exec
> when possible and use the spawn family of calls instead.

  It's a ton less overhead too, hopefully this will show up in faster test
runs; thanks for that.

    cheers,
      DaveK

Re: [PATCH] Use cygwin spawn functions

[NightStrike]

On Mon, Oct 11, 2010 at 8:41 PM, Richard Henderson <rth@redhat.com> wrote:
> Emulating fork(2) on cygwin is a bit of a challenge; the hoop jumping is
> a regular circus act.  Worse, fork has a habit of failing on Windows 7 64-bit,
> as seen in the many "Bad address" and "Resource temporarily unavailable"
> errors during testing
> (e.g. http://gcc.gnu.org/ml/gcc-testresults/2010-10/msg00624.html).
>
> Cygwin's own faq (sec 5.5) says that it's better to avoid using fork+exec
> when possible and use the spawn family of calls instead.
>
> With this, I managed an entire check-gcc run without a single spurious
> fork failure for --host=i686-cygwin --target=x86_64-w64-mingw64.
>
> Like so.  Ok to commit?

THANK YOU!!!!!!!!!11oneone!!!!!!!!!!!

Re: [PATCH] Use cygwin spawn functions

[Richard Henderson]

On 10/11/2010 05:41 PM, Richard Henderson wrote:
> Emulating fork(2) on cygwin is a bit of a challenge; the hoop jumping is
> a regular circus act.  Worse, fork has a habit of failing on Windows 7 64-bit,
> as seen in the many "Bad address" and "Resource temporarily unavailable"
> errors during testing
> (e.g. http://gcc.gnu.org/ml/gcc-testresults/2010-10/msg00624.html).
> 
> Cygwin's own faq (sec 5.5) says that it's better to avoid using fork+exec
> when possible and use the spawn family of calls instead.
> 
> With this, I managed an entire check-gcc run without a single spurious
> fork failure for --host=i686-cygwin --target=x86_64-w64-mingw64.
> 
> Like so.  Ok to commit?

Hum.  Actually, something is Not Quite Right with it.

On both WinXP-32 and Win7-64 hosts, a full test run eventually got to a
point where every test would time out.  In both cases, collect2 appears
to have gotten stuck.  More investigation is required.


r~

Re: [PATCH] Use cygwin spawn functions

[Richard Henderson]

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

On 10/11/2010 05:41 PM, Richard Henderson wrote:
> Emulating fork(2) on cygwin is a bit of a challenge; the hoop jumping is
> a regular circus act.  Worse, fork has a habit of failing on Windows 7 64-bit,
> as seen in the many "Bad address" and "Resource temporarily unavailable"
> errors during testing
> (e.g. http://gcc.gnu.org/ml/gcc-testresults/2010-10/msg00624.html).
> 
> Cygwin's own faq (sec 5.5) says that it's better to avoid using fork+exec
> when possible and use the spawn family of calls instead.

The previous attempt failed to close all of the descriptors within
the parent, which lead to collect2 holding an open input end of a
pipe, which of course meant that it would wait forever for end-of-data.

I'm currently running a full test cycle on i686-cygwin, but this has
passed a few LTO tests run by hand, which had been the path that lead
to the failure case described above.

Ok?


r~

[-- Attachment #2: d-spawn-3 --]
[-- Type: text/plain, Size: 9852 bytes --]

diff --git a/libiberty/config.in b/libiberty/config.in
index 02d93da..8500860 100644
--- a/libiberty/config.in
+++ b/libiberty/config.in
@@ -91,6 +91,9 @@
    don't. */
 #undef HAVE_DECL_VSNPRINTF
 
+/* Define to 1 if you have the `dup3' function. */
+#undef HAVE_DUP3
+
 /* Define to 1 if you have the <fcntl.h> header file. */
 #undef HAVE_FCNTL_H
 
@@ -169,6 +172,9 @@
 /* Define if you have prctl PR_SET_NAME */
 #undef HAVE_PRCTL_SET_NAME
 
+/* Define to 1 if you have the <process.h> header file. */
+#undef HAVE_PROCESS_H
+
 /* Define to 1 if you have the `psignal' function. */
 #undef HAVE_PSIGNAL
 
@@ -208,6 +214,9 @@
 /* Define to 1 if you have the `snprintf' function. */
 #undef HAVE_SNPRINTF
 
+/* Define to 1 if you have the `spawnvpe' function. */
+#undef HAVE_SPAWNVPE
+
 /* Define to 1 if you have the <stdint.h> header file. */
 #undef HAVE_STDINT_H
 
diff --git a/libiberty/configure b/libiberty/configure
index 7579000..1aad26f 100755
--- a/libiberty/configure
+++ b/libiberty/configure
@@ -4895,7 +4895,7 @@ host_makefile_frag=${frag}
 # It's OK to check for header files.  Although the compiler may not be
 # able to link anything, it had better be able to at least compile
 # something.
-for ac_header in sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h
+for ac_header in sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h process.h
 do :
   as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
 ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
@@ -5284,14 +5284,14 @@ vars="sys_errlist sys_nerr sys_siglist"
 
 checkfuncs="getrusage on_exit psignal strerror strsignal sysconf times sbrk gettimeofday"
 checkfuncs="$checkfuncs realpath canonicalize_file_name pstat_getstatic pstat_getdynamic sysmp"
-checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking"
+checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking dup3 spawnvpe"
 
 # These are neither executed nor required, but they help keep
 # autoheader happy without adding a bunch of text to acconfig.h.
 if test "x" = "y"; then
   for ac_func in asprintf atexit \
     basename bcmp bcopy bsearch bzero \
-    calloc canonicalize_file_name clock \
+    calloc canonicalize_file_name clock dup3 \
     ffs __fsetlocking \
     getcwd getpagesize getrusage getsysinfo gettimeofday \
     index insque \
@@ -5299,10 +5299,10 @@ if test "x" = "y"; then
     on_exit \
     psignal pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
-    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy strcasecmp strchr \
-    strdup \
+    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy \
+     strcasecmp strchr strdup \
      strerror strncasecmp strndup strrchr strsignal strstr strtod strtol \
-     strtoul strverscmp sysconf sysctl sysmp \
+     strtoul strverscmp sysconf sysctl sysmp spawnvpe \
     table times tmpnam \
     vasprintf vfprintf vprintf vsprintf \
     wait3 wait4 waitpid
diff --git a/libiberty/configure.ac b/libiberty/configure.ac
index 73ea6c9..34a1a7e 100644
--- a/libiberty/configure.ac
+++ b/libiberty/configure.ac
@@ -246,7 +246,7 @@ AC_SUBST_FILE(host_makefile_frag)
 # It's OK to check for header files.  Although the compiler may not be
 # able to link anything, it had better be able to at least compile
 # something.
-AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h)
+AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h process.h)
 AC_HEADER_SYS_WAIT
 AC_HEADER_TIME
 
@@ -359,14 +359,14 @@ vars="sys_errlist sys_nerr sys_siglist"
 
 checkfuncs="getrusage on_exit psignal strerror strsignal sysconf times sbrk gettimeofday"
 checkfuncs="$checkfuncs realpath canonicalize_file_name pstat_getstatic pstat_getdynamic sysmp"
-checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking"
+checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking dup3 spawnvpe"
 
 # These are neither executed nor required, but they help keep
 # autoheader happy without adding a bunch of text to acconfig.h.
 if test "x" = "y"; then
   AC_CHECK_FUNCS(asprintf atexit \
     basename bcmp bcopy bsearch bzero \
-    calloc canonicalize_file_name clock \
+    calloc canonicalize_file_name clock dup3 \
     ffs __fsetlocking \
     getcwd getpagesize getrusage getsysinfo gettimeofday \
     index insque \
@@ -374,10 +374,10 @@ if test "x" = "y"; then
     on_exit \
     psignal pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
-    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy strcasecmp strchr \
-    strdup \
+    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy \
+     strcasecmp strchr strdup \
      strerror strncasecmp strndup strrchr strsignal strstr strtod strtol \
-     strtoul strverscmp sysconf sysctl sysmp \
+     strtoul strverscmp sysconf sysctl sysmp spawnvpe \
     table times tmpnam \
     vasprintf vfprintf vprintf vsprintf \
     wait3 wait4 waitpid)
diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
index 85733a6..0727051 100644
--- a/libiberty/pex-unix.c
+++ b/libiberty/pex-unix.c
@@ -55,7 +55,9 @@ extern int errno;
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
-
+#ifdef HAVE_PROCESS_H
+#include <process.h>
+#endif
 
 #ifdef vfork /* Autoconf may define this to fork for us. */
 # define VFORK_STRING "fork"
@@ -387,6 +389,149 @@ pex_child_error (struct pex_obj *obj, const char *executable,
 
 extern char **environ;
 
+#ifdef HAVE_SPAWNVPE
+/* Helpers for saving file descriptors around spawning a child.  */
+
+static int
+save_and_install_fd(int *pflags, int old_fd, int child_fd)
+{
+  int new_fd, flags;
+
+  flags = fcntl (old_fd, F_GETFD);
+  if (child_fd == -1)
+    {
+      new_fd = old_fd;
+      if ((flags & FD_CLOEXEC) == 0
+	  && fcntl (old_fd, F_SETFD, FD_CLOEXEC) < 0)
+	return -1;
+    }
+  else
+    {
+      new_fd = fcntl (old_fd, F_DUPFD_CLOEXEC, 3);
+      if (new_fd < 0)
+	return -1;
+    }
+
+  *pflags = flags;
+  return new_fd;
+}
+
+static void
+restore_fd(int old_fd, int new_fd, int flags)
+{
+  if (new_fd == old_fd)
+    {
+      if ((flags & FD_CLOEXEC) == 0)
+	fcntl (old_fd, F_SETFD, flags);
+    }
+  else
+    {
+#ifdef HAVE_DUP3
+      if (flags == FD_CLOEXEC)
+	dup3 (new_fd, old_fd, O_CLOEXEC);
+      else
+#endif
+	{
+	  dup2 (new_fd, old_fd);
+	  if (flags != 0)
+	    fcntl (old_fd, F_SETFD, flags);
+	}
+      close (new_fd);
+    }
+}
+
+static pid_t
+pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
+		     int flags, const char *executable,
+		     char * const * argv, char * const * env,
+                     int in, int out, int errdes, int toclose,
+		     const char **errmsg, int *err)
+{
+  typedef const char * const *cc_cp;
+  int fl_in = 0, fl_out = 0, fl_err = 0, fl_tc = 0;
+  int save_in = -1, save_out = -1, save_err = -1;
+  int retries;
+  pid_t pid;
+
+  if (flags & PEX_STDERR_TO_STDOUT)
+    errdes = out;
+
+  if (in != STDIN_FILE_NO)
+    {
+      save_in = save_and_install_fd(&fl_in, STDIN_FILE_NO, in);
+      if (save_in < 0)
+	goto error_dup2;
+    }
+  if (out != STDOUT_FILE_NO)
+    {
+      save_out = save_and_install_fd(&fl_out, STDOUT_FILE_NO, out);
+      if (save_out < 0)
+	goto error_dup2;
+    }
+  if (errdes != STDERR_FILE_NO)
+    {
+      save_err = save_and_install_fd(&fl_err, STDIN_FILE_NO, errdes);
+      if (save_err < 0)
+	goto error_dup2;
+    }
+  if (toclose >= 0)
+    save_and_install_fd(&fl_tc, toclose, -1);
+
+  if (env == NULL)
+    env = environ;
+
+  retries = 0;
+  while (1)
+    {
+      if (flags & PEX_SEARCH)
+	pid = spawnvpe (_P_NOWAITO, executable, (cc_cp)argv, (cc_cp)env);
+      else
+	pid = spawnve (_P_NOWAITO, executable, (cc_cp)argv, (cc_cp)env);
+
+      if (pid > 0)
+	break;
+
+      *err = errno;
+      *errmsg = "spawn";
+      if (errno != EAGAIN || ++retries == 4)
+	return -1;
+      sleep (1 << retries);
+    }
+
+  if (toclose >= 0)
+    restore_fd (toclose, toclose, fl_tc);
+  if (in != STDIN_FILE_NO)
+    {
+      restore_fd (STDIN_FILE_NO, save_in, fl_in);
+      if (close (in) < 0)
+	goto error_close;
+    }
+  if (out != STDOUT_FILE_NO)
+    {
+      restore_fd (STDOUT_FILE_NO, save_out, fl_out);
+      if (close (out) < 0)
+	goto error_close;
+    }
+  if (errdes != STDERR_FILE_NO)
+    {
+      restore_fd (STDERR_FILE_NO, save_err, fl_err);
+      if (errdes != out && close (errdes) < 0)
+	goto error_close;
+    }
+
+  return pid;
+
+ error_dup2:
+  *err = errno;
+  *errmsg = "dup2";
+  return (pid_t) -1;
+
+ error_close:
+  *err = errno;
+  *errmsg = "close";
+  return (pid_t) -1;
+}
+#else
 static pid_t
 pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
 		     char * const * argv, char * const * env,
@@ -521,6 +666,7 @@ pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
       return pid;
     }
 }
+#endif /* SPAWN */
 
 /* Wait for a child process to complete.  */
 

[PATCH, v4] Use cygwin spawn functions

[Richard Henderson]

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

Version 4.  The last attempt, while it fixed the hanging due
to pipe fds left open, closed some file descriptors too early.
This one has actually passed the entire testsuite.

Ok?


r~

[-- Attachment #2: d-spawn-4 --]
[-- Type: text/plain, Size: 12072 bytes --]

diff --git a/libiberty/config.in b/libiberty/config.in
index 02d93da..8500860 100644
--- a/libiberty/config.in
+++ b/libiberty/config.in
@@ -91,6 +91,9 @@
    don't. */
 #undef HAVE_DECL_VSNPRINTF
 
+/* Define to 1 if you have the `dup3' function. */
+#undef HAVE_DUP3
+
 /* Define to 1 if you have the <fcntl.h> header file. */
 #undef HAVE_FCNTL_H
 
@@ -169,6 +172,9 @@
 /* Define if you have prctl PR_SET_NAME */
 #undef HAVE_PRCTL_SET_NAME
 
+/* Define to 1 if you have the <process.h> header file. */
+#undef HAVE_PROCESS_H
+
 /* Define to 1 if you have the `psignal' function. */
 #undef HAVE_PSIGNAL
 
@@ -208,6 +214,9 @@
 /* Define to 1 if you have the `snprintf' function. */
 #undef HAVE_SNPRINTF
 
+/* Define to 1 if you have the `spawnvpe' function. */
+#undef HAVE_SPAWNVPE
+
 /* Define to 1 if you have the <stdint.h> header file. */
 #undef HAVE_STDINT_H
 
diff --git a/libiberty/configure b/libiberty/configure
index 7579000..1aad26f 100755
--- a/libiberty/configure
+++ b/libiberty/configure
@@ -4895,7 +4895,7 @@ host_makefile_frag=${frag}
 # It's OK to check for header files.  Although the compiler may not be
 # able to link anything, it had better be able to at least compile
 # something.
-for ac_header in sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h
+for ac_header in sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h process.h
 do :
   as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
 ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
@@ -5284,14 +5284,14 @@ vars="sys_errlist sys_nerr sys_siglist"
 
 checkfuncs="getrusage on_exit psignal strerror strsignal sysconf times sbrk gettimeofday"
 checkfuncs="$checkfuncs realpath canonicalize_file_name pstat_getstatic pstat_getdynamic sysmp"
-checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking"
+checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking dup3 spawnvpe"
 
 # These are neither executed nor required, but they help keep
 # autoheader happy without adding a bunch of text to acconfig.h.
 if test "x" = "y"; then
   for ac_func in asprintf atexit \
     basename bcmp bcopy bsearch bzero \
-    calloc canonicalize_file_name clock \
+    calloc canonicalize_file_name clock dup3 \
     ffs __fsetlocking \
     getcwd getpagesize getrusage getsysinfo gettimeofday \
     index insque \
@@ -5299,10 +5299,10 @@ if test "x" = "y"; then
     on_exit \
     psignal pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
-    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy strcasecmp strchr \
-    strdup \
+    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy \
+     strcasecmp strchr strdup \
      strerror strncasecmp strndup strrchr strsignal strstr strtod strtol \
-     strtoul strverscmp sysconf sysctl sysmp \
+     strtoul strverscmp sysconf sysctl sysmp spawnvpe \
     table times tmpnam \
     vasprintf vfprintf vprintf vsprintf \
     wait3 wait4 waitpid
diff --git a/libiberty/configure.ac b/libiberty/configure.ac
index 73ea6c9..34a1a7e 100644
--- a/libiberty/configure.ac
+++ b/libiberty/configure.ac
@@ -246,7 +246,7 @@ AC_SUBST_FILE(host_makefile_frag)
 # It's OK to check for header files.  Although the compiler may not be
 # able to link anything, it had better be able to at least compile
 # something.
-AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h)
+AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h process.h)
 AC_HEADER_SYS_WAIT
 AC_HEADER_TIME
 
@@ -359,14 +359,14 @@ vars="sys_errlist sys_nerr sys_siglist"
 
 checkfuncs="getrusage on_exit psignal strerror strsignal sysconf times sbrk gettimeofday"
 checkfuncs="$checkfuncs realpath canonicalize_file_name pstat_getstatic pstat_getdynamic sysmp"
-checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking"
+checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking dup3 spawnvpe"
 
 # These are neither executed nor required, but they help keep
 # autoheader happy without adding a bunch of text to acconfig.h.
 if test "x" = "y"; then
   AC_CHECK_FUNCS(asprintf atexit \
     basename bcmp bcopy bsearch bzero \
-    calloc canonicalize_file_name clock \
+    calloc canonicalize_file_name clock dup3 \
     ffs __fsetlocking \
     getcwd getpagesize getrusage getsysinfo gettimeofday \
     index insque \
@@ -374,10 +374,10 @@ if test "x" = "y"; then
     on_exit \
     psignal pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
-    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy strcasecmp strchr \
-    strdup \
+    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy \
+     strcasecmp strchr strdup \
      strerror strncasecmp strndup strrchr strsignal strstr strtod strtol \
-     strtoul strverscmp sysconf sysctl sysmp \
+     strtoul strverscmp sysconf sysctl sysmp spawnvpe \
     table times tmpnam \
     vasprintf vfprintf vprintf vsprintf \
     wait3 wait4 waitpid)
diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
index 85733a6..ea828d3 100644
--- a/libiberty/pex-unix.c
+++ b/libiberty/pex-unix.c
@@ -55,7 +55,9 @@ extern int errno;
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
-
+#ifdef HAVE_PROCESS_H
+#include <process.h>
+#endif
 
 #ifdef vfork /* Autoconf may define this to fork for us. */
 # define VFORK_STRING "fork"
@@ -387,6 +389,191 @@ pex_child_error (struct pex_obj *obj, const char *executable,
 
 extern char **environ;
 
+#ifdef HAVE_SPAWNVPE
+/* Implementation of pex->exec_child using the Cygwin spawn operation.  */
+
+/* Subroutine of pex_unix_exec_child.  Move OLD_FD to a new file descriptor
+   to be stored in *PNEW_FD, save the flags in *PFLAGS, and arrange for the
+   saved copy to be close-on-exec.  Move CHILD_FD into OLD_FD.  If CHILD_FD
+   is -1, OLD_FD is to be closed.  Return -1 on error.  */
+
+static int
+save_and_install_fd(int *pnew_fd, int *pflags, int old_fd, int child_fd)
+{
+  int new_fd, flags;
+
+  flags = fcntl (old_fd, F_GETFD);
+
+  /* If we could not retrieve the flags, then OLD_FD was not open.  */
+  if (flags < 0)
+    {
+      new_fd = -1, flags = 0;
+      if (child_fd >= 0 && dup2 (child_fd, old_fd) < 0)
+	return -1;
+    }
+  /* If we wish to close OLD_FD, just mark it CLOEXEC.  */
+  else if (child_fd == -1)
+    {
+      new_fd = old_fd;
+      if ((flags & FD_CLOEXEC) == 0 && fcntl (old_fd, F_SETFD, FD_CLOEXEC) < 0)
+	return -1;
+    }
+  /* Otherwise we need to save a copy of OLD_FD before installing CHILD_FD.  */
+  else
+    {
+      new_fd = fcntl (old_fd, F_DUPFD_CLOEXEC, 3);
+      if (new_fd < 0)
+	return -1;
+      if (dup2 (child_fd, old_fd) < 0)
+	return -1;
+    }
+
+  *pflags = flags;
+  if (pnew_fd)
+    *pnew_fd = new_fd;
+  else if (new_fd != old_fd)
+    abort ();
+
+  return 0;
+}
+
+/* Subroutine of pex_unix_exec_child.  Move SAVE_FD back to OLD_FD
+   restoring FLAGS.  If SAVE_FD < 0, OLD_FD is to be closed.  */
+
+static int
+restore_fd(int old_fd, int save_fd, int flags)
+{
+  /* For SAVE_FD < 0, all we have to do is restore the
+     "closed-ness" of the original.  */
+  if (save_fd < 0)
+    return close (old_fd);
+
+  /* For SAVE_FD == OLD_FD, all we have to do is restore the
+     original setting of the CLOEXEC flag.  */
+  if (save_fd == old_fd)
+    {
+      if (flags & FD_CLOEXEC)
+	return 0;
+      return fcntl (old_fd, F_SETFD, flags);
+    }
+
+  /* Otherwise we have to move the descriptor back, restore the flags,
+     and close the saved copy.  */
+#ifdef HAVE_DUP3
+  if (flags == FD_CLOEXEC)
+    {
+      if (dup3 (save_fd, old_fd, O_CLOEXEC) < 0)
+	return -1;
+    }
+  else
+#endif
+    {
+      if (dup2 (save_fd, old_fd) < 0)
+	return -1;
+      if (flags != 0 && fcntl (old_fd, F_SETFD, flags) < 0)
+	return -1;
+    }
+  return close (save_fd);
+}
+
+static pid_t
+pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
+		     int flags, const char *executable,
+		     char * const * argv, char * const * env,
+                     int in, int out, int errdes, int toclose,
+		     const char **errmsg, int *err)
+{
+  typedef const char * const *cc_cp;
+  int fl_in = 0, fl_out = 0, fl_err = 0, fl_tc = 0;
+  int save_in = -1, save_out = -1, save_err = -1;
+  int max, retries;
+  pid_t pid;
+
+  if (flags & PEX_STDERR_TO_STDOUT)
+    errdes = out;
+
+  /* We need the three standard file descriptors to be set up as for
+     the child before we perform the spawn.  The file descriptors for
+     the parent need to be moved and marked for close-on-exec.  */
+  if (in != STDIN_FILE_NO
+      && save_and_install_fd (&save_in, &fl_in, STDIN_FILE_NO, in) < 0)
+    goto error_dup2;
+  if (out != STDOUT_FILE_NO
+      && save_and_install_fd (&save_out, &fl_out, STDOUT_FILE_NO, out) < 0)
+    goto error_dup2;
+  if (errdes != STDERR_FILE_NO
+      && save_and_install_fd (&save_err, &fl_err, STDERR_FILE_NO, errdes) < 0)
+    goto error_dup2;
+  if (toclose >= 0
+      && save_and_install_fd (NULL, &fl_tc, toclose, -1) < 0)
+    goto error_dup2;
+
+  /* Now that we've moved the file descriptors for the child into place,
+     close the originals.  Be careful not to close any of the standard
+     file descriptors that we just set up.  */
+  max = -1;
+  if (errdes >= 0)
+    max = STDERR_FILE_NO;
+  else if (out >= 0)
+    max = STDOUT_FILE_NO;
+  else if (in >= 0)
+    max = STDIN_FILE_NO;
+  if (in > max)
+    close (in);
+  if (out > max)
+    close (out);
+  if (errdes > max && errdes != out)
+    close (errdes);
+
+  /* If we were not given an environment, use the global environment.  */
+  if (env == NULL)
+    env = environ;
+
+  /* Launch the program.  If we get EAGAIN (normally out of pid's), try
+     again a few times with increasing backoff times.  */
+  retries = 0;
+  while (1)
+    {
+      if (flags & PEX_SEARCH)
+	pid = spawnvpe (_P_NOWAITO, executable, (cc_cp)argv, (cc_cp)env);
+      else
+	pid = spawnve (_P_NOWAITO, executable, (cc_cp)argv, (cc_cp)env);
+
+      if (pid > 0)
+	break;
+
+      *err = errno;
+      *errmsg = "spawn";
+      if (errno != EAGAIN || ++retries == 4)
+	return -1;
+      sleep (1 << retries);
+    }
+
+  /* Success.  Restore the parent's file descriptors that we saved above.  */
+  if (toclose >= 0
+      && restore_fd (toclose, toclose, fl_tc) < 0)
+    goto error_dup2;
+  if (in != STDIN_FILE_NO
+      && restore_fd (STDIN_FILE_NO, save_in, fl_in) < 0)
+    goto error_dup2;
+  if (out != STDOUT_FILE_NO
+      && restore_fd (STDOUT_FILE_NO, save_out, fl_out) < 0)
+    goto error_dup2;
+  if (errdes != STDERR_FILE_NO
+      && restore_fd (STDERR_FILE_NO, save_err, fl_err) < 0)
+    goto error_dup2;
+
+  return pid;
+
+ error_dup2:
+  *err = errno;
+  *errmsg = "dup2";
+  return (pid_t) -1;
+}
+
+#else
+/* Implementation of pex->exec_child using standard vfork + exec.  */
+
 static pid_t
 pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
 		     char * const * argv, char * const * env,
@@ -521,6 +708,7 @@ pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
       return pid;
     }
 }
+#endif /* SPAWN */
 
 /* Wait for a child process to complete.  */
 

Re: [PATCH, v4] Use cygwin spawn functions

[DJ Delorie]

I hope I'm reviewing the latest version... ;-)

-    calloc canonicalize_file_name clock \
+    calloc canonicalize_file_name clock dup3 \

dup3 would be on its own line.  They're grouped by starting letter.

-     strtoul strverscmp sysconf sysctl sysmp \
+     strtoul strverscmp sysconf sysctl sysmp spawnvpe \

Alphabetical please.

+  flags = fcntl (old_fd, F_GETFD);
+
+  /* If we could not retrieve the flags, then OLD_FD was not open.  */

Should we first check for old_fd < 0 ?

+static pid_t
+pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
+		     int flags, const char *executable,
+		     char * const * argv, char * const * env,
+                     int in, int out, int errdes, int toclose,
+		     const char **errmsg, int *err)

Note that DJGPP (pex-djgpp.c) also uses the spawn* functions, but
there are four - spawnv, spawnve, spawnvp, and spawnvpe.  Did you look
through pex-djgpp.c to see if there are other cases that it checks for
that this patch doesn't?

Look in setenv.c for how to deal with the "environ" variable.  You
can't just declare it.

Also, Microsoft deprecated spawn*() five years ago, replacing them
with _spawn*() instead.  Perhaps we should take the time to
future-proof this now, and prefer _spawn*() if present?

You call spawnve() but don't check for it in configure.  I've seen
implementations with some but not all exec*() calls, let's be paranoid
about spawn*() too.

Re: [PATCH, v4] Use cygwin spawn functions

[Richard Henderson]

On 10/19/2010 03:16 PM, DJ Delorie wrote:
> +  /* If we could not retrieve the flags, then OLD_FD was not open.  */
> 
> Should we first check for old_fd < 0 ?

It doesn't happen.  This function is called on the standard fds.

The "OLD_FD" was not open test is for when e.g. stderr is closed.
Which happened, somehow, in the testsuite.

> Note that DJGPP (pex-djgpp.c) also uses the spawn* functions, but
> there are four - spawnv, spawnve, spawnvp, and spawnvpe.  Did you look
> through pex-djgpp.c to see if there are other cases that it checks for
> that this patch doesn't?

No, they look fairly similar, except for the fact that DJGPP does
not have close-on-exec status to set or preserve around the spawn.

> Look in setenv.c for how to deal with the "environ" variable.  You
> can't just declare it.

I didn't add or change that.  It's pre-existing in that file.

> Also, Microsoft deprecated spawn*() five years ago, replacing them
> with _spawn*() instead.  Perhaps we should take the time to
> future-proof this now, and prefer _spawn*() if present?

*shrug* Today's Cygwin still does not have _spawn*.  Given that
this file is *not* used with mingw, and thus the Microsoft rtl,
it hardly seems worthwhile.

> You call spawnve() but don't check for it in configure.  I've seen
> implementations with some but not all exec*() calls, let's be paranoid
> about spawn*() too.

Ok.


r~

Re: [PATCH, v4] Use cygwin spawn functions

[DJ Delorie]

> Which happened, somehow, in the testsuite.

Ick?  ;-)

> > Look in setenv.c for how to deal with the "environ" variable.  You
> > can't just declare it.
> 
> I didn't add or change that.  It's pre-existing in that file.

Hmmm... so it was.

> *shrug* Today's Cygwin still does not have _spawn*.  Given that
> this file is *not* used with mingw, and thus the Microsoft rtl,
> it hardly seems worthwhile.

I suppose, until cygwin "catches up".  I suppose we can fix it then.

Re: [PATCH, v4] Use cygwin spawn functions

[Dave Korn]

On 16/10/2010 20:53, Richard Henderson wrote:
> Version 4.  The last attempt, while it fixed the hanging due
> to pipe fds left open, closed some file descriptors too early.
> This one has actually passed the entire testsuite.
> 
> Ok?

  :-( This caused a number of regressions in a native testsuite run on
i686-pc-cygwin.

> FAIL: g++.dg/parse/stack1.C (test for excess errors)

and

> FAIL: gcc.c-torture/compile/limits-declparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-declparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-declparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-exprparen.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-pointer.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-pointer.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-pointer.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)
> FAIL: gcc.c-torture/compile/limits-structnest.c (test for excess errors)

are the most noticeable ones.  The common factor appears to be that these are
all tests that involve deeply recursive heavy stack usage.  The bug only
arises when the driver is invoked to launch cc1plus.exe, I couldn't reproduce
it by invoking the language compiler directly from the shell.

  Interestingly enough, even the clean cc1plus build failed the stack1.C test
when I ran it under GDB.  For some reason the stack ended up at a really low
address; the default reservation of stack space for w32 console apps is 2MB,
and although there was that much space, there wasn't any further room for it
to expand down into, which there would have been if the stack had been located
at a more normal address.

  It might be suitable to address this in gcc by adding "-Wl,--stack,4194304"
or simlar to the host build flags.  Or there could be some kind of bug or
anomaly in cygwin's spawn function.  I won't have time to investigate further
in the next few days, I'm rushing to get another stage 1 patch in.

    cheers,
      DaveK

Re: [PATCH, v4] Use cygwin spawn functions

[Joseph S. Myers]

On Sat, 23 Oct 2010, Dave Korn wrote:

>   It might be suitable to address this in gcc by adding "-Wl,--stack,4194304"
> or simlar to the host build flags.  Or there could be some kind of bug or

Toplevel config/mh-{mingw,cygwin} already contain

LDFLAGS += -Wl,--stack,8388608

for this purpose.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH, v4] Use cygwin spawn functions

[Dave Korn]

On 23/10/2010 18:04, Joseph S. Myers wrote:
> On Sat, 23 Oct 2010, Dave Korn wrote:
> 
>>   It might be suitable to address this in gcc by adding "-Wl,--stack,4194304"
>> or simlar to the host build flags.  Or there could be some kind of bug or
> 
> Toplevel config/mh-{mingw,cygwin} already contain
> 
> LDFLAGS += -Wl,--stack,8388608
> 
> for this purpose.
> 

  It's not working:

> admin@ubik /gnu/gcc/obj-lto
> $ objdump -p gcc/cc1.exe | grep Stack
> SizeOfStackReserve      00200000
> SizeOfStackCommit       00001000
> 
> admin@ubik /gnu/gcc/obj-lto
> $

  (I'm sure we've had this discussion before in a java-related context.)

    cheers,
      DaveK

Re: [PATCH, v4] Use cygwin spawn functions

[Dave Korn]

On 23/10/2010 18:45, Dave Korn wrote:
> On 23/10/2010 18:04, Joseph S. Myers wrote:
>> On Sat, 23 Oct 2010, Dave Korn wrote:
>>
>>>   It might be suitable to address this in gcc by adding "-Wl,--stack,4194304"
>>> or simlar to the host build flags.  Or there could be some kind of bug or
>> Toplevel config/mh-{mingw,cygwin} already contain
>>
>> LDFLAGS += -Wl,--stack,8388608
>>
>> for this purpose.
>>
> 
>   It's not working:
> 
>> admin@ubik /gnu/gcc/obj-lto
>> $ objdump -p gcc/cc1.exe | grep Stack
>> SizeOfStackReserve      00200000
>> SizeOfStackCommit       00001000
>>
>> admin@ubik /gnu/gcc/obj-lto
>> $
> 
>   (I'm sure we've had this discussion before in a java-related context.)

  Yep, found it(*).  The top-level config/mh-* flags are only used for stage1,
that's why.

    cheers,
      DaveK
-- 
(*) - http://gcc.gnu.org/ml/gcc-patches/2009-04/msg00881.html
and related thread.

Re: [PATCH, v4] Use cygwin spawn functions

[Paolo Bonzini]

On 10/23/2010 07:54 PM, Dave Korn wrote:
>>> LDFLAGS += -Wl,--stack,8388608
>
> Yep, found it(*).  The top-level config/mh-* flags are only used for
> stage1, that's why.

Just add BOOT_LDFLAGS too.

Paolo

Re: [PATCH, v4] Use cygwin spawn functions

[Paolo Bonzini]

On 10/23/2010 07:54 PM, Dave Korn wrote:
>>> LDFLAGS += -Wl,--stack,8388608
>
> Yep, found it(*).  The top-level config/mh-* flags are only used for
> stage1, that's why.

Just add BOOT_LDFLAGS too.

Paolo

Re: [PATCH, v4] Use cygwin spawn functions

[Richard Henderson]

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

v4 -> v5
  * I believe I've addressed all of DJ's comments

  * Test for F_DUPFD_CLOEXEC, not present in cygwin 1.5, which the last version that
    works on Win98 and related systems.


Ok?


r~

[-- Attachment #2: d-spawn-5 --]
[-- Type: text/plain, Size: 12636 bytes --]

diff --git a/libiberty/config.in b/libiberty/config.in
index 02d93da..9b6dbd2 100644
--- a/libiberty/config.in
+++ b/libiberty/config.in
@@ -91,6 +91,9 @@
    don't. */
 #undef HAVE_DECL_VSNPRINTF
 
+/* Define to 1 if you have the `dup3' function. */
+#undef HAVE_DUP3
+
 /* Define to 1 if you have the <fcntl.h> header file. */
 #undef HAVE_FCNTL_H
 
@@ -169,6 +172,9 @@
 /* Define if you have prctl PR_SET_NAME */
 #undef HAVE_PRCTL_SET_NAME
 
+/* Define to 1 if you have the <process.h> header file. */
+#undef HAVE_PROCESS_H
+
 /* Define to 1 if you have the `psignal' function. */
 #undef HAVE_PSIGNAL
 
@@ -208,6 +214,12 @@
 /* Define to 1 if you have the `snprintf' function. */
 #undef HAVE_SNPRINTF
 
+/* Define to 1 if you have the `spawnve' function. */
+#undef HAVE_SPAWNVE
+
+/* Define to 1 if you have the `spawnvpe' function. */
+#undef HAVE_SPAWNVPE
+
 /* Define to 1 if you have the <stdint.h> header file. */
 #undef HAVE_STDINT_H
 
diff --git a/libiberty/configure b/libiberty/configure
index 142c81c..4b3426c 100755
--- a/libiberty/configure
+++ b/libiberty/configure
@@ -4895,7 +4895,7 @@ host_makefile_frag=${frag}
 # It's OK to check for header files.  Although the compiler may not be
 # able to link anything, it had better be able to at least compile
 # something.
-for ac_header in sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h
+for ac_header in sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h process.h
 do :
   as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
 ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
@@ -5282,9 +5282,10 @@ funcs="$funcs setproctitle"
 
 vars="sys_errlist sys_nerr sys_siglist"
 
-checkfuncs="getrusage on_exit psignal strerror strsignal sysconf times sbrk gettimeofday"
-checkfuncs="$checkfuncs realpath canonicalize_file_name pstat_getstatic pstat_getdynamic sysmp"
-checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking"
+checkfuncs="__fsetlocking canonicalize_file_name dup3 getrusage getsysinfo \
+ gettimeofday on_exit psignal pstat_getdynamic pstat_getstatic realpath \
+ sbrk spawnve spawnvpe strerror strsignal sysconf sysctl sysmp table \
+ times wait3 wait4"
 
 # These are neither executed nor required, but they help keep
 # autoheader happy without adding a bunch of text to acconfig.h.
@@ -5292,6 +5293,7 @@ if test "x" = "y"; then
   for ac_func in asprintf atexit \
     basename bcmp bcopy bsearch bzero \
     calloc canonicalize_file_name clock \
+    dup3 \
     ffs __fsetlocking \
     getcwd getpagesize getrusage getsysinfo gettimeofday \
     index insque \
@@ -5299,8 +5301,8 @@ if test "x" = "y"; then
     on_exit \
     psignal pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
-    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy strcasecmp strchr \
-    strdup \
+    sbrk setenv setproctitle sigsetmask snprintf spawnve spawnvpe \
+     stpcpy stpncpy strcasecmp strchr strdup \
      strerror strncasecmp strndup strrchr strsignal strstr strtod strtol \
      strtoul strverscmp sysconf sysctl sysmp \
     table times tmpnam \
diff --git a/libiberty/configure.ac b/libiberty/configure.ac
index 73ea6c9..df19b3c 100644
--- a/libiberty/configure.ac
+++ b/libiberty/configure.ac
@@ -246,7 +246,7 @@ AC_SUBST_FILE(host_makefile_frag)
 # It's OK to check for header files.  Although the compiler may not be
 # able to link anything, it had better be able to at least compile
 # something.
-AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h)
+AC_CHECK_HEADERS(sys/file.h sys/param.h limits.h stdlib.h malloc.h string.h unistd.h strings.h sys/time.h time.h sys/resource.h sys/stat.h sys/mman.h fcntl.h alloca.h sys/pstat.h sys/sysmp.h sys/sysinfo.h machine/hal_sysinfo.h sys/table.h sys/sysctl.h sys/systemcfg.h stdint.h stdio_ext.h process.h)
 AC_HEADER_SYS_WAIT
 AC_HEADER_TIME
 
@@ -357,9 +357,10 @@ funcs="$funcs setproctitle"
 
 vars="sys_errlist sys_nerr sys_siglist"
 
-checkfuncs="getrusage on_exit psignal strerror strsignal sysconf times sbrk gettimeofday"
-checkfuncs="$checkfuncs realpath canonicalize_file_name pstat_getstatic pstat_getdynamic sysmp"
-checkfuncs="$checkfuncs getsysinfo table sysctl wait3 wait4 __fsetlocking"
+checkfuncs="__fsetlocking canonicalize_file_name dup3 getrusage getsysinfo \
+ gettimeofday on_exit psignal pstat_getdynamic pstat_getstatic realpath \
+ sbrk spawnve spawnvpe strerror strsignal sysconf sysctl sysmp table \
+ times wait3 wait4"
 
 # These are neither executed nor required, but they help keep
 # autoheader happy without adding a bunch of text to acconfig.h.
@@ -367,6 +368,7 @@ if test "x" = "y"; then
   AC_CHECK_FUNCS(asprintf atexit \
     basename bcmp bcopy bsearch bzero \
     calloc canonicalize_file_name clock \
+    dup3 \
     ffs __fsetlocking \
     getcwd getpagesize getrusage getsysinfo gettimeofday \
     index insque \
@@ -374,8 +376,8 @@ if test "x" = "y"; then
     on_exit \
     psignal pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
-    sbrk setenv setproctitle sigsetmask snprintf stpcpy stpncpy strcasecmp strchr \
-    strdup \
+    sbrk setenv setproctitle sigsetmask snprintf spawnve spawnvpe \
+     stpcpy stpncpy strcasecmp strchr strdup \
      strerror strncasecmp strndup strrchr strsignal strstr strtod strtol \
      strtoul strverscmp sysconf sysctl sysmp \
     table times tmpnam \
diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
index 85733a6..c4a69ad 100644
--- a/libiberty/pex-unix.c
+++ b/libiberty/pex-unix.c
@@ -55,7 +55,9 @@ extern int errno;
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
-
+#ifdef HAVE_PROCESS_H
+#include <process.h>
+#endif
 
 #ifdef vfork /* Autoconf may define this to fork for us. */
 # define VFORK_STRING "fork"
@@ -387,6 +389,202 @@ pex_child_error (struct pex_obj *obj, const char *executable,
 
 extern char **environ;
 
+#if defined(HAVE_SPAWNVE) && defined(HAVE_SPAWNVPE)
+/* Implementation of pex->exec_child using the Cygwin spawn operation.  */
+
+/* Subroutine of pex_unix_exec_child.  Move OLD_FD to a new file descriptor
+   to be stored in *PNEW_FD, save the flags in *PFLAGS, and arrange for the
+   saved copy to be close-on-exec.  Move CHILD_FD into OLD_FD.  If CHILD_FD
+   is -1, OLD_FD is to be closed.  Return -1 on error.  */
+
+static int
+save_and_install_fd(int *pnew_fd, int *pflags, int old_fd, int child_fd)
+{
+  int new_fd, flags;
+
+  flags = fcntl (old_fd, F_GETFD);
+
+  /* If we could not retrieve the flags, then OLD_FD was not open.  */
+  if (flags < 0)
+    {
+      new_fd = -1, flags = 0;
+      if (child_fd >= 0 && dup2 (child_fd, old_fd) < 0)
+	return -1;
+    }
+  /* If we wish to close OLD_FD, just mark it CLOEXEC.  */
+  else if (child_fd == -1)
+    {
+      new_fd = old_fd;
+      if ((flags & FD_CLOEXEC) == 0 && fcntl (old_fd, F_SETFD, FD_CLOEXEC) < 0)
+	return -1;
+    }
+  /* Otherwise we need to save a copy of OLD_FD before installing CHILD_FD.  */
+  else
+    {
+#ifdef F_DUPFD_CLOEXEC
+      new_fd = fcntl (old_fd, F_DUPFD_CLOEXEC, 3);
+      if (new_fd < 0)
+	return -1;
+#else
+      /* Prefer F_DUPFD over dup in order to avoid getting a new fd
+	 in the range 0-2, right where a new stderr fd might get put.  */
+      new_fd = fcntl (old_fd, F_DUPFD, 3);
+      if (new_fd < 0)
+	return -1;
+      if (fcntl (new_fd, F_SETFD, FD_CLOEXEC) < 0)
+	return -1;
+#endif
+      if (dup2 (child_fd, old_fd) < 0)
+	return -1;
+    }
+
+  *pflags = flags;
+  if (pnew_fd)
+    *pnew_fd = new_fd;
+  else if (new_fd != old_fd)
+    abort ();
+
+  return 0;
+}
+
+/* Subroutine of pex_unix_exec_child.  Move SAVE_FD back to OLD_FD
+   restoring FLAGS.  If SAVE_FD < 0, OLD_FD is to be closed.  */
+
+static int
+restore_fd(int old_fd, int save_fd, int flags)
+{
+  /* For SAVE_FD < 0, all we have to do is restore the
+     "closed-ness" of the original.  */
+  if (save_fd < 0)
+    return close (old_fd);
+
+  /* For SAVE_FD == OLD_FD, all we have to do is restore the
+     original setting of the CLOEXEC flag.  */
+  if (save_fd == old_fd)
+    {
+      if (flags & FD_CLOEXEC)
+	return 0;
+      return fcntl (old_fd, F_SETFD, flags);
+    }
+
+  /* Otherwise we have to move the descriptor back, restore the flags,
+     and close the saved copy.  */
+#ifdef HAVE_DUP3
+  if (flags == FD_CLOEXEC)
+    {
+      if (dup3 (save_fd, old_fd, O_CLOEXEC) < 0)
+	return -1;
+    }
+  else
+#endif
+    {
+      if (dup2 (save_fd, old_fd) < 0)
+	return -1;
+      if (flags != 0 && fcntl (old_fd, F_SETFD, flags) < 0)
+	return -1;
+    }
+  return close (save_fd);
+}
+
+static pid_t
+pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
+		     int flags, const char *executable,
+		     char * const * argv, char * const * env,
+                     int in, int out, int errdes, int toclose,
+		     const char **errmsg, int *err)
+{
+  int fl_in = 0, fl_out = 0, fl_err = 0, fl_tc = 0;
+  int save_in = -1, save_out = -1, save_err = -1;
+  int max, retries;
+  pid_t pid;
+
+  if (flags & PEX_STDERR_TO_STDOUT)
+    errdes = out;
+
+  /* We need the three standard file descriptors to be set up as for
+     the child before we perform the spawn.  The file descriptors for
+     the parent need to be moved and marked for close-on-exec.  */
+  if (in != STDIN_FILE_NO
+      && save_and_install_fd (&save_in, &fl_in, STDIN_FILE_NO, in) < 0)
+    goto error_dup2;
+  if (out != STDOUT_FILE_NO
+      && save_and_install_fd (&save_out, &fl_out, STDOUT_FILE_NO, out) < 0)
+    goto error_dup2;
+  if (errdes != STDERR_FILE_NO
+      && save_and_install_fd (&save_err, &fl_err, STDERR_FILE_NO, errdes) < 0)
+    goto error_dup2;
+  if (toclose >= 0
+      && save_and_install_fd (NULL, &fl_tc, toclose, -1) < 0)
+    goto error_dup2;
+
+  /* Now that we've moved the file descriptors for the child into place,
+     close the originals.  Be careful not to close any of the standard
+     file descriptors that we just set up.  */
+  max = -1;
+  if (errdes >= 0)
+    max = STDERR_FILE_NO;
+  else if (out >= 0)
+    max = STDOUT_FILE_NO;
+  else if (in >= 0)
+    max = STDIN_FILE_NO;
+  if (in > max)
+    close (in);
+  if (out > max)
+    close (out);
+  if (errdes > max && errdes != out)
+    close (errdes);
+
+  /* If we were not given an environment, use the global environment.  */
+  if (env == NULL)
+    env = environ;
+
+  /* Launch the program.  If we get EAGAIN (normally out of pid's), try
+     again a few times with increasing backoff times.  */
+  retries = 0;
+  while (1)
+    {
+      typedef const char * const *cc_cp;
+
+      if (flags & PEX_SEARCH)
+	pid = spawnvpe (_P_NOWAITO, executable, (cc_cp)argv, (cc_cp)env);
+      else
+	pid = spawnve (_P_NOWAITO, executable, (cc_cp)argv, (cc_cp)env);
+
+      if (pid > 0)
+	break;
+
+      *err = errno;
+      *errmsg = "spawn";
+      if (errno != EAGAIN || ++retries == 4)
+	return (pid_t) -1;
+      sleep (1 << retries);
+    }
+
+  /* Success.  Restore the parent's file descriptors that we saved above.  */
+  if (toclose >= 0
+      && restore_fd (toclose, toclose, fl_tc) < 0)
+    goto error_dup2;
+  if (in != STDIN_FILE_NO
+      && restore_fd (STDIN_FILE_NO, save_in, fl_in) < 0)
+    goto error_dup2;
+  if (out != STDOUT_FILE_NO
+      && restore_fd (STDOUT_FILE_NO, save_out, fl_out) < 0)
+    goto error_dup2;
+  if (errdes != STDERR_FILE_NO
+      && restore_fd (STDERR_FILE_NO, save_err, fl_err) < 0)
+    goto error_dup2;
+
+  return pid;
+
+ error_dup2:
+  *err = errno;
+  *errmsg = "dup2";
+  return (pid_t) -1;
+}
+
+#else
+/* Implementation of pex->exec_child using standard vfork + exec.  */
+
 static pid_t
 pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
 		     char * const * argv, char * const * env,
@@ -521,6 +719,7 @@ pex_unix_exec_child (struct pex_obj *obj, int flags, const char *executable,
       return pid;
     }
 }
+#endif /* SPAWN */
 
 /* Wait for a child process to complete.  */
 

Re: [PATCH, v4] Use cygwin spawn functions

[Dave Korn]

On 30/10/2010 03:46, Richard Henderson wrote:
> v4 -> v5
>   * I believe I've addressed all of DJ's comments
> 
>   * Test for F_DUPFD_CLOEXEC, not present in cygwin 1.5, which the last version that
>     works on Win98 and related systems.
> 
> 
> Ok?

  I still had a bunch of regressions in the limits-exprparen.c compile
tests(*), even after turning up the default stack size to 8MB, sigh.  For some
reason using spawn rather than fork appears to result in the stack getting
allocated down at the lowest end of memory where it doesn't have as much room
to grow beyond the default allocation as it otherwise might if it were located
higher up.

  Maybe, given that this is potentially troublesome, we should have a
configure option to en/disable it?  DJ, would you in principle accept a
follow-on patch making the use of spawn configurable by a --enable option?
(And if so, do you have a preferred name you'd like used for it?)

    cheers,
      DaveK

Re: [PATCH, v4] Use cygwin spawn functions

[Richard Henderson]

On 10/30/2010 04:04 AM, Dave Korn wrote:
> On 30/10/2010 03:46, Richard Henderson wrote:
>> v4 -> v5
>>   * I believe I've addressed all of DJ's comments
>>
>>   * Test for F_DUPFD_CLOEXEC, not present in cygwin 1.5, which the last version that
>>     works on Win98 and related systems.
>>
>>
>> Ok?
> 
>   I still had a bunch of regressions in the limits-exprparen.c compile
> tests(*), even after turning up the default stack size to 8MB, sigh. 

That test fails on linux as well:

  http://gcc.gnu.org/ml/gcc-testresults/2010-10/msg02408.html


r~

Re: [PATCH, v4] Use cygwin spawn functions

[Dave Korn]

On 30/10/2010 19:08, Richard Henderson wrote:
> On 10/30/2010 04:04 AM, Dave Korn wrote:
>> On 30/10/2010 03:46, Richard Henderson wrote:
>>> v4 -> v5
>>>   * I believe I've addressed all of DJ's comments
>>>
>>>   * Test for F_DUPFD_CLOEXEC, not present in cygwin 1.5, which the last version that
>>>     works on Win98 and related systems.
>>>
>>>
>>> Ok?
>>   I still had a bunch of regressions in the limits-exprparen.c compile
>> tests(*), even after turning up the default stack size to 8MB, sigh. 
> 
> That test fails on linux as well:
> 
>   http://gcc.gnu.org/ml/gcc-testresults/2010-10/msg02408.html
> 
> 
> r~

  Oh, I didn't think to check that!  It started failing for me at just the
same moment as I applied the patch, along with a bunch of the other limits-*
tests; I assumed they were all caused by the same change.

    cheers,
      DaveK

Re: [PATCH, v4] Use cygwin spawn functions

[DJ Delorie]

I'm OK with this one if you've appeased Dave's concerns about the
might-not-be-regressions.

Re: [PATCH, v4] Use cygwin spawn functions

[Dave Korn]

On 04/11/2010 20:43, DJ Delorie wrote:
> I'm OK with this one if you've appeased Dave's concerns about the
> might-not-be-regressions.

  Yeh, let it go in.  If I think there's a problem, I'll send a patch to make
it configure-time selectable.

    cheers,
      DaveK