Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 9e44f0d

Browse filesBrowse files
labathlabath
authored
Reapply "[lldb] Inherit DuplicateFileAction(HANDLE, HANDLE) handles on windows (#137978)" (#138896)
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).
1 parent 80c61de commit 9e44f0d
Copy full SHA for 9e44f0d

File tree

Expand file treeCollapse file tree

5 files changed

+66
-23
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+66
-23
lines changed

‎lldb/source/Host/windows/PipeWindows.cpp

Copy file name to clipboardExpand all lines: lldb/source/Host/windows/PipeWindows.cpp
+6-4Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,9 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
8888
std::string pipe_path = g_pipe_name_prefix.str();
8989
pipe_path.append(name.str());
9090

91-
SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), 0,
92-
child_process_inherit ? TRUE : FALSE};
91+
// We always create inheritable handles, but we won't pass them to a child
92+
// process unless explicitly requested (cf. ProcessLauncherWindows.cpp).
93+
SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), 0, TRUE};
9394

9495
// Always open for overlapped i/o. We implement blocking manually in Read
9596
// and Write.
@@ -165,8 +166,9 @@ Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
165166

166167
assert(is_read ? !CanRead() : !CanWrite());
167168

168-
SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), 0,
169-
child_process_inherit ? TRUE : FALSE};
169+
// We always create inheritable handles, but we won't pass them to a child
170+
// process unless explicitly requested (cf. ProcessLauncherWindows.cpp).
171+
SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), 0, TRUE};
170172

171173
std::string pipe_path = g_pipe_name_prefix.str();
172174
pipe_path.append(name.str());

‎lldb/source/Host/windows/ProcessLauncherWindows.cpp

Copy file name to clipboardExpand all lines: lldb/source/Host/windows/ProcessLauncherWindows.cpp
+60-13Lines changed: 60 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "lldb/Host/HostProcess.h"
1111
#include "lldb/Host/ProcessLaunchInfo.h"
1212

13+
#include "llvm/ADT/ScopeExit.h"
1314
#include "llvm/ADT/SmallVector.h"
1415
#include "llvm/Support/ConvertUTF.h"
1516
#include "llvm/Support/Program.h"
@@ -65,14 +66,23 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
6566

6667
std::string executable;
6768
std::vector<char> environment;
68-
STARTUPINFO startupinfo = {};
69+
STARTUPINFOEX startupinfoex = {};
70+
STARTUPINFO &startupinfo = startupinfoex.StartupInfo;
6971
PROCESS_INFORMATION pi = {};
7072

7173
HANDLE stdin_handle = GetStdioHandle(launch_info, STDIN_FILENO);
7274
HANDLE stdout_handle = GetStdioHandle(launch_info, STDOUT_FILENO);
7375
HANDLE stderr_handle = GetStdioHandle(launch_info, STDERR_FILENO);
74-
75-
startupinfo.cb = sizeof(startupinfo);
76+
auto close_handles = llvm::make_scope_exit([&] {
77+
if (stdin_handle)
78+
::CloseHandle(stdin_handle);
79+
if (stdout_handle)
80+
::CloseHandle(stdout_handle);
81+
if (stderr_handle)
82+
::CloseHandle(stderr_handle);
83+
});
84+
85+
startupinfo.cb = sizeof(startupinfoex);
7686
startupinfo.dwFlags |= STARTF_USESTDHANDLES;
7787
startupinfo.hStdError =
7888
stderr_handle ? stderr_handle : ::GetStdHandle(STD_ERROR_HANDLE);
@@ -81,6 +91,48 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
8191
startupinfo.hStdOutput =
8292
stdout_handle ? stdout_handle : ::GetStdHandle(STD_OUTPUT_HANDLE);
8393

94+
std::vector<HANDLE> inherited_handles;
95+
if (startupinfo.hStdError)
96+
inherited_handles.push_back(startupinfo.hStdError);
97+
if (startupinfo.hStdInput)
98+
inherited_handles.push_back(startupinfo.hStdInput);
99+
if (startupinfo.hStdOutput)
100+
inherited_handles.push_back(startupinfo.hStdOutput);
101+
102+
size_t attributelist_size = 0;
103+
InitializeProcThreadAttributeList(/*lpAttributeList=*/nullptr,
104+
/*dwAttributeCount=*/1, /*dwFlags=*/0,
105+
&attributelist_size);
106+
107+
startupinfoex.lpAttributeList =
108+
static_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(malloc(attributelist_size));
109+
auto free_attributelist =
110+
llvm::make_scope_exit([&] { free(startupinfoex.lpAttributeList); });
111+
if (!InitializeProcThreadAttributeList(startupinfoex.lpAttributeList,
112+
/*dwAttributeCount=*/1, /*dwFlags=*/0,
113+
&attributelist_size)) {
114+
error = Status(::GetLastError(), eErrorTypeWin32);
115+
return HostProcess();
116+
}
117+
auto delete_attributelist = llvm::make_scope_exit(
118+
[&] { DeleteProcThreadAttributeList(startupinfoex.lpAttributeList); });
119+
for (size_t i = 0; i < launch_info.GetNumFileActions(); ++i) {
120+
const FileAction *act = launch_info.GetFileActionAtIndex(i);
121+
if (act->GetAction() == FileAction::eFileActionDuplicate &&
122+
act->GetFD() == act->GetActionArgument())
123+
inherited_handles.push_back(reinterpret_cast<HANDLE>(act->GetFD()));
124+
}
125+
if (!inherited_handles.empty()) {
126+
if (!UpdateProcThreadAttribute(
127+
startupinfoex.lpAttributeList, /*dwFlags=*/0,
128+
PROC_THREAD_ATTRIBUTE_HANDLE_LIST, inherited_handles.data(),
129+
inherited_handles.size() * sizeof(HANDLE),
130+
/*lpPreviousValue=*/nullptr, /*lpReturnSize=*/nullptr)) {
131+
error = Status(::GetLastError(), eErrorTypeWin32);
132+
return HostProcess();
133+
}
134+
}
135+
84136
const char *hide_console_var =
85137
getenv("LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE");
86138
if (hide_console_var &&
@@ -89,7 +141,8 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
89141
startupinfo.wShowWindow = SW_HIDE;
90142
}
91143

