[PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Brendan Shanks]

Hi,

This patch implements pex_unix_exec_child using posix_spawn when
available.

This should especially benefit recent macOS (where vfork just calls
fork), but should have equivalent or faster performance on all
platforms.
In addition, the implementation is substantially simpler than the
vfork+exec code path.

Tested on x86_64-linux.

v2: Fix error handling (previously the function would be run twice in
case of error), and don't use a macro that changes control flow.

v3: Match file style for error-handling blocks, don't close
in/out/errdes on error, and check close() for errors.

libiberty/
	* configure.ac (AC_CHECK_HEADERS): Add spawn.h.
	(checkfuncs): Add posix_spawn, posix_spawnp.
	(AC_CHECK_FUNCS): Add posix_spawn, posix_spawnp.
	* configure, config.in: Rebuild.
	* pex-unix.c [HAVE_POSIX_SPAWN] (pex_unix_exec_child): New function.

Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
---
 libiberty/configure.ac |   8 +-
 libiberty/pex-unix.c   | 168 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+), 3 deletions(-)

diff --git a/libiberty/configure.ac b/libiberty/configure.ac
index 0748c592704..2488b031bc8 100644
--- a/libiberty/configure.ac
+++ b/libiberty/configure.ac
@@ -289,7 +289,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 process.h sys/prctl.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 sys/prctl.h spawn.h)
 AC_HEADER_SYS_WAIT
 AC_HEADER_TIME
 
@@ -412,7 +412,8 @@ funcs="$funcs setproctitle"
 vars="sys_errlist sys_nerr sys_siglist"
 
 checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
- getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
+ getsysinfo gettimeofday on_exit pipe2 posix_spawn posix_spawnp psignal \
+ pstat_getdynamic pstat_getstatic \
  realpath setrlimit spawnve spawnvpe strerror strsignal sysconf sysctl \
  sysmp table times wait3 wait4"
 
@@ -435,7 +436,8 @@ if test "x" = "y"; then
     index insque \
     memchr memcmp memcpy memmem memmove memset mkstemps \
     on_exit \
-    pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
+    pipe2 posix_spawn posix_spawnp psignal \
+    pstat_getdynamic pstat_getstatic putenv \
     random realpath rename rindex \
     sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
      stpcpy stpncpy strcasecmp strchr strdup \
diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
index 33b5bce31c2..336799d1125 100644
--- a/libiberty/pex-unix.c
+++ b/libiberty/pex-unix.c
@@ -58,6 +58,9 @@ extern int errno;
 #ifdef HAVE_PROCESS_H
 #include <process.h>
 #endif
+#ifdef HAVE_SPAWN_H
+#include <spawn.h>
+#endif
 
 #ifdef vfork /* Autoconf may define this to fork for us. */
 # define VFORK_STRING "fork"
@@ -559,6 +562,171 @@ pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
   return (pid_t) -1;
 }
 
+#elif defined(HAVE_POSIX_SPAWN) && defined(HAVE_POSIX_SPAWNP)
+/* Implementation of pex->exec_child using posix_spawn.            */
+
+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 ret;
+  pid_t pid = -1;
+  posix_spawnattr_t attr;
+  posix_spawn_file_actions_t actions;
+  int attr_initialized = 0, actions_initialized = 0;
+
+  *err = 0;
+
+  ret = posix_spawnattr_init (&attr);
+  if (ret)
+    {
+      *err = ret;
+      *errmsg = "posix_spawnattr_init";
+      goto exit;
+    }
+  attr_initialized = 1;
+
+  /* Use vfork() on glibc <=2.24. */
+#ifdef POSIX_SPAWN_USEVFORK
+  ret = posix_spawnattr_setflags (&attr, POSIX_SPAWN_USEVFORK);
+  if (ret)
+    {
+      *err = ret;
+      *errmsg = "posix_spawnattr_setflags";
+      goto exit;
+    }
+#endif
+
+  ret = posix_spawn_file_actions_init (&actions);
+  if (ret)
+    {
+      *err = ret;
+      *errmsg = "posix_spawn_file_actions_init";
+      goto exit;
+    }
+  actions_initialized = 1;
+
+  if (in != STDIN_FILE_NO)
+    {
+      ret = posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILE_NO);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_adddup2";
+          goto exit;
+        }
+
+      ret = posix_spawn_file_actions_addclose (&actions, in);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_addclose";
+          goto exit;
+        }
+    }
+
+  if (out != STDOUT_FILE_NO)
+    {
+      ret = posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILE_NO);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_adddup2";
+          goto exit;
+        }
+
+      ret = posix_spawn_file_actions_addclose (&actions, out);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_addclose";
+          goto exit;
+        }
+    }
+
+  if (errdes != STDERR_FILE_NO)
+    {
+      ret = posix_spawn_file_actions_adddup2 (&actions, errdes, STDERR_FILE_NO);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_adddup2";
+          goto exit;
+        }
+
+      ret = posix_spawn_file_actions_addclose (&actions, errdes);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_addclose";
+          goto exit;
+        }
+    }
+
+  if (toclose >= 0)
+    {
+      ret = posix_spawn_file_actions_addclose (&actions, toclose);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_addclose";
+          goto exit;
+        }
+    }
+
+  if ((flags & PEX_STDERR_TO_STDOUT) != 0)
+    {
+      ret = posix_spawn_file_actions_adddup2 (&actions, STDOUT_FILE_NO, STDERR_FILE_NO);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn_file_actions_adddup2";
+          goto exit;
+        }
+    }
+
+  if ((flags & PEX_SEARCH) != 0)
+    {
+      ret = posix_spawnp (&pid, executable, &actions, &attr, argv, env ? env : environ);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawnp";
+          goto exit;
+        }
+    }
+  else
+    {
+      ret = posix_spawn (&pid, executable, &actions, &attr, argv, env ? env : environ);
+      if (ret)
+        {
+          *err = ret;
+          *errmsg = "posix_spawn";
+          goto exit;
+        }
+    }
+
+exit:
+  if (actions_initialized)
+    posix_spawn_file_actions_destroy (&actions);
+  if (attr_initialized)
+    posix_spawnattr_destroy (&attr);
+
+  if (!*err && in != STDIN_FILE_NO)
+    if (close (in))
+      *errmsg = "close", *err = errno, pid = -1;
+  if (!*err && out != STDOUT_FILE_NO)
+    if (close (out))
+      *errmsg = "close", *err = errno, pid = -1;
+  if (!*err && errdes != STDERR_FILE_NO)
+    if (close (errdes))
+      *errmsg = "close", *err = errno, pid = -1;
+
+  return pid;
+}
 #else
 /* Implementation of pex->exec_child using standard vfork + exec.  */
 
