diff mbox series

libfortran/90038: Use posix_spawn instead of fork

Message ID 20190516195532.30433-1-blomqvist.janne@gmail.com
State New
Headers show
Series libfortran/90038: Use posix_spawn instead of fork | expand

Commit Message

Janne Blomqvist May 16, 2019, 7:55 p.m. UTC 
fork() semantics can be problematic.  Most unix style OS'es have
posix_spawn which can be used to replace fork + exec in many cases.
For more information see
e.g. https://www.microsoft.com/en-us/research/uploads/prod/2019/04/fork-hotos19.pdf

This replaces the one use of fork in libgfortran with posix_spawn.

2019-05-16  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/90038
	* configure.ac (AC_CHECK_FUNCS_ONCE): Check for posix_spawn
	instead of fork.
	* intrinsics/execute_command_line (execute_command_line): Use
	posix_spawn instead of fork.
	* Makefile.in: Regenerated.
	* config.h.in: Regenerated.
	* configure: Regenerated.

Regtested on x86_64-pc-linux-gnu, Ok for trunk?
---
 libgfortran/configure.ac                      |  2 +-
 libgfortran/intrinsics/execute_command_line.c | 19 +++++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)

Comments

Thomas Koenig May 16, 2019, 7:59 p.m. UTC | #1 
Hi Janne,

> fork() semantics can be problematic.  Most unix style OS'es have
> posix_spawn which can be used to replace fork + exec in many cases.
> For more information see
> e.g. https://www.microsoft.com/en-us/research/uploads/prod/2019/04/fork-hotos19.pdf
> 
> This replaces the one use of fork in libgfortran with posix_spawn.

Do I understand the patch correctly that we would no longer use fork()
if posix_spawn is not available?  I think we should leave that in as
a fallback option.

Regards

	Thomas
Janne Blomqvist May 16, 2019, 8:10 p.m. UTC | #2 
On Thu, May 16, 2019 at 10:59 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
>
> Hi Janne,
>
> > fork() semantics can be problematic.  Most unix style OS'es have
> > posix_spawn which can be used to replace fork + exec in many cases.
> > For more information see
> > e.g. https://www.microsoft.com/en-us/research/uploads/prod/2019/04/fork-hotos19.pdf
> >
> > This replaces the one use of fork in libgfortran with posix_spawn.
>
> Do I understand the patch correctly that we would no longer use fork()
> if posix_spawn is not available?  I think we should leave that in as
> a fallback option.

Yes. But there is already a fallback in case posix_spawn (or
previously, fork) is not available, namely falling back to synchronous
behavior. Since this is anyway somewhat of a corner case (namely, with
wait=.false.), and posix_spawn is supported on all (well, at least
Linux, macOS, *BSD, cygwin, Solarix, AIX) remotely modern unix type
systems, a further fallback to fork() is IMHO not warranted.
Thomas Koenig May 16, 2019, 8:43 p.m. UTC | #3 
Am 16.05.19 um 22:10 schrieb Janne Blomqvist:
> On Thu, May 16, 2019 at 10:59 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
>>
>> Hi Janne,
>>
>>> fork() semantics can be problematic.  Most unix style OS'es have
>>> posix_spawn which can be used to replace fork + exec in many cases.
>>> For more information see
>>> e.g. https://www.microsoft.com/en-us/research/uploads/prod/2019/04/fork-hotos19.pdf
>>>
>>> This replaces the one use of fork in libgfortran with posix_spawn.
>>
>> Do I understand the patch correctly that we would no longer use fork()
>> if posix_spawn is not available?  I think we should leave that in as
>> a fallback option.
> 
> Yes. But there is already a fallback in case posix_spawn (or
> previously, fork) is not available, namely falling back to synchronous
> behavior. Since this is anyway somewhat of a corner case (namely, with
> wait=.false.), and posix_spawn is supported on all (well, at least
> Linux, macOS, *BSD, cygwin, Solarix, AIX) remotely modern unix type
> systems, a further fallback to fork() is IMHO not warranted.

I differ there.

Regards

	Thomas
Thomas Koenig May 16, 2019, 8:54 p.m. UTC | #4 
Hi Janne,

> I differ there.

A longer explanation:

fork() is standard POSIX. Not all systems have posix_spawn.  For
those systems which do not have it, we would cause a regression
by simply removing that functionality for this.

The patch is OK from my side if you add fork() as a fallback option.

Regards

	Thomas
Janne Blomqvist May 17, 2019, 9:04 a.m. UTC | #5 
On Thu, May 16, 2019 at 11:54 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
>
> Hi Janne,
>
> > I differ there.
>
> A longer explanation:
>
> fork() is standard POSIX.

So is posix_spawn, as the name implies.

>Not all systems have posix_spawn.

Do you know of any target we support that has fork but not posix_spawn?

> The patch is OK from my side if you add fork() as a fallback option.

