[lldb] Adding pipe support to lldb_private::MainLoopWindows. #145621
Conversation
ashgti
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ashgti
force-pushed
the
lldb-mainloopwindows
branch
5 times, most recently
from
3db6bad to
954f107
Compare
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis updates MainLoopWindows to support events for reading from a file and a socket type. This unifies both handle types using This should allow us to unify how we handle watching files/pipes/sockets on Windows and Posix systems. Full diff: https://github.com/llvm/llvm-project/pull/145621.diff 11 Files Affected:
diff --git a/lldb/include/lldb/Host/File.h b/lldb/include/lldb/Host/File.h
index 9e2d0abe0b1af..36cb192281289 100644
--- a/lldb/include/lldb/Host/File.h
+++ b/lldb/include/lldb/Host/File.h
@@ -127,6 +127,7 @@ class File : public IOObject {
/// \return
/// a valid handle or IOObject::kInvalidHandleValue
WaitableHandle GetWaitableHandle() override;
+ bool HasReadableData() override;
/// Get the file specification for this file, if possible.
///
@@ -400,6 +401,7 @@ class NativeFile : public File {
Status Write(const void *buf, size_t &num_bytes) override;
Status Close() override;
WaitableHandle GetWaitableHandle() override;
+ bool HasReadableData() override;
Status GetFileSpec(FileSpec &file_spec) const override;
int GetDescriptor() const override;
FILE *GetStream() override;
diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 89953ee7fd5b6..6569e9e6ea818 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -158,6 +158,7 @@ class Socket : public IOObject {
bool IsValid() const override { return m_socket != kInvalidSocketValue; }
WaitableHandle GetWaitableHandle() override;
+ bool HasReadableData() override;
static llvm::Expected<HostAndPort>
DecodeHostAndPort(llvm::StringRef host_and_port);
@@ -185,6 +186,7 @@ class Socket : public IOObject {
SocketProtocol m_protocol;
NativeSocket m_socket;
+ WaitableHandle m_waitable_handle;
bool m_should_close_fd;
};
diff --git a/lldb/include/lldb/Host/windows/MainLoopWindows.h b/lldb/include/lldb/Host/windows/MainLoopWindows.h
index 3937a24645d95..43b7d13a0e445 100644
--- a/lldb/include/lldb/Host/windows/MainLoopWindows.h
+++ b/lldb/include/lldb/Host/windows/MainLoopWindows.h
@@ -37,11 +37,10 @@ class MainLoopWindows : public MainLoopBase {
void Interrupt() override;
private:
- void ProcessReadObject(IOObject::WaitableHandle handle);
llvm::Expected<size_t> Poll();
struct FdInfo {
- void *event;
+ lldb::IOObjectSP object_sp;
Callback callback;
};
llvm::DenseMap<IOObject::WaitableHandle, FdInfo> m_read_fds;
diff --git a/lldb/include/lldb/Utility/IOObject.h b/lldb/include/lldb/Utility/IOObject.h
index 8cf42992e7be5..48a8a2076581f 100644
--- a/lldb/include/lldb/Utility/IOObject.h
+++ b/lldb/include/lldb/Utility/IOObject.h
@@ -14,6 +14,7 @@
#include <sys/types.h>
#include "lldb/lldb-private.h"
+#include "lldb/lldb-types.h"
namespace lldb_private {
@@ -24,9 +25,9 @@ class IOObject {
eFDTypeSocket, // Socket requiring send/recv
};
- // TODO: On Windows this should be a HANDLE, and wait should use
- // WaitForMultipleObjects
- typedef int WaitableHandle;
+ // A handle for integrating with the host event loop model.
+ using WaitableHandle = lldb::file_t;
+
static const WaitableHandle kInvalidHandleValue;
IOObject(FDType type) : m_fd_type(type) {}
@@ -40,6 +41,7 @@ class IOObject {
FDType GetFdType() const { return m_fd_type; }
virtual WaitableHandle GetWaitableHandle() = 0;
+ virtual bool HasReadableData() = 0;
protected:
FDType m_fd_type;
diff --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp
index 9aa95ffda44cb..2d33f9e2028c4 100644
--- a/lldb/source/Host/common/File.cpp
+++ b/lldb/source/Host/common/File.cpp
@@ -118,6 +118,8 @@ IOObject::WaitableHandle File::GetWaitableHandle() {
return IOObject::kInvalidHandleValue;
}
+bool File::HasReadableData() { return false; }
+
Status File::GetFileSpec(FileSpec &file_spec) const {
file_spec.Clear();
return std::error_code(ENOTSUP, std::system_category());
@@ -274,7 +276,23 @@ int NativeFile::GetDescriptor() const {
}
IOObject::WaitableHandle NativeFile::GetWaitableHandle() {
+#ifdef _WIN32
+ return (HANDLE)_get_osfhandle(GetDescriptor());
+#else
return GetDescriptor();
+#endif
+}
+
+bool NativeFile::HasReadableData() {
+#ifdef _WIN32
+ DWORD available_bytes = 0;
+ return !PeekNamedPipe((HANDLE)_get_osfhandle(GetDescriptor()), NULL, 0, NULL,
+ &available_bytes, NULL) ||
+ available_bytes > 0;
+#else
+ size_t buffer_size = 0;
+ return ioctl(GetDescriptor(), FIONREAD, buffer_size) != -1 && buffer_size > 0;
+#endif
}
FILE *NativeFile::GetStream() {
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index 2b23fd1e6e57e..47ccbd94ce510 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -31,8 +31,11 @@
#include <netdb.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
+#include <sys/ioctl.h>
#include <sys/socket.h>
+#include <sys/stat.h>
#include <sys/un.h>
+#include <termios.h>
#include <unistd.h>
#endif
@@ -169,7 +172,9 @@ bool Socket::FindProtocolByScheme(const char *scheme,
Socket::Socket(SocketProtocol protocol, bool should_close)
: IOObject(eFDTypeSocket), m_protocol(protocol),
- m_socket(kInvalidSocketValue), m_should_close_fd(should_close) {}
+ m_socket(kInvalidSocketValue),
+ m_waitable_handle(IOObject::kInvalidHandleValue),
+ m_should_close_fd(should_close) {}
Socket::~Socket() { Close(); }
@@ -313,8 +318,39 @@ Socket::DecodeHostAndPort(llvm::StringRef host_and_port) {
}
IOObject::WaitableHandle Socket::GetWaitableHandle() {
- // TODO: On Windows, use WSAEventSelect
+#ifdef _WIN32
+ if (m_socket == kInvalidSocketValue)
+ return kInvalidHandleValue;
+
+ if (m_waitable_handle == kInvalidHandleValue) {
+ m_waitable_handle = WSACreateEvent();
+ assert(m_waitable_handle != WSA_INVALID_EVENT);
+ if (WSAEventSelect(m_socket, m_waitable_handle,
+ FD_ACCEPT | FD_READ | FD_WRITE) != 0)
+ return kInvalidHandleValue;
+ }
+
+ return m_waitable_handle;
+#else
return m_socket;
+#endif
+}
+
+bool Socket::HasReadableData() {
+#ifdef _WIN32
+ if (!IsValid() || m_waitable_handle == kInvalidHandleValue)
+ return false;
+
+ WSANETWORKEVENTS events;
+ if (WSAEnumNetworkEvents(m_socket, m_waitable_handle, &events) != 0)
+ return false;
+
+ return events.lNetworkEvents & FD_CLOSE ||
+ events.lNetworkEvents & FD_ACCEPT || events.lNetworkEvents & FD_READ;
+#else
+ size_t buffer_size = 0;
+ return ioctl(m_socket, FIONREAD, buffer_size) != -1 && buffer_size > 0;
+#endif
}
Status Socket::Read(void *buf, size_t &num_bytes) {
@@ -380,7 +416,14 @@ Status Socket::Close() {
Log *log = GetLog(LLDBLog::Connection);
LLDB_LOGF(log, "%p Socket::Close (fd = %" PRIu64 ")",
static_cast<void *>(this), static_cast<uint64_t>(m_socket));
-
+#ifdef _WIN32
+ if (m_waitable_handle != kInvalidHandleValue) {
+ if (WSACloseEvent(m_waitable_handle) == 0)
+ m_waitable_handle = kInvalidHandleValue;
+ else
+ error = GetLastError();
+ }
+#endif
bool success = CloseSocket(m_socket) == 0;
// A reference to a FD was passed in, set it to an invalid value
m_socket = kInvalidSocketValue;
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 57dce44812c89..2fcad7f193e1a 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -276,7 +276,7 @@ size_t ConnectionFileDescriptor::Read(void *dst, size_t dst_len,
"%p ConnectionFileDescriptor::Read() fd = %" PRIu64
", dst = %p, dst_len = %" PRIu64 ") => %" PRIu64 ", error = %s",
static_cast<void *>(this),
- static_cast<uint64_t>(m_io_sp->GetWaitableHandle()),
+ static_cast<file_t>(m_io_sp->GetWaitableHandle()),
static_cast<void *>(dst), static_cast<uint64_t>(dst_len),
static_cast<uint64_t>(bytes_read), error.AsCString());
}
@@ -380,7 +380,7 @@ size_t ConnectionFileDescriptor::Write(const void *src, size_t src_len,
"%p ConnectionFileDescriptor::Write(fd = %" PRIu64
", src = %p, src_len = %" PRIu64 ") => %" PRIu64 " (error = %s)",
static_cast<void *>(this),
- static_cast<uint64_t>(m_io_sp->GetWaitableHandle()),
+ static_cast<file_t>(m_io_sp->GetWaitableHandle()),
static_cast<const void *>(src), static_cast<uint64_t>(src_len),
static_cast<uint64_t>(bytes_sent), error.AsCString());
}
@@ -451,14 +451,17 @@ ConnectionFileDescriptor::BytesAvailable(const Timeout<std::micro> &timeout,
if (timeout)
select_helper.SetTimeout(*timeout);
- select_helper.FDSetRead(handle);
+ // FIXME: Migrate to MainLoop.
#if defined(_WIN32)
+ if (const auto *sock = static_cast<Socket *>(m_io_sp.get()))
+ select_helper.FDSetRead((socket_t)sock->GetNativeSocket());
// select() won't accept pipes on Windows. The entire Windows codepath
// needs to be converted over to using WaitForMultipleObjects and event
// HANDLEs, but for now at least this will allow ::select() to not return
// an error.
const bool have_pipe_fd = false;
#else
+ select_helper.FDSetRead(handle);
const bool have_pipe_fd = pipe_fd >= 0;
#endif
if (have_pipe_fd)
@@ -493,7 +496,12 @@ ConnectionFileDescriptor::BytesAvailable(const Timeout<std::micro> &timeout,
break; // Lets keep reading to until we timeout
}
} else {
+#if defined(_WIN32)
+ if (const auto *sock = static_cast<Socket *>(m_io_sp.get());
+ select_helper.FDIsSetRead(sock->GetNativeSocket()))
+#else
if (select_helper.FDIsSetRead(handle))
+#endif
return eConnectionStatusSuccess;
if (select_helper.FDIsSetRead(pipe_fd)) {
diff --git a/lldb/source/Host/windows/MainLoopWindows.cpp b/lldb/source/Host/windows/MainLoopWindows.cpp
index f3ab2a710cd01..33cc06266ce03 100644
--- a/lldb/source/Host/windows/MainLoopWindows.cpp
+++ b/lldb/source/Host/windows/MainLoopWindows.cpp
@@ -8,8 +8,11 @@
#include "lldb/Host/windows/MainLoopWindows.h"
#include "lldb/Host/Config.h"
+#include "lldb/Host/Socket.h"
#include "lldb/Utility/Status.h"
#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/WindowsError.h"
#include <algorithm>
#include <cassert>
#include <cerrno>
@@ -21,16 +24,6 @@
using namespace lldb;
using namespace lldb_private;
-static DWORD ToTimeout(std::optional<MainLoopWindows::TimePoint> point) {
- using namespace std::chrono;
-
- if (!point)
- return WSA_INFINITE;
-
- nanoseconds dur = (std::max)(*point - steady_clock::now(), nanoseconds(0));
- return ceil<milliseconds>(dur).count();
-}
-
MainLoopWindows::MainLoopWindows() {
m_interrupt_event = WSACreateEvent();
assert(m_interrupt_event != WSA_INVALID_EVENT);
@@ -44,36 +37,61 @@ MainLoopWindows::~MainLoopWindows() {
}
llvm::Expected<size_t> MainLoopWindows::Poll() {
- std::vector<WSAEVENT> events;
+ std::vector<HANDLE> events;
events.reserve(m_read_fds.size() + 1);
- for (auto &[fd, info] : m_read_fds) {
- int result = WSAEventSelect(fd, info.event, FD_READ | FD_ACCEPT | FD_CLOSE);
- assert(result == 0);
- UNUSED_IF_ASSERT_DISABLED(result);
-
- events.push_back(info.event);
+ for (auto &[fd, fd_info] : m_read_fds) {
+ // short circuit waiting if a handle is already ready.
+ if (fd_info.object_sp->HasReadableData())
+ return events.size();
+ events.push_back(fd);
}
events.push_back(m_interrupt_event);
- DWORD result =
- WSAWaitForMultipleEvents(events.size(), events.data(), FALSE,
- ToTimeout(GetNextWakeupTime()), FALSE);
+ while (true) {
+ DWORD timeout = INFINITY;
+ std::optional<lldb_private::MainLoopBase::TimePoint> deadline =
+ GetNextWakeupTime();
+ if (deadline) {
+ // Check how much time is remaining, we may have woken up early for an
+ // unrelated reason on a file descriptor (e.g. a stat was triggered).
+ std::chrono::milliseconds remaining =
+ std::chrono::duration_cast<std::chrono::milliseconds>(
+ deadline.value() - std::chrono::steady_clock::now());
+ if (remaining.count() <= 0)
+ return events.size() - 1;
+ timeout = remaining.count();
+ }
- for (auto &fd : m_read_fds) {
- int result = WSAEventSelect(fd.first, WSA_INVALID_EVENT, 0);
- assert(result == 0);
- UNUSED_IF_ASSERT_DISABLED(result);
+ DWORD result =
+ WaitForMultipleObjects(events.size(), events.data(), FALSE, timeout);
+
+ // A timeout is treated as a (premature) signalization of the interrupt
+ // event.
+ if (result == WAIT_TIMEOUT)
+ return events.size() - 1;
+
+ if (result == WAIT_FAILED)
+ return llvm::createStringError(llvm::mapLastWindowsError(),
+ "WaitForMultipleObjects failed");
+
+ // check if interrupt requested.
+ if (result == WAIT_OBJECT_0 + events.size())
+ return result - WAIT_OBJECT_0;
+
+ // An object may be signaled before data is ready for reading, verify it has
+ // data.
+ if (result >= WAIT_OBJECT_0 &&
+ result < WAIT_OBJECT_0 + (events.size() - 1) &&
+ std::next(m_read_fds.begin(), result - WAIT_OBJECT_0)
+ ->second.object_sp->HasReadableData())
+ return result - WAIT_OBJECT_0;
+
+ // If no handles are actually ready then yield the thread to allow the CPU
+ // to progress.
+ std::this_thread::yield();
}
- if (result >= WSA_WAIT_EVENT_0 && result < WSA_WAIT_EVENT_0 + events.size())
- return result - WSA_WAIT_EVENT_0;
-
- // A timeout is treated as a (premature) signalization of the interrupt event.
- if (result == WSA_WAIT_TIMEOUT)
- return events.size() - 1;
-
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "WSAWaitForMultipleEvents failed");
+ llvm_unreachable();
}
MainLoopWindows::ReadHandleUP
@@ -83,28 +101,16 @@ MainLoopWindows::RegisterReadObject(const IOObjectSP &object_sp,
error = Status::FromErrorString("IO object is not valid.");
return nullptr;
}
- if (object_sp->GetFdType() != IOObject::eFDTypeSocket) {
- error = Status::FromErrorString(
- "MainLoopWindows: non-socket types unsupported on Windows");
- return nullptr;
- }
- WSAEVENT event = WSACreateEvent();
- if (event == WSA_INVALID_EVENT) {
- error =
- Status::FromErrorStringWithFormat("Cannot create monitoring event.");
- return nullptr;
- }
+ IOObject::WaitableHandle waitable_handle = object_sp->GetWaitableHandle();
+ assert(waitable_handle != IOObject::kInvalidHandleValue);
const bool inserted =
- m_read_fds
- .try_emplace(object_sp->GetWaitableHandle(), FdInfo{event, callback})
+ m_read_fds.try_emplace(waitable_handle, FdInfo{object_sp, callback})
.second;
if (!inserted) {
- WSACloseEvent(event);
error = Status::FromErrorStringWithFormat(
- "File descriptor %d already monitored.",
- object_sp->GetWaitableHandle());
+ "File descriptor %d already monitored.", waitable_handle);
return nullptr;
}
@@ -114,18 +120,9 @@ MainLoopWindows::RegisterReadObject(const IOObjectSP &object_sp,
void MainLoopWindows::UnregisterReadObject(IOObject::WaitableHandle handle) {
auto it = m_read_fds.find(handle);
assert(it != m_read_fds.end());
- BOOL result = WSACloseEvent(it->second.event);
- assert(result == TRUE);
- UNUSED_IF_ASSERT_DISABLED(result);
m_read_fds.erase(it);
}
-void MainLoopWindows::ProcessReadObject(IOObject::WaitableHandle handle) {
- auto it = m_read_fds.find(handle);
- if (it != m_read_fds.end())
- it->second.callback(*this); // Do the work
-}
-
Status MainLoopWindows::Run() {
m_terminate_request = false;
@@ -138,8 +135,7 @@ Status MainLoopWindows::Run() {
if (*signaled_event < m_read_fds.size()) {
auto &KV = *std::next(m_read_fds.begin(), *signaled_event);
- WSAResetEvent(KV.second.event);
- ProcessReadObject(KV.first);
+ KV.second.callback(*this); // Do the work.
} else {
assert(*signaled_event == m_read_fds.size());
WSAResetEvent(m_interrupt_event);
diff --git a/lldb/source/Utility/IOObject.cpp b/lldb/source/Utility/IOObject.cpp
index 964edce0ce10d..c0c07cc0b68e3 100644
--- a/lldb/source/Utility/IOObject.cpp
+++ b/lldb/source/Utility/IOObject.cpp
@@ -8,7 +8,16 @@
#include "lldb/Utility/IOObject.h"
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
+#endif
+
using namespace lldb_private;
+#ifdef _WIN32
+const IOObject::WaitableHandle IOObject::kInvalidHandleValue =
+ INVALID_HANDLE_VALUE;
+#else
const IOObject::WaitableHandle IOObject::kInvalidHandleValue = -1;
+#endif
IOObject::~IOObject() = default;
diff --git a/lldb/unittests/Host/FileTest.cpp b/lldb/unittests/Host/FileTest.cpp
index 35c87bb200fad..d973d19430596 100644
--- a/lldb/unittests/Host/FileTest.cpp
+++ b/lldb/unittests/Host/FileTest.cpp
@@ -14,6 +14,10 @@
#include "llvm/Support/Program.h"
#include "gtest/gtest.h"
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
+#endif
+
using namespace lldb;
using namespace lldb_private;
@@ -32,7 +36,11 @@ TEST(File, GetWaitableHandleFileno) {
ASSERT_TRUE(stream);
NativeFile file(stream, true);
- EXPECT_EQ(file.GetWaitableHandle(), fd);
+#ifdef _WIN32
+ EXPECT_EQ(file.GetWaitableHandle(), (HANDLE)_get_osfhandle(fd));
+#else
+ EXPECT_EQ(file.GetWaitableHandle(), (file_t)fd);
+#endif
}
TEST(File, GetStreamFromDescriptor) {
@@ -53,5 +61,9 @@ TEST(File, GetStreamFromDescriptor) {
ASSERT_TRUE(stream != NULL);
EXPECT_EQ(file.GetDescriptor(), fd);
- EXPECT_EQ(file.GetWaitableHandle(), fd);
+#ifdef _WIN32
+ EXPECT_EQ(file.GetWaitableHandle(), (HANDLE)_get_osfhandle(fd));
+#else
+ EXPECT_EQ(file.GetWaitableHandle(), (file_t)fd);
+#endif
}
diff --git a/lldb/unittests/Host/MainLoopTest.cpp b/lldb/unittests/Host/MainLoopTest.cpp
index e18489978e90c..b8d7f57108017 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -79,6 +79,28 @@ TEST_F(MainLoopTest, ReadObject) {
ASSERT_EQ(1u, callback_count);
}
+TEST_F(MainLoopTest, ReadPipeObject) {
+ Pipe pipe;
+
+ ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded());
+
+ MainLoop loop;
+
+ char X = 'X';
+ size_t len = sizeof(X);
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::Succeeded());
+
+ Status error;
+ auto handle = loop.RegisterReadObject(
+ std::make_shared<NativeFile>(pipe.GetReadFileDescriptor(),
+ File::eOpenOptionReadOnly, false),
+ make_callback(), error);
+ ASSERT_TRUE(error.Success());
+ ASSERT_TRUE(handle);
+ ASSERT_TRUE(loop.Run().Success());
+ ASSERT_EQ(1u, callback_count);
+}
+
TEST_F(MainLoopTest, NoSpuriousReads) {
// Write one byte into the socket.
char X = 'X';
@@ -166,6 +188,10 @@ TEST_F(MainLoopTest, PendingCallbackCalledOnlyOnce) {
if (callback_count == 0) {
loop.AddPendingCallback([&](MainLoopBase &loop) {
callback_count++;
+ char X = 'X';
+ size_t len = sizeof(X);
+ // Write to trigger read object again.
+ ASSERT_TRUE(socketpair[0]->Write(&X, len).Success());
});
}
// Terminate the loop on second iteration.
|
There was a problem hiding this comment.
Thank you again for looking into this.
I think that the most important question we need to figure out is what's the danger of this turning into a busy wait. A function like HasReadableData could be useful in some situation, so I don't have a problem with it per-se, but the way you're using it makes me very suspicious that can turn into a busy wait under some circumstances. I think we at least need to know what those circumstances are, so we can evaluate whether that worth the (very tangible) benefit of being able to use this functionality portably.
lldb/source/Host/common/File.cpp
Outdated
| bool NativeFile::HasReadableData() { | ||
| #ifdef _WIN32 | ||
| DWORD available_bytes = 0; | ||
| return !PeekNamedPipe((HANDLE)_get_osfhandle(GetDescriptor()), NULL, 0, NULL, |
There was a problem hiding this comment.
This will only work for "files" that are actually pipes, right?
If so, I don't think that's a good long-term direction. My medium-to-long term plan would be to make Pipe an IOObject, but that requires splitting it into two (one IOObject for each end). I don't think that would be a particularly hard refactor, but if you don't want to take that on, maybe we can find a way to pass a Pipe to MainLoop directly (and keep NativeFile out of it)? I'd be fine with an RegisterReadObject overload that takes a Pipe (and ignores the write end of the pipe -- we don't select on writes anyway).
There was a problem hiding this comment.
I've split this up for checking between files and pipes. For files, we'll get notified anytime the file changes on disk, for pipes we'll use the zero byte read in a monitor thread to signal when new data is available. That strategy should work with both named pipes and anonymous pipes. I'm checking the file type we get out of the IOObject though, so this should work if you passed in stdin as well.
There was a problem hiding this comment.
That's better, but I do have a question, and a comment :)
-
Do we need to support selecting on actual files? That doesn't even work on posix systems (the file just always comes back as "readable" -- which makes sense because select just tells you whether the operation will block), so if anything, I think we should emulate that. Waiting for file modification requires things like inotify(7), and fortunately, noone has had a use case for that for now. If we ever needed to support it, that should probably be a separate kind of an event. I'm assuming that
GetFileTypeon a pipe handle will returnFILE_TYPE_PIPE, even if that handle was passed asstdin. -
it my long term vision, I would like lldb-dap to create Pipe IOObject directly (perhaps via some helper on the File class) instead of having the MainLoop class guess it. However, given that Pipe is not an IOObject right now, this is something I can live with.
lldb/source/Host/common/Socket.cpp
Outdated
| m_waitable_handle = WSACreateEvent(); | ||
| assert(m_waitable_handle != WSA_INVALID_EVENT); | ||
| if (WSAEventSelect(m_socket, m_waitable_handle, | ||
| FD_ACCEPT | FD_READ | FD_WRITE) != 0) |
There was a problem hiding this comment.
Does this mean that the event will be triggered whenever the socket is writable (triggering the busy wait I mentioned before)? That would definitely be undesirable.
If WSAEventSelect supports it, we might be able to deal with that by using separate events for reads and writes. The question is somewhat academic because noone is currently selecting for writes, but it's interesting from the perspective of where should that event live. You're moving it from the MainLoop to the Socket, which kind of makes sense, but it may not be possible if there isn't a single event (or a set of events) that can be used for everything.
In that world, I think that the original setup (where the event is managed inside the main loop) makes more sense as the loop can WSAEventSelect precisely the events it is interested it. And it should mostly (as long as they don't attempt that in parallel) work with sockets that are added to multiple main loops (which definitely works on posix).
In principle, I'd be fine with a one-mainloop-per-socket restriction, but we'd need to do some refactoring to make that happen as we currently in fact use multiple instances of selector in some situations (ConnectionFileDescriptor uses a SelectHelper internally and lldb-server adds the connection to its top level main loop).
There was a problem hiding this comment.
I didn't mean to add FD_WRITE to this, so I reverted that part specifically. We should be checking for FD_ACCEPT, FD_READ and FD_CLOSE.
I moved this into its own helper and updated this to use WSAWaitForMultipleEvents() which is internally calling WaitForMultipleObjects so you can use it with more than just sockets.
With my updated approach, I removed the new handle I added to the Socket type, so we should be able to have the same socket registered to multiple handlers, although in practice I'm not exactly sure how that would work...
There was a problem hiding this comment.
I think it should be fine as long as we call (like you do now) WSAEventSelect right before blocking. It probably won't work if those selects happen on different threads (concurrently), but I'm not worried about that. The "use case" I have in mind is someone getting a callback notification doing another select to confirm that the data is indeed readable (that's kind of what happens with ConnectionFileDescriptor right now).
|
In case it helps, here's a method to wait on a pipe using zero-length reads (inspired by the libdispatch implementation you found earlier). In case waiting on the pipe handle itself produces spurious wakeups, I think this should be a fairly reliable alternative. The actual implementation probably should handle the zero-length write like libdispatch, but retrying there should be fine as it should only happen if/when someone does a zero-length write. I'm hoping we can ignore PIPE_NOWAIT pipes since they're kind of deprecated. |
| } | ||
|
|
||
| ~PipeFdInfo() override { | ||
| if (monitor_thread.joinable()) { |
There was a problem hiding this comment.
Can we drop the read thread? Did this implementation not work? I believe it should be possible to plug the event from the read operation into the WaitForMultipleObjects call.
| intptr_t event; | ||
| Callback callback; |
There was a problem hiding this comment.
A struct with virtual methods reminds me of lldb-dap, in a bad way. Let's put these behind an accessor.
There was a problem hiding this comment.
I reverted this so we have the previous struct FdInfo and made the event into its own IOEvent type instead. I also made this a class and used accessors instead of making ivars public.
lldb/source/Host/common/File.cpp
Outdated
| bool NativeFile::HasReadableData() { | ||
| #ifdef _WIN32 | ||
| DWORD available_bytes = 0; | ||
| return !PeekNamedPipe((HANDLE)_get_osfhandle(GetDescriptor()), NULL, 0, NULL, |
There was a problem hiding this comment.
That's better, but I do have a question, and a comment :)
-
Do we need to support selecting on actual files? That doesn't even work on posix systems (the file just always comes back as "readable" -- which makes sense because select just tells you whether the operation will block), so if anything, I think we should emulate that. Waiting for file modification requires things like inotify(7), and fortunately, noone has had a use case for that for now. If we ever needed to support it, that should probably be a separate kind of an event. I'm assuming that
GetFileTypeon a pipe handle will returnFILE_TYPE_PIPE, even if that handle was passed asstdin. -
it my long term vision, I would like lldb-dap to create Pipe IOObject directly (perhaps via some helper on the File class) instead of having the MainLoop class guess it. However, given that Pipe is not an IOObject right now, this is something I can live with.
lldb/source/Host/common/Socket.cpp
Outdated
| m_waitable_handle = WSACreateEvent(); | ||
| assert(m_waitable_handle != WSA_INVALID_EVENT); | ||
| if (WSAEventSelect(m_socket, m_waitable_handle, | ||
| FD_ACCEPT | FD_READ | FD_WRITE) != 0) |
There was a problem hiding this comment.
I think it should be fine as long as we call (like you do now) WSAEventSelect right before blocking. It probably won't work if those selects happen on different threads (concurrently), but I'm not worried about that. The "use case" I have in mind is someone getting a callback notification doing another select to confirm that the data is indeed readable (that's kind of what happens with ConnectionFileDescriptor right now).
We don't strictly need this for anything I am aware of, I can remove it for now. A 'better' solution would be to stat the file and then watch the directory for changes and re-stat the file. Thats used in libuv (nodejs). I think libdispatch on Windows doesn't actually do anything for files, so you don't get notified if a file on disk changes.
In order to support both anonymous pipes, like stdin, and named pipes we would need the thread. We could specialize this further and have the thread only for anonymous pipes and use a different strategy for named pipes, but the current solution works for both. |
Yeah, let's remove that. I think that "watching for file/directory" changes is a completely different feature..
I see. I take it that's because anonymous pipes are created without FILE_FLAG_OVERLAPPED. Btw, my understanding is that an anonymous pipe is just a named pipe with a funny name (at least until windows 10, or something) and some with some hardcoded flags (like the absence of overlapped I/O). This is why lldb creates "anonymous pipes" as named -- so it can enable overlapped I/O. So for internal pipe uses, I think we're safe. It gets trickier for pipes that get passed from the outside, but I wouldn't be surprised if VSCode did something similar -- it's likely it also wants to do some overlapped I/O. Could you check if that is the case? I'm not sure what that would mean for the implementation (I like the generality, but I also hate the idea of creating a thread for every pipe object), but I think it'd be good to have know this. |
I made a sample binary (https://gist.github.com/ashgti/1649fde8ef28783ace1e414f87a6b680) and it seems I'm getting |
This updates MainLoopWindows to support events for reading from a file and a socket type. This unifies both handle types using WaitForMultipleEvents which can listen to both sockets and files for change events. This should allow us to unify how we handle watching files/pipes/sockets on Windows and Posix systems.
For pipes, we use a monitor thread that performs a zero byte read to detect changes before signaling.
For files, WaitFor{SingleObject,MultipleObjects}() is signaled if the file changes on disk.
ashgti
force-pushed
the
lldb-mainloopwindows
branch
from
ed3a07a to
ea1ed2b
Compare
|
On my windows VM, all tests are passing now and it looks like this is also working for Linux. I think the current CI check is a no-op on Windows for lldb. |
ashgti
changed the title
There was a problem hiding this comment.
I made a sample binary (https://gist.github.com/ashgti/1649fde8ef28783ace1e414f87a6b680) and it seems stdin is opened as an anonymous pipe.
Got it. Thanks for checking it out. I guess we have no choice, but to create the extra thread. It's unfortunate that windows doesn't allow you to do async I/O if the file handle wasn't created as such. The "just create it the right way from the start" argument makes sense when everything happens within a single process, but this use case (passing a pipe handle from one process to another) is kind of a counterexample to that.
I also wouldn't be surprised if we needed to add threads to wait for other kinds of events in the future. One thing that would be occasionally useful to do is to wait for some data to arrive (e.g. a process writes something to a pipe) OR for a process to die (e.g. the writing process crashes). Linux can pull that off without threads using a pidfd, but I'm not sure all systems can.
It's also reassuring to know that we can avoid the extra thread if we need to for pipes we have create ourselves, but I think we don't need to do that now. I think most of the internal uses of pipes would be better off by switching to AddPendingCallback (which internally uses a pipe on posix, but directly signals an event on windows).
| } | ||
|
|
||
| void WillPoll() override { | ||
| if (!m_monitor_thread.joinable()) |
There was a problem hiding this comment.
What would you say to creating the thread in the constructor? It would remove the joinable() calls, and thing that adding a pipe to the loop is quickly followed by a poll operation anyway.
| /*bInitialState=*/FALSE, NULL)), | ||
| m_handle(handle), m_ready(CreateEventW(NULL, /*bManualReset=*/FALSE, | ||
| /*bInitialState=*/FALSE, NULL)) { | ||
| assert(event && ready); |
There was a problem hiding this comment.
| assert(event && ready); | |
| assert(m_event && m_ready); |
| for (auto &[_, fd_info] : m_read_fds) { | ||
| fd_info.event->DidPoll(); | ||
| } |
There was a problem hiding this comment.
| for (auto &[_, fd_info] : m_read_fds) { | |
| fd_info.event->DidPoll(); | |
| } | |
| for (auto &[_, fd_info] : m_read_fds) | |
| fd_info.event->DidPoll(); |
|
|
||
| char X = 'X'; | ||
| size_t len = sizeof(X); | ||
| ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::Succeeded()); |
There was a problem hiding this comment.
| ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::Succeeded()); | |
| ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::HasValue(1)); |
| EXPECT_THAT_ERROR(r->Read(&X, len).ToError(), llvm::Succeeded()); | ||
| EXPECT_EQ(len, sizeof(X)); |
There was a problem hiding this comment.
| EXPECT_THAT_ERROR(r->Read(&X, len).ToError(), llvm::Succeeded()); | |
| EXPECT_EQ(len, sizeof(X)); | |
| EXPECT_THAT_EXPECTED(r->Read(&X, len).ToError(), llvm::HasValue(1)); |
This updates MainLoopWindows to support events for reading from a pipe (both anonymous and named pipes) as well as sockets.
This unifies both handle types using
WSAWaitForMultipleEventswhich can listen to both sockets and handles for change events.This should allow us to unify how we handle watching pipes/sockets on Windows and Posix systems.