This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[libiberty] Use pipe inside pex_run
- From: Nathan Sidwell <nathan at acm dot org>
- To: Ian Lance Taylor <iant at google dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Martin Liška <mliska at suse dot cz>
- Date: Mon, 1 Oct 2018 13:53:30 -0400
- Subject: [libiberty] Use pipe inside pex_run
Ian,
this patch implements the pipe error channel you suggested a while back.
Before the (v)fork we create a pipe and set it up for CLOEXEC. If
exec failure happens in the child, we write errno & the fnname to the
pipe. In the parent we attempt to read the pipe. We'll get EOF if the
child was successful. Otherwise we get the error and fn, which we pass
back to our caller. As both child and parent are in the same address
space, it's perfectly fine to pass string literal addresses down the pipe.
An example of the difference is the behaviour of:
./xg++ -Bbogus ...
before the patch we get:
xg++: error trying to exec 'cc1plus': execvp: No such file or directory
with the patch we get:
xg++: fatal error: cannot execute ‘cc1plus’: execvp: No such file or
directory
compilation terminated.
Martin has been kind enough to test this patch in his profiled-bootstrap
build, and I've tested on AIX (where vfork is fork) as well asx86_64-linux.
nathan
--
Nathan Sidwell
2018-10-01 Nathan Sidwell <nathan@acm.org>
* configure.ac (checkfuncs): Add pipe2.
* config.in, configure: Rebuilt.
* pex-unix.c (pex_unix_exec_child): Comminicate errors from child
to parent with a pipe, when possible.
Index: config.in
===================================================================
--- config.in (revision 264744)
+++ config.in (working copy)
@@ -195,6 +195,9 @@
/* Define to 1 if you have the `on_exit' function. */
#undef HAVE_ON_EXIT
+/* Define to 1 if you have the `pipe2' function. */
+#undef HAVE_PIPE2
+
/* Define to 1 if you have the <process.h> header file. */
#undef HAVE_PROCESS_H
Index: configure
===================================================================
--- configure (revision 264744)
+++ configure (working copy)
@@ -5727,7 +5727,7 @@ funcs="$funcs setproctitle"
vars="sys_errlist sys_nerr sys_siglist"
checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
- getsysinfo gettimeofday on_exit psignal pstat_getdynamic pstat_getstatic \
+ getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
realpath setrlimit sbrk spawnve spawnvpe strerror strsignal sysconf sysctl \
sysmp table times wait3 wait4"
@@ -5743,7 +5743,7 @@ if test "x" = "y"; then
index insque \
memchr memcmp memcpy memmem memmove memset mkstemps \
on_exit \
- psignal pstat_getdynamic pstat_getstatic putenv \
+ pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
random realpath rename rindex \
sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
stpcpy stpncpy strcasecmp strchr strdup \
Index: configure.ac
===================================================================
--- configure.ac (revision 264744)
+++ configure.ac (working copy)
@@ -391,7 +391,7 @@ funcs="$funcs setproctitle"
vars="sys_errlist sys_nerr sys_siglist"
checkfuncs="__fsetlocking canonicalize_file_name dup3 getrlimit getrusage \
- getsysinfo gettimeofday on_exit psignal pstat_getdynamic pstat_getstatic \
+ getsysinfo gettimeofday on_exit pipe2 psignal pstat_getdynamic pstat_getstatic \
realpath setrlimit sbrk spawnve spawnvpe strerror strsignal sysconf sysctl \
sysmp table times wait3 wait4"
@@ -407,7 +407,7 @@ if test "x" = "y"; then
index insque \
memchr memcmp memcpy memmem memmove memset mkstemps \
on_exit \
- psignal pstat_getdynamic pstat_getstatic putenv \
+ pipe2 psignal pstat_getdynamic pstat_getstatic putenv \
random realpath rename rindex \
sbrk setenv setproctitle setrlimit sigsetmask snprintf spawnve spawnvpe \
stpcpy stpncpy strcasecmp strchr strdup \
Index: pex-unix.c
===================================================================
--- pex-unix.c (revision 264744)
+++ pex-unix.c (working copy)
@@ -569,6 +569,38 @@ pex_unix_exec_child (struct pex_obj *obj
int toclose, const char **errmsg, int *err)
{
pid_t pid = -1;
+ /* Tuple to communicate error from child to parent. We can safely
+ transfer string literal pointers as both run with identical
+ address mappings. */
+ struct fn_err
+ {
+ const char *fn;
+ int err;
+ };
+ volatile int do_pipe = 0;
+ volatile int pipes[2]; /* [0]:reader,[1]:writer. */
+#ifdef O_CLOEXEC
+ do_pipe = 1;
+#endif
+ if (do_pipe)
+ {
+#ifdef HAVE_PIPE2
+ if (pipe2 ((int *)pipes, O_CLOEXEC))
+ do_pipe = 0;
+#else
+ if (pipe ((int *)pipes))
+ do_pipe = 0;
+ else
+ {
+ if (fcntl (pipes[1], F_SETFD, FD_CLOEXEC) == -1)
+ {
+ close (pipes[0]);
+ close (pipes[1]);
+ do_pipe = 0;
+ }
+ }
+#endif
+ }
/* We declare these to be volatile to avoid warnings from gcc about
them being clobbered by vfork. */
@@ -579,8 +611,9 @@ pex_unix_exec_child (struct pex_obj *obj
This clobbers the parent's environ so we need to restore it.
It would be nice to use one of the exec* functions that takes an
environment as a parameter, but that may have portability
- issues. */
- char **save_environ = environ;
+ issues. It is marked volatile so the child doesn't consider it a
+ dead variable and therefore clobber where ever it is stored. */
+ char **volatile save_environ = environ;
for (retries = 0; retries < 4; ++retries)
{
@@ -594,6 +627,11 @@ pex_unix_exec_child (struct pex_obj *obj
switch (pid)
{
case -1:
+ if (do_pipe)
+ {
+ close (pipes[0]);
+ close (pipes[1]);
+ }
*err = errno;
*errmsg = VFORK_STRING;
return (pid_t) -1;
@@ -601,40 +639,43 @@ pex_unix_exec_child (struct pex_obj *obj
case 0:
/* Child process. */
{
- const char *bad_fn = NULL;
+ struct fn_err failed;
+ failed.fn = NULL;
- if (!bad_fn && in != STDIN_FILE_NO)
+ if (do_pipe)
+ close (pipes[0]);
+ if (!failed.fn && in != STDIN_FILE_NO)
{
if (dup2 (in, STDIN_FILE_NO) < 0)
- bad_fn = "dup2";
+ failed.fn = "dup2", failed.err = errno;
else if (close (in) < 0)
- bad_fn = "close";
+ failed.fn = "close", failed.err = errno;
}
- if (!bad_fn && out != STDOUT_FILE_NO)
+ if (!failed.fn && out != STDOUT_FILE_NO)
{
if (dup2 (out, STDOUT_FILE_NO) < 0)
- bad_fn = "dup2";
+ failed.fn = "dup2", failed.err = errno;
else if (close (out) < 0)
- bad_fn = "close";
+ failed.fn = "close", failed.err = errno;
}
- if (!bad_fn && errdes != STDERR_FILE_NO)
+ if (!failed.fn && errdes != STDERR_FILE_NO)
{
if (dup2 (errdes, STDERR_FILE_NO) < 0)
- bad_fn = "dup2";
+ failed.fn = "dup2", failed.err = errno;
else if (close (errdes) < 0)
- bad_fn = "close";
+ failed.fn = "close", failed.err = errno;
}
- if (!bad_fn && toclose >= 0)
+ if (!failed.fn && toclose >= 0)
{
if (close (toclose) < 0)
- bad_fn = "close";
+ failed.fn = "close", failed.err = errno;
}
- if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
+ if (!failed.fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
{
if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
- bad_fn = "dup2";
+ failed.fn = "dup2", failed.err = errno;
}
- if (!bad_fn)
+ if (!failed.fn)
{
if (env)
/* NOTE: In a standard vfork implementation this clobbers
@@ -644,30 +685,35 @@ pex_unix_exec_child (struct pex_obj *obj
if ((flags & PEX_SEARCH) != 0)
{
execvp (executable, to_ptr32 (argv));
- bad_fn = "execvp";
+ failed.fn = "execvp", failed.err = errno;
}
else
{
execv (executable, to_ptr32 (argv));
- bad_fn = "execv";
+ failed.fn = "execv", failed.err = errno;
}
}
/* Something failed, report an error. We don't use stdio
routines, because we might be here due to a vfork call. */
ssize_t retval = 0;
- int eno = errno;
-
+
+ if (!do_pipe
+ || write (pipes[1], &failed, sizeof (failed)) != sizeof (failed))
+ {
+ /* The parent will not see our scream above, so write to
+ stdout. */
#define writeerr(s) (retval |= write (STDERR_FILE_NO, s, strlen (s)))
- writeerr (obj->pname);
- writeerr (": error trying to exec '");
- writeerr (executable);
- writeerr ("': ");
- writeerr (bad_fn);
- writeerr (": ");
- writeerr (xstrerror (eno));
- writeerr ("\n");
+ writeerr (obj->pname);
+ writeerr (": error trying to exec '");
+ writeerr (executable);
+ writeerr ("': ");
+ writeerr (failed.fn);
+ writeerr (": ");
+ writeerr (xstrerror (failed.err));
+ writeerr ("\n");
#undef writeerr
+ }
/* Exit with -2 if the error output failed, too. */
_exit (retval < 0 ? -2 : -1);
@@ -678,8 +724,6 @@ pex_unix_exec_child (struct pex_obj *obj
default:
/* Parent process. */
{
- const char *bad_fn = NULL;
-
/* Restore environ. Note that the parent either doesn't run
until the child execs/exits (standard vfork behaviour), or
if it does run then vfork is behaving more like fork. In
@@ -687,24 +731,34 @@ pex_unix_exec_child (struct pex_obj *obj
copy of environ. */
environ = save_environ;
- if (!bad_fn && in != STDIN_FILE_NO)
+ struct fn_err failed;
+ failed.fn = NULL;
+ if (do_pipe)
+ {
+ close (pipes[1]);
+ ssize_t len = read (pipes[0], &failed, sizeof (failed));
+ if (len < 0)
+ failed.fn = NULL;
+ close (pipes[0]);
+ }
+
+ if (!failed.fn && in != STDIN_FILE_NO)
if (close (in) < 0)
- bad_fn = "close";
- if (!bad_fn && out != STDOUT_FILE_NO)
+ failed.fn = "close", failed.err = errno;
+ if (!failed.fn && out != STDOUT_FILE_NO)
if (close (out) < 0)
- bad_fn = "close";
- if (!bad_fn && errdes != STDERR_FILE_NO)
+ failed.fn = "close", failed.err = errno;
+ if (!failed.fn && errdes != STDERR_FILE_NO)
if (close (errdes) < 0)
- bad_fn = "close";
+ failed.fn = "close", failed.err = errno;
- if (bad_fn)
+ if (failed.fn)
{
- *err = errno;
- *errmsg = bad_fn;
+ *err = failed.err;
+ *errmsg = failed.fn;
return (pid_t) -1;
}
}
-
return pid;
}
}