[Lldb-commits] [lldb] [lldb] Assorted improvements to the Pipe class (PR #128719)
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Tue Feb 25 06:31:03 PST 2025
https://github.com/labath created https://github.com/llvm/llvm-project/pull/128719
The main motivation for this was the inconsistency in handling of partial reads/writes between the windows and posix implementations (windows was returning partial reads, posix was trying to fill the buffer completely). I settle on the windows implementation, as that's the more common behavior, and the "eager" version can be implemented on top of that (in most cases, it isn't necessary, since we're writing just a single byte).
Since this also required auditing the callers to make sure they're handling partial reads/writes correctly, I used the opportunity to modernize the function signatures as a forcing function. They now use the `Timeout` class (basically an `optional<duration>`) to support both polls (timeout=0) and blocking (timeout=nullopt) operations in a single function, and use an `Expected` instead of a by-ref result to return the number of bytes read/written.
As a drive-by, I also fix a problem with the windows implementation where we were rounding the timeout value down, which meant that calls could time out slightly sooner than expected.
>From 9116f3110146965d18eab7e50465855954c87f26 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Tue, 25 Feb 2025 13:30:12 +0100
Subject: [PATCH] [lldb] Assorted improvements to the Pipe class
The main motivation for this was the inconsistency in handling of
partial reads/writes between the windows and posix implementations
(windows was returning partial reads, posix was trying to fill the
buffer completely). I settle on the windows implementation, as that's
the more common behavior, and the "eager" version can be implemented on
top of that (in most cases, it isn't necessary, since we're writing just
a single byte).
Since this also required auditing the callers to make sure they're
handling partial reads/writes correctly, I used the opportunity to
modernize the function signatures as a forcing function. They now use
the `Timeout` class (basically an `optional<duration>`) to support both
polls (timeout=0) and blocking (timeout=nullopt) operations in a single
function, and use an `Expected` instead of a by-ref result to return the
number of bytes read/written.
As a drive-by, I also fix a problem with the windows implementation
where we were rounding the timeout value down, which meant that calls
could time out slightly sooner than expected.
---
lldb/include/lldb/Host/PipeBase.h | 27 ++---
lldb/include/lldb/Host/posix/PipePosix.h | 19 ++--
lldb/include/lldb/Host/windows/PipeWindows.h | 18 +--
lldb/source/Host/common/PipeBase.cpp | 16 ---
lldb/source/Host/common/Socket.cpp | 29 ++---
.../posix/ConnectionFileDescriptorPosix.cpp | 16 +--
lldb/source/Host/posix/MainLoopPosix.cpp | 6 +-
lldb/source/Host/posix/PipePosix.cpp | 107 ++++++++----------
lldb/source/Host/windows/PipeWindows.cpp | 87 ++++++--------
.../gdb-remote/GDBRemoteCommunication.cpp | 24 ++--
lldb/source/Target/Process.cpp | 17 +--
lldb/tools/lldb-server/lldb-gdbserver.cpp | 28 +++--
lldb/unittests/Host/PipeTest.cpp | 61 +++++-----
13 files changed, 203 insertions(+), 252 deletions(-)
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 <chrono>
-#include <string>
-
#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<std::micro> &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<size_t>
+ Write(const void *buf, size_t size,
+ const Timeout<std::micro> &timeout = std::nullopt) = 0;
+
+ virtual llvm::Expected<size_t>
+ Read(void *buf, size_t size,
+ const Timeout<std::micro> &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 <mutex>
@@ -38,9 +39,8 @@ class PipePosix : public PipeBase {
llvm::SmallVectorImpl<char> &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<std::micro> &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<size_t>
+ Write(const void *buf, size_t size,
+ const Timeout<std::micro> &timeout = std::nullopt) override;
+
+ llvm::Expected<size_t>
+ Read(void *buf, size_t size,
+ const Timeout<std::micro> &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<char> &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<std::micro> &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<size_t>
+ Write(const void *buf, size_t size,
+ const Timeout<std::micro> &timeout = std::nullopt) override;
+
+ llvm::Expected<size_t>
+ Read(void *buf, size_t size,
+ const Timeout<std::micro> &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..24418cd570904 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -93,15 +93,13 @@ 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<size_t> 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: {0} bytes", *num_bytes);
#endif
return Status();
}
@@ -113,16 +111,13 @@ 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<size_t> 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);
+ "socket_pipe.Read(WSAPROTOCOL_INFO) failed: {0} bytes", *num_bytes);
}
}
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<std::recursive_mutex> 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<void *>(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 <functional>
+#include <system_error>
#include <thread>
#include <cerrno>
@@ -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<std::micro> &timeout) {
std::lock_guard<std::mutex> 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<time_point<steady_clock>> finish_time;
+ if (timeout)
+ finish_time = Now() + *timeout;
while (!CanWriteUnlocked()) {
- if (timeout != microseconds::zero()) {
- const auto dur = duration_cast<microseconds>(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<size_t> PipePosix::Read(void *buf, size_t size,
+ const Timeout<std::micro> &timeout) {
std::lock_guard<std::mutex> 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<char *>(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<size_t> PipePosix::Write(const void *buf, size_t size,
+ const Timeout<std::micro> &timeout) {
std::lock_guard<std::mutex> 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<const char *>(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<std::micro> &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<size_t> PipeWindows::Read(void *buf, size_t size,
+ const Timeout<std::micro> &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<std::chrono::milliseconds>(*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<size_t> PipeWindows::Write(const void *buf, size_t size,
+ const Timeout<std::micro> &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<std::chrono::milliseconds>(*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..a5d828a45e8a7 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<size_t> num_bytes = socket_pipe.Read(
+ buf, std::size(buf), std::chrono::seconds(10))) {
+ port_str.append(buf, *num_bytes);
+ if (*num_bytes == 0)
+ break;
+ } 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<size_t> 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<size_t> 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..20c2b54ddf7d8 100644
--- a/lldb/unittests/Host/PipeTest.cpp
+++ b/lldb/unittests/Host/PipeTest.cpp
@@ -55,8 +55,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 +85,53 @@ TEST_F(PipeTest, WriteWithTimeout) {
char *read_ptr = reinterpret_cast<char *>(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<size_t> 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<size_t> 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 +140,7 @@ 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());
}
-#endif /*ifndef _WIN32*/
More information about the lldb-commits
mailing list