Skip to content

Reland "[llvm][Support] Add support for executing a detached process (#81708)" #83367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

cpsughrue
Copy link
Contributor

@cpsughrue cpsughrue commented Feb 29, 2024

Relands #81708, which was reverted by f410f74, now with a corrected unit test. Origionally the test failed on Windows when run with lit as GetConsoleWindow could not retrieve a window handle regardless of whether DetachProcess was true or false. The test now uses GetStdHandle(STD_OUTPUT_HANDLE) which does not rely on a console window existing. Original commit message below.

Adds a new parameter, bool DetachProcess with a default option of false, to llvm::sys::ExecuteNoWait, which, when set to true, executes the specified program without a controlling terminal.

Functionality added so that the module build daemon can be run without a controlling terminal.

@llvmbot
Copy link
Member

llvmbot commented Mar 6, 2024

@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-llvm-support

Author: Connor Sughrue (cpsughrue)

Changes

Relands #81708, which was reverted by f410f74, now with a corrected unit test. Origionally the test failed on Windows when run with lit as GetConsoleWindow could not retrieve a window handle regardless of whether DetachProcess was true or false. The test now uses GetStdHandle(STD_OUTPUT_HANDLE) to verify whether a controlling terminal exists. Original commit message below.

Adds a new parameter, bool DetachProcess with a default option of false, to llvm::sys::ExecuteNoWait, which, when set to true, executes the specified program without a controlling terminal.

Functionality added so that the module build daemon can be run without a controlling terminal.


Full diff: https://github.com/llvm/llvm-project/pull/83367.diff

5 Files Affected:

  • (modified) llvm/include/llvm/Support/Program.h (+14-11)
  • (modified) llvm/lib/Support/Program.cpp (+5-4)
  • (modified) llvm/lib/Support/Unix/Program.inc (+14-4)
  • (modified) llvm/lib/Support/Windows/Program.inc (+5-2)
  • (modified) llvm/unittests/Support/ProgramTest.cpp (+90)
diff --git a/llvm/include/llvm/Support/Program.h b/llvm/include/llvm/Support/Program.h
index 4c1133e44a21c9..9df94eb604c7d6 100644
--- a/llvm/include/llvm/Support/Program.h
+++ b/llvm/include/llvm/Support/Program.h
@@ -141,18 +141,21 @@ namespace sys {
                                         /// program shall run on.
   );
 
-  /// Similar to ExecuteAndWait, but returns immediately.
-  /// @returns The \see ProcessInfo of the newly launched process.
+  /// Similar to \ref ExecuteAndWait, but returns immediately.
+  /// \returns The \ref ProcessInfo of the newly launched process.
   /// \note On Microsoft Windows systems, users will need to either call
-  /// \see Wait until the process finished execution or win32 CloseHandle() API
-  /// on ProcessInfo.ProcessHandle to avoid memory leaks.
-  ProcessInfo ExecuteNoWait(StringRef Program, ArrayRef<StringRef> Args,
-                            std::optional<ArrayRef<StringRef>> Env,
-                            ArrayRef<std::optional<StringRef>> Redirects = {},
-                            unsigned MemoryLimit = 0,
-                            std::string *ErrMsg = nullptr,
-                            bool *ExecutionFailed = nullptr,
-                            BitVector *AffinityMask = nullptr);
+  /// \ref Wait until the process has finished executing or win32's CloseHandle
+  /// API on ProcessInfo.ProcessHandle to avoid memory leaks.
+  ProcessInfo ExecuteNoWait(
+      StringRef Program, ArrayRef<StringRef> Args,
+      std::optional<ArrayRef<StringRef>> Env,
+      ArrayRef<std::optional<StringRef>> Redirects = {},
+      unsigned MemoryLimit = 0, std::string *ErrMsg = nullptr,
+      bool *ExecutionFailed = nullptr, BitVector *AffinityMask = nullptr,
+      /// If true the executed program detatches from the controlling
+      /// terminal. I/O streams such as llvm::outs, llvm::errs, and stdin will
+      /// be closed until redirected to another output location
+      bool DetachProcess = false);
 
   /// Return true if the given arguments fit within system-specific
   /// argument length limits.
diff --git a/llvm/lib/Support/Program.cpp b/llvm/lib/Support/Program.cpp
index 1dcd45e2d69e89..181f68cfbb8c3f 100644
--- a/llvm/lib/Support/Program.cpp
+++ b/llvm/lib/Support/Program.cpp
@@ -27,7 +27,7 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
                     std::optional<ArrayRef<StringRef>> Env,
                     ArrayRef<std::optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg,
