Reapply "[lldb] Inherit DuplicateFileAction(HANDLE, HANDLE) handles on windows (#137978)" #138896
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n windows (llvm#137978)" This reverts commit a0260a9, reapplying 7c5f5f3, with a fix that makes *both* pipe handles inheritable. The original commit description was: This is a follow-up to llvm#126935, which enables passing handles to a child process on windows systems. Unlike on unix-like systems, the handles need to be created with the "inheritable" flag because there's to way to change the flag value after it has been created. This is why I don't respect the child_process_inherit flag but rather always set the flag to true. (My next step is to delete the flag entirely.) This does mean that pipe may be created as inheritable even if its not necessary, but I think this is offset by the fact that windows (unlike unixes, which pass all ~O_CLOEXEC descriptors through execve and *all* descriptors through fork) has a way to specify the precise set of handles to pass to a specific child process. If this turns out to be insufficient, instead of a constructor flag, I'd rather go with creating a separate api to create an inheritable copy of a handle (as typically, you only want to inherit one end of the pipe).
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThis reverts commit a0260a9, reapplying The original commit description was: This is a follow-up to #126935, This does mean that pipe may be created as inheritable even if its not If this turns out to be insufficient, instead of a constructor flag, I'd Full diff: https://github.com/llvm/llvm-project/pull/138896.diff 5 Files Affected:
diff --git a/lldb/source/Host/windows/PipeWindows.cpp b/lldb/source/Host/windows/PipeWindows.cpp
index e3f5b629a0590..30b9d1c41695b 100644
--- a/lldb/source/Host/windows/PipeWindows.cpp
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -88,8 +88,9 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
std::string pipe_path = g_pipe_name_prefix.str();
pipe_path.append(name.str());
- SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), 0,
- child_process_inherit ? TRUE : FALSE};
+ // We always create inheritable handles, but we won't pass them to a child
+ // process unless explicitly requested (cf. ProcessLauncherWindows.cpp).
+ SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), 0, TRUE};
// Always open for overlapped i/o. We implement blocking manually in Read
// and Write.
@@ -165,8 +166,9 @@ Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
assert(is_read ? !CanRead() : !CanWrite());
- SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), 0,
- child_process_inherit ? TRUE : FALSE};
+ // We always create inheritable handles, but we won't pass them to a child
+ // process unless explicitly requested (cf. ProcessLauncherWindows.cpp).
+ SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), 0, TRUE};
std::string pipe_path = g_pipe_name_prefix.str();
pipe_path.append(name.str());
diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
index 065ba9271ad0d..bc35667ea9a23 100644
--- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -10,6 +10,7 @@
#include "lldb/Host/HostProcess.h"
#include "lldb/Host/ProcessLaunchInfo.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/ConvertUTF.h"
#include "llvm/Support/Program.h"
@@ -65,14 +66,23 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
std::string executable;
std::vector<char> environment;
- STARTUPINFO startupinfo = {};
+ STARTUPINFOEX startupinfoex = {};
+ STARTUPINFO &startupinfo = startupinfoex.StartupInfo;
PROCESS_INFORMATION pi = {};
HANDLE stdin_handle = GetStdioHandle(launch_info, STDIN_FILENO);
HANDLE stdout_handle = GetStdioHandle(launch_info, STDOUT_FILENO);
HANDLE stderr_handle = GetStdioHandle(launch_info, STDERR_FILENO);
-
- startupinfo.cb = sizeof(startupinfo);
+ auto close_handles = llvm::make_scope_exit([&] {
+ if (stdin_handle)
+ ::CloseHandle(stdin_handle);
+ if (stdout_handle)
+ ::CloseHandle(stdout_handle);
+ if (stderr_handle)
+ ::CloseHandle(stderr_handle);
+ });
+
+ startupinfo.cb = sizeof(startupinfoex);
startupinfo.dwFlags |= STARTF_USESTDHANDLES;
startupinfo.hStdError =
stderr_handle ? stderr_handle : ::GetStdHandle(STD_ERROR_HANDLE);
@@ -81,6 +91,48 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
startupinfo.hStdOutput =
stdout_handle ? stdout_handle : ::GetStdHandle(STD_OUTPUT_HANDLE);
+ std::vector<HANDLE> inherited_handles;
+ if (startupinfo.hStdError)
+ inherited_handles.push_back(startupinfo.hStdError);
+ if (startupinfo.hStdInput)
+ inherited_handles.push_back(startupinfo.hStdInput);
+ if (startupinfo.hStdOutput)
+ inherited_handles.push_back(startupinfo.hStdOutput);
+
+ size_t attributelist_size = 0;
+ InitializeProcThreadAttributeList(/*lpAttributeList=*/nullptr,
+ /*dwAttributeCount=*/1, /*dwFlags=*/0,
+ &attributelist_size);
+
+ startupinfoex.lpAttributeList =
+ static_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(malloc(attributelist_size));
+ auto free_attributelist =
+ llvm::make_scope_exit([&] { free(startupinfoex.lpAttributeList); });
+ if (!InitializeProcThreadAttributeList(startupinfoex.lpAttributeList,
+ /*dwAttributeCount=*/1, /*dwFlags=*/0,
+ &attributelist_size)) {
+ error = Status(::GetLastError(), eErrorTypeWin32);
+ return HostProcess();
+ }
+ auto delete_attributelist = llvm::make_scope_exit(
+ [&] { DeleteProcThreadAttributeList(startupinfoex.lpAttributeList); });
+ for (size_t i = 0; i < launch_info.GetNumFileActions(); ++i) {
+ const FileAction *act = launch_info.GetFileActionAtIndex(i);
+ if (act->GetAction() == FileAction::eFileActionDuplicate &&
+ act->GetFD() == act->GetActionArgument())
+ inherited_handles.push_back(reinterpret_cast<HANDLE>(act->GetFD()));
+ }
+ if (!inherited_handles.empty()) {
+ if (!UpdateProcThreadAttribute(
+ startupinfoex.lpAttributeList, /*dwFlags=*/0,
+ PROC_THREAD_ATTRIBUTE_HANDLE_LIST, inherited_handles.data(),
+ inherited_handles.size() * sizeof(HANDLE),
+ /*lpPreviousValue=*/nullptr, /*lpReturnSize=*/nullptr)) {
+ error = Status(::GetLastError(), eErrorTypeWin32);
+ return HostProcess();
+ }
+ }
+
const char *hide_console_var =
getenv("LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE");
if (hide_console_var &&
@@ -89,7 +141,8 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
startupinfo.wShowWindow = SW_HIDE;
}
- DWORD flags = CREATE_NEW_CONSOLE | CREATE_UNICODE_ENVIRONMENT;
+ DWORD flags = CREATE_NEW_CONSOLE | CREATE_UNICODE_ENVIRONMENT |
+ EXTENDED_STARTUPINFO_PRESENT;
if (launch_info.GetFlags().Test(eLaunchFlagDebug))
flags |= DEBUG_ONLY_THIS_PROCESS;
@@ -114,9 +167,10 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
WCHAR *pwcommandLine = wcommandLine.empty() ? nullptr : &wcommandLine[0];
BOOL result = ::CreateProcessW(
- wexecutable.c_str(), pwcommandLine, NULL, NULL, TRUE, flags, env_block,
+ wexecutable.c_str(), pwcommandLine, NULL, NULL,
+ /*bInheritHandles=*/!inherited_handles.empty(), flags, env_block,
wworkingDirectory.size() == 0 ? NULL : wworkingDirectory.c_str(),
- &startupinfo, &pi);
+ reinterpret_cast<STARTUPINFO *>(&startupinfoex), &pi);
if (!result) {
// Call GetLastError before we make any other system calls.
@@ -131,13 +185,6 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
::CloseHandle(pi.hThread);
}
- if (stdin_handle)
- ::CloseHandle(stdin_handle);
- if (stdout_handle)
- ::CloseHandle(stdout_handle);
- if (stderr_handle)
- ::CloseHandle(stderr_handle);
-
if (!result)
return HostProcess();
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index d8c7e436f3f8b..332b9255f226f 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -924,9 +924,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
debugserver_args.AppendArgument(fd_arg.GetString());
// Send "pass_comm_fd" down to the inferior so it can use it to
// communicate back with this process. Ignored on Windows.
-#ifndef _WIN32
launch_info.AppendDuplicateFileAction((int)pass_comm_fd, (int)pass_comm_fd);
-#endif
}
// use native registers, not the GDB registers
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 10d79c63af994..5b0a8ade01025 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -274,10 +274,8 @@ static Status spawn_process(const char *progname, const FileSpec &prog,
self_args.AppendArgument(llvm::StringRef("platform"));
self_args.AppendArgument(llvm::StringRef("--child-platform-fd"));
self_args.AppendArgument(llvm::to_string(shared_socket.GetSendableFD()));
-#ifndef _WIN32
launch_info.AppendDuplicateFileAction((int)shared_socket.GetSendableFD(),
(int)shared_socket.GetSendableFD());
-#endif
if (gdb_port) {
self_args.AppendArgument(llvm::StringRef("--gdbserver-port"));
self_args.AppendArgument(llvm::to_string(gdb_port));
diff --git a/lldb/unittests/Host/HostTest.cpp b/lldb/unittests/Host/HostTest.cpp
index 9306a868ebe35..52224bfd28e61 100644
--- a/lldb/unittests/Host/HostTest.cpp
+++ b/lldb/unittests/Host/HostTest.cpp
@@ -90,7 +90,6 @@ TEST(Host, LaunchProcessSetsArgv0) {
ASSERT_THAT(exit_status.get_future().get(), 0);
}
-#ifdef LLVM_ON_UNIX
TEST(Host, LaunchProcessDuplicatesHandle) {
static constexpr llvm::StringLiteral test_msg("Hello subprocess!");
@@ -130,4 +129,3 @@ TEST(Host, LaunchProcessDuplicatesHandle) {
ASSERT_THAT_EXPECTED(bytes_read, llvm::Succeeded());
ASSERT_EQ(llvm::StringRef(msg, *bytes_read), test_msg);
}
-#endif
|
|
I'm not sure how this happened -- I thought I had tested the final form of the patch. I can only assume some of the changes got lost while shuttling the patch back and forth between systems. |
|
(the fix is in the second commit) |
slydiman
approved these changes
May 7, 2025
There was a problem hiding this comment.
I have tested the failed Host.LaunchProcessDuplicatesHandle test on the setup similar to lldb-remote-linux-win. It is fixed now.
|
Hi Pavel! Unfortunately, it seems like this change has broken compilation of LLDB for mingw, on all architectures. With MSVC and clang-cl, it is broken for 32 bit platforms, while the issue only shows up as a warning for 64 bit architectures there. On 64 bit mingw: On 32 bit mingw: On 32 bit clang-cl: 32 bit MSVC: On 64 bit clang-cl, it is only a warning: And the same for 64 bit MSVC: |
|
Thanks. I think I can fix that. Let me whip something up. |
labath
added a commit
to labath/llvm-project
that referenced
this pull request
May 13, 2025
- s/size_t/SIZE_T to match the windows API - case HANDLE to int64_t to avoid cast-to-int-of-different-size errors/warnings
labath
added a commit
to labath/llvm-project
that referenced
this pull request
May 13, 2025
- s/size_t/SIZE_T to match the windows API - case HANDLE to int64_t to avoid cast-to-int-of-different-size errors/warnings
labath
added a commit
that referenced
this pull request
May 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit a0260a9, reapplying
7c5f5f3, with a fix that makes both
pipe handles inheritable.
The original commit description was:
This is a follow-up to #126935,
which enables passing handles to a child
process on windows systems. Unlike on unix-like systems, the handles
need to be created with the "inheritable" flag because there's to way to
change the flag value after it has been created. This is why I don't
respect the child_process_inherit flag but rather always set the flag to
true. (My next step is to delete the flag entirely.)
This does mean that pipe may be created as inheritable even if its not
necessary, but I think this is offset by the fact that windows (unlike
unixes, which pass all ~O_CLOEXEC descriptors through execve and all
descriptors through fork) has a way to specify the precise set of
handles to pass to a specific child process.
If this turns out to be insufficient, instead of a constructor flag, I'd
rather go with creating a separate api to create an inheritable copy of
a handle (as typically, you only want to inherit one end of the pipe).