-- 
2.41.0

Ping: [PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Brendan Shanks]

Polite ping on this.

> On Oct 4, 2023, at 11:28 AM, Brendan Shanks <bshanks@codeweavers.com> wrote:
> 
> Hi,
> 
> This patch implements pex_unix_exec_child using posix_spawn when
> available.
> 
> This should especially benefit recent macOS (where vfork just calls
> fork), but should have equivalent or faster performance on all
> platforms.
> In addition, the implementation is substantially simpler than the
> vfork+exec code path.
> 
> Tested on x86_64-linux.
> 
> v2: Fix error handling (previously the function would be run twice in
> case of error), and don't use a macro that changes control flow.
> 
> v3: Match file style for error-handling blocks, don't close
> in/out/errdes on error, and check close() for errors.
> 
> libiberty/
> * configure.ac (AC_CHECK_HEADERS): Add spawn.h.
> (checkfuncs): Add posix_spawn, posix_spawnp.
> (AC_CHECK_FUNCS): Add posix_spawn, posix_spawnp.
> * configure, config.in: Rebuild.
> * pex-unix.c [HAVE_POSIX_SPAWN] (pex_unix_exec_child): New function.
> 
> Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
> ---
> libiberty/configure.ac |   8 +-
> libiberty/pex-unix.c   | 168 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 173 insertions(+), 3 deletions(-)
> 
> diff --git a/libiberty/configure.ac b/libiberty/configure.ac
> index 0748c592704..2488b031bc8 100644
> --- a/libiberty/configure.ac
> +++ b/libiberty/configure.ac
> @@ -289,7 +289,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 process.h sys/prctl.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 sys/prctl.h spawn.h)
> AC_HEADER_SYS_WAIT
> AC_HEADER_TIME
> 
> @@ -412,7 +412,8 @@ funcs="$funcs setproctitle"
> vars="sys_errlist sys_nerr sys_siglist"
> 
> checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
> - getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
> + getsysinfo gettimeofday on_exit pipe2 posix_spawn posix_spawnp psignal \
> + pstat_getdynamic pstat_getstatic \
>  realpath setrlimit spawnve spawnvpe strerror strsignal sysconf sysctl \
>  sysmp table times wait3 wait4"
> 
> @@ -435,7 +436,8 @@ if test "x" = "y"; then
>     index insque \
>     memchr memcmp memcpy memmem memmove memset mkstemps \
>     on_exit \
> -    pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
> +    pipe2 posix_spawn posix_spawnp psignal \
> +    pstat_getdynamic pstat_getstatic putenv \
>     random realpath rename rindex \
>     sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
>      stpcpy stpncpy strcasecmp strchr strdup \
> diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
> index 33b5bce31c2..336799d1125 100644
> --- a/libiberty/pex-unix.c
> +++ b/libiberty/pex-unix.c
> @@ -58,6 +58,9 @@ extern int errno;
> #ifdef HAVE_PROCESS_H
> #include <process.h>
> #endif
> +#ifdef HAVE_SPAWN_H
> +#include <spawn.h>
> +#endif
> 
> #ifdef vfork /* Autoconf may define this to fork for us. */
> # define VFORK_STRING "fork"
> @@ -559,6 +562,171 @@ pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
>   return (pid_t) -1;
> }
> 
> +#elif defined(HAVE_POSIX_SPAWN) && defined(HAVE_POSIX_SPAWNP)
> +/* Implementation of pex->exec_child using posix_spawn.            */
> +
> +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 ret;
> +  pid_t pid = -1;
> +  posix_spawnattr_t attr;
> +  posix_spawn_file_actions_t actions;
> +  int attr_initialized = 0, actions_initialized = 0;
> +
> +  *err = 0;
> +
> +  ret = posix_spawnattr_init (&attr);
> +  if (ret)
> +    {
> +      *err = ret;
> +      *errmsg = "posix_spawnattr_init";
> +      goto exit;
> +    }
> +  attr_initialized = 1;
> +
> +  /* Use vfork() on glibc <=2.24. */
> +#ifdef POSIX_SPAWN_USEVFORK
> +  ret = posix_spawnattr_setflags (&attr, POSIX_SPAWN_USEVFORK);
> +  if (ret)
> +    {
> +      *err = ret;
> +      *errmsg = "posix_spawnattr_setflags";
> +      goto exit;
> +    }
> +#endif
> +
> +  ret = posix_spawn_file_actions_init (&actions);
> +  if (ret)
> +    {
> +      *err = ret;
> +      *errmsg = "posix_spawn_file_actions_init";
> +      goto exit;
> +    }
> +  actions_initialized = 1;
> +
> +  if (in != STDIN_FILE_NO)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +
> +      ret = posix_spawn_file_actions_addclose (&actions, in);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if (out != STDOUT_FILE_NO)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +
> +      ret = posix_spawn_file_actions_addclose (&actions, out);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if (errdes != STDERR_FILE_NO)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, errdes, STDERR_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +
> +      ret = posix_spawn_file_actions_addclose (&actions, errdes);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if (toclose >= 0)
> +    {
> +      ret = posix_spawn_file_actions_addclose (&actions, toclose);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if ((flags & PEX_STDERR_TO_STDOUT) != 0)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, STDOUT_FILE_NO, STDERR_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +    }
> +
> +  if ((flags & PEX_SEARCH) != 0)
> +    {
> +      ret = posix_spawnp (&pid, executable, &actions, &attr, argv, env ? env : environ);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawnp";
> +          goto exit;
> +        }
> +    }
> +  else
> +    {
> +      ret = posix_spawn (&pid, executable, &actions, &attr, argv, env ? env : environ);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn";
> +          goto exit;
> +        }
> +    }
> +
> +exit:
> +  if (actions_initialized)
> +    posix_spawn_file_actions_destroy (&actions);
> +  if (attr_initialized)
> +    posix_spawnattr_destroy (&attr);
> +
> +  if (!*err && in != STDIN_FILE_NO)
> +    if (close (in))
> +      *errmsg = "close", *err = errno, pid = -1;
> +  if (!*err && out != STDOUT_FILE_NO)
> +    if (close (out))
> +      *errmsg = "close", *err = errno, pid = -1;
> +  if (!*err && errdes != STDERR_FILE_NO)
> +    if (close (errdes))
> +      *errmsg = "close", *err = errno, pid = -1;
> +
> +  return pid;
> +}
> #else
> /* Implementation of pex->exec_child using standard vfork + exec.  */
> 
> -- 
> 2.41.0
> 