92-
DWORD flags = CREATE_NEW_CONSOLE | CREATE_UNICODE_ENVIRONMENT;
144+
DWORD flags = CREATE_NEW_CONSOLE | CREATE_UNICODE_ENVIRONMENT |
145+
EXTENDED_STARTUPINFO_PRESENT;
93146
if (launch_info.GetFlags().Test(eLaunchFlagDebug))
94147
flags |= DEBUG_ONLY_THIS_PROCESS;
95148

@@ -114,9 +167,10 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
114167
WCHAR *pwcommandLine = wcommandLine.empty() ? nullptr : &wcommandLine[0];
115168

116169
BOOL result = ::CreateProcessW(
117-
wexecutable.c_str(), pwcommandLine, NULL, NULL, TRUE, flags, env_block,
170+
wexecutable.c_str(), pwcommandLine, NULL, NULL,
171+
/*bInheritHandles=*/!inherited_handles.empty(), flags, env_block,
118172
wworkingDirectory.size() == 0 ? NULL : wworkingDirectory.c_str(),
119-
&startupinfo, &pi);
173+
reinterpret_cast<STARTUPINFO *>(&startupinfoex), &pi);
120174

121175
if (!result) {
122176
// Call GetLastError before we make any other system calls.
@@ -131,13 +185,6 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
131185
::CloseHandle(pi.hThread);
132186
}
133187

134-
if (stdin_handle)
135-
::CloseHandle(stdin_handle);
136-
if (stdout_handle)
137-
::CloseHandle(stdout_handle);
138-
if (stderr_handle)
139-
::CloseHandle(stderr_handle);
140-
141188
if (!result)
142189
return HostProcess();
143190

‎lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

Copy file name to clipboardExpand all lines: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -924,9 +924,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
924924
debugserver_args.AppendArgument(fd_arg.GetString());
925925
// Send "pass_comm_fd" down to the inferior so it can use it to
926926
// communicate back with this process. Ignored on Windows.
927-
#ifndef _WIN32
928927
launch_info.AppendDuplicateFileAction((int)pass_comm_fd, (int)pass_comm_fd);
929-
#endif
930928
}
931929

932930
// use native registers, not the GDB registers

‎lldb/tools/lldb-server/lldb-platform.cpp

Copy file name to clipboardExpand all lines: lldb/tools/lldb-server/lldb-platform.cpp
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -274,10 +274,8 @@ static Status spawn_process(const char *progname, const FileSpec &prog,
274274
self_args.AppendArgument(llvm::StringRef("platform"));
275275
self_args.AppendArgument(llvm::StringRef("--child-platform-fd"));
276276
self_args.AppendArgument(llvm::to_string(shared_socket.GetSendableFD()));
277-
#ifndef _WIN32
278277
launch_info.AppendDuplicateFileAction((int)shared_socket.GetSendableFD(),
279278
(int)shared_socket.GetSendableFD());
280-
#endif
281279
if (gdb_port) {
282280
self_args.AppendArgument(llvm::StringRef("--gdbserver-port"));
283281
self_args.AppendArgument(llvm::to_string(gdb_port));

‎lldb/unittests/Host/HostTest.cpp

Copy file name to clipboardExpand all lines: lldb/unittests/Host/HostTest.cpp
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,6 @@ TEST(Host, LaunchProcessSetsArgv0) {
9090
ASSERT_THAT(exit_status.get_future().get(), 0);
9191
}
9292

93-
#ifdef LLVM_ON_UNIX
9493
TEST(Host, LaunchProcessDuplicatesHandle) {
9594
static constexpr llvm::StringLiteral test_msg("Hello subprocess!");
9695

@@ -130,4 +129,3 @@ TEST(Host, LaunchProcessDuplicatesHandle) {
130129
ASSERT_THAT_EXPECTED(bytes_read, llvm::Succeeded());
131130
ASSERT_EQ(llvm::StringRef(msg, *bytes_read), test_msg);
132131
}
133-
#endif

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.