From f410f74cd5b26319b5796e0404c6a0f3b5cc00a5 Mon Sep 17 00:00:00 2001 From: cpsughrue Date: Mon, 26 Feb 2024 21:40:21 -0500 Subject: [PATCH] Revert "[llvm][Support] Add support for executing a detached process (#81708)" This reverts commit 86f6caa562255f81b93e72a501a926b17f5ad244. Unit test was failing on a few windows build bots --- llvm/include/llvm/Support/Program.h | 25 ++++----- llvm/lib/Support/Program.cpp | 9 ++-- llvm/lib/Support/Unix/Program.inc | 18 ++----- llvm/lib/Support/Windows/Program.inc | 7 +-- llvm/unittests/Support/ProgramTest.cpp | 74 -------------------------- 5 files changed, 21 insertions(+), 112 deletions(-) diff --git a/llvm/include/llvm/Support/Program.h b/llvm/include/llvm/Support/Program.h index 9df94eb604c7d6..4c1133e44a21c9 100644 --- a/llvm/include/llvm/Support/Program.h +++ b/llvm/include/llvm/Support/Program.h @@ -141,21 +141,18 @@ namespace sys { /// program shall run on. ); - /// Similar to \ref ExecuteAndWait, but returns immediately. - /// \returns The \ref ProcessInfo of the newly launched process. + /// Similar to ExecuteAndWait, but returns immediately. + /// @returns The \see ProcessInfo of the newly launched process. /// \note On Microsoft Windows systems, users will need to either call - /// \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 Args, - std::optional> Env, - ArrayRef> 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); + /// \see Wait until the process finished execution or win32 CloseHandle() API + /// on ProcessInfo.ProcessHandle to avoid memory leaks. + ProcessInfo ExecuteNoWait(StringRef Program, ArrayRef Args, + std::optional> Env, + ArrayRef> Redirects = {}, + unsigned MemoryLimit = 0, + std::string *ErrMsg = nullptr, + bool *ExecutionFailed = nullptr, + BitVector *AffinityMask = nullptr); /// 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 181f68cfbb8c3f..1dcd45e2d69e89 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> Env, ArrayRef> Redirects, unsigned MemoryLimit, std::string *ErrMsg, - BitVector *AffinityMask, bool DetachProcess); + BitVector *AffinityMask); int sys::ExecuteAndWait(StringRef Program, ArrayRef Args, std::optional> Env, @@ -39,7 +39,7 @@ int sys::ExecuteAndWait(StringRef Program, ArrayRef Args, assert(Redirects.empty() || Redirects.size() == 3); ProcessInfo PI; if (Execute(PI, Program, Args, Env, Redirects, MemoryLimit, ErrMsg, - AffinityMask, /*DetachProcess=*/false)) { + AffinityMask)) { if (ExecutionFailed) *ExecutionFailed = false; ProcessInfo Result = Wait( @@ -58,14 +58,13 @@ ProcessInfo sys::ExecuteNoWait(StringRef Program, ArrayRef Args, std::optional> Env, ArrayRef> Redirects, unsigned MemoryLimit, std::string *ErrMsg, - bool *ExecutionFailed, BitVector *AffinityMask, - bool DetachProcess) { + bool *ExecutionFailed, BitVector *AffinityMask) { assert(Redirects.empty() || Redirects.size() == 3); ProcessInfo PI; if (ExecutionFailed) *ExecutionFailed = false; if (!Execute(PI, Program, Args, Env, Redirects, MemoryLimit, ErrMsg, - AffinityMask, DetachProcess)) + AffinityMask)) if (ExecutionFailed) *ExecutionFailed = true; diff --git a/llvm/lib/Support/Unix/Program.inc b/llvm/lib/Support/Unix/Program.inc index 2742734bb11ed0..5d9757bcc51b3e 100644 --- a/llvm/lib/Support/Unix/Program.inc +++ b/llvm/lib/Support/Unix/Program.inc @@ -173,11 +173,10 @@ toNullTerminatedCStringArray(ArrayRef Strings, StringSaver &Saver) { } static bool Execute(ProcessInfo &PI, StringRef Program, - ArrayRef Args, - std::optional> Env, + ArrayRef Args, std::optional> Env, ArrayRef> Redirects, unsigned MemoryLimit, std::string *ErrMsg, - BitVector *AffinityMask, bool DetachProcess) { + BitVector *AffinityMask) { if (!llvm::sys::fs::exists(Program)) { if (ErrMsg) *ErrMsg = std::string("Executable \"") + Program.str() + @@ -203,8 +202,7 @@ 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 - // Cannot use posix_spawn if you would like to detach the process - if (MemoryLimit == 0 && !DetachProcess) { + if (MemoryLimit == 0) { posix_spawn_file_actions_t FileActionsStore; posix_spawn_file_actions_t *FileActions = nullptr; @@ -272,7 +270,7 @@ static bool Execute(ProcessInfo &PI, StringRef Program, return true; } -#endif // HAVE_POSIX_SPAWN +#endif // Create a child process. int child = fork(); @@ -309,14 +307,6 @@ 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 d98d55f317a35a..0de9d3f7564481 100644 --- a/llvm/lib/Support/Windows/Program.inc +++ b/llvm/lib/Support/Windows/Program.inc @@ -172,11 +172,10 @@ static HANDLE RedirectIO(std::optional Path, int fd, } // namespace llvm static bool Execute(ProcessInfo &PI, StringRef Program, - ArrayRef Args, - std::optional> Env, + ArrayRef Args, std::optional> Env, ArrayRef> Redirects, unsigned MemoryLimit, std::string *ErrMsg, - BitVector *AffinityMask, bool DetachProcess) { + BitVector *AffinityMask) { if (!sys::fs::can_execute(Program)) { if (ErrMsg) *ErrMsg = "program not executable"; @@ -285,8 +284,6 @@ static bool Execute(ProcessInfo &PI, StringRef Program, unsigned CreateFlags = CREATE_UNICODE_ENVIRONMENT; if (AffinityMask) CreateFlags |= CREATE_SUSPENDED; - if (DetachProcess) - CreateFlags |= DETACHED_PROCESS; std::vector 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 58f98892c9cc72..2e2b1958b9ac9d 100644 --- a/llvm/unittests/Support/ProgramTest.cpp +++ b/llvm/unittests/Support/ProgramTest.cpp @@ -260,80 +260,6 @@ 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); - -#if _WIN32 - HWND ConsoleWnd = GetConsoleWindow(); - if (ConsoleWnd == NULL) - exit(100); - else - 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); - } - - char *Detached = getenv("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT_DETACHED_TRUE"); - 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"); - -#ifndef _WIN32 - 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 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); - } -} - TEST_F(ProgramEnvTest, TestExecuteAndWaitTimeout) { using namespace llvm::sys;