-                    BitVector *AffinityMask);
+                    BitVector *AffinityMask, bool DetachProcess);
 
 int sys::ExecuteAndWait(StringRef Program, ArrayRef<StringRef> Args,
                         std::optional<ArrayRef<StringRef>> Env,
@@ -39,7 +39,7 @@ int sys::ExecuteAndWait(StringRef Program, ArrayRef<StringRef> Args,
   assert(Redirects.empty() || Redirects.size() == 3);
   ProcessInfo PI;
   if (Execute(PI, Program, Args, Env, Redirects, MemoryLimit, ErrMsg,
-              AffinityMask)) {
+              AffinityMask, /*DetachProcess=*/false)) {
     if (ExecutionFailed)
       *ExecutionFailed = false;
     ProcessInfo Result = Wait(
@@ -58,13 +58,14 @@ ProcessInfo sys::ExecuteNoWait(StringRef Program, ArrayRef<StringRef> Args,
                                std::optional<ArrayRef<StringRef>> Env,
                                ArrayRef<std::optional<StringRef>> Redirects,
                                unsigned MemoryLimit, std::string *ErrMsg,
-                               bool *ExecutionFailed, BitVector *AffinityMask) {
+                               bool *ExecutionFailed, BitVector *AffinityMask,
+                               bool DetachProcess) {
   assert(Redirects.empty() || Redirects.size() == 3);
   ProcessInfo PI;
   if (ExecutionFailed)
     *ExecutionFailed = false;
   if (!Execute(PI, Program, Args, Env, Redirects, MemoryLimit, ErrMsg,
-               AffinityMask))
+               AffinityMask, DetachProcess))
     if (ExecutionFailed)
       *ExecutionFailed = true;
 
diff --git a/llvm/lib/Support/Unix/Program.inc b/llvm/lib/Support/Unix/Program.inc
index 5d9757bcc51b3e..2742734bb11ed0 100644
--- a/llvm/lib/Support/Unix/Program.inc
+++ b/llvm/lib/Support/Unix/Program.inc
@@ -173,10 +173,11 @@ toNullTerminatedCStringArray(ArrayRef<StringRef> Strings, StringSaver &Saver) {
 }
 
 static bool Execute(ProcessInfo &PI, StringRef Program,
-                    ArrayRef<StringRef> Args, std::optional<ArrayRef<StringRef>> Env,
+                    ArrayRef<StringRef> Args,
+                    std::optional<ArrayRef<StringRef>> Env,
                     ArrayRef<std::optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg,
-                    BitVector *AffinityMask) {
+                    BitVector *AffinityMask, bool DetachProcess) {
   if (!llvm::sys::fs::exists(Program)) {
     if (ErrMsg)
       *ErrMsg = std::string("Executable \"") + Program.str() +
@@ -202,7 +203,8 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
   // 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) {
+  // Cannot use posix_spawn if you would like to detach the process
+  if (MemoryLimit == 0 && !DetachProcess) {
     posix_spawn_file_actions_t FileActionsStore;
     posix_spawn_file_actions_t *FileActions = nullptr;
 
@@ -270,7 +272,7 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
 
     return true;
   }
-#endif
+#endif // HAVE_POSIX_SPAWN
 
   // Create a child process.
   int child = fork();
@@ -307,6 +309,14 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
       }
     }
 
+    if (DetachProcess) {
+      // Detach from controlling terminal
+      if (::setsid() == -1) {
+        MakeErrMsg(ErrMsg, "Could not detach process, ::setsid failed");
+        return false;
+      }
+    }
+
     // Set memory limits
     if (MemoryLimit != 0) {
       SetMemoryLimits(MemoryLimit);
diff --git a/llvm/lib/Support/Windows/Program.inc b/llvm/lib/Support/Windows/Program.inc
index 0de9d3f7564481..d98d55f317a35a 100644
--- a/llvm/lib/Support/Windows/Program.inc
+++ b/llvm/lib/Support/Windows/Program.inc
@@ -172,10 +172,11 @@ static HANDLE RedirectIO(std::optional<StringRef> Path, int fd,
 } // namespace llvm
 
 static bool Execute(ProcessInfo &PI, StringRef Program,
-                    ArrayRef<StringRef> Args, std::optional<ArrayRef<StringRef>> Env,
+                    ArrayRef<StringRef> Args,
+                    std::optional<ArrayRef<StringRef>> Env,
                     ArrayRef<std::optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg,
-                    BitVector *AffinityMask) {
+                    BitVector *AffinityMask, bool DetachProcess) {
   if (!sys::fs::can_execute(Program)) {
     if (ErrMsg)
       *ErrMsg = "program not executable";
@@ -284,6 +285,8 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
   unsigned CreateFlags = CREATE_UNICODE_ENVIRONMENT;
   if (AffinityMask)
     CreateFlags |= CREATE_SUSPENDED;
+  if (DetachProcess)
+    CreateFlags |= DETACHED_PROCESS;
 
   std::vector<wchar_t> CommandUtf16(Command.size() + 1, 0);
   std::copy(Command.begin(), Command.end(), CommandUtf16.begin());
diff --git a/llvm/unittests/Support/ProgramTest.cpp b/llvm/unittests/Support/ProgramTest.cpp
index 2e2b1958b9ac9d..b1b35eacd1f618 100644
--- a/llvm/unittests/Support/ProgramTest.cpp
+++ b/llvm/unittests/Support/ProgramTest.cpp
@@ -260,6 +260,96 @@ TEST_F(ProgramEnvTest, TestExecuteNoWait) {
   ASSERT_GT(LoopCount, 1u) << "LoopCount should be >1";
 }
 
+TEST_F(ProgramEnvTest, TestExecuteNoWaitDetached) {
+  using namespace llvm::sys;
+
+  if (getenv("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT_DETACHED")) {
+    sleep_for(/*seconds=*/5);
+    char *Detached = getenv("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT_DETACHED_TRUE");
+#if _WIN32
+    HANDLE StdHandle = GetStdHandle(STD_OUTPUT_HANDLE);
+
+    if (Detached && (StdHandle == INVALID_HANDLE_VALUE || StdHandle == NULL))
+      exit(100);
+    if (!Detached && (StdHandle != INVALID_HANDLE_VALUE && StdHandle != NULL))
+      exit(200);
+#else
+    int ParentSID = std::stoi(
+        std::string(getenv("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT_DETACHED_SID")));
+
+    pid_t ChildSID = ::getsid(0);
+    if (ChildSID == -1) {
+      llvm::errs() << "Could not get process SID: " << strerror(errno) << '\n';
+      exit(1);
+    }
+
+    if (Detached && (ChildSID != ParentSID))
+      exit(100);
+    if (!Detached && (ChildSID == ParentSID))
+      exit(200);
+#endif
+    exit(0);
+  }
+
+  std::string Executable =
+      sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
+  StringRef argv[] = {
+      Executable, "--gtest_filter=ProgramEnvTest.TestExecuteNoWaitDetached"};
+  addEnvVar("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT_DETACHED=1");
+
+#if _WIN32
+  // Depending on how the test is run it may already be detached from a
+  // console. Temporarily allocate a new console. If a console already
+  // exists AllocConsole will harmlessly fail and return false
+  BOOL AllocConsoleSuccess = AllocConsole();
+
+  // Confirm existence of console
+  HANDLE StdHandle = GetStdHandle(STD_OUTPUT_HANDLE);
+  ASSERT_TRUE(StdHandle != INVALID_HANDLE_VALUE && StdHandle != NULL);
+#else
+  pid_t SID = ::getsid(0);
+  ASSERT_NE(SID, -1);
+  std::string SIDEnvVar =
+      "LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT_DETACHED_SID=" + std::to_string(SID);
+  addEnvVar(SIDEnvVar);
+#endif
+
+  // DetachProcess = true
+  {
+    std::string Error;
+    bool ExecutionFailed;
+    std::vector<llvm::StringRef> Env = getEnviron();
+    Env.emplace_back("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT_DETACHED_TRUE=1");
+    ProcessInfo PI1 =
+        ExecuteNoWait(Executable, argv, Env, {}, 0, &Error, &ExecutionFailed,
+                      nullptr, /*DetachProcess=*/true);
+    ASSERT_FALSE(ExecutionFailed) << Error;
+    ASSERT_NE(PI1.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
+    ProcessInfo WaitResult = Wait(PI1, std::nullopt, &Error);
+    ASSERT_EQ(WaitResult.ReturnCode, 100);
+  }
+
+  // DetachProcess = false
+  {
+    std::string Error;
+    bool ExecutionFailed;
+    ProcessInfo PI2 =
+        ExecuteNoWait(Executable, argv, getEnviron(), {}, 0, &Error,
+                      &ExecutionFailed, nullptr, /*DetachProcess=*/false);
+    ASSERT_FALSE(ExecutionFailed) << Error;
+    ASSERT_NE(PI2.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
+    ProcessInfo WaitResult = Wait(PI2, std::nullopt, &Error);
+    ASSERT_EQ(WaitResult.ReturnCode, 200);
+  }
+#if _WIN32
+  // If console was allocated then free the console
+  if (AllocConsoleSuccess) {
+    BOOL FreeConsoleSuccess = FreeConsole();
+    ASSERT_NE(FreeConsoleSuccess, 0);
+  }
+#endif
+}
+
 TEST_F(ProgramEnvTest, TestExecuteAndWaitTimeout) {
   using namespace llvm::sys;
 

@cpsughrue cpsughrue requested a review from aganea March 6, 2024 04:47
@cpsughrue cpsughrue merged commit 2fcf248 into llvm:main Mar 7, 2024
@cpsughrue cpsughrue deleted the detach-process branch April 10, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants