[lldb] Assorted improvements to the Pipe class #128719
Conversation
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.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThe 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 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. Patch is 32.98 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/128719.diff 13 Files Affected:
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 ...
[truncated]
|
| if (result == -1) | ||
| return llvm::errorCodeToError( | ||
| std::error_code(errno, std::generic_category())); |
There was a problem hiding this comment.
Should we check for EINTR? (or RetryAfterSignal)?
There was a problem hiding this comment.
I don't believe that necessary because EINTR is returned when a blocking syscall is interrupted, and the select call above basically guarantees that this will not block:
If a signal handler is invoked while a system call or library function call is blocked, then either:
• the call is automatically restarted after the signal handler returns; or
• the call fails with the error EINTR.
select() allows a program to monitor multiple file descriptors, waiting until one or more of the
file descriptors become "ready" for some class of I/O operation (e.g., input possible). A file
descriptor is considered ready if it is possible to perform a corresponding I/O operation (e.g.,
read(2), or a sufficiently small write(2)) without blocking.
This is based on the linux documentation, but I'd expect other systems to behave the same way. That said, the check doesn't hurt much, so I'd fine with adding it if you think it helps.
| if (result == -1) | ||
| return llvm::errorCodeToError( | ||
| std::error_code(errno, std::generic_category())); |
There was a problem hiding this comment.
Should we check for EINTR? (or RetryAfterSignal)?
| port_cstr, num_bytes, std::chrono::seconds{10}, num_bytes); | ||
| std::string port_str; | ||
| while (error.Success()) { | ||
| char buf[10]; |
There was a problem hiding this comment.
Does this need to be initialized to zero? = {0};?
There was a problem hiding this comment.
Not really. I'm only accessing the part that has been filled in by the read call.
| @@ -55,8 +55,6 @@ TEST_F(PipeTest, OpenAsReader) { | |||
| } | |||
| #endif | |||
|
|
|||
| // This test is flaky on Windows on Arm. | |||
| #ifndef _WIN32 | |||
| TEST_F(PipeTest, WriteWithTimeout) { | |||
There was a problem hiding this comment.
Should we add some tests for the Reader as well?
Something along the lines of:
* Read(buf[100], timeout=10s)
* Write("hello world") (under the buffer size)
* AssertEqual(buf, "hello world")
* Read(buf[100], timeout=10s)
* Write("hello world" * 200) (over the buffer size)
* AssertEqual(buf, ("hello world" * 200).slice(0, 100))
* Read(buf[100]) // read the rest
There was a problem hiding this comment.
Good idea. I should have done that originally.
| DWORD timeout_msec = | ||
| timeout ? ceil<std::chrono::milliseconds>(*timeout).count() : INFINITE; | ||
| DWORD wait_result = | ||
| ::WaitForSingleObject(m_read_overlapped.hEvent, timeout_msec); |
There was a problem hiding this comment.
More for my understanding, but could we use WaitForSingleObject / WaitForMultipleObjects to emulate select on a FileHandle? That could make SelectHelper work on files and sockets on all platforms instead of just sockets on Windows.
There was a problem hiding this comment.
Some form of multiplexing is possible, but not in the same way as on posix systems. Windows doesn't have a general select capability like the posix systems to (sockets are an exception, which I think was specially added for BSD compatibility). Windows multiplexing works more like async i/o on (some?) posix systems. You can't ask whether there is a data in pipe. You have to actually try to read the data (and then maybe give up after a while).
My long term plan for this is to integrate this functionality into the MainLoop class. However, that's tricky because we'd need to change the way the data is read. MainLoop can't just call us back when the data is ready. It actually has to (in one way or another) read the data (and then hand it back through the callback?). It's never been much of a priority, so I don't know when/if I can get around to it, but if you find yourself needing something like this, I think I'll be able to help you with the windows bits at least.
There was a problem hiding this comment.
Gotcha, I looked at libdispatch (from swift) and libuv (used by nodejs) for how they perform async IO on windows, to see if I could figure out how to do that for MainLoop to support File/NativeFile handles. That would be helpful if it did work for simplifying some parts of lldb-dap, but I've never really developed for Windows beyond just getting the lldb-dap tests to run. I'm not familiar with these APIs enough to understand the nuances in behavior. From my reading of those libraries, the main APIs are around CreateIoCompletionPort / PostQueuedCompletionStatus / GetQueuedCompletionStatus and I think they're using a monitoring thread to detect changes.
There was a problem hiding this comment.
Hm... this is interesting. I didn't know about the CreateIoCompletionPort API. It appears to me a mechanism to notify a thread when an async operation completes, but it doesn't quite explain how you can tell whether there is data in the pipe without trying to read from it (which is the main problem I have).
This part, however might just do that. It appears as if they are using a zero-length read as a way to block until there is data in the pipe -- without actually reading that data. If that works as it looks it might, then we might just have a way to implement something select-like on a pipe object (even without the extra thread).
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
All my questions answered, I leave it to @ashgti as the relative expert to be the approver. |
lldb/unittests/Host/PipeTest.cpp
Outdated
| memset(buf, 0, sizeof(buf)); | ||
| std::future<llvm::Expected<size_t>> future_num_bytes = std::async( | ||
| std::launch::async, [&] { return pipe.Read(buf, sizeof(buf)); }); | ||
| std::this_thread::sleep_for(std::chrono::seconds(1)); |
There was a problem hiding this comment.
maybe std::chrono::milliseconds(10)?
There was a problem hiding this comment.
Yeah, that should be enough. Even if e.g. the thread doesn't manage to get scheduled in those 10ms in some runs, it's very unlikely to do that in every test run.
| DWORD timeout_msec = | ||
| timeout ? ceil<std::chrono::milliseconds>(*timeout).count() : INFINITE; | ||
| DWORD wait_result = | ||
| ::WaitForSingleObject(m_read_overlapped.hEvent, timeout_msec); |
There was a problem hiding this comment.
Gotcha, I looked at libdispatch (from swift) and libuv (used by nodejs) for how they perform async IO on windows, to see if I could figure out how to do that for MainLoop to support File/NativeFile handles. That would be helpful if it did work for simplifying some parts of lldb-dap, but I've never really developed for Windows beyond just getting the lldb-dap tests to run. I'm not familiar with these APIs enough to understand the nuances in behavior. From my reading of those libraries, the main APIs are around CreateIoCompletionPort / PostQueuedCompletionStatus / GetQueuedCompletionStatus and I think they're using a monitoring thread to detect changes.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/197/builds/2536 Here is the relevant piece of the build log for the reference |
|
It looks like that the windows timeout fix didn't work -- the new test woke ended up waking slightly sooner than expected (198 vs 200ms). At this point I'm guessing that this is due to the use different clocks in the test ( I can see two ways to fix this: relax the test to accept a value slightly less than the expected interval; or add a loop to PipeWindows to wait until the the specified amount of time really elapses. I'm sort of in favor of the second option because it gives a nice guarantee, although I don't expect anybody but tests to care about the few missing milliseconds. Any other opinions? |
The tests are flaky because the read/write calls return sooner than they should (and #128719 does not fix them). Skip them until we figure the best way to fix this.
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.
The tests are flaky because the read/write calls return sooner than they should (and llvm#128719 does not fix them). Skip them until we figure the best way to fix this.
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
Timeoutclass (basically anoptional<duration>) to support both polls (timeout=0) and blocking (timeout=nullopt) operations in a single function, and use anExpectedinstead 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.