On Fri, Nov 10, 2023 at 12:01 PM Prathamesh Kulkarni
<prathamesh.kulka...@linaro.org> wrote:
>
> On Thu, 5 Oct 2023 at 00:00, Brendan Shanks <bsha...@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 <bsha...@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
> >
