Processes that spawn other processes should wait for their children to exit before exiting due to a signal.

[beanz]

Summary

If a process that spawns other processes (like the clang driver) gets killed via a SIGINT, it should wait until its children exit before returning from the signal handler. This prevents the child processes from getting re-parented, which causes problems for build systems that are the parent of clang processes.

[silvas]

lib/Support/Unix/Program.inc

72

Do pids ever get pushed onto here? or is that an upcoming patch?

[• ddunbar]

It doesn't look like ChildPIDs is ever added to?

This implementation can possibly call free() from ChildPIDs->clear(), and
while sys::Wait() looks safe as invoked, I could see how someone might
change it in a way that would cause it to break. It might be safer to call
waitpid() directly, or at least to add a comment to sys::Wait() noting that
it needs to stay signal safe when WaitUntilTerminates == 0 && ErrMsg ==
nullptr.

Also, I think you should take care not to register WaitForChildren()
multiple times.

Probably also good to include a note about why we are waiting for children.

• Daniel

[majnemer]

lib/Support/Unix/Program.inc

188

You need a space between the if and the paren.

190

Might be nice to do something like:

Wait(PI, /*SecondsUntilTerminate=*/0, /*WaitUntilTerminate=*/true, /*ErrMsg=*/nullptr);

I find that bare constants in function parameters can be confusing without a little context.

[beanz]

Updating based on feedback from majnemer, ddunbar, and silvas.

[beanz]

Ping?

[• ddunbar]

lib/Support/Unix/Program.inc

209

This can race on ProgramSignalHandlerAdded.

342

Shouldn't we also make sure to remove (or maybe just clear to 0, to indicate it is gone?) the PID when the child goes away?

352

This comment should be adjusted to not this is only necessary when ErrMsg=0, since the code trivially fails the comment if not.

lib/Support/Unix/Program.inc

 //===          is guaranteed to work on *all* UNIX variants.
 //===----------------------------------------------------------------------===//
 
 #include "Unix.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/Signals.h"
 #if HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
 #if HAVE_SYS_RESOURCE_H
 #include <sys/resource.h>
 #endif
 #if HAVE_SIGNAL_H
 #include <signal.h>

 
 #if !USE_NSGETENVIRON
   extern char **environ;
 #else
 #include <crt_externs.h> // _NSGetEnviron
 #endif
 #endif
 
+#include <vector>
+
 namespace llvm {
 
 using namespace sys;
 
+static bool ProgramSignalHandlerAdded = false;
+static ManagedStatic<std::vector<ProcessInfo>> ChildPIDs;
+
 ProcessInfo::ProcessInfo() : Pid(0), ReturnCode(0) {}
 
 ErrorOr<std::string> sys::findProgramByName(StringRef Name,
                                             ArrayRef<StringRef> Paths) {
   assert(!Name.empty() && "Must have a name!");
   // Use the given path verbatim if it contains any slashes; this matches
   // the behavior of sh(1) and friends.
   if (Name.find('/') != StringRef::npos)
#if !LLVM_MEMORY_SANITIZER_BUILD && !LLVM_ADDRESS_SANITIZER_BUILD
   setrlimit (RLIMIT_AS, &r);
 #endif
 #endif
 #endif
 }
 
 }
 
+// If a process that spawns other processes (like the clang driver) gets killed
+// via a SIGINT, it should wait until its children exit before returning from
+// the signal handler. This prevents the child processes from getting
+// re-parented, which causes problems for build systems that are the parent of
+// clang processes.
+static void WaitForChildren(void *) {
+  if (ChildPIDs.isConstructed())
+    for (auto PI : *ChildPIDs)
+      Wait(PI, /*SecondsUntilTerminate=*/0, /*WaitUntilTerminate=*/true,
+           /*ErrMsg=*/nullptr);
+}
+
 static bool Execute(ProcessInfo &PI, StringRef Program, const char **args,
                     const char **envp, const StringRef **redirects,
                     unsigned memoryLimit, std::string *ErrMsg) {
   if (!llvm::sys::fs::exists(Program)) {
     if (ErrMsg)
       *ErrMsg = std::string("Executable \"") + Program.str() +
                 std::string("\" doesn't exist!");
     return false;
   }
 
+  if (!ProgramSignalHandlerAdded) {
+    AddSignalHandler(WaitForChildren, nullptr);
+    ProgramSignalHandlerAdded = true;
+  }
+
   // If this OS has posix_spawn and there is no memory limit being implied, use
   // posix_spawn.  It is more efficient than fork/exec.
 #ifdef HAVE_POSIX_SPAWN
   if (memoryLimit == 0) {
     posix_spawn_file_actions_t FileActionsStore;
     posix_spawn_file_actions_t *FileActions = nullptr;
 
     // If we call posix_spawn_file_actions_addopen we have to make sure the
#endif
 
     if (FileActions)
       posix_spawn_file_actions_destroy(FileActions);
 
     if (Err)
      return !MakeErrMsg(ErrMsg, "posix_spawn failed", Err);
 
     PI.Pid = PID;
+    ChildPIDs->push_back(PI);
 
     return true;
   }
 #endif
 
   // Create a child process.
   int child = fork();
   switch (child) {
switch (child) {
     }
 
     // Parent process: Break out of the switch to do our processing.
     default:
       break;
   }
 
   PI.Pid = child;
+  ChildPIDs->push_back(PI);
 
   return true;
 }
 
 namespace llvm {
 
+// This is assumed to be safe to call from within a signal handler. That means
+// it can't allocate or de-allocate memory or use any C interfaces not listed in
+// the sigaction man page.
 ProcessInfo sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
                       bool WaitUntilTerminates, std::string *ErrMsg) {
 #ifdef HAVE_SYS_WAIT_H
   struct sigaction Act, Old;
   assert(PI.Pid && "invalid pid to wait on, process not started?");
 
   int WaitPidOptions = 0;
   pid_t ChildPid = PI.Pid;
   if (WaitUntilTerminates) {