[lldb] Remove child_process_inherit argument from Pipe #145516
+60
−91
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
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.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesIt's not necessary on posix platforms as of #126935 and it's ignored on windows as of #138896. For both platforms, we have a better way of inheriting FDs/HANDLEs. Patch is 22.46 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/145516.diff 18 Files Affected:
diff --git a/lldb/include/lldb/Host/PipeBase.h b/lldb/include/lldb/Host/PipeBase.h
index ed8df6bf1e511..6a37f3f2445b0 100644
--- a/lldb/include/lldb/Host/PipeBase.h
+++ b/lldb/include/lldb/Host/PipeBase.h
@@ -21,18 +21,14 @@ class PipeBase {
public:
virtual ~PipeBase();
- virtual Status CreateNew(bool child_process_inherit) = 0;
- virtual Status CreateNew(llvm::StringRef name,
- bool child_process_inherit) = 0;
+ virtual Status CreateNew() = 0;
+ virtual Status CreateNew(llvm::StringRef name) = 0;
virtual Status CreateWithUniqueName(llvm::StringRef prefix,
- bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) = 0;
- virtual Status OpenAsReader(llvm::StringRef name,
- bool child_process_inherit) = 0;
+ virtual Status OpenAsReader(llvm::StringRef name) = 0;
virtual llvm::Error OpenAsWriter(llvm::StringRef name,
- bool child_process_inherit,
const Timeout<std::micro> &timeout) = 0;
virtual bool CanRead() const = 0;
diff --git a/lldb/include/lldb/Host/posix/PipePosix.h b/lldb/include/lldb/Host/posix/PipePosix.h
index effd33fba7eb0..0bec2061f3164 100644
--- a/lldb/include/lldb/Host/posix/PipePosix.h
+++ b/lldb/include/lldb/Host/posix/PipePosix.h
@@ -32,14 +32,12 @@ class PipePosix : public PipeBase {
~PipePosix() override;
- Status CreateNew(bool child_process_inherit) override;
- Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
+ Status CreateNew() override;
+ Status CreateNew(llvm::StringRef name) override;
Status CreateWithUniqueName(llvm::StringRef prefix,
- bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) override;
- Status OpenAsReader(llvm::StringRef name,
- bool child_process_inherit) override;
- llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit,
+ Status OpenAsReader(llvm::StringRef name) override;
+ llvm::Error OpenAsWriter(llvm::StringRef name,
const Timeout<std::micro> &timeout) override;
bool CanRead() const override;
diff --git a/lldb/include/lldb/Host/windows/PipeWindows.h b/lldb/include/lldb/Host/windows/PipeWindows.h
index 9cf591a2d4629..a8bd3cecb9afe 100644
--- a/lldb/include/lldb/Host/windows/PipeWindows.h
+++ b/lldb/include/lldb/Host/windows/PipeWindows.h
@@ -29,16 +29,14 @@ class PipeWindows : public PipeBase {
~PipeWindows() override;
// Create an unnamed pipe.
- Status CreateNew(bool child_process_inherit) override;
+ Status CreateNew() override;
// Create a named pipe.
- Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
+ Status CreateNew(llvm::StringRef name) override;
Status CreateWithUniqueName(llvm::StringRef prefix,
- bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) override;
- Status OpenAsReader(llvm::StringRef name,
- bool child_process_inherit) override;
- llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit,
+ Status OpenAsReader(llvm::StringRef name) override;
+ llvm::Error OpenAsWriter(llvm::StringRef name,
const Timeout<std::micro> &timeout) override;
bool CanRead() const override;
@@ -72,8 +70,7 @@ class PipeWindows : public PipeBase {
HANDLE GetWriteNativeHandle();
private:
- Status OpenNamedPipe(llvm::StringRef name, bool child_process_inherit,
- bool is_read);
+ Status OpenNamedPipe(llvm::StringRef name, bool is_read);
HANDLE m_read;
HANDLE m_write;
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index 2b23fd1e6e57e..802ff9a1b5d1d 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -69,7 +69,7 @@ SharedSocket::SharedSocket(const Socket *socket, Status &error) {
m_fd = kInvalidFD;
// Create a pipe to transfer WSAPROTOCOL_INFO to the child process.
- error = m_socket_pipe.CreateNew(true);
+ error = m_socket_pipe.CreateNew();
if (error.Fail())
return;
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 57dce44812c89..16e3f9910eefc 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -92,7 +92,7 @@ void ConnectionFileDescriptor::OpenCommandPipe() {
Log *log = GetLog(LLDBLog::Connection);
// Make the command file descriptor here:
- Status result = m_pipe.CreateNew(/*child_processes_inherit=*/false);
+ Status result = m_pipe.CreateNew();
if (!result.Success()) {
LLDB_LOGF(log,
"%p ConnectionFileDescriptor::OpenCommandPipe () - could not "
diff --git a/lldb/source/Host/posix/MainLoopPosix.cpp b/lldb/source/Host/posix/MainLoopPosix.cpp
index b4d629708eb3f..19a7128fbe407 100644
--- a/lldb/source/Host/posix/MainLoopPosix.cpp
+++ b/lldb/source/Host/posix/MainLoopPosix.cpp
@@ -208,7 +208,7 @@ void MainLoopPosix::RunImpl::ProcessReadEvents() {
#endif
MainLoopPosix::MainLoopPosix() {
- Status error = m_interrupt_pipe.CreateNew(/*child_process_inherit=*/false);
+ Status error = m_interrupt_pipe.CreateNew();
assert(error.Success());
// Make the write end of the pipe non-blocking.
diff --git a/lldb/source/Host/posix/PipePosix.cpp b/lldb/source/Host/posix/PipePosix.cpp
index a8c4f8df333a4..68b02a9aa465b 100644
--- a/lldb/source/Host/posix/PipePosix.cpp
+++ b/lldb/source/Host/posix/PipePosix.cpp
@@ -79,24 +79,22 @@ PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {
PipePosix::~PipePosix() { Close(); }
-Status PipePosix::CreateNew(bool child_processes_inherit) {
+Status PipePosix::CreateNew() {
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
if (CanReadUnlocked() || CanWriteUnlocked())
return Status(EINVAL, eErrorTypePOSIX);
Status error;
#if PIPE2_SUPPORTED
- if (::pipe2(m_fds, (child_processes_inherit) ? 0 : O_CLOEXEC) == 0)
+ if (::pipe2(m_fds, O_CLOEXEC) == 0)
return error;
#else
if (::pipe(m_fds) == 0) {
#ifdef FD_CLOEXEC
- if (!child_processes_inherit) {
- if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
- error = Status::FromErrno();
- CloseUnlocked();
- return error;
- }
+ if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
+ error = Status::FromErrno();
+ CloseUnlocked();
+ return error;
}
#endif
return error;
@@ -109,7 +107,7 @@ Status PipePosix::CreateNew(bool child_processes_inherit) {
return error;
}
-Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
+Status PipePosix::CreateNew(llvm::StringRef name) {
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
if (CanReadUnlocked() || CanWriteUnlocked())
return Status::FromErrorString("Pipe is already opened");
@@ -121,7 +119,6 @@ Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
}
Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
- bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) {
llvm::SmallString<128> named_pipe_path;
llvm::SmallString<128> pipe_spec((prefix + ".%%%%%%").str());
@@ -137,7 +134,7 @@ Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
do {
llvm::sys::fs::createUniquePath(tmpdir_file_spec.GetPath(), named_pipe_path,
/*MakeAbsolute=*/false);
- error = CreateNew(named_pipe_path, child_process_inherit);
+ error = CreateNew(named_pipe_path);
} while (error.GetError() == EEXIST);
if (error.Success())
@@ -145,16 +142,13 @@ Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
return error;
}
-Status PipePosix::OpenAsReader(llvm::StringRef name,
- bool child_process_inherit) {
+Status PipePosix::OpenAsReader(llvm::StringRef name) {
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
if (CanReadUnlocked() || CanWriteUnlocked())
return Status::FromErrorString("Pipe is already opened");
- int flags = O_RDONLY | O_NONBLOCK;
- if (!child_process_inherit)
- flags |= O_CLOEXEC;
+ int flags = O_RDONLY | O_NONBLOCK | O_CLOEXEC;
Status error;
int fd = FileSystem::Instance().Open(name.str().c_str(), flags);
@@ -167,15 +161,12 @@ Status PipePosix::OpenAsReader(llvm::StringRef name,
}
llvm::Error PipePosix::OpenAsWriter(llvm::StringRef name,
- bool child_process_inherit,
const Timeout<std::micro> &timeout) {
std::lock_guard<std::mutex> guard(m_write_mutex);
if (CanReadUnlocked() || CanWriteUnlocked())
return llvm::createStringError("Pipe is already opened");
- int flags = O_WRONLY | O_NONBLOCK;
- if (!child_process_inherit)
- flags |= O_CLOEXEC;
+ int flags = O_WRONLY | O_NONBLOCK | O_CLOEXEC;
using namespace std::chrono;
std::optional<time_point<steady_clock>> finish_time;
diff --git a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
index 698524349e16a..15a809784fbc4 100644
--- a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -255,8 +255,7 @@ ProcessLauncherPosixFork::LaunchProcess(const ProcessLaunchInfo &launch_info,
Status &error) {
// A pipe used by the child process to report errors.
PipePosix pipe;
- const bool child_processes_inherit = false;
- error = pipe.CreateNew(child_processes_inherit);
+ error = pipe.CreateNew();
if (error.Fail())
return HostProcess();
diff --git a/lldb/source/Host/windows/PipeWindows.cpp b/lldb/source/Host/windows/PipeWindows.cpp
index 30b9d1c41695b..0b495fff69dfa 100644
--- a/lldb/source/Host/windows/PipeWindows.cpp
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -66,7 +66,7 @@ PipeWindows::PipeWindows(pipe_t read, pipe_t write)
PipeWindows::~PipeWindows() { Close(); }
-Status PipeWindows::CreateNew(bool child_process_inherit) {
+Status PipeWindows::CreateNew() {
// Even for anonymous pipes, we open a named pipe. This is because you
// cannot get overlapped i/o on Windows without using a named pipe. So we
// synthesize a unique name.
@@ -74,11 +74,10 @@ Status PipeWindows::CreateNew(bool child_process_inherit) {
std::string pipe_name = llvm::formatv(
"lldb.pipe.{0}.{1}.{2}", GetCurrentProcessId(), &g_pipe_serial, serial);
- return CreateNew(pipe_name.c_str(), child_process_inherit);
+ return CreateNew(pipe_name.c_str());
}
-Status PipeWindows::CreateNew(llvm::StringRef name,
- bool child_process_inherit) {
+Status PipeWindows::CreateNew(llvm::StringRef name) {
if (name.empty())
return Status(ERROR_INVALID_PARAMETER, eErrorTypeWin32);
@@ -109,7 +108,7 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
// Open the write end of the pipe. Note that closing either the read or
// write end of the pipe could directly close the pipe itself.
- Status result = OpenNamedPipe(name, child_process_inherit, false);
+ Status result = OpenNamedPipe(name, false);
if (!result.Success()) {
CloseReadFileDescriptor();
return result;
@@ -119,7 +118,6 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
}
Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
- bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) {
llvm::SmallString<128> pipe_name;
Status error;
@@ -133,7 +131,7 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
pipe_name += "-";
pipe_name += reinterpret_cast<char *>(unique_string);
::RpcStringFreeA(&unique_string);
- error = CreateNew(pipe_name, child_process_inherit);
+ error = CreateNew(pipe_name);
} else {
error = Status(status, eErrorTypeWin32);
}
@@ -142,25 +140,22 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
return error;
}
-Status PipeWindows::OpenAsReader(llvm::StringRef name,
- bool child_process_inherit) {
+Status PipeWindows::OpenAsReader(llvm::StringRef name) {
if (CanRead())
return Status(); // Note the name is ignored.
- return OpenNamedPipe(name, child_process_inherit, true);
+ return OpenNamedPipe(name, true);
}
llvm::Error PipeWindows::OpenAsWriter(llvm::StringRef name,
- bool child_process_inherit,
const Timeout<std::micro> &timeout) {
if (CanWrite())
return llvm::Error::success(); // Note the name is ignored.
- return OpenNamedPipe(name, child_process_inherit, false).takeError();
+ return OpenNamedPipe(name, false).takeError();
}
-Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
- bool child_process_inherit, bool is_read) {
+Status PipeWindows::OpenNamedPipe(llvm::StringRef name, bool is_read) {
if (name.empty())
return Status(ERROR_INVALID_PARAMETER, eErrorTypeWin32);
diff --git a/lldb/source/Interpreter/ScriptInterpreter.cpp b/lldb/source/Interpreter/ScriptInterpreter.cpp
index 63655cc5a50c6..ae913e68feb19 100644
--- a/lldb/source/Interpreter/ScriptInterpreter.cpp
+++ b/lldb/source/Interpreter/ScriptInterpreter.cpp
@@ -220,7 +220,7 @@ ScriptInterpreterIORedirect::ScriptInterpreterIORedirect(
m_input_file_sp = debugger.GetInputFileSP();
Pipe pipe;
- Status pipe_result = pipe.CreateNew(false);
+ Status pipe_result = pipe.CreateNew();
#if defined(_WIN32)
lldb::file_t read_file = pipe.GetReadNativeHandle();
pipe.ReleaseReadFileDescriptor();
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 0d3ead840b080..20cd689d7131f 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -955,7 +955,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
// Binding to port zero, we need to figure out what port it ends up
// using using a named pipe...
Status error = socket_pipe.CreateWithUniqueName("debugserver-named-pipe",
- false, named_pipe_path);
+ named_pipe_path);
if (error.Fail()) {
LLDB_LOG(log, "named pipe creation failed: {0}", error);
return error;
@@ -965,7 +965,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
#else
// Binding to port zero, we need to figure out what port it ends up
// using using an unnamed pipe...
- Status error = socket_pipe.CreateNew(true);
+ Status error = socket_pipe.CreateNew();
if (error.Fail()) {
LLDB_LOG(log, "unnamed pipe creation failed: {0}", error);
return error;
@@ -973,7 +973,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
pipe_t write = socket_pipe.GetWritePipe();
debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
debugserver_args.AppendArgument(llvm::to_string(write));
- launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
+ launch_info.AppendDuplicateFileAction((int64_t)socket_pipe.GetWritePipe(),
+ (int64_t)socket_pipe.GetWritePipe());
#endif
} else {
// No host and port given, so lets listen on our end and make the
@@ -1075,7 +1076,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
Status error;
if (named_pipe_path.size() > 0) {
- error = socket_pipe.OpenAsReader(named_pipe_path, false);
+ error = socket_pipe.OpenAsReader(named_pipe_path);
if (error.Fail()) {
LLDB_LOG(log, "failed to open named pipe {0} for reading: {1}",
named_pipe_path, error);
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 61a3d05bc3746..bba1230c79920 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4614,7 +4614,7 @@ class IOHandlerProcessSTDIO : public IOHandler {
m_process(process),
m_read_file(GetInputFD(), File::eOpenOptionReadOnly, false),
m_write_file(write_fd, File::eOpenOptionWriteOnly, false) {
- m_pipe.CreateNew(false);
+ m_pipe.CreateNew();
}
~IOHandlerProcessSTDIO() override = default;
diff --git a/lldb/tools/lldb-server/lldb-gdbserver.cpp b/lldb/tools/lldb-server/lldb-gdbserver.cpp
index 843354147f2da..2b3925f67e058 100644
--- a/lldb/tools/lldb-server/lldb-gdbserver.cpp
+++ b/lldb/tools/lldb-server/lldb-gdbserver.cpp
@@ -185,7 +185,7 @@ Status writeSocketIdToPipe(const char *const named_pipe_path,
llvm::StringRef socket_id) {
Pipe port_name_pipe;
// Wait for 10 seconds for pipe to be opened.
- if (llvm::Error err = port_name_pipe.OpenAsWriter(named_pipe_path, false,
+ if (llvm::Error err = port_name_pipe.OpenAsWriter(named_pipe_path,
std::chrono::seconds{10}))
return Status::FromError(std::move(err));
diff --git a/lldb/unittests/Core/CommunicationTest.cpp b/lldb/unittests/Core/CommunicationTest.cpp
index 51e218af8baf4..f8c584ae142be 100644
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ b/lldb/unittests/Core/CommunicationTest.cpp
@@ -141,8 +141,7 @@ TEST_F(CommunicationTest, SynchronizeWhileClosing) {
#if LLDB_ENABLE_POSIX
TEST_F(CommunicationTest, WriteAll) {
Pipe pipe;
- ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
- llvm::Succeeded());
+ ASSERT_THAT_ERROR(pipe.CreateNew().ToError(), llvm::Succeeded());
// Make the write end non-blocking in order to easily reproduce a partial
// write.
diff --git a/lldb/unittests/Host/HostTest.cpp b/lldb/unittests/Host/HostTest.cpp
index dca32c7547cf7..1424e4472eddb 100644
--- a/lldb/unittests/Host/HostTest.cpp
+++ b/lldb/unittests/Host/HostTest.cpp
@@ -145,8 +145,7 @@ TEST(Host, LaunchProcessDuplicatesHandle) {
exit(1);
}
Pipe pipe;
- ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).takeError(),
- llvm::Succeeded());
+ ASSERT_THAT_ERROR(pipe.CreateNew().takeError(), llvm::Succeeded());
SCOPED_TRACE(llvm::formatv("Pipe handles are: {0}/{1}",
(uint64_t)pipe.GetReadPipe(),
(uint64_t)pipe.GetWritePipe())
diff --git a/lldb/unittests/Host/PipeTest.cpp b/lldb/unittests/Host/PipeTest.cpp
index d33797adb7fc3..4ee3b53cc552b 100644
--- a/lldb/unittests/Host/PipeTest.cpp
+++ b/lldb/unittests/Host/PipeTest.cpp
@@ -29,11 +29,10 @@ class PipeTest : public testing::Test {
TEST_F(PipeTest, CreateWithUniqueName) {
Pipe pipe;
llvm::SmallString<0> name;
- ASSERT_THAT_ERROR(pipe.CreateWithUniqueName("PipeTest-CreateWithUniqueName",
- /*child_process_inherit=*/false,
- name)
- .ToError(),
- llvm::Succeeded());
+ ASSERT_THAT_ERROR(
+ pipe.CreateWithUniqueName("PipeTest-CreateWithUniqueName", name)
+ .ToError(),
+ llvm::Succeeded());
}
// Test broken
@@ -41,19 +40,15 @@ TEST_F(PipeTest, CreateWithUniqueName) {
TEST_F(PipeTest, OpenAsReader) {
Pipe pipe;
llvm::SmallString<0> name;
- ASSERT_THAT_ERROR(pipe.CreateWithUniqueName("PipeTest-OpenAsReader",
- /*child_process_inherit=*/false,
- name)
- .ToError(),
- llvm::Succeeded());
+ ASSERT_THAT_ERROR(
+ pipe.CreateWithUniqueName("PipeTest-OpenAsReader", name).ToError(),
+ llvm::Succeeded());
// Ensure name is not null-terminated...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
It's not necessary on posix platforms as of #126935 and it's ignored on windows as of #138896. For both platforms, we have a better way of inheriting FDs/HANDLEs.