Skip to content

Commit b51c03f

Browse files
labathlabath
committed
[lldb] Remove child_process_inherit argument from Pipe
It's not necessary on posix platforms as of llvm#126935 and it's ignored on windows as of llvm#138896. For both platforms, we have a better way of inheriting FDs/HANDLEs.
1 parent 4d2b79b commit b51c03f

File tree

18 files changed

+59
-91
lines changed

18 files changed

+59
-91
lines changed

‎lldb/include/lldb/Host/PipeBase.h

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,14 @@ class PipeBase {
2121
public:
2222
virtual ~PipeBase();
2323

24-
virtual Status CreateNew(bool child_process_inherit) = 0;
25-
virtual Status CreateNew(llvm::StringRef name,
26-
bool child_process_inherit) = 0;
24+
virtual Status CreateNew() = 0;
25+
virtual Status CreateNew(llvm::StringRef name) = 0;
2726
virtual Status CreateWithUniqueName(llvm::StringRef prefix,
28-
bool child_process_inherit,
2927
llvm::SmallVectorImpl<char> &name) = 0;
3028

31-
virtual Status OpenAsReader(llvm::StringRef name,
32-
bool child_process_inherit) = 0;
29+
virtual Status OpenAsReader(llvm::StringRef name) = 0;
3330

3431
virtual llvm::Error OpenAsWriter(llvm::StringRef name,
35-
bool child_process_inherit,
3632
const Timeout<std::micro> &timeout) = 0;
3733

3834
virtual bool CanRead() const = 0;

‎lldb/include/lldb/Host/posix/PipePosix.h

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,12 @@ class PipePosix : public PipeBase {
3232

3333
~PipePosix() override;
3434

35-
Status CreateNew(bool child_process_inherit) override;
36-
Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
35+
Status CreateNew() override;
36+
Status CreateNew(llvm::StringRef name) override;
3737
Status CreateWithUniqueName(llvm::StringRef prefix,
38-
bool child_process_inherit,
3938
llvm::SmallVectorImpl<char> &name) override;
40-
Status OpenAsReader(llvm::StringRef name,
41-
bool child_process_inherit) override;
42-
llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit,
39+
Status OpenAsReader(llvm::StringRef name) override;
40+
llvm::Error OpenAsWriter(llvm::StringRef name,
4341
const Timeout<std::micro> &timeout) override;
4442

4543
bool CanRead() const override;

‎lldb/include/lldb/Host/windows/PipeWindows.h

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,14 @@ class PipeWindows : public PipeBase {
2929
~PipeWindows() override;
3030

3131
// Create an unnamed pipe.
32-
Status CreateNew(bool child_process_inherit) override;
32+
Status CreateNew() override;
3333

3434
// Create a named pipe.
35-
Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
35+
Status CreateNew(llvm::StringRef name) override;
3636
Status CreateWithUniqueName(llvm::StringRef prefix,
37-
bool child_process_inherit,
3837
llvm::SmallVectorImpl<char> &name) override;
39-
Status OpenAsReader(llvm::StringRef name,
40-
bool child_process_inherit) override;
41-
llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit,
38+
Status OpenAsReader(llvm::StringRef name) override;
39+
llvm::Error OpenAsWriter(llvm::StringRef name,
4240
const Timeout<std::micro> &timeout) override;
4341

4442
bool CanRead() const override;
@@ -72,8 +70,7 @@ class PipeWindows : public PipeBase {
7270
HANDLE GetWriteNativeHandle();
7371

7472
private:
75-
Status OpenNamedPipe(llvm::StringRef name, bool child_process_inherit,
76-
bool is_read);
73+
Status OpenNamedPipe(llvm::StringRef name, bool is_read);
7774

7875
HANDLE m_read;
7976
HANDLE m_write;

‎lldb/source/Host/common/Socket.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ SharedSocket::SharedSocket(const Socket *socket, Status &error) {
6969
m_fd = kInvalidFD;
7070

7171
// Create a pipe to transfer WSAPROTOCOL_INFO to the child process.
72-
error = m_socket_pipe.CreateNew(true);
72+
error = m_socket_pipe.CreateNew();
7373
if (error.Fail())
7474
return;
7575

‎lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ void ConnectionFileDescriptor::OpenCommandPipe() {
9292

9393
Log *log = GetLog(LLDBLog::Connection);
9494
// Make the command file descriptor here:
95-
Status result = m_pipe.CreateNew(/*child_processes_inherit=*/false);
95+
Status result = m_pipe.CreateNew();
9696
if (!result.Success()) {
9797
LLDB_LOGF(log,
9898
"%p ConnectionFileDescriptor::OpenCommandPipe () - could not "

‎lldb/source/Host/posix/MainLoopPosix.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ void MainLoopPosix::RunImpl::ProcessReadEvents() {
208208
#endif
209209

210210
MainLoopPosix::MainLoopPosix() {
211-
Status error = m_interrupt_pipe.CreateNew(/*child_process_inherit=*/false);
211+
Status error = m_interrupt_pipe.CreateNew();
212212
assert(error.Success());
213213

214214
// Make the write end of the pipe non-blocking.

‎lldb/source/Host/posix/PipePosix.cpp

Lines changed: 11 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -79,24 +79,22 @@ PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {
7979

8080
PipePosix::~PipePosix() { Close(); }
8181

82-
Status PipePosix::CreateNew(bool child_processes_inherit) {
82+
Status PipePosix::CreateNew() {
8383
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
8484
if (CanReadUnlocked() || CanWriteUnlocked())
8585
return Status(EINVAL, eErrorTypePOSIX);
8686

8787
Status error;
8888
#if PIPE2_SUPPORTED
89-
if (::pipe2(m_fds, (child_processes_inherit) ? 0 : O_CLOEXEC) == 0)
89+
if (::pipe2(m_fds, O_CLOEXEC) == 0)
9090
return error;
9191
#else
9292
if (::pipe(m_fds) == 0) {
9393
#ifdef FD_CLOEXEC
94-
if (!child_processes_inherit) {
95-
if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
96-
error = Status::FromErrno();
97-
CloseUnlocked();
98-
return error;
99-
}
94+
if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
95+
error = Status::FromErrno();
96+
CloseUnlocked();
97+
return error;
10098
}
10199
#endif
102100
return error;
@@ -109,7 +107,7 @@ Status PipePosix::CreateNew(bool child_processes_inherit) {
109107
return error;
110108
}
111109

112-
Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
110+
Status PipePosix::CreateNew(llvm::StringRef name) {
113111
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
114112
if (CanReadUnlocked() || CanWriteUnlocked())
115113
return Status::FromErrorString("Pipe is already opened");
@@ -121,7 +119,6 @@ Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
121119
}
122120

123121
Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
124-
bool child_process_inherit,
125122
llvm::SmallVectorImpl<char> &name) {
126123
llvm::SmallString<128> named_pipe_path;
127124
llvm::SmallString<128> pipe_spec((prefix + ".%%%%%%").str());
@@ -137,24 +134,21 @@ Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
137134
do {
138135
llvm::sys::fs::createUniquePath(tmpdir_file_spec.GetPath(), named_pipe_path,
139136
/*MakeAbsolute=*/false);
140-
error = CreateNew(named_pipe_path, child_process_inherit);
137+
error = CreateNew(named_pipe_path);
141138
} while (error.GetError() == EEXIST);
142139

143140
if (error.Success())
144141
name = named_pipe_path;
145142
return error;
146143
}
147144

148-
Status PipePosix::OpenAsReader(llvm::StringRef name,
149-
bool child_process_inherit) {
145+
Status PipePosix::OpenAsReader(llvm::StringRef name) {
150146
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
151147

152148
if (CanReadUnlocked() || CanWriteUnlocked())
153149
return Status::FromErrorString("Pipe is already opened");
154150

155-
int flags = O_RDONLY | O_NONBLOCK;
156-
if (!child_process_inherit)
157-
flags |= O_CLOEXEC;
151+
int flags = O_RDONLY | O_NONBLOCK | O_CLOEXEC;
158152

159153
Status error;
160154
int fd = FileSystem::Instance().Open(name.str().c_str(), flags);
@@ -167,15 +161,12 @@ Status PipePosix::OpenAsReader(llvm::StringRef name,
167161
}
168162

169163
llvm::Error PipePosix::OpenAsWriter(llvm::StringRef name,
170-
bool child_process_inherit,
171164
const Timeout<std::micro> &timeout) {
172165
std::lock_guard<std::mutex> guard(m_write_mutex);
173166
if (CanReadUnlocked() || CanWriteUnlocked())
174167
return llvm::createStringError("Pipe is already opened");
175168

176-
int flags = O_WRONLY | O_NONBLOCK;
177-
if (!child_process_inherit)
178-
flags |= O_CLOEXEC;
169+
int flags = O_WRONLY | O_NONBLOCK | O_CLOEXEC;
179170

180171
using namespace std::chrono;
181172
std::optional<time_point<steady_clock>> finish_time;

‎lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -255,8 +255,7 @@ ProcessLauncherPosixFork::LaunchProcess(const ProcessLaunchInfo &launch_info,
255255
Status &error) {
256256
// A pipe used by the child process to report errors.
257257
PipePosix pipe;
258-
const bool child_processes_inherit = false;
259-
error = pipe.CreateNew(child_processes_inherit);
258+
error = pipe.CreateNew();
260259
if (error.Fail())
261260
return HostProcess();
262261

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

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,19 +66,18 @@ PipeWindows::PipeWindows(pipe_t read, pipe_t write)
6666

6767
PipeWindows::~PipeWindows() { Close(); }
6868

69-
Status PipeWindows::CreateNew(bool child_process_inherit) {
69+
Status PipeWindows::CreateNew() {
7070
// Even for anonymous pipes, we open a named pipe. This is because you
7171
// cannot get overlapped i/o on Windows without using a named pipe. So we
7272
// synthesize a unique name.
7373
uint32_t serial = g_pipe_serial.fetch_add(1);
7474
std::string pipe_name = llvm::formatv(
7575
"lldb.pipe.{0}.{1}.{2}", GetCurrentProcessId(), &g_pipe_serial, serial);
7676

77-
return CreateNew(pipe_name.c_str(), child_process_inherit);
77+
return CreateNew(pipe_name.c_str());
7878
}
7979

80-
Status PipeWindows::CreateNew(llvm::StringRef name,
81-
bool child_process_inherit) {
80+
Status PipeWindows::CreateNew(llvm::StringRef name) {
8281
if (name.empty())
8382
return Status(ERROR_INVALID_PARAMETER, eErrorTypeWin32);
8483

@@ -109,7 +108,7 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
109108

110109
// Open the write end of the pipe. Note that closing either the read or
111110
// write end of the pipe could directly close the pipe itself.
112-
Status result = OpenNamedPipe(name, child_process_inherit, false);
111+
Status result = OpenNamedPipe(name, false);
113112
if (!result.Success()) {
114113
CloseReadFileDescriptor();
115114
return result;
@@ -119,7 +118,6 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
119118
}
120119

121120
Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
122-
bool child_process_inherit,
123121
llvm::SmallVectorImpl<char> &name) {
124122
llvm::SmallString<128> pipe_name;
125123
Status error;
@@ -133,7 +131,7 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
133131
pipe_name += "-";
134132
pipe_name += reinterpret_cast<char *>(unique_string);
135133
::RpcStringFreeA(&unique_string);
136-
error = CreateNew(pipe_name, child_process_inherit);
134+
error = CreateNew(pipe_name);
137135
} else {
138136
error = Status(status, eErrorTypeWin32);
139137
}
@@ -142,25 +140,22 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
142140
return error;
143141
}
144142

145-
Status PipeWindows::OpenAsReader(llvm::StringRef name,
146-
bool child_process_inherit) {
143+
Status PipeWindows::OpenAsReader(llvm::StringRef name) {
147144
if (CanRead())
148145
return Status(); // Note the name is ignored.
149146

150-
return OpenNamedPipe(name, child_process_inherit, true);
147+
return OpenNamedPipe(name, true);
151148
}
152149

153150
llvm::Error PipeWindows::OpenAsWriter(llvm::StringRef name,
154-
bool child_process_inherit,
155151
const Timeout<std::micro> &timeout) {
156152
if (CanWrite())
157153
return llvm::Error::success(); // Note the name is ignored.
158154

159-
return OpenNamedPipe(name, child_process_inherit, false).takeError();
155+
return OpenNamedPipe(name, false).takeError();
160156
}
161157

162-
Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
163-
bool child_process_inherit, bool is_read) {
158+
Status PipeWindows::OpenNamedPipe(llvm::StringRef name, bool is_read) {
164159
if (name.empty())
165160
return Status(ERROR_INVALID_PARAMETER, eErrorTypeWin32);
166161

‎lldb/source/Interpreter/ScriptInterpreter.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ ScriptInterpreterIORedirect::ScriptInterpreterIORedirect(
220220
m_input_file_sp = debugger.GetInputFileSP();
221221

222222
Pipe pipe;
223-
Status pipe_result = pipe.CreateNew(false);
223+
Status pipe_result = pipe.CreateNew();
224224
#if defined(_WIN32)
225225
lldb::file_t read_file = pipe.GetReadNativeHandle();
226226
pipe.ReleaseReadFileDescriptor();

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -955,7 +955,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
955955
// Binding to port zero, we need to figure out what port it ends up
956956
// using using a named pipe...
957957
Status error = socket_pipe.CreateWithUniqueName("debugserver-named-pipe",
958-
false, named_pipe_path);
958+
named_pipe_path);
959959
if (error.Fail()) {
960960
LLDB_LOG(log, "named pipe creation failed: {0}", error);
961961
return error;
@@ -965,15 +965,16 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
965965
#else
966966
// Binding to port zero, we need to figure out what port it ends up
967967
// using using an unnamed pipe...
968-
Status error = socket_pipe.CreateNew(true);
968+
Status error = socket_pipe.CreateNew();
969969
if (error.Fail()) {
970970
LLDB_LOG(log, "unnamed pipe creation failed: {0}", error);
971971
return error;
972972
}
973973
pipe_t write = socket_pipe.GetWritePipe();
974974
debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
975975
debugserver_args.AppendArgument(llvm::to_string(write));
976-
launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
976+
launch_info.AppendDuplicateFileAction((int64_t)socket_pipe.GetWritePipe(),
977+
(int64_t)socket_pipe.GetWritePipe());
977978
#endif
978979
} else {
979980
// No host and port given, so lets listen on our end and make the
@@ -1075,7 +1076,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
10751076

10761077
Status error;
10771078
if (named_pipe_path.size() > 0) {
1078-
error = socket_pipe.OpenAsReader(named_pipe_path, false);
1079+
error = socket_pipe.OpenAsReader(named_pipe_path);
10791080
if (error.Fail()) {
10801081
LLDB_LOG(log, "failed to open named pipe {0} for reading: {1}",
10811082
named_pipe_path, error);

‎lldb/source/Target/Process.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4614,7 +4614,7 @@ class IOHandlerProcessSTDIO : public IOHandler {
46144614
m_process(process),
46154615
m_read_file(GetInputFD(), File::eOpenOptionReadOnly, false),
46164616
m_write_file(write_fd, File::eOpenOptionWriteOnly, false) {
4617-
m_pipe.CreateNew(false);
4617+
m_pipe.CreateNew();
46184618
}
46194619

46204620
~IOHandlerProcessSTDIO() override = default;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ Status writeSocketIdToPipe(const char *const named_pipe_path,
185185
llvm::StringRef socket_id) {
186186
Pipe port_name_pipe;
187187
// Wait for 10 seconds for pipe to be opened.
188-
if (llvm::Error err = port_name_pipe.OpenAsWriter(named_pipe_path, false,
188+
if (llvm::Error err = port_name_pipe.OpenAsWriter(named_pipe_path,
189189
std::chrono::seconds{10}))
190190
return Status::FromError(std::move(err));
191191

‎lldb/unittests/Core/CommunicationTest.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,7 @@ TEST_F(CommunicationTest, SynchronizeWhileClosing) {
141141
#if LLDB_ENABLE_POSIX
142142
TEST_F(CommunicationTest, WriteAll) {
143143
Pipe pipe;
144-
ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
145-
llvm::Succeeded());
144+
ASSERT_THAT_ERROR(pipe.CreateNew().ToError(), llvm::Succeeded());
146145

147146
// Make the write end non-blocking in order to easily reproduce a partial
148147
// write.

‎lldb/unittests/Host/HostTest.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,7 @@ TEST(Host, LaunchProcessDuplicatesHandle) {
145145
exit(1);
146146
}
147147
Pipe pipe;
148-
ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).takeError(),
149-
llvm::Succeeded());
148+
ASSERT_THAT_ERROR(pipe.CreateNew().takeError(), llvm::Succeeded());
150149
SCOPED_TRACE(llvm::formatv("Pipe handles are: {0}/{1}",
151150
(uint64_t)pipe.GetReadPipe(),
152151
(uint64_t)pipe.GetWritePipe())

0 commit comments

Comments
 (0)