[llvm][Support] Add support for executing a detached process #81708

cpsughrue · 2024-02-14T05:56:53Z

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 · 2024-02-14T05:57:22Z

@llvm/pr-subscribers-llvm-support

Author: Connor Sughrue (cpsughrue)

Changes

Added a new parameter, bool DetachProcess with a default option of false, to llvm::sys::ExecuteNoWait, which, when set to true, executes the specified program in a detached state.

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

4 Files Affected:

(modified) llvm/include/llvm/Support/Program.h (+2-1)
(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)

diff --git a/llvm/include/llvm/Support/Program.h b/llvm/include/llvm/Support/Program.h
index 4c1133e44a21c9..a6a897704f2081 100644
--- a/llvm/include/llvm/Support/Program.h
+++ b/llvm/include/llvm/Support/Program.h
@@ -152,7 +152,8 @@ namespace sys {
                             unsigned MemoryLimit = 0,
                             std::string *ErrMsg = nullptr,
                             bool *ExecutionFailed = nullptr,
-                            BitVector *AffinityMask = nullptr);
+                            BitVector *AffinityMask = nullptr,
+                            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..642f6e73f32a77 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..56f653a165bfb5 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, "Couldn't 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());

llvmbot · 2024-02-14T05:57:22Z

@llvm/pr-subscribers-platform-windows

Author: Connor Sughrue (cpsughrue)

Changes

Added a new parameter, bool DetachProcess with a default option of false, to llvm::sys::ExecuteNoWait, which, when set to true, executes the specified program in a detached state.

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

4 Files Affected:

(modified) llvm/include/llvm/Support/Program.h (+2-1)
(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)

diff --git a/llvm/include/llvm/Support/Program.h b/llvm/include/llvm/Support/Program.h
index 4c1133e44a21c9..a6a897704f2081 100644
--- a/llvm/include/llvm/Support/Program.h
+++ b/llvm/include/llvm/Support/Program.h
@@ -152,7 +152,8 @@ namespace sys {
                             unsigned MemoryLimit = 0,
                             std::string *ErrMsg = nullptr,
                             bool *ExecutionFailed = nullptr,
-                            BitVector *AffinityMask = nullptr);
+                            BitVector *AffinityMask = nullptr,
+                            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..642f6e73f32a77 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..56f653a165bfb5 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, "Couldn't 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());

aganea

Would you mind please explaining what is the purpose of this change? (in the description ideally) ~~I know it's for the LLVM CAS but I'm not sure I understand exactly what is it for.~~

llvm/lib/Support/Program.cpp

aganea · 2024-02-14T13:07:13Z

llvm/lib/Support/Windows/Program.inc

@@ -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;


I don't know for Unix, but on Windows this flag prevents the I/O streams to be open on process startup, so any attempt to use llvm::outs, llvm::errs or stdin will most likely fail. Is that the intent?

Another way for executing "detached" processes is to use ShellExecuteEx: https://learn.microsoft.com/en-us/windows/win32/shell/launch

I would not say I am intentionally trying to prevent I/O streams from opening on process startup but the goal is to launch a process without a controlling terminal. When using DETACHED_PROCESS I do not expect llvm::outs, llvm::errs, or stdin to be useful until they have been redirected to a file (or another output location).

Regarding ShellExecuteEx is looks like I can have the process create it's own console but I don't see a way to execute a process without a console (https://learn.microsoft.com/en-us/windows/win32/api/shellapi/ns-shellapi-shellexecuteinfoa). I could be missing something.

We discussed using ShellExecute, but it still gives you a console. The goal here is to not have a console at all. DETACHED_PROCESS gives the correct semantics for a daemon spawned by any given clang instance that doesn't belong to any specific instance.

Understood, SGTM.

cpsughrue · 2024-02-14T16:50:08Z

Would you mind please explaining what is the purpose of this change? (in the description ideally) ~~I know it's for the LLVM CAS but I'm not sure I understand exactly what is it for.~~

I will add this to the description but I am working on a daemon that will make building modules easier and more efficient (https://discourse.llvm.org/t/rfc-modules-build-daemon-build-system-agnostic-support-for-explicitly-built-modules/71524). The module build daemon needs to be executed in a detached state so that it continues to run even when the clang invocation that spawned it terminates. Here is the first module build daemon pull request for context (#67562).

cpsughrue · 2024-02-14T16:52:36Z

FYI I am currently working on writing a test for this

cpsughrue · 2024-02-20T04:19:23Z

Here is a before and after for the doxygen changes

Pulled from llvm.org

Generated locally

EDIT: I realized that my generated doxygen does not have the last section with source location and references. SOURCE_BROWSER is set to YES in doxygen.cfg, so I am unsure why at least the source locations aren't there.

aganea · 2024-02-20T11:54:20Z

Looks good so far. Could you possibly add a unit test like

llvm-project/llvm/unittests/Support/ProgramTest.cpp

Line 287 in 26d4afc

TEST_F(ProgramEnvTest, TestExecuteNoWaitTimeoutPolling) {

please?

cpsughrue · 2024-02-20T16:29:37Z

Looks good so far. Could you possibly add a unit test like

llvm-project/llvm/unittests/Support/ProgramTest.cpp

Line 287 in 26d4afc

TEST_F(ProgramEnvTest, TestExecuteNoWaitTimeoutPolling) {

please?

Definitely! I have a test I am working on locally that pretty much copies the example you provided. For some reasons the test hangs when I do not include the do while loop at the end and passes without issue when I include the do while loop. Once I get that sorted out I will push the unittest.

github-actions · 2024-02-26T02:50:17Z

✅ With the latest revision this PR passed the C/C++ code formatter.

cpsughrue · 2024-02-26T14:31:41Z

@aganea I have pushed the unit test. If you have a chance can you let me know what you think? It looks like the CI failed due to an unrelated flang test.

aganea

Just a small nit. LGTM otherwise, thank you!

aganea · 2024-02-26T14:45:17Z

llvm/unittests/Support/ProgramTest.cpp

+    std::string Error;
+    bool ExecutionFailed;
+    ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(), {}, 0,
+                                    &Error, &ExecutionFailed, nullptr);


I would add , /*DetachedProcess=*/false to make it explicit what we're testing, and not rely on a default value elsewhere.

I left out /*DetachProcess=*/false intentionally to codify that ExecuteNoWait's default value for DetachProcess should be false. Happy to make that change though, any feedback is always appreciated.

…81708)" This reverts commit 86f6caa. Unit test was failing on a few windows build bots

…81708)" (#83367) 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 added platform:windows llvm:support labels Feb 14, 2024

aganea reviewed Feb 14, 2024

View reviewed changes

aganea requested review from mstorsjo and MaskRay February 14, 2024 13:09

cpsughrue added 3 commits February 25, 2024 16:25

Add support for executing a detached process

9a5b27d

Correct inline comment format

cffa46e

Clean up doxygen comments for llvm::sys::ExecuteNoWait

69bb682

cpsughrue force-pushed the detach-process branch 3 times, most recently from f261d5a to e19962e Compare February 26, 2024 02:47

Add unittest

3372093

cpsughrue force-pushed the detach-process branch from e19962e to 3372093 Compare February 26, 2024 02:53

aganea approved these changes Feb 26, 2024

View reviewed changes

Explicitly set DetachProcess to false in unit test

a3aca38

cpsughrue merged commit 86f6caa into llvm:main Feb 27, 2024
3 of 4 checks passed

cpsughrue added a commit that referenced this pull request Feb 27, 2024

Revert "[llvm][Support] Add support for executing a detached process (#…

f410f74

…81708)" This reverts commit 86f6caa. Unit test was failing on a few windows build bots

This was referenced Feb 29, 2024

Not all precommit CI unittests are always run #83371

Closed

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm][Support] Add support for executing a detached process #81708

[llvm][Support] Add support for executing a detached process #81708

cpsughrue commented Feb 14, 2024 •

edited

llvmbot commented Feb 14, 2024

llvmbot commented Feb 14, 2024

aganea left a comment •

edited

aganea Feb 14, 2024 •

edited

aganea Feb 14, 2024

cpsughrue Feb 14, 2024

Bigcheese Feb 14, 2024

aganea Feb 14, 2024

cpsughrue commented Feb 14, 2024

cpsughrue commented Feb 14, 2024

cpsughrue commented Feb 20, 2024 •

edited

aganea commented Feb 20, 2024

cpsughrue commented Feb 20, 2024

github-actions bot commented Feb 26, 2024 •

edited

cpsughrue commented Feb 26, 2024

aganea left a comment

aganea Feb 26, 2024

cpsughrue Feb 26, 2024 •

edited

[llvm][Support] Add support for executing a detached process #81708

[llvm][Support] Add support for executing a detached process #81708

Conversation

cpsughrue commented Feb 14, 2024 • edited

llvmbot commented Feb 14, 2024

llvmbot commented Feb 14, 2024

aganea left a comment • edited

Choose a reason for hiding this comment

aganea Feb 14, 2024 • edited

Choose a reason for hiding this comment

aganea Feb 14, 2024

Choose a reason for hiding this comment

cpsughrue Feb 14, 2024

Choose a reason for hiding this comment

Bigcheese Feb 14, 2024

Choose a reason for hiding this comment

aganea Feb 14, 2024

Choose a reason for hiding this comment

cpsughrue commented Feb 14, 2024

cpsughrue commented Feb 14, 2024

cpsughrue commented Feb 20, 2024 • edited

aganea commented Feb 20, 2024

cpsughrue commented Feb 20, 2024

github-actions bot commented Feb 26, 2024 • edited

cpsughrue commented Feb 26, 2024

aganea left a comment

Choose a reason for hiding this comment

aganea Feb 26, 2024

Choose a reason for hiding this comment

cpsughrue Feb 26, 2024 • edited

Choose a reason for hiding this comment

cpsughrue commented Feb 14, 2024 •

edited

aganea left a comment •

edited

aganea Feb 14, 2024 •

edited

cpsughrue commented Feb 20, 2024 •

edited

github-actions bot commented Feb 26, 2024 •

edited

cpsughrue Feb 26, 2024 •

edited