From patchwork Mon Oct 1 17:53:30 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Nathan Sidwell X-Patchwork-Id: 977342 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-486736-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=acm.org Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="vgmU6Sev"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 42P8zQ5N8Fz9s4V for ; Tue, 2 Oct 2018 03:53:51 +1000 (AEST) Received: (qmail 129951 invoked by alias); 1 Oct 2018 17:53:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 129746 invoked by uid 89); 1 Oct 2018 17:53:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, TIME_LIMIT_EXCEEDED, UNSUBSCRIBE_BODY autolearn=unavailable version=3.3.2 spammy=writer, H*r:sk:mail-yw, HX-HELO:sk:mail-yw, H*RU:sk:mail-yw X-HELO: mail-yw1-f45.google.com Received: from mail-yw1-f45.google.com (HELO mail-yw1-f45.google.com) (209.85.161.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 01 Oct 2018 17:53:34 +0000 Received: by mail-yw1-f45.google.com with SMTP id j202-v6so1369738ywa.13 for ; Mon, 01 Oct 2018 10:53:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:to:cc:from:subject:message-id:date:user-agent:mime-version :content-language; bh=ABeos9U8YasziC38lrd6cavcDBvSs7bx17iXnOL7Zmc=; b=vgmU6Sev924L/CM/b3sbGr4hcnWQ625KpPCTJDnmZ4ShJjnUaGz0gYni7erL93N/Zk 3lxDFmCzVBM2Y8t3yhCEb2dHyCOSWgYdpJ51tz92rfyvoeh8X9kO6daOTNfH0/IIp4JA B958CaQw7pwoUyfZmwFrogpy84ywqiORWvIzSvYNKS4dSF+YKjjnZo25cBEvGAEp8gQ4 CGBIz32SdAS0mt5GNcSDjO0p2bBDyVTD9+OlvfpQXq+K3yM8hr0u6sWqB1fFrFeOKAsh Ag8Iqi8XDIlBg0pjWwnpEkE3II+GnimMna7zyEpNwJLf3rVRyt1VqYTGbuYhyaUGfecL lCPA== Received: from ?IPv6:2620:10d:c0a3:20fb::830? ([2620:10d:c091:200::5:7a41]) by smtp.googlemail.com with ESMTPSA id m18-v6sm10066083ywh.63.2018.10.01.10.53.31 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Oct 2018 10:53:31 -0700 (PDT) Sender: Nathan Sidwell To: Ian Lance Taylor Cc: GCC Patches , =?utf-8?q?Martin_Li=C5=A1ka?= From: Nathan Sidwell Subject: [libiberty] Use pipe inside pex_run Message-ID: Date: Mon, 1 Oct 2018 13:53:30 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 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 2018-10-01 Nathan Sidwell * 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 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; } }