| Message ID | 20190519104059.19382-1-blomqvist.janne@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | libfortran/90038 Reap dead children when wait=.false. | expand |
On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote: > > +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID) > + static bool sig_init_saved; > + bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED); > + if (!sig_init) > + { > + struct sigaction sa; > + sa.sa_handler = &sigchld_handler; > + sigemptyset(&sa.sa_mask); > + sa.sa_flags = SA_RESTART | SA_NOCLDSTOP; > + sigaction(SIGCHLD, &sa, 0); > + __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED); > + } > +#endif Where do the prototypes for __atomic_load_n and __atomic_store_n come from? On FreeBSD, it seems these are in stdatomic.h. Do we need a HAVE_ATOMIC to include the header? On a slightly different note, the nonstandard SYSTEM intrinsic uses system(3) to execute a command. I believe that it will leave zombies/orphaned children if a process is interrupted. Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE.
On Sun, May 19, 2019 at 7:15 PM Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: > > On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote: > > > > +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID) > > + static bool sig_init_saved; > > + bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED); > > + if (!sig_init) > > + { > > + struct sigaction sa; > > + sa.sa_handler = &sigchld_handler; > > + sigemptyset(&sa.sa_mask); > > + sa.sa_flags = SA_RESTART | SA_NOCLDSTOP; > > + sigaction(SIGCHLD, &sa, 0); > > + __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED); > > + } > > +#endif > > Where do the prototypes for __atomic_load_n and __atomic_store_n > come from? On FreeBSD, it seems these are in stdatomic.h. Do > we need a HAVE_ATOMIC to include the header? They are GCC atomic builtins, they are always available, no need to include a header: https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html > On a slightly different note, the nonstandard SYSTEM intrinsic > uses system(3) to execute a command. I believe that it will > leave zombies/orphaned children if a process is interrupted. > Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE. I believe any competent implementation of system() would wait for the child process. Since system() is synchronous, it can do the waiting inline without having to use a signal handler.
On Sun, May 19, 2019 at 09:10:57PM +0300, Janne Blomqvist wrote: > On Sun, May 19, 2019 at 7:15 PM Steve Kargl > <sgk@troutmask.apl.washington.edu> wrote: > > > > On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote: > > > > > > +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID) > > > + static bool sig_init_saved; > > > + bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED); > > > + if (!sig_init) > > > + { > > > + struct sigaction sa; > > > + sa.sa_handler = &sigchld_handler; > > > + sigemptyset(&sa.sa_mask); > > > + sa.sa_flags = SA_RESTART | SA_NOCLDSTOP; > > > + sigaction(SIGCHLD, &sa, 0); > > > + __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED); > > > + } > > > +#endif > > > > Where do the prototypes for __atomic_load_n and __atomic_store_n > > come from? On FreeBSD, it seems these are in stdatomic.h. Do > > we need a HAVE_ATOMIC to include the header? > > They are GCC atomic builtins, they are always available, no need to > include a header: > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html Thanks for the clarification. OK for trunk. > > On a slightly different note, the nonstandard SYSTEM intrinsic > > uses system(3) to execute a command. I believe that it will > > leave zombies/orphaned children if a process is interrupted. > > Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE. > > I believe any competent implementation of system() would wait for the > child process. Since system() is synchronous, it can do the waiting > inline without having to use a signal handler. I'll need to check. I recall that program foo call system("Something_that_takes_along_time_to_execute") end program foo gfortran -o foo foo.f90 % foo ^C Results in termination of the parent foo, and Something_that_takes_along_time_to_execute is orphaned and continues to run. It would probably be better to deliver SIGKILL to child. I suppose that that is for another day.
On Sun, May 19, 2019 at 9:26 PM Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: > > On Sun, May 19, 2019 at 09:10:57PM +0300, Janne Blomqvist wrote: > > On Sun, May 19, 2019 at 7:15 PM Steve Kargl > > <sgk@troutmask.apl.washington.edu> wrote: > > > > > > On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote: > > > > > > > > +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID) > > > > + static bool sig_init_saved; > > > > + bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED); > > > > + if (!sig_init) > > > > + { > > > > + struct sigaction sa; > > > > + sa.sa_handler = &sigchld_handler; > > > > + sigemptyset(&sa.sa_mask); > > > > + sa.sa_flags = SA_RESTART | SA_NOCLDSTOP; > > > > + sigaction(SIGCHLD, &sa, 0); > > > > + __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED); > > > > + } > > > > +#endif > > > > > > Where do the prototypes for __atomic_load_n and __atomic_store_n > > > come from? On FreeBSD, it seems these are in stdatomic.h. Do > > > we need a HAVE_ATOMIC to include the header? > > > > They are GCC atomic builtins, they are always available, no need to > > include a header: > > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html > > Thanks for the clarification. OK for trunk. Thanks, committed as r271384. > > > On a slightly different note, the nonstandard SYSTEM intrinsic > > > uses system(3) to execute a command. I believe that it will > > > leave zombies/orphaned children if a process is interrupted. > > > Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE. > > > > I believe any competent implementation of system() would wait for the > > child process. Since system() is synchronous, it can do the waiting > > inline without having to use a signal handler. > > I'll need to check. I recall that > > program foo > call system("Something_that_takes_along_time_to_execute") > end program foo > > gfortran -o foo foo.f90 > > % foo > ^C > > Results in termination of the parent foo, and > Something_that_takes_along_time_to_execute is orphaned and > continues to run. It would probably be better to deliver > SIGKILL to child. I suppose that that is for another day. I guess the expected behavior is that is the parent dies, the child gets orphaned and thus re-parented to PID 1 (that is, init) which takes care of wait()'ing for it so it's not left as a zombie when it dies. Not sure it's possible to change this is a robust and portable way. But if someone figures it out, we can think about it then.
diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac index 8fd5a1a30a9..7cfce28ab69 100644 --- a/libgfortran/configure.ac +++ b/libgfortran/configure.ac @@ -314,7 +314,7 @@ if test "${hardwire_newlib:-0}" -eq 1; then else AC_CHECK_FUNCS_ONCE(getrusage times mkstemp strtof strtold snprintf \ ftruncate chsize chdir getentropy getlogin gethostname kill link symlink \ - sleep ttyname \ + sleep ttyname sigaction waitpid \ alarm access fork posix_spawn setmode fcntl writev \ gettimeofday stat fstat lstat getpwuid vsnprintf dup \ getcwd localtime_r gmtime_r getpwuid_r ttyname_r clock_gettime \ diff --git a/libgfortran/intrinsics/execute_command_line.c b/libgfortran/intrinsics/execute_command_line.c index 2ef2324b243..1a471632172 100644 --- a/libgfortran/intrinsics/execute_command_line.c +++ b/libgfortran/intrinsics/execute_command_line.c @@ -36,6 +36,9 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #include <spawn.h> extern char **environ; #endif +#if defined(HAVE_POSIX_SPAWN) || defined(HAVE_FORK) +#include <signal.h> +#endif enum { EXEC_SYNCHRONOUS = -2, EXEC_NOERROR = 0, EXEC_SYSTEMFAILED, EXEC_CHILDFAILED, EXEC_INVALIDCOMMAND }; @@ -62,6 +65,14 @@ set_cmdstat (int *cmdstat, int value) } +#if defined(HAVE_WAITPID) && defined(HAVE_SIGACTION) +static void +sigchld_handler (int signum __attribute__((unused))) +{ + while (waitpid ((pid_t)(-1), NULL, WNOHANG) > 0) {} +} +#endif + static void execute_command_line (const char *command, bool wait, int *exitstat, int *cmdstat, char *cmdmsg, @@ -82,6 +93,20 @@ execute_command_line (const char *command, bool wait, int *exitstat, set_cmdstat (cmdstat, EXEC_NOERROR); +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID) + static bool sig_init_saved; + bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED); + if (!sig_init) + { + struct sigaction sa; + sa.sa_handler = &sigchld_handler; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESTART | SA_NOCLDSTOP; + sigaction(SIGCHLD, &sa, 0); + __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED); + } +#endif + #ifdef HAVE_POSIX_SPAWN const char * const argv[] = {"sh", "-c", cmd, NULL}; if (posix_spawn (&pid, "/bin/sh", NULL, NULL,
When using posix_spawn or fork to launch a child process, the parent needs to wait for the child, otherwise the dead child is left as a zombie process. For this purpose one can install a signal handler for SIGCHLD. 2019-05-19 Janne Blomqvist <jb@gcc.gnu.org> PR libfortran/90038 * intrinsics/execute_command_line (sigchld_handler): New function. (execute_command_line): Install handler for SIGCHLD. * configure.ac: Check for presence of sigaction and waitpid. * 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 | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-)