caio/clang-p2996
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[lldb] Remove child_process_inherit argument from Pipe (#145516)

Browse Source 
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.
This commit is contained in:
Pavel Labath
2025-06-26 13:51:14 +02:00
committed by GitHub
parent f64d5df3c2
commit b77114b723
18 changed files with 58 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
lldb/include/lldb/Host/PipeBase.h
View File

@@ -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;

10
lldb/include/lldb/Host/posix/PipePosix.h
View File

@@ -32,14 +32,12 @@ public:
~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;

13
lldb/include/lldb/Host/windows/PipeWindows.h
View File

@@ -29,16 +29,14 @@ public:
~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 @@ public:
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;

2
lldb/source/Host/common/Socket.cpp
View File

@@ -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;

2
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
View File

@@ -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 "

2
lldb/source/Host/posix/MainLoopPosix.cpp
View File

@@ -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.

31
lldb/source/Host/posix/PipePosix.cpp
View File

@@ -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;

3
lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
View File

@@ -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();

23
lldb/source/Host/windows/PipeWindows.cpp
View File

@@ -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);

2
lldb/source/Interpreter/ScriptInterpreter.cpp
View File

@@ -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();

8
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
View File

@@ -944,7 +944,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
#if defined(__APPLE__)
// Using a named pipe as debugserver does not support --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;
@@ -953,7 +953,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
debugserver_args.AppendArgument(named_pipe_path);
#else
// Using an unnamed pipe as it's simpler.
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;
@@ -961,7 +961,7 @@ 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)write, (int64_t)write);
#endif
}
@@ -1044,7 +1044,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);

2
lldb/source/Target/Process.cpp
View File

@@ -4614,7 +4614,7 @@ public:
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;

2
lldb/tools/lldb-server/lldb-gdbserver.cpp
View File

@@ -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));

3
lldb/unittests/Core/CommunicationTest.cpp
View File

@@ -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.

3
lldb/unittests/Host/HostTest.cpp
View File

@@ -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())

25
lldb/unittests/Host/PipeTest.cpp
View File

@@ -29,11 +29,10 @@ public:
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
size_t name_len = name.size();
name += "foobar";
llvm::StringRef name_ref(name.data(), name_len);
ASSERT_THAT_ERROR(
pipe.OpenAsReader(name_ref, /*child_process_inherit=*/false).ToError(),
llvm::Succeeded());
ASSERT_THAT_ERROR(pipe.OpenAsReader(name_ref).ToError(), llvm::Succeeded());
ASSERT_TRUE(pipe.CanRead());
}
@@ -63,7 +58,7 @@ TEST_F(PipeTest, OpenAsReader) {
#ifndef _WIN32
TEST_F(PipeTest, WriteWithTimeout) {
Pipe pipe;
ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded());
ASSERT_THAT_ERROR(pipe.CreateNew().ToError(), llvm::Succeeded());
// The pipe buffer is 1024 for PipeWindows and at least 512 on Darwin.
// In Linux versions before 2.6.11, the capacity of a pipe was the same as the
@@ -153,7 +148,7 @@ TEST_F(PipeTest, WriteWithTimeout) {
TEST_F(PipeTest, ReadWithTimeout) {
Pipe pipe;
ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded());
ASSERT_THAT_ERROR(pipe.CreateNew().ToError(), llvm::Succeeded());
char buf[100];
// The pipe is initially empty. A polling read returns immediately.

4
lldb/unittests/Host/SocketTest.cpp
View File

@@ -199,9 +199,7 @@ TEST_P(SocketTest, TCPAcceptTimeout) {
if (!HostSupportsProtocol())
return;
const bool child_processes_inherit = false;
auto listen_socket_up =
std::make_unique<TCPSocket>(true, child_processes_inherit);
auto listen_socket_up = std::make_unique<TCPSocket>(true);
Status error = listen_socket_up->Listen(
llvm::formatv("[{0}]:0", GetParam().localhost_ip).str(), 5);
ASSERT_THAT_ERROR(error.ToError(), llvm::Succeeded());

4
lldb/unittests/TestingSupport/Host/PipeTestUtilities.h
View File

@@ -20,8 +20,8 @@ protected:
lldb_private::Pipe output;
void SetUp() override {
ASSERT_THAT_ERROR(input.CreateNew(false).ToError(), llvm::Succeeded());
ASSERT_THAT_ERROR(output.CreateNew(false).ToError(), llvm::Succeeded());
ASSERT_THAT_ERROR(input.CreateNew().ToError(), llvm::Succeeded());
ASSERT_THAT_ERROR(output.CreateNew().ToError(), llvm::Succeeded());
}
};