Re: Ping: [PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Richard Biener]

On Wed, Nov 1, 2023 at 7:16 PM Brendan Shanks <bshanks@codeweavers.com> wrote:
>
> Polite ping on this.

OK.

Thanks,
Richard.

> > On Oct 4, 2023, at 11:28 AM, Brendan Shanks <bshanks@codeweavers.com> wrote:
> >
> > Hi,
> >
> > This patch implements pex_unix_exec_child using posix_spawn when
> > available.
> >
> > This should especially benefit recent macOS (where vfork just calls
> > fork), but should have equivalent or faster performance on all
> > platforms.
> > In addition, the implementation is substantially simpler than the
> > vfork+exec code path.
> >
> > Tested on x86_64-linux.
> >
> > v2: Fix error handling (previously the function would be run twice in
> > case of error), and don't use a macro that changes control flow.
> >
> > v3: Match file style for error-handling blocks, don't close
> > in/out/errdes on error, and check close() for errors.
> >
> > libiberty/
> > * configure.ac (AC_CHECK_HEADERS): Add spawn.h.
> > (checkfuncs): Add posix_spawn, posix_spawnp.
> > (AC_CHECK_FUNCS): Add posix_spawn, posix_spawnp.
> > * configure, config.in: Rebuild.
> > * pex-unix.c [HAVE_POSIX_SPAWN] (pex_unix_exec_child): New function.
> >
> > Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
> > ---
> > libiberty/configure.ac |   8 +-
> > libiberty/pex-unix.c   | 168 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 173 insertions(+), 3 deletions(-)
> >
> > diff --git a/libiberty/configure.ac b/libiberty/configure.ac
> > index 0748c592704..2488b031bc8 100644
> > --- a/libiberty/configure.ac
> > +++ b/libiberty/configure.ac
> > @@ -289,7 +289,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 process.h sys/prctl.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 sys/prctl.h spawn.h)
> > AC_HEADER_SYS_WAIT
> > AC_HEADER_TIME
> >
> > @@ -412,7 +412,8 @@ funcs="$funcs setproctitle"
> > vars="sys_errlist sys_nerr sys_siglist"
> >
> > checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
> > - getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
> > + getsysinfo gettimeofday on_exit pipe2 posix_spawn posix_spawnp psignal \
> > + pstat_getdynamic pstat_getstatic \
> >  realpath setrlimit spawnve spawnvpe strerror strsignal sysconf sysctl \
> >  sysmp table times wait3 wait4"
> >
> > @@ -435,7 +436,8 @@ if test "x" = "y"; then
> >     index insque \
> >     memchr memcmp memcpy memmem memmove memset mkstemps \
> >     on_exit \
> > -    pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
> > +    pipe2 posix_spawn posix_spawnp psignal \
> > +    pstat_getdynamic pstat_getstatic putenv \
> >     random realpath rename rindex \
> >     sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
> >      stpcpy stpncpy strcasecmp strchr strdup \
> > diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
> > index 33b5bce31c2..336799d1125 100644
> > --- a/libiberty/pex-unix.c
> > +++ b/libiberty/pex-unix.c
> > @@ -58,6 +58,9 @@ extern int errno;
> > #ifdef HAVE_PROCESS_H
> > #include <process.h>
> > #endif
> > +#ifdef HAVE_SPAWN_H
> > +#include <spawn.h>
> > +#endif
> >
> > #ifdef vfork /* Autoconf may define this to fork for us. */
> > # define VFORK_STRING "fork"
> > @@ -559,6 +562,171 @@ pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
> >   return (pid_t) -1;
> > }
> >
> > +#elif defined(HAVE_POSIX_SPAWN) && defined(HAVE_POSIX_SPAWNP)
> > +/* Implementation of pex->exec_child using posix_spawn.            */
> > +
> > +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 ret;
> > +  pid_t pid = -1;
> > +  posix_spawnattr_t attr;
> > +  posix_spawn_file_actions_t actions;
> > +  int attr_initialized = 0, actions_initialized = 0;
> > +
> > +  *err = 0;
> > +
> > +  ret = posix_spawnattr_init (&attr);
> > +  if (ret)
> > +    {
> > +      *err = ret;
> > +      *errmsg = "posix_spawnattr_init";
> > +      goto exit;
> > +    }
> > +  attr_initialized = 1;
> > +
> > +  /* Use vfork() on glibc <=2.24. */
> > +#ifdef POSIX_SPAWN_USEVFORK
> > +  ret = posix_spawnattr_setflags (&attr, POSIX_SPAWN_USEVFORK);
> > +  if (ret)
> > +    {
> > +      *err = ret;
> > +      *errmsg = "posix_spawnattr_setflags";
> > +      goto exit;
> > +    }
> > +#endif
> > +
> > +  ret = posix_spawn_file_actions_init (&actions);
> > +  if (ret)
> > +    {
> > +      *err = ret;
> > +      *errmsg = "posix_spawn_file_actions_init";
> > +      goto exit;
> > +    }
> > +  actions_initialized = 1;
> > +
> > +  if (in != STDIN_FILE_NO)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +
> > +      ret = posix_spawn_file_actions_addclose (&actions, in);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if (out != STDOUT_FILE_NO)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +
> > +      ret = posix_spawn_file_actions_addclose (&actions, out);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if (errdes != STDERR_FILE_NO)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, errdes, STDERR_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +
> > +      ret = posix_spawn_file_actions_addclose (&actions, errdes);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if (toclose >= 0)
> > +    {
> > +      ret = posix_spawn_file_actions_addclose (&actions, toclose);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if ((flags & PEX_STDERR_TO_STDOUT) != 0)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, STDOUT_FILE_NO, STDERR_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if ((flags & PEX_SEARCH) != 0)
> > +    {
> > +      ret = posix_spawnp (&pid, executable, &actions, &attr, argv, env ? env : environ);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawnp";
> > +          goto exit;
> > +        }
> > +    }
> > +  else
> > +    {
> > +      ret = posix_spawn (&pid, executable, &actions, &attr, argv, env ? env : environ);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +exit:
> > +  if (actions_initialized)
> > +    posix_spawn_file_actions_destroy (&actions);
> > +  if (attr_initialized)
> > +    posix_spawnattr_destroy (&attr);
> > +
> > +  if (!*err && in != STDIN_FILE_NO)
> > +    if (close (in))
> > +      *errmsg = "close", *err = errno, pid = -1;
> > +  if (!*err && out != STDOUT_FILE_NO)
> > +    if (close (out))
> > +      *errmsg = "close", *err = errno, pid = -1;
> > +  if (!*err && errdes != STDERR_FILE_NO)
> > +    if (close (errdes))
> > +      *errmsg = "close", *err = errno, pid = -1;
> > +
> > +  return pid;
> > +}
> > #else
> > /* Implementation of pex->exec_child using standard vfork + exec.  */
> >
> > --
> > 2.41.0
> >
>

Re: [PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Jeff Law]

On 10/4/23 12:28, Brendan Shanks wrote:
> Hi,
> 
> This patch implements pex_unix_exec_child using posix_spawn when
> available.
> 
> This should especially benefit recent macOS (where vfork just calls
> fork), but should have equivalent or faster performance on all
> platforms.
> In addition, the implementation is substantially simpler than the
> vfork+exec code path.
> 
> Tested on x86_64-linux.
> 
> v2: Fix error handling (previously the function would be run twice in
> case of error), and don't use a macro that changes control flow.
> 
> v3: Match file style for error-handling blocks, don't close
> in/out/errdes on error, and check close() for errors.
> 
> libiberty/
> 	* configure.ac (AC_CHECK_HEADERS): Add spawn.h.
> 	(checkfuncs): Add posix_spawn, posix_spawnp.
> 	(AC_CHECK_FUNCS): Add posix_spawn, posix_spawnp.
> 	* configure, config.in: Rebuild.
> 	* pex-unix.c [HAVE_POSIX_SPAWN] (pex_unix_exec_child): New function.
Thanks.  I pushed this based on Richi's ACK after fixing some minor 
whitespace problems and rebuilding the generated files.

Sorry about the long delay.

jeff

Re: [PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Prathamesh Kulkarni]

On Thu, 5 Oct 2023 at 00:00, Brendan Shanks <bshanks@codeweavers.com> wrote:
>
> Hi,
>
> This patch implements pex_unix_exec_child using posix_spawn when
> available.
>
> This should especially benefit recent macOS (where vfork just calls
> fork), but should have equivalent or faster performance on all
> platforms.
> In addition, the implementation is substantially simpler than the
> vfork+exec code path.
>
> Tested on x86_64-linux.
Hi Brendan,
It seems this patch caused the following regressions on aarch64:

FAIL: g++.dg/modules/bad-mapper-1.C -std=c++17  at line 3 (test for
errors, line )
FAIL: g++.dg/modules/bad-mapper-1.C -std=c++17 (test for excess errors)
FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2a  at line 3 (test for
errors, line )
FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2a (test for excess errors)
FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2b  at line 3 (test for
errors, line )
FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2b (test for excess errors)

Looking at g++.log:
/home/tcwg-buildslave/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/testsuite/g++.dg/modules/bad-mapper-1.C:
error: failed posix_spawnp mapper 'this-will-not-work'
In module imported at
/home/tcwg-buildslave/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/testsuite/g++.dg/modules/bad-mapper-1.C:2:1:
unique1.bob: error: failed to read compiled module: No such file or directory
unique1.bob: note: compiled module file is 'gcm.cache/unique1.bob.gcm'
unique1.bob: note: imports must be built before being imported
unique1.bob: fatal error: returning to the gate for a mechanical issue
compilation terminated.

Link to log files:
https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-build/1159/artifact/artifacts/00-sumfiles/
Could you please investigate ?

Thanks,
Prathamesh
>
> v2: Fix error handling (previously the function would be run twice in
> case of error), and don't use a macro that changes control flow.
>
> v3: Match file style for error-handling blocks, don't close
> in/out/errdes on error, and check close() for errors.
>
> libiberty/
>         * configure.ac (AC_CHECK_HEADERS): Add spawn.h.
>         (checkfuncs): Add posix_spawn, posix_spawnp.
>         (AC_CHECK_FUNCS): Add posix_spawn, posix_spawnp.
>         * configure, config.in: Rebuild.
>         * pex-unix.c [HAVE_POSIX_SPAWN] (pex_unix_exec_child): New function.
>
> Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
> ---
>  libiberty/configure.ac |   8 +-
>  libiberty/pex-unix.c   | 168 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 173 insertions(+), 3 deletions(-)
>
> diff --git a/libiberty/configure.ac b/libiberty/configure.ac
> index 0748c592704..2488b031bc8 100644
> --- a/libiberty/configure.ac
> +++ b/libiberty/configure.ac
> @@ -289,7 +289,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 process.h sys/prctl.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 sys/prctl.h spawn.h)
>  AC_HEADER_SYS_WAIT
>  AC_HEADER_TIME
>
> @@ -412,7 +412,8 @@ funcs="$funcs setproctitle"
>  vars="sys_errlist sys_nerr sys_siglist"
>
>  checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
> - getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
> + getsysinfo gettimeofday on_exit pipe2 posix_spawn posix_spawnp psignal \
> + pstat_getdynamic pstat_getstatic \
>   realpath setrlimit spawnve spawnvpe strerror strsignal sysconf sysctl \
>   sysmp table times wait3 wait4"
>
> @@ -435,7 +436,8 @@ if test "x" = "y"; then
>      index insque \
>      memchr memcmp memcpy memmem memmove memset mkstemps \
>      on_exit \
> -    pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
> +    pipe2 posix_spawn posix_spawnp psignal \
> +    pstat_getdynamic pstat_getstatic putenv \
>      random realpath rename rindex \
>      sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
>       stpcpy stpncpy strcasecmp strchr strdup \
> diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
> index 33b5bce31c2..336799d1125 100644
> --- a/libiberty/pex-unix.c
> +++ b/libiberty/pex-unix.c
> @@ -58,6 +58,9 @@ extern int errno;
>  #ifdef HAVE_PROCESS_H
>  #include <process.h>
>  #endif
> +#ifdef HAVE_SPAWN_H
> +#include <spawn.h>
> +#endif
>
>  #ifdef vfork /* Autoconf may define this to fork for us. */
>  # define VFORK_STRING "fork"
> @@ -559,6 +562,171 @@ pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
>    return (pid_t) -1;
>  }
>
> +#elif defined(HAVE_POSIX_SPAWN) && defined(HAVE_POSIX_SPAWNP)
> +/* Implementation of pex->exec_child using posix_spawn.            */
> +
> +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 ret;
> +  pid_t pid = -1;
> +  posix_spawnattr_t attr;
> +  posix_spawn_file_actions_t actions;
> +  int attr_initialized = 0, actions_initialized = 0;
> +
> +  *err = 0;
> +
> +  ret = posix_spawnattr_init (&attr);
> +  if (ret)
> +    {
> +      *err = ret;
> +      *errmsg = "posix_spawnattr_init";
> +      goto exit;
> +    }
> +  attr_initialized = 1;
> +
> +  /* Use vfork() on glibc <=2.24. */
> +#ifdef POSIX_SPAWN_USEVFORK
> +  ret = posix_spawnattr_setflags (&attr, POSIX_SPAWN_USEVFORK);
> +  if (ret)
> +    {
> +      *err = ret;
> +      *errmsg = "posix_spawnattr_setflags";
> +      goto exit;
> +    }
> +#endif
> +
> +  ret = posix_spawn_file_actions_init (&actions);
> +  if (ret)
> +    {
> +      *err = ret;
> +      *errmsg = "posix_spawn_file_actions_init";
> +      goto exit;
> +    }
> +  actions_initialized = 1;
> +
> +  if (in != STDIN_FILE_NO)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +
> +      ret = posix_spawn_file_actions_addclose (&actions, in);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if (out != STDOUT_FILE_NO)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +
> +      ret = posix_spawn_file_actions_addclose (&actions, out);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if (errdes != STDERR_FILE_NO)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, errdes, STDERR_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +
> +      ret = posix_spawn_file_actions_addclose (&actions, errdes);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if (toclose >= 0)
> +    {
> +      ret = posix_spawn_file_actions_addclose (&actions, toclose);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_addclose";
> +          goto exit;
> +        }
> +    }
> +
> +  if ((flags & PEX_STDERR_TO_STDOUT) != 0)
> +    {
> +      ret = posix_spawn_file_actions_adddup2 (&actions, STDOUT_FILE_NO, STDERR_FILE_NO);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn_file_actions_adddup2";
> +          goto exit;
> +        }
> +    }
> +
> +  if ((flags & PEX_SEARCH) != 0)
> +    {
> +      ret = posix_spawnp (&pid, executable, &actions, &attr, argv, env ? env : environ);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawnp";
> +          goto exit;
> +        }
> +    }
> +  else
> +    {
> +      ret = posix_spawn (&pid, executable, &actions, &attr, argv, env ? env : environ);
> +      if (ret)
> +        {
> +          *err = ret;
> +          *errmsg = "posix_spawn";
> +          goto exit;
> +        }
> +    }
> +
> +exit:
> +  if (actions_initialized)
> +    posix_spawn_file_actions_destroy (&actions);
> +  if (attr_initialized)
> +    posix_spawnattr_destroy (&attr);
> +
> +  if (!*err && in != STDIN_FILE_NO)
> +    if (close (in))
> +      *errmsg = "close", *err = errno, pid = -1;
> +  if (!*err && out != STDOUT_FILE_NO)
> +    if (close (out))
> +      *errmsg = "close", *err = errno, pid = -1;
> +  if (!*err && errdes != STDERR_FILE_NO)
> +    if (close (errdes))
> +      *errmsg = "close", *err = errno, pid = -1;
> +
> +  return pid;
> +}
>  #else
>  /* Implementation of pex->exec_child using standard vfork + exec.  */
>
> --
> 2.41.0
>

Re: [PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Richard Biener]

On Fri, Nov 10, 2023 at 12:01 PM Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>
> On Thu, 5 Oct 2023 at 00:00, Brendan Shanks <bshanks@codeweavers.com> wrote:
> >
> > Hi,
> >
> > This patch implements pex_unix_exec_child using posix_spawn when
> > available.
> >
> > This should especially benefit recent macOS (where vfork just calls
> > fork), but should have equivalent or faster performance on all
> > platforms.
> > In addition, the implementation is substantially simpler than the
> > vfork+exec code path.
> >
> > Tested on x86_64-linux.
> Hi Brendan,
> It seems this patch caused the following regressions on aarch64:
>
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++17  at line 3 (test for
> errors, line )
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++17 (test for excess errors)
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2a  at line 3 (test for
> errors, line )
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2a (test for excess errors)
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2b  at line 3 (test for
> errors, line )
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2b (test for excess errors)
>
> Looking at g++.log:
> /home/tcwg-buildslave/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/testsuite/g++.dg/modules/bad-mapper-1.C:
> error: failed posix_spawnp mapper 'this-will-not-work'
> In module imported at
> /home/tcwg-buildslave/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/testsuite/g++.dg/modules/bad-mapper-1.C:2:1:
> unique1.bob: error: failed to read compiled module: No such file or directory
> unique1.bob: note: compiled module file is 'gcm.cache/unique1.bob.gcm'
> unique1.bob: note: imports must be built before being imported
> unique1.bob: fatal error: returning to the gate for a mechanical issue
> compilation terminated.
>
> Link to log files:
> https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-build/1159/artifact/artifacts/00-sumfiles/
> Could you please investigate ?

The testcase needs adjustment, it looks for

// { dg-error "-:failed (exec|CreateProcess).*mapper.*
.*this-will-not-work" "" { target { ! { *-*-darwin[89]* *-*-darwin10*
} } } 0 }

adding |posix_spawnp probably works

>
> Thanks,
> Prathamesh
> >
> > v2: Fix error handling (previously the function would be run twice in
> > case of error), and don't use a macro that changes control flow.
> >
> > v3: Match file style for error-handling blocks, don't close
> > in/out/errdes on error, and check close() for errors.
> >
> > libiberty/
> >         * configure.ac (AC_CHECK_HEADERS): Add spawn.h.
> >         (checkfuncs): Add posix_spawn, posix_spawnp.
> >         (AC_CHECK_FUNCS): Add posix_spawn, posix_spawnp.
> >         * configure, config.in: Rebuild.
> >         * pex-unix.c [HAVE_POSIX_SPAWN] (pex_unix_exec_child): New function.
> >
> > Signed-off-by: Brendan Shanks <bshanks@codeweavers.com>
> > ---
> >  libiberty/configure.ac |   8 +-
> >  libiberty/pex-unix.c   | 168 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 173 insertions(+), 3 deletions(-)
> >
> > diff --git a/libiberty/configure.ac b/libiberty/configure.ac
> > index 0748c592704..2488b031bc8 100644
> > --- a/libiberty/configure.ac
> > +++ b/libiberty/configure.ac
> > @@ -289,7 +289,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 process.h sys/prctl.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 sys/prctl.h spawn.h)
> >  AC_HEADER_SYS_WAIT
> >  AC_HEADER_TIME
> >
> > @@ -412,7 +412,8 @@ funcs="$funcs setproctitle"
> >  vars="sys_errlist sys_nerr sys_siglist"
> >
> >  checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
> > - getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
> > + getsysinfo gettimeofday on_exit pipe2 posix_spawn posix_spawnp psignal \
> > + pstat_getdynamic pstat_getstatic \
> >   realpath setrlimit spawnve spawnvpe strerror strsignal sysconf sysctl \
> >   sysmp table times wait3 wait4"
> >
> > @@ -435,7 +436,8 @@ if test "x" = "y"; then
> >      index insque \
> >      memchr memcmp memcpy memmem memmove memset mkstemps \
> >      on_exit \
> > -    pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
> > +    pipe2 posix_spawn posix_spawnp psignal \
> > +    pstat_getdynamic pstat_getstatic putenv \
> >      random realpath rename rindex \
> >      sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
> >       stpcpy stpncpy strcasecmp strchr strdup \
> > diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
> > index 33b5bce31c2..336799d1125 100644
> > --- a/libiberty/pex-unix.c
> > +++ b/libiberty/pex-unix.c
> > @@ -58,6 +58,9 @@ extern int errno;
> >  #ifdef HAVE_PROCESS_H
> >  #include <process.h>
> >  #endif
> > +#ifdef HAVE_SPAWN_H
> > +#include <spawn.h>
> > +#endif
> >
> >  #ifdef vfork /* Autoconf may define this to fork for us. */
> >  # define VFORK_STRING "fork"
> > @@ -559,6 +562,171 @@ pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
> >    return (pid_t) -1;
> >  }
> >
> > +#elif defined(HAVE_POSIX_SPAWN) && defined(HAVE_POSIX_SPAWNP)
> > +/* Implementation of pex->exec_child using posix_spawn.            */
> > +
> > +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 ret;
> > +  pid_t pid = -1;
> > +  posix_spawnattr_t attr;
> > +  posix_spawn_file_actions_t actions;
> > +  int attr_initialized = 0, actions_initialized = 0;
> > +
> > +  *err = 0;
> > +
> > +  ret = posix_spawnattr_init (&attr);
> > +  if (ret)
> > +    {
> > +      *err = ret;
> > +      *errmsg = "posix_spawnattr_init";
> > +      goto exit;
> > +    }
> > +  attr_initialized = 1;
> > +
> > +  /* Use vfork() on glibc <=2.24. */
> > +#ifdef POSIX_SPAWN_USEVFORK
> > +  ret = posix_spawnattr_setflags (&attr, POSIX_SPAWN_USEVFORK);
> > +  if (ret)
> > +    {
> > +      *err = ret;
> > +      *errmsg = "posix_spawnattr_setflags";
> > +      goto exit;
> > +    }
> > +#endif
> > +
> > +  ret = posix_spawn_file_actions_init (&actions);
> > +  if (ret)
> > +    {
> > +      *err = ret;
> > +      *errmsg = "posix_spawn_file_actions_init";
> > +      goto exit;
> > +    }
> > +  actions_initialized = 1;
> > +
> > +  if (in != STDIN_FILE_NO)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +
> > +      ret = posix_spawn_file_actions_addclose (&actions, in);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if (out != STDOUT_FILE_NO)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +
> > +      ret = posix_spawn_file_actions_addclose (&actions, out);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if (errdes != STDERR_FILE_NO)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, errdes, STDERR_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +
> > +      ret = posix_spawn_file_actions_addclose (&actions, errdes);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if (toclose >= 0)
> > +    {
> > +      ret = posix_spawn_file_actions_addclose (&actions, toclose);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_addclose";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if ((flags & PEX_STDERR_TO_STDOUT) != 0)
> > +    {
> > +      ret = posix_spawn_file_actions_adddup2 (&actions, STDOUT_FILE_NO, STDERR_FILE_NO);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn_file_actions_adddup2";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +  if ((flags & PEX_SEARCH) != 0)
> > +    {
> > +      ret = posix_spawnp (&pid, executable, &actions, &attr, argv, env ? env : environ);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawnp";
> > +          goto exit;
> > +        }
> > +    }
> > +  else
> > +    {
> > +      ret = posix_spawn (&pid, executable, &actions, &attr, argv, env ? env : environ);
> > +      if (ret)
> > +        {
> > +          *err = ret;
> > +          *errmsg = "posix_spawn";
> > +          goto exit;
> > +        }
> > +    }
> > +
> > +exit:
> > +  if (actions_initialized)
> > +    posix_spawn_file_actions_destroy (&actions);
> > +  if (attr_initialized)
> > +    posix_spawnattr_destroy (&attr);
> > +
> > +  if (!*err && in != STDIN_FILE_NO)
> > +    if (close (in))
> > +      *errmsg = "close", *err = errno, pid = -1;
> > +  if (!*err && out != STDOUT_FILE_NO)
> > +    if (close (out))
> > +      *errmsg = "close", *err = errno, pid = -1;
> > +  if (!*err && errdes != STDERR_FILE_NO)
> > +    if (close (errdes))
> > +      *errmsg = "close", *err = errno, pid = -1;
> > +
> > +  return pid;
> > +}
> >  #else
> >  /* Implementation of pex->exec_child using standard vfork + exec.  */
> >
> > --
> > 2.41.0
> >

Re: [PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Patrick O'Neill]

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

On 11/10/23 03:00, Prathamesh Kulkarni wrote:

> On Thu, 5 Oct 2023 at 00:00, Brendan Shanks<bshanks@codeweavers.com>  wrote:
>> Hi,
>>
>> This patch implements pex_unix_exec_child using posix_spawn when
>> available.
>>
>> This should especially benefit recent macOS (where vfork just calls
>> fork), but should have equivalent or faster performance on all
>> platforms.
>> In addition, the implementation is substantially simpler than the
>> vfork+exec code path.
>>
>> Tested on x86_64-linux.
> Hi Brendan,
> It seems this patch caused the following regressions on aarch64:

I'm also seeing the same failures on risc-v targets bisected to this commit.

Thanks,
Patrick

>
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++17  at line 3 (test for
> errors, line )
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++17 (test for excess errors)
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2a  at line 3 (test for
> errors, line )
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2a (test for excess errors)
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2b  at line 3 (test for
> errors, line )
> FAIL: g++.dg/modules/bad-mapper-1.C -std=c++2b (test for excess errors)
>
> Looking at g++.log:
> /home/tcwg-buildslave/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/testsuite/g++.dg/modules/bad-mapper-1.C:
> error: failed posix_spawnp mapper 'this-will-not-work'
> In module imported at
> /home/tcwg-buildslave/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/testsuite/g++.dg/modules/bad-mapper-1.C:2:1:
> unique1.bob: error: failed to read compiled module: No such file or directory
> unique1.bob: note: compiled module file is 'gcm.cache/unique1.bob.gcm'
> unique1.bob: note: imports must be built before being imported
> unique1.bob: fatal error: returning to the gate for a mechanical issue
> compilation terminated.
>
> Link to log files:
> https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-build/1159/artifact/artifacts/00-sumfiles/
> Could you please investigate ?
>
> Thanks,
> Prathamesh
>> v2: Fix error handling (previously the function would be run twice in
>> case of error), and don't use a macro that changes control flow.
>>
>> v3: Match file style for error-handling blocks, don't close
>> in/out/errdes on error, and check close() for errors.
>>
>> libiberty/
>>          * configure.ac (AC_CHECK_HEADERS): Add spawn.h.
>>          (checkfuncs): Add posix_spawn, posix_spawnp.
>>          (AC_CHECK_FUNCS): Add posix_spawn, posix_spawnp.
>>          * configure, config.in: Rebuild.
>>          * pex-unix.c [HAVE_POSIX_SPAWN] (pex_unix_exec_child): New function.
>>
>> Signed-off-by: Brendan Shanks<bshanks@codeweavers.com>
>> ---
>>   libiberty/configure.ac |   8 +-
>>   libiberty/pex-unix.c   | 168 +++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 173 insertions(+), 3 deletions(-)
>>
>> diff --git a/libiberty/configure.ac b/libiberty/configure.ac
>> index 0748c592704..2488b031bc8 100644
>> --- a/libiberty/configure.ac
>> +++ b/libiberty/configure.ac
>> @@ -289,7 +289,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 process.h sys/prctl.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 sys/prctl.h spawn.h)
>>   AC_HEADER_SYS_WAIT
>>   AC_HEADER_TIME
>>
>> @@ -412,7 +412,8 @@ funcs="$funcs setproctitle"
>>   vars="sys_errlist sys_nerr sys_siglist"
>>
>>   checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
>> - getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
>> + getsysinfo gettimeofday on_exit pipe2 posix_spawn posix_spawnp psignal \
>> + pstat_getdynamic pstat_getstatic \
>>    realpath setrlimit spawnve spawnvpe strerror strsignal sysconf sysctl \
>>    sysmp table times wait3 wait4"
>>
>> @@ -435,7 +436,8 @@ if test "x" = "y"; then
>>       index insque \
>>       memchr memcmp memcpy memmem memmove memset mkstemps \
>>       on_exit \
>> -    pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
>> +    pipe2 posix_spawn posix_spawnp psignal \
>> +    pstat_getdynamic pstat_getstatic putenv \
>>       random realpath rename rindex \
>>       sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
>>        stpcpy stpncpy strcasecmp strchr strdup \
>> diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
>> index 33b5bce31c2..336799d1125 100644
>> --- a/libiberty/pex-unix.c
>> +++ b/libiberty/pex-unix.c
>> @@ -58,6 +58,9 @@ extern int errno;
>>   #ifdef HAVE_PROCESS_H
>>   #include <process.h>
>>   #endif
>> +#ifdef HAVE_SPAWN_H
>> +#include <spawn.h>
>> +#endif
>>
>>   #ifdef vfork /* Autoconf may define this to fork for us. */
>>   # define VFORK_STRING "fork"
>> @@ -559,6 +562,171 @@ pex_unix_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED,
>>     return (pid_t) -1;
>>   }
>>
>> +#elif defined(HAVE_POSIX_SPAWN) && defined(HAVE_POSIX_SPAWNP)
>> +/* Implementation of pex->exec_child using posix_spawn.            */
>> +
>> +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 ret;
>> +  pid_t pid = -1;
>> +  posix_spawnattr_t attr;
>> +  posix_spawn_file_actions_t actions;
>> +  int attr_initialized = 0, actions_initialized = 0;
>> +
>> +  *err = 0;
>> +
>> +  ret = posix_spawnattr_init (&attr);
>> +  if (ret)
>> +    {
>> +      *err = ret;
>> +      *errmsg = "posix_spawnattr_init";
>> +      goto exit;
>> +    }
>> +  attr_initialized = 1;
>> +
>> +  /* Use vfork() on glibc <=2.24. */
>> +#ifdef POSIX_SPAWN_USEVFORK
>> +  ret = posix_spawnattr_setflags (&attr, POSIX_SPAWN_USEVFORK);
>> +  if (ret)
>> +    {
>> +      *err = ret;
>> +      *errmsg = "posix_spawnattr_setflags";
>> +      goto exit;
>> +    }
>> +#endif
>> +
>> +  ret = posix_spawn_file_actions_init (&actions);
>> +  if (ret)
>> +    {
>> +      *err = ret;
>> +      *errmsg = "posix_spawn_file_actions_init";
>> +      goto exit;
>> +    }
>> +  actions_initialized = 1;
>> +
>> +  if (in != STDIN_FILE_NO)
>> +    {
>> +      ret = posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILE_NO);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_adddup2";
>> +          goto exit;
>> +        }
>> +
>> +      ret = posix_spawn_file_actions_addclose (&actions, in);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_addclose";
>> +          goto exit;
>> +        }
>> +    }
>> +
>> +  if (out != STDOUT_FILE_NO)
>> +    {
>> +      ret = posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILE_NO);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_adddup2";
>> +          goto exit;
>> +        }
>> +
>> +      ret = posix_spawn_file_actions_addclose (&actions, out);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_addclose";
>> +          goto exit;
>> +        }
>> +    }
>> +
>> +  if (errdes != STDERR_FILE_NO)
>> +    {
>> +      ret = posix_spawn_file_actions_adddup2 (&actions, errdes, STDERR_FILE_NO);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_adddup2";
>> +          goto exit;
>> +        }
>> +
>> +      ret = posix_spawn_file_actions_addclose (&actions, errdes);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_addclose";
>> +          goto exit;
>> +        }
>> +    }
>> +
>> +  if (toclose >= 0)
>> +    {
>> +      ret = posix_spawn_file_actions_addclose (&actions, toclose);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_addclose";
>> +          goto exit;
>> +        }
>> +    }
>> +
>> +  if ((flags & PEX_STDERR_TO_STDOUT) != 0)
>> +    {
>> +      ret = posix_spawn_file_actions_adddup2 (&actions, STDOUT_FILE_NO, STDERR_FILE_NO);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn_file_actions_adddup2";
>> +          goto exit;
>> +        }
>> +    }
>> +
>> +  if ((flags & PEX_SEARCH) != 0)
>> +    {
>> +      ret = posix_spawnp (&pid, executable, &actions, &attr, argv, env ? env : environ);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawnp";
>> +          goto exit;
>> +        }
>> +    }
>> +  else
>> +    {
>> +      ret = posix_spawn (&pid, executable, &actions, &attr, argv, env ? env : environ);
>> +      if (ret)
>> +        {
>> +          *err = ret;
>> +          *errmsg = "posix_spawn";
>> +          goto exit;
>> +        }
>> +    }
>> +
>> +exit:
>> +  if (actions_initialized)
>> +    posix_spawn_file_actions_destroy (&actions);
>> +  if (attr_initialized)
>> +    posix_spawnattr_destroy (&attr);
>> +
>> +  if (!*err && in != STDIN_FILE_NO)
>> +    if (close (in))
>> +      *errmsg = "close", *err = errno, pid = -1;
>> +  if (!*err && out != STDOUT_FILE_NO)
>> +    if (close (out))
>> +      *errmsg = "close", *err = errno, pid = -1;
>> +  if (!*err && errdes != STDERR_FILE_NO)
>> +    if (close (errdes))
>> +      *errmsg = "close", *err = errno, pid = -1;
>> +
>> +  return pid;
>> +}
>>   #else
>>   /* Implementation of pex->exec_child using standard vfork + exec.  */
>>
>> --
>> 2.41.0
>>

Re: [PATCH v3] libiberty: Use posix_spawn in pex-unix when available.

[Jeff Law]

On 11/10/23 17:22, Patrick O'Neill wrote:
> On 11/10/23 03:00, Prathamesh Kulkarni wrote:
> 
>> On Thu, 5 Oct 2023 at 00:00, Brendan Shanks<bshanks@codeweavers.com>  wrote:
>>> Hi,
>>>
>>> This patch implements pex_unix_exec_child using posix_spawn when
>>> available.
>>>
>>> This should especially benefit recent macOS (where vfork just calls
>>> fork), but should have equivalent or faster performance on all
>>> platforms.
>>> In addition, the implementation is substantially simpler than the
>>> vfork+exec code path.
>>>
>>> Tested on x86_64-linux.
>> Hi Brendan,
>> It seems this patch caused the following regressions on aarch64:
> 
> I'm also seeing the same failures on risc-v targets bisected to this commit.
My tester flagged it on ppc64le.

I've got Richi's testsuite adjustment in my local tree for testing. 
Figure results on ppc64le sometime tomorrow.


jeff