llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Pavel Labath (labath) <details> <summary>Changes</summary> 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. --- 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: - (modified) lldb/include/lldb/Host/PipeBase.h (+3-7) - (modified) lldb/include/lldb/Host/posix/PipePosix.h (+4-6) - (modified) lldb/include/lldb/Host/windows/PipeWindows.h (+5-8) - (modified) lldb/source/Host/common/Socket.cpp (+1-1) - (modified) lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp (+1-1) - (modified) lldb/source/Host/posix/MainLoopPosix.cpp (+1-1) - (modified) lldb/source/Host/posix/PipePosix.cpp (+11-20) - (modified) lldb/source/Host/posix/ProcessLauncherPosixFork.cpp (+1-2) - (modified) lldb/source/Host/windows/PipeWindows.cpp (+9-14) - (modified) lldb/source/Interpreter/ScriptInterpreter.cpp (+1-1) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (+5-4) - (modified) lldb/source/Target/Process.cpp (+1-1) - (modified) lldb/tools/lldb-server/lldb-gdbserver.cpp (+1-1) - (modified) lldb/unittests/Core/CommunicationTest.cpp (+1-2) - (modified) lldb/unittests/Host/HostTest.cpp (+1-2) - (modified) lldb/unittests/Host/PipeTest.cpp (+10-15) - (modified) lldb/unittests/Host/SocketTest.cpp (+1-3) - (modified) lldb/unittests/TestingSupport/Host/PipeTestUtilities.h (+2-2) ``````````diff 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] `````````` </details> https://github.com/llvm/llvm-project/pull/145516 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