Sure. I'm quite sure it's dead code that will never be executed, but
hey, it's ifdeffed away so whatever.
Janne Blomqvist May 17, 2019, 9:11 a.m. UTC | #6 
On Thu, May 16, 2019 at 11:37 PM Nicolas Koenig
<koenigni@student.ethz.ch> wrote:
>
> Hi everyone,
>
> a quick heads up, that the native coarray patch I'm working on at the
> moment (1200 loc and growing :D) will be based upon forked processes, so
> we won't be able to eliminate fork().

I would guess that's a case where fork semantics are useful, as
presumably copying the memory of a process is exactly what you want.
posix_spawn() is a replacement for some common uses of fork+exec.

> P.S.: About the paper: A microsoft guy who doesn't like fork()? Color me
> surprised :D

Well, it's one author out of four. And it's Microsoft Research, which
does serious CS research, not PR department vomit. The paper does make
some good points, and it's worth reading.

As an interesting aside, the original libgfortran IO library was based
upon a paper by one of the authors of this paper. One of my first
contributions to GFortran back in the day was ripping out that.

>
>
> On 16/05/2019 22:10, Janne Blomqvist wrote:
> > On Thu, May 16, 2019 at 10:59 PM Thomas Koenig <tkoenig@netcologne.de> wrote:
> >>
> >> Hi Janne,
> >>
> >>> fork() semantics can be problematic.  Most unix style OS'es have
> >>> posix_spawn which can be used to replace fork + exec in many cases.
> >>> For more information see
> >>> e.g. https://www.microsoft.com/en-us/research/uploads/prod/2019/04/fork-hotos19.pdf
> >>>
> >>> This replaces the one use of fork in libgfortran with posix_spawn.
> >>
> >> Do I understand the patch correctly that we would no longer use fork()
> >> if posix_spawn is not available?  I think we should leave that in as
> >> a fallback option.
> >
> > Yes. But there is already a fallback in case posix_spawn (or
> > previously, fork) is not available, namely falling back to synchronous
> > behavior. Since this is anyway somewhat of a corner case (namely, with
> > wait=.false.), and posix_spawn is supported on all (well, at least
> > Linux, macOS, *BSD, cygwin, Solarix, AIX) remotely modern unix type
> > systems, a further fallback to fork() is IMHO not warranted.
> >
> >
diff mbox series

Patch

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index c06db7b1a78..66af512a292 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -315,7 +315,7 @@  else
    AC_CHECK_FUNCS_ONCE(getrusage times mkstemp strtof strtold snprintf \
    ftruncate chsize chdir getentropy getlogin gethostname kill link symlink \
    sleep ttyname \
-   alarm access fork setmode fcntl writev \
+   alarm access posix_spawn setmode fcntl writev \
    gettimeofday stat fstat lstat getpwuid vsnprintf dup \
    getcwd localtime_r gmtime_r getpwuid_r ttyname_r clock_gettime \
    getgid getpid getuid geteuid umask getegid \
diff --git a/libgfortran/intrinsics/execute_command_line.c b/libgfortran/intrinsics/execute_command_line.c
index a234bc328b5..3df99c10678 100644
--- a/libgfortran/intrinsics/execute_command_line.c
+++ b/libgfortran/intrinsics/execute_command_line.c
@@ -32,7 +32,10 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #ifdef  HAVE_SYS_WAIT_H
 #include <sys/wait.h>
 #endif
-
+#ifdef HAVE_POSIX_SPAWN
+#include <spawn.h>
+extern char **environ;
+#endif
 
 enum { EXEC_SYNCHRONOUS = -2, EXEC_NOERROR = 0, EXEC_SYSTEMFAILED,
        EXEC_CHILDFAILED, EXEC_INVALIDCOMMAND };
@@ -71,7 +74,7 @@  execute_command_line (const char *command, bool wait, int *exitstat,
   /* Flush all I/O units before executing the command.  */
   flush_all_units();
 
-#if defined(HAVE_FORK)
+#if defined(HAVE_POSIX_SPAWN)
   if (!wait)
     {
       /* Asynchronous execution.  */
@@ -79,14 +82,10 @@  execute_command_line (const char *command, bool wait, int *exitstat,
 
       set_cmdstat (cmdstat, EXEC_NOERROR);
 
-      if ((pid = fork()) < 0)
+      const char * const argv[] = {"sh", "-c", cmd, NULL};
+      if (posix_spawn (&pid, "/bin/sh", NULL, NULL,
+		       (char * const* restrict) argv, environ))
 	set_cmdstat (cmdstat, EXEC_CHILDFAILED);
-      else if (pid == 0)
-	{
-	  /* Child process.  */
-	  int res = system (cmd);
-	  _exit (WIFEXITED(res) ? WEXITSTATUS(res) : res);
-	}
     }
   else
 #endif
@@ -96,7 +95,7 @@  execute_command_line (const char *command, bool wait, int *exitstat,
 
       if (res == -1)
 	set_cmdstat (cmdstat, EXEC_SYSTEMFAILED);
-#ifndef HAVE_FORK
+#ifndef HAVE_POSIX_SPAWN
       else if (!wait)
 	set_cmdstat (cmdstat, EXEC_SYNCHRONOUS);
 #endif