diff --git a/lldb/include/lldb/Host/PipeBase.h b/lldb/include/lldb/Host/PipeBase.h index d51d0cd54e036..ed8df6bf1e511 100644 --- a/lldb/include/lldb/Host/PipeBase.h +++ b/lldb/include/lldb/Host/PipeBase.h @@ -10,12 +10,11 @@ #ifndef LLDB_HOST_PIPEBASE_H #define LLDB_HOST_PIPEBASE_H -#include -#include - #include "lldb/Utility/Status.h" +#include "lldb/Utility/Timeout.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" namespace lldb_private { class PipeBase { @@ -32,10 +31,9 @@ class PipeBase { virtual Status OpenAsReader(llvm::StringRef name, bool child_process_inherit) = 0; - Status OpenAsWriter(llvm::StringRef name, bool child_process_inherit); - virtual Status - OpenAsWriterWithTimeout(llvm::StringRef name, bool child_process_inherit, - const std::chrono::microseconds &timeout) = 0; + virtual llvm::Error OpenAsWriter(llvm::StringRef name, + bool child_process_inherit, + const Timeout &timeout) = 0; virtual bool CanRead() const = 0; virtual bool CanWrite() const = 0; @@ -56,14 +54,13 @@ class PipeBase { // Delete named pipe. virtual Status Delete(llvm::StringRef name) = 0; - virtual Status WriteWithTimeout(const void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_written) = 0; - Status Write(const void *buf, size_t size, size_t &bytes_written); - virtual Status ReadWithTimeout(void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_read) = 0; - Status Read(void *buf, size_t size, size_t &bytes_read); + virtual llvm::Expected + Write(const void *buf, size_t size, + const Timeout &timeout = std::nullopt) = 0; + + virtual llvm::Expected + Read(void *buf, size_t size, + const Timeout &timeout = std::nullopt) = 0; }; } diff --git a/lldb/include/lldb/Host/posix/PipePosix.h b/lldb/include/lldb/Host/posix/PipePosix.h index 2e291160817c4..effd33fba7eb0 100644 --- a/lldb/include/lldb/Host/posix/PipePosix.h +++ b/lldb/include/lldb/Host/posix/PipePosix.h @@ -8,6 +8,7 @@ #ifndef LLDB_HOST_POSIX_PIPEPOSIX_H #define LLDB_HOST_POSIX_PIPEPOSIX_H + #include "lldb/Host/PipeBase.h" #include @@ -38,9 +39,8 @@ class PipePosix : public PipeBase { llvm::SmallVectorImpl &name) override; Status OpenAsReader(llvm::StringRef name, bool child_process_inherit) override; - Status - OpenAsWriterWithTimeout(llvm::StringRef name, bool child_process_inherit, - const std::chrono::microseconds &timeout) override; + llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit, + const Timeout &timeout) override; bool CanRead() const override; bool CanWrite() const override; @@ -64,12 +64,13 @@ class PipePosix : public PipeBase { Status Delete(llvm::StringRef name) override; - Status WriteWithTimeout(const void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_written) override; - Status ReadWithTimeout(void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_read) override; + llvm::Expected + Write(const void *buf, size_t size, + const Timeout &timeout = std::nullopt) override; + + llvm::Expected + Read(void *buf, size_t size, + const Timeout &timeout = std::nullopt) override; private: bool CanReadUnlocked() const; diff --git a/lldb/include/lldb/Host/windows/PipeWindows.h b/lldb/include/lldb/Host/windows/PipeWindows.h index e28d104cc60ec..9cf591a2d4629 100644 --- a/lldb/include/lldb/Host/windows/PipeWindows.h +++ b/lldb/include/lldb/Host/windows/PipeWindows.h @@ -38,9 +38,8 @@ class PipeWindows : public PipeBase { llvm::SmallVectorImpl &name) override; Status OpenAsReader(llvm::StringRef name, bool child_process_inherit) override; - Status - OpenAsWriterWithTimeout(llvm::StringRef name, bool child_process_inherit, - const std::chrono::microseconds &timeout) override; + llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit, + const Timeout &timeout) override; bool CanRead() const override; bool CanWrite() const override; @@ -59,12 +58,13 @@ class PipeWindows : public PipeBase { Status Delete(llvm::StringRef name) override; - Status WriteWithTimeout(const void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_written) override; - Status ReadWithTimeout(void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_read) override; + llvm::Expected + Write(const void *buf, size_t size, + const Timeout &timeout = std::nullopt) override; + + llvm::Expected + Read(void *buf, size_t size, + const Timeout &timeout = std::nullopt) override; // PipeWindows specific methods. These allow access to the underlying OS // handle. diff --git a/lldb/source/Host/common/PipeBase.cpp b/lldb/source/Host/common/PipeBase.cpp index 904a2df12392d..400990f4e41b9 100644 --- a/lldb/source/Host/common/PipeBase.cpp +++ b/lldb/source/Host/common/PipeBase.cpp @@ -11,19 +11,3 @@ using namespace lldb_private; PipeBase::~PipeBase() = default; - -Status PipeBase::OpenAsWriter(llvm::StringRef name, - bool child_process_inherit) { - return OpenAsWriterWithTimeout(name, child_process_inherit, - std::chrono::microseconds::zero()); -} - -Status PipeBase::Write(const void *buf, size_t size, size_t &bytes_written) { - return WriteWithTimeout(buf, size, std::chrono::microseconds::zero(), - bytes_written); -} - -Status PipeBase::Read(void *buf, size_t size, size_t &bytes_read) { - return ReadWithTimeout(buf, size, std::chrono::microseconds::zero(), - bytes_read); -} diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp index f35e5ff43595b..76f74401ac4d0 100644 --- a/lldb/source/Host/common/Socket.cpp +++ b/lldb/source/Host/common/Socket.cpp @@ -93,15 +93,14 @@ Status SharedSocket::CompleteSending(lldb::pid_t child_pid) { "WSADuplicateSocket() failed, error: %d", last_error); } - size_t num_bytes; - Status error = - m_socket_pipe.WriteWithTimeout(&protocol_info, sizeof(protocol_info), - std::chrono::seconds(10), num_bytes); - if (error.Fail()) - return error; - if (num_bytes != sizeof(protocol_info)) + llvm::Expected num_bytes = m_socket_pipe.Write( + &protocol_info, sizeof(protocol_info), std::chrono::seconds(10)); + if (!num_bytes) + return Status::FromError(num_bytes.takeError()); + if (*num_bytes != sizeof(protocol_info)) return Status::FromErrorStringWithFormatv( - "WriteWithTimeout(WSAPROTOCOL_INFO) failed: {0} bytes", num_bytes); + "Write(WSAPROTOCOL_INFO) failed: wrote {0}/{1} bytes", *num_bytes, + sizeof(protocol_info)); #endif return Status(); } @@ -113,16 +112,14 @@ Status SharedSocket::GetNativeSocket(shared_fd_t fd, NativeSocket &socket) { WSAPROTOCOL_INFO protocol_info; { Pipe socket_pipe(fd, LLDB_INVALID_PIPE); - size_t num_bytes; - Status error = - socket_pipe.ReadWithTimeout(&protocol_info, sizeof(protocol_info), - std::chrono::seconds(10), num_bytes); - if (error.Fail()) - return error; - if (num_bytes != sizeof(protocol_info)) { + llvm::Expected num_bytes = socket_pipe.Read( + &protocol_info, sizeof(protocol_info), std::chrono::seconds(10)); + if (!num_bytes) + return Status::FromError(num_bytes.takeError()); + if (*num_bytes != sizeof(protocol_info)) { return Status::FromErrorStringWithFormatv( - "socket_pipe.ReadWithTimeout(WSAPROTOCOL_INFO) failed: {0} bytes", - num_bytes); + "Read(WSAPROTOCOL_INFO) failed: read {0}/{1} bytes", *num_bytes, + sizeof(protocol_info)); } } socket = ::WSASocket(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO, diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp index 0ed2016667162..ae935dda5af4e 100644 --- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp +++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp @@ -178,9 +178,7 @@ ConnectionFileDescriptor::Connect(llvm::StringRef path, } bool ConnectionFileDescriptor::InterruptRead() { - size_t bytes_written = 0; - Status result = m_pipe.Write("i", 1, bytes_written); - return result.Success(); + return !errorToBool(m_pipe.Write("i", 1).takeError()); } ConnectionStatus ConnectionFileDescriptor::Disconnect(Status *error_ptr) { @@ -205,13 +203,11 @@ ConnectionStatus ConnectionFileDescriptor::Disconnect(Status *error_ptr) { std::unique_lock locker(m_mutex, std::defer_lock); if (!locker.try_lock()) { if (m_pipe.CanWrite()) { - size_t bytes_written = 0; - Status result = m_pipe.Write("q", 1, bytes_written); - LLDB_LOGF(log, - "%p ConnectionFileDescriptor::Disconnect(): Couldn't get " - "the lock, sent 'q' to %d, error = '%s'.", - static_cast(this), m_pipe.GetWriteFileDescriptor(), - result.AsCString()); + llvm::Error err = m_pipe.Write("q", 1).takeError(); + LLDB_LOG(log, + "{0}: Couldn't get the lock, sent 'q' to {1}, error = '{2}'.", + this, m_pipe.GetWriteFileDescriptor(), err); + consumeError(std::move(err)); } else if (log) { LLDB_LOGF(log, "%p ConnectionFileDescriptor::Disconnect(): Couldn't get the " diff --git a/lldb/source/Host/posix/MainLoopPosix.cpp b/lldb/source/Host/posix/MainLoopPosix.cpp index ce7caa3041dd0..b4d629708eb3f 100644 --- a/lldb/source/Host/posix/MainLoopPosix.cpp +++ b/lldb/source/Host/posix/MainLoopPosix.cpp @@ -392,9 +392,5 @@ void MainLoopPosix::Interrupt() { return; char c = '.'; - size_t bytes_written; - Status error = m_interrupt_pipe.Write(&c, 1, bytes_written); - assert(error.Success()); - UNUSED_IF_ASSERT_DISABLED(error); - assert(bytes_written == 1); + cantFail(m_interrupt_pipe.Write(&c, 1)); } diff --git a/lldb/source/Host/posix/PipePosix.cpp b/lldb/source/Host/posix/PipePosix.cpp index 24c563d8c24bd..a8c4f8df333a4 100644 --- a/lldb/source/Host/posix/PipePosix.cpp +++ b/lldb/source/Host/posix/PipePosix.cpp @@ -12,7 +12,9 @@ #include "lldb/Utility/SelectHelper.h" #include "llvm/ADT/SmallString.h" #include "llvm/Support/Errno.h" +#include "llvm/Support/Error.h" #include +#include #include #include @@ -164,26 +166,27 @@ Status PipePosix::OpenAsReader(llvm::StringRef name, return error; } -Status -PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name, - bool child_process_inherit, - const std::chrono::microseconds &timeout) { +llvm::Error PipePosix::OpenAsWriter(llvm::StringRef name, + bool child_process_inherit, + const Timeout &timeout) { std::lock_guard guard(m_write_mutex); if (CanReadUnlocked() || CanWriteUnlocked()) - return Status::FromErrorString("Pipe is already opened"); + return llvm::createStringError("Pipe is already opened"); int flags = O_WRONLY | O_NONBLOCK; if (!child_process_inherit) flags |= O_CLOEXEC; using namespace std::chrono; - const auto finish_time = Now() + timeout; + std::optional> finish_time; + if (timeout) + finish_time = Now() + *timeout; while (!CanWriteUnlocked()) { - if (timeout != microseconds::zero()) { - const auto dur = duration_cast(finish_time - Now()).count(); - if (dur <= 0) - return Status::FromErrorString( + if (timeout) { + if (Now() > finish_time) + return llvm::createStringError( + std::make_error_code(std::errc::timed_out), "timeout exceeded - reader hasn't opened so far"); } @@ -193,7 +196,8 @@ PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name, const auto errno_copy = errno; // We may get ENXIO if a reader side of the pipe hasn't opened yet. if (errno_copy != ENXIO && errno_copy != EINTR) - return Status(errno_copy, eErrorTypePOSIX); + return llvm::errorCodeToError( + std::error_code(errno_copy, std::generic_category())); std::this_thread::sleep_for( milliseconds(OPEN_WRITER_SLEEP_TIMEOUT_MSECS)); @@ -202,7 +206,7 @@ PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name, } } - return Status(); + return llvm::Error::success(); } int PipePosix::GetReadFileDescriptor() const { @@ -300,70 +304,51 @@ void PipePosix::CloseWriteFileDescriptorUnlocked() { } } -Status PipePosix::ReadWithTimeout(void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_read) { +llvm::Expected PipePosix::Read(void *buf, size_t size, + const Timeout &timeout) { std::lock_guard guard(m_read_mutex); - bytes_read = 0; if (!CanReadUnlocked()) - return Status(EINVAL, eErrorTypePOSIX); + return llvm::errorCodeToError( + std::make_error_code(std::errc::invalid_argument)); const int fd = GetReadFileDescriptorUnlocked(); SelectHelper select_helper; - select_helper.SetTimeout(timeout); + if (timeout) + select_helper.SetTimeout(*timeout); select_helper.FDSetRead(fd); - Status error; - while (error.Success()) { - error = select_helper.Select(); - if (error.Success()) { - auto result = - ::read(fd, static_cast(buf) + bytes_read, size - bytes_read); - if (result != -1) { - bytes_read += result; - if (bytes_read == size || result == 0) - break; - } else if (errno == EINTR) { - continue; - } else { - error = Status::FromErrno(); - break; - } - } - } - return error; + if (llvm::Error error = select_helper.Select().takeError()) + return error; + + ssize_t result = ::read(fd, buf, size); + if (result == -1) + return llvm::errorCodeToError( + std::error_code(errno, std::generic_category())); + + return result; } -Status PipePosix::WriteWithTimeout(const void *buf, size_t size, - const std::chrono::microseconds &timeout, - size_t &bytes_written) { +llvm::Expected PipePosix::Write(const void *buf, size_t size, + const Timeout &timeout) { std::lock_guard guard(m_write_mutex); - bytes_written = 0; if (!CanWriteUnlocked()) - return Status(EINVAL, eErrorTypePOSIX); + return llvm::errorCodeToError( + std::make_error_code(std::errc::invalid_argument)); const int fd = GetWriteFileDescriptorUnlocked(); SelectHelper select_helper; - select_helper.SetTimeout(timeout); + if (timeout) + select_helper.SetTimeout(*timeout); select_helper.FDSetWrite(fd); - Status error; - while (error.Success()) { - error = select_helper.Select(); - if (error.Success()) { - auto result = ::write(fd, static_cast(buf) + bytes_written, - size - bytes_written); - if (result != -1) { - bytes_written += result; - if (bytes_written == size) - break; - } else if (errno == EINTR) { - continue; - } else { - error = Status::FromErrno(); - } - } - } - return error; + if (llvm::Error error = select_helper.Select().takeError()) + return error; + + ssize_t result = ::write(fd, buf, size); + if (result == -1) + return llvm::errorCodeToError( + std::error_code(errno, std::generic_category())); + + return result; } diff --git a/lldb/source/Host/windows/PipeWindows.cpp b/lldb/source/Host/windows/PipeWindows.cpp index e95007ae8fd16..e3f5b629a0590 100644 --- a/lldb/source/Host/windows/PipeWindows.cpp +++ b/lldb/source/Host/windows/PipeWindows.cpp @@ -149,14 +149,13 @@ Status PipeWindows::OpenAsReader(llvm::StringRef name, return OpenNamedPipe(name, child_process_inherit, true); } -Status -PipeWindows::OpenAsWriterWithTimeout(llvm::StringRef name, - bool child_process_inherit, - const std::chrono::microseconds &timeout) { +llvm::Error PipeWindows::OpenAsWriter(llvm::StringRef name, + bool child_process_inherit, + const Timeout &timeout) { if (CanWrite()) - return Status(); // Note the name is ignored. + return llvm::Error::success(); // Note the name is ignored. - return OpenNamedPipe(name, child_process_inherit, false); + return OpenNamedPipe(name, child_process_inherit, false).takeError(); } Status PipeWindows::OpenNamedPipe(llvm::StringRef name, @@ -268,29 +267,24 @@ PipeWindows::GetReadNativeHandle() { return m_read; } HANDLE PipeWindows::GetWriteNativeHandle() { return m_write; } -Status PipeWindows::ReadWithTimeout(void *buf, size_t size, - const std::chrono::microseconds &duration, - size_t &bytes_read) { +llvm::Expected PipeWindows::Read(void *buf, size_t size, + const Timeout &timeout) { if (!CanRead()) - return Status(ERROR_INVALID_HANDLE, eErrorTypeWin32); + return Status(ERROR_INVALID_HANDLE, eErrorTypeWin32).takeError(); - bytes_read = 0; - DWORD sys_bytes_read = 0; - BOOL result = - ::ReadFile(m_read, buf, size, &sys_bytes_read, &m_read_overlapped); - if (result) { - bytes_read = sys_bytes_read; - return Status(); - } + DWORD bytes_read = 0; + BOOL result = ::ReadFile(m_read, buf, size, &bytes_read, &m_read_overlapped); + if (result) + return bytes_read; DWORD failure_error = ::GetLastError(); if (failure_error != ERROR_IO_PENDING) - return Status(failure_error, eErrorTypeWin32); + return Status(failure_error, eErrorTypeWin32).takeError(); - DWORD timeout = (duration == std::chrono::microseconds::zero()) - ? INFINITE - : duration.count() / 1000; - DWORD wait_result = ::WaitForSingleObject(m_read_overlapped.hEvent, timeout); + DWORD timeout_msec = + timeout ? ceil(*timeout).count() : INFINITE; + DWORD wait_result = + ::WaitForSingleObject(m_read_overlapped.hEvent, timeout_msec); if (wait_result != WAIT_OBJECT_0) { // The operation probably failed. However, if it timed out, we need to // cancel the I/O. Between the time we returned from WaitForSingleObject @@ -306,42 +300,36 @@ Status PipeWindows::ReadWithTimeout(void *buf, size_t size, failed = false; } if (failed) - return Status(failure_error, eErrorTypeWin32); + return Status(failure_error, eErrorTypeWin32).takeError(); } // Now we call GetOverlappedResult setting bWait to false, since we've // already waited as long as we're willing to. - if (!::GetOverlappedResult(m_read, &m_read_overlapped, &sys_bytes_read, - FALSE)) - return Status(::GetLastError(), eErrorTypeWin32); + if (!::GetOverlappedResult(m_read, &m_read_overlapped, &bytes_read, FALSE)) + return Status(::GetLastError(), eErrorTypeWin32).takeError(); - bytes_read = sys_bytes_read; - return Status(); + return bytes_read; } -Status PipeWindows::WriteWithTimeout(const void *buf, size_t size, - const std::chrono::microseconds &duration, - size_t &bytes_written) { +llvm::Expected PipeWindows::Write(const void *buf, size_t size, + const Timeout &timeout) { if (!CanWrite()) - return Status(ERROR_INVALID_HANDLE, eErrorTypeWin32); + return Status(ERROR_INVALID_HANDLE, eErrorTypeWin32).takeError(); - bytes_written = 0; - DWORD sys_bytes_write = 0; + DWORD bytes_written = 0; BOOL result = - ::WriteFile(m_write, buf, size, &sys_bytes_write, &m_write_overlapped); - if (result) { - bytes_written = sys_bytes_write; - return Status(); - } + ::WriteFile(m_write, buf, size, &bytes_written, &m_write_overlapped); + if (result) + return bytes_written; DWORD failure_error = ::GetLastError(); if (failure_error != ERROR_IO_PENDING) - return Status(failure_error, eErrorTypeWin32); + return Status(failure_error, eErrorTypeWin32).takeError(); - DWORD timeout = (duration == std::chrono::microseconds::zero()) - ? INFINITE - : duration.count() / 1000; - DWORD wait_result = ::WaitForSingleObject(m_write_overlapped.hEvent, timeout); + DWORD timeout_msec = + timeout ? ceil(*timeout).count() : INFINITE; + DWORD wait_result = + ::WaitForSingleObject(m_write_overlapped.hEvent, timeout_msec); if (wait_result != WAIT_OBJECT_0) { // The operation probably failed. However, if it timed out, we need to // cancel the I/O. Between the time we returned from WaitForSingleObject @@ -357,15 +345,14 @@ Status PipeWindows::WriteWithTimeout(const void *buf, size_t size, failed = false; } if (failed) - return Status(failure_error, eErrorTypeWin32); + return Status(failure_error, eErrorTypeWin32).takeError(); } // Now we call GetOverlappedResult setting bWait to false, since we've // already waited as long as we're willing to. - if (!::GetOverlappedResult(m_write, &m_write_overlapped, &sys_bytes_write, + if (!::GetOverlappedResult(m_write, &m_write_overlapped, &bytes_written, FALSE)) - return Status(::GetLastError(), eErrorTypeWin32); + return Status(::GetLastError(), eErrorTypeWin32).takeError(); - bytes_written = sys_bytes_write; - return Status(); + return bytes_written; } diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index f746ced1ff413..dad72a176b5fa 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -30,7 +30,9 @@ #include "lldb/Utility/Log.h" #include "lldb/Utility/RegularExpression.h" #include "lldb/Utility/StreamString.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Config/llvm-config.h" // for LLVM_ENABLE_ZLIB #include "llvm/Support/ScopedPrinter.h" @@ -1154,17 +1156,25 @@ Status GDBRemoteCommunication::StartDebugserverProcess( if (socket_pipe.CanWrite()) socket_pipe.CloseWriteFileDescriptor(); if (socket_pipe.CanRead()) { - // The port number may be up to "65535\0". - char port_cstr[6] = {0}; - size_t num_bytes = sizeof(port_cstr); // Read port from pipe with 10 second timeout. - error = socket_pipe.ReadWithTimeout( - port_cstr, num_bytes, std::chrono::seconds{10}, num_bytes); + std::string port_str; + while (error.Success()) { + char buf[10]; + if (llvm::Expected num_bytes = socket_pipe.Read( + buf, std::size(buf), std::chrono::seconds(10))) { + if (*num_bytes == 0) + break; + port_str.append(buf, *num_bytes); + } else { + error = Status::FromError(num_bytes.takeError()); + } + } if (error.Success() && (port != nullptr)) { - assert(num_bytes > 0 && port_cstr[num_bytes - 1] == '\0'); + // NB: Deliberately using .c_str() to stop at embedded '\0's + llvm::StringRef port_ref = port_str.c_str(); uint16_t child_port = 0; // FIXME: improve error handling - llvm::to_integer(port_cstr, child_port); + llvm::to_integer(port_ref, child_port); if (*port == 0 || *port == child_port) { *port = child_port; LLDB_LOGF(log, diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 6db582096155f..f2f5598f0ab53 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -4675,15 +4675,16 @@ class IOHandlerProcessSTDIO : public IOHandler { } if (select_helper.FDIsSetRead(pipe_read_fd)) { - size_t bytes_read; // Consume the interrupt byte - Status error = m_pipe.Read(&ch, 1, bytes_read); - if (error.Success()) { + if (llvm::Expected bytes_read = m_pipe.Read(&ch, 1)) { if (ch == 'q') break; if (ch == 'i') if (StateIsRunningState(m_process->GetState())) m_process->SendAsyncInterrupt(); + } else { + LLDB_LOG_ERROR(GetLog(LLDBLog::Process), bytes_read.takeError(), + "Pipe read failed: {0}"); } } } @@ -4707,8 +4708,10 @@ class IOHandlerProcessSTDIO : public IOHandler { // deadlocking when the pipe gets fed up and blocks until data is consumed. if (m_is_running) { char ch = 'q'; // Send 'q' for quit - size_t bytes_written = 0; - m_pipe.Write(&ch, 1, bytes_written); + if (llvm::Error err = m_pipe.Write(&ch, 1).takeError()) { + LLDB_LOG_ERROR(GetLog(LLDBLog::Process), std::move(err), + "Pipe write failed: {0}"); + } } } @@ -4720,9 +4723,7 @@ class IOHandlerProcessSTDIO : public IOHandler { // m_process->SendAsyncInterrupt() from a much safer location in code. if (m_active) { char ch = 'i'; // Send 'i' for interrupt - size_t bytes_written = 0; - Status result = m_pipe.Write(&ch, 1, bytes_written); - return result.Success(); + return !errorToBool(m_pipe.Write(&ch, 1).takeError()); } else { // This IOHandler might be pushed on the stack, but not being run // currently so do the right thing if we aren't actively watching for diff --git a/lldb/tools/lldb-server/lldb-gdbserver.cpp b/lldb/tools/lldb-server/lldb-gdbserver.cpp index fec868b1fa9a1..843354147f2da 100644 --- a/lldb/tools/lldb-server/lldb-gdbserver.cpp +++ b/lldb/tools/lldb-server/lldb-gdbserver.cpp @@ -167,27 +167,35 @@ void handle_launch(GDBRemoteCommunicationServerLLGS &gdb_server, } } -Status writeSocketIdToPipe(Pipe &port_pipe, llvm::StringRef socket_id) { - size_t bytes_written = 0; - // Write the port number as a C string with the NULL terminator. - return port_pipe.Write(socket_id.data(), socket_id.size() + 1, bytes_written); +static Status writeSocketIdToPipe(Pipe &port_pipe, + const std::string &socket_id) { + // NB: Include the nul character at the end. + llvm::StringRef buf(socket_id.data(), socket_id.size() + 1); + while (!buf.empty()) { + if (llvm::Expected written = + port_pipe.Write(buf.data(), buf.size())) + buf = buf.drop_front(*written); + else + return Status::FromError(written.takeError()); + } + return Status(); } 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. - auto error = port_name_pipe.OpenAsWriterWithTimeout(named_pipe_path, false, - std::chrono::seconds{10}); - if (error.Fail()) - return error; - return writeSocketIdToPipe(port_name_pipe, socket_id); + if (llvm::Error err = port_name_pipe.OpenAsWriter(named_pipe_path, false, + std::chrono::seconds{10})) + return Status::FromError(std::move(err)); + + return writeSocketIdToPipe(port_name_pipe, socket_id.str()); } Status writeSocketIdToPipe(lldb::pipe_t unnamed_pipe, llvm::StringRef socket_id) { Pipe port_pipe{LLDB_INVALID_PIPE, unnamed_pipe}; - return writeSocketIdToPipe(port_pipe, socket_id); + return writeSocketIdToPipe(port_pipe, socket_id.str()); } void ConnectToRemote(MainLoop &mainloop, diff --git a/lldb/unittests/Host/PipeTest.cpp b/lldb/unittests/Host/PipeTest.cpp index f8fb254b5009c..a3a492648def6 100644 --- a/lldb/unittests/Host/PipeTest.cpp +++ b/lldb/unittests/Host/PipeTest.cpp @@ -10,9 +10,13 @@ #include "TestingSupport/SubsystemRAII.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" +#include "llvm/Testing/Support/Error.h" #include "gtest/gtest.h" +#include #include +#include #include +#include #include using namespace lldb_private; @@ -55,8 +59,6 @@ TEST_F(PipeTest, OpenAsReader) { } #endif -// This test is flaky on Windows on Arm. -#ifndef _WIN32 TEST_F(PipeTest, WriteWithTimeout) { Pipe pipe; ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded()); @@ -87,57 +89,53 @@ TEST_F(PipeTest, WriteWithTimeout) { char *read_ptr = reinterpret_cast(read_buf.data()); size_t write_bytes = 0; size_t read_bytes = 0; - size_t num_bytes = 0; // Write to the pipe until it is full. while (write_bytes + write_chunk_size <= buf_size) { - Status error = - pipe.WriteWithTimeout(write_ptr + write_bytes, write_chunk_size, - std::chrono::milliseconds(10), num_bytes); - if (error.Fail()) + llvm::Expected num_bytes = + pipe.Write(write_ptr + write_bytes, write_chunk_size, + std::chrono::milliseconds(10)); + if (num_bytes) { + write_bytes += *num_bytes; + } else { + ASSERT_THAT_ERROR(num_bytes.takeError(), llvm::Failed()); break; // The write buffer is full. - write_bytes += num_bytes; + } } ASSERT_LE(write_bytes + write_chunk_size, buf_size) << "Pipe buffer larger than expected"; // Attempt a write with a long timeout. auto start_time = std::chrono::steady_clock::now(); - ASSERT_THAT_ERROR(pipe.WriteWithTimeout(write_ptr + write_bytes, - write_chunk_size, - std::chrono::seconds(2), num_bytes) - .ToError(), - llvm::Failed()); + // TODO: Assert a specific error (EAGAIN?) here. + ASSERT_THAT_EXPECTED(pipe.Write(write_ptr + write_bytes, write_chunk_size, + std::chrono::seconds(2)), + llvm::Failed()); auto dur = std::chrono::steady_clock::now() - start_time; ASSERT_GE(dur, std::chrono::seconds(2)); // Attempt a write with a short timeout. start_time = std::chrono::steady_clock::now(); - ASSERT_THAT_ERROR( - pipe.WriteWithTimeout(write_ptr + write_bytes, write_chunk_size, - std::chrono::milliseconds(200), num_bytes) - .ToError(), - llvm::Failed()); + ASSERT_THAT_EXPECTED(pipe.Write(write_ptr + write_bytes, write_chunk_size, + std::chrono::milliseconds(200)), + llvm::Failed()); dur = std::chrono::steady_clock::now() - start_time; ASSERT_GE(dur, std::chrono::milliseconds(200)); ASSERT_LT(dur, std::chrono::seconds(2)); // Drain the pipe. while (read_bytes < write_bytes) { - ASSERT_THAT_ERROR( - pipe.ReadWithTimeout(read_ptr + read_bytes, write_bytes - read_bytes, - std::chrono::milliseconds(10), num_bytes) - .ToError(), - llvm::Succeeded()); - read_bytes += num_bytes; + llvm::Expected num_bytes = + pipe.Read(read_ptr + read_bytes, write_bytes - read_bytes, + std::chrono::milliseconds(10)); + ASSERT_THAT_EXPECTED(num_bytes, llvm::Succeeded()); + read_bytes += *num_bytes; } // Be sure the pipe is empty. - ASSERT_THAT_ERROR(pipe.ReadWithTimeout(read_ptr + read_bytes, 100, - std::chrono::milliseconds(10), - num_bytes) - .ToError(), - llvm::Failed()); + ASSERT_THAT_EXPECTED( + pipe.Read(read_ptr + read_bytes, 100, std::chrono::milliseconds(10)), + llvm::Failed()); // Check that we got what we wrote. ASSERT_EQ(write_bytes, read_bytes); @@ -146,10 +144,56 @@ TEST_F(PipeTest, WriteWithTimeout) { read_buf.begin())); // Write to the pipe again and check that it succeeds. - ASSERT_THAT_ERROR(pipe.WriteWithTimeout(write_ptr, write_chunk_size, - std::chrono::milliseconds(10), - num_bytes) - .ToError(), - llvm::Succeeded()); + ASSERT_THAT_EXPECTED( + pipe.Write(write_ptr, write_chunk_size, std::chrono::milliseconds(10)), + llvm::Succeeded()); +} + +TEST_F(PipeTest, ReadWithTimeout) { + Pipe pipe; + ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded()); + + char buf[100]; + // The pipe is initially empty. A polling read returns immediately. + ASSERT_THAT_EXPECTED(pipe.Read(buf, sizeof(buf), std::chrono::seconds(0)), + llvm::Failed()); + + // With a timeout, we should wait for at least this amount of time (but not + // too much). + auto start = std::chrono::steady_clock::now(); + ASSERT_THAT_EXPECTED( + pipe.Read(buf, sizeof(buf), std::chrono::milliseconds(200)), + llvm::Failed()); + auto dur = std::chrono::steady_clock::now() - start; + EXPECT_GT(dur, std::chrono::milliseconds(200)); + EXPECT_LT(dur, std::chrono::seconds(2)); + + // Write something into the pipe, and read it back. The blocking read call + // should return even though it hasn't filled the buffer. + llvm::StringRef hello_world("Hello world!"); + ASSERT_THAT_EXPECTED(pipe.Write(hello_world.data(), hello_world.size()), + llvm::HasValue(hello_world.size())); + ASSERT_THAT_EXPECTED(pipe.Read(buf, sizeof(buf)), + llvm::HasValue(hello_world.size())); + EXPECT_EQ(llvm::StringRef(buf, hello_world.size()), hello_world); + + // Now write something and try to read it in chunks. + memset(buf, 0, sizeof(buf)); + ASSERT_THAT_EXPECTED(pipe.Write(hello_world.data(), hello_world.size()), + llvm::HasValue(hello_world.size())); + ASSERT_THAT_EXPECTED(pipe.Read(buf, 4), llvm::HasValue(4)); + ASSERT_THAT_EXPECTED(pipe.Read(buf + 4, sizeof(buf) - 4), + llvm::HasValue(hello_world.size() - 4)); + EXPECT_EQ(llvm::StringRef(buf, hello_world.size()), hello_world); + + // A blocking read should wait until the data arrives. + memset(buf, 0, sizeof(buf)); + std::future> future_num_bytes = std::async( + std::launch::async, [&] { return pipe.Read(buf, sizeof(buf)); }); + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + ASSERT_THAT_EXPECTED(pipe.Write(hello_world.data(), hello_world.size()), + llvm::HasValue(hello_world.size())); + ASSERT_THAT_EXPECTED(future_num_bytes.get(), + llvm::HasValue(hello_world.size())); + EXPECT_EQ(llvm::StringRef(buf, hello_world.size()), hello_world); } -#endif /*ifndef _WIN32*/