https://github.com/ashgti updated
https://github.com/llvm/llvm-project/pull/147438
>From 8a60d1cbd3d75e640d5efddf23c717278e7d6b80 Mon Sep 17 00:00:00 2001
From: John Harrison <harj...@google.com>
Date: Mon, 7 Jul 2025 17:31:25 -0700
Subject: [PATCH 1/3] [lldb] Improving synchronization of MainLoopWindows.
This should improve synchronizing the MainLoopWindows monitor thread with the
main loop state.
This uses the `m_ready` and `m_event` event handles to manage when the Monitor
thread continues and adds new tests to cover additional use cases.
---
lldb/source/Host/windows/MainLoopWindows.cpp | 24 ++--
lldb/unittests/Host/MainLoopTest.cpp | 141 ++++++++++++++++++-
2 files changed, 156 insertions(+), 9 deletions(-)
diff --git a/lldb/source/Host/windows/MainLoopWindows.cpp
b/lldb/source/Host/windows/MainLoopWindows.cpp
index b3322e8b3ae59..4e2c1d6de894a 100644
--- a/lldb/source/Host/windows/MainLoopWindows.cpp
+++ b/lldb/source/Host/windows/MainLoopWindows.cpp
@@ -12,16 +12,16 @@
#include "lldb/Host/windows/windows.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>
-#include <csignal>
#include <ctime>
#include <io.h>
+#include <synchapi.h>
#include <thread>
#include <vector>
+#include <winbase.h>
+#include <winerror.h>
#include <winsock2.h>
using namespace lldb;
@@ -42,11 +42,12 @@ namespace {
class PipeEvent : public MainLoopWindows::IOEvent {
public:
explicit PipeEvent(HANDLE handle)
- : IOEvent(CreateEventW(NULL, /*bManualReset=*/FALSE,
+ : IOEvent(CreateEventW(NULL, /*bManualReset=*/TRUE,
/*bInitialState=*/FALSE, NULL)),
- m_handle(handle), m_ready(CreateEventW(NULL, /*bManualReset=*/FALSE,
+ m_handle(handle), m_ready(CreateEventW(NULL, /*bManualReset=*/TRUE,
/*bInitialState=*/FALSE, NULL))
{
assert(m_event && m_ready);
+ m_monitor_thread = std::thread(&PipeEvent::Monitor, this);
}
~PipeEvent() override {
@@ -65,15 +66,21 @@ class PipeEvent : public MainLoopWindows::IOEvent {
}
void WillPoll() override {
- if (!m_monitor_thread.joinable())
- m_monitor_thread = std::thread(&PipeEvent::Monitor, this);
+ // If the m_event is signaled, wait until it is consumed before telling the
+ // monitor thread to continue.
+ if (WaitForSingleObject(m_event, /*dwMilliseconds=*/0) == WAIT_TIMEOUT &&
+ WaitForSingleObject(m_ready, /*dwMilliseconds=*/0) == WAIT_TIMEOUT)
+ SetEvent(m_ready);
}
- void Disarm() override { SetEvent(m_ready); }
+ void Disarm() override { ResetEvent(m_event); }
/// Monitors the handle performing a zero byte read to determine when data is
/// avaiable.
void Monitor() {
+ // Wait until the MainLoop tells us to start.
+ WaitForSingleObject(m_ready, INFINITE);
+
do {
char buf[1];
DWORD bytes_read = 0;
@@ -110,6 +117,7 @@ class PipeEvent : public MainLoopWindows::IOEvent {
continue;
}
+ ResetEvent(m_ready);
SetEvent(m_event);
// Wait until the current read is consumed before doing the next read.
diff --git a/lldb/unittests/Host/MainLoopTest.cpp
b/lldb/unittests/Host/MainLoopTest.cpp
index 502028ae1a343..30585d12fe81d 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -10,6 +10,7 @@
#include "TestingSupport/SubsystemRAII.h"
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/MainLoopBase.h"
#include "lldb/Host/PseudoTerminal.h"
#include "lldb/Host/common/TCPSocket.h"
#include "llvm/Config/llvm-config.h" // for LLVM_ON_UNIX
@@ -64,7 +65,7 @@ class MainLoopTest : public testing::Test {
};
} // namespace
-TEST_F(MainLoopTest, ReadObject) {
+TEST_F(MainLoopTest, ReadSocketObject) {
char X = 'X';
size_t len = sizeof(X);
ASSERT_TRUE(socketpair[0]->Write(&X, len).Success());
@@ -101,6 +102,144 @@ TEST_F(MainLoopTest, ReadPipeObject) {
ASSERT_EQ(1u, callback_count);
}
+TEST_F(MainLoopTest, MultipleReadsPipeObject) {
+ Pipe pipe;
+
+ ASSERT_TRUE(pipe.CreateNew().Success());
+
+ MainLoop loop;
+
+ std::future<void> async_writer = std::async(std::launch::async, [&] {
+ for (int i = 0; i < 5; ++i) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(500));
+ char X = 'X';
+ size_t len = sizeof(X);
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::HasValue(1));
+ }
+ });
+
+ Status error;
+ lldb::FileSP file = std::make_shared<NativeFile>(
+ pipe.GetReadFileDescriptor(), File::eOpenOptionReadOnly, false);
+ auto handle = loop.RegisterReadObject(
+ file,
+ [&](MainLoopBase &loop) {
+ callback_count++;
+ if (callback_count == 5)
+ loop.RequestTermination();
+
+ // Read some data to ensure the handle is not in a readable state.
+ char buf[1024] = {0};
+ size_t len = sizeof(buf);
+ ASSERT_THAT_ERROR(file->Read(buf, len).ToError(), llvm::Succeeded());
+ EXPECT_EQ(len, 1);
+ EXPECT_EQ(buf[0], 'X');
+ },
+ error);
+ ASSERT_TRUE(error.Success());
+ ASSERT_TRUE(handle);
+ ASSERT_TRUE(loop.Run().Success());
+ ASSERT_EQ(5u, callback_count);
+ async_writer.wait();
+}
+
+TEST_F(MainLoopTest, PipeDelayBetweenRegisterAndRun) {
+ Pipe pipe;
+
+ ASSERT_TRUE(pipe.CreateNew().Success());
+
+ MainLoop loop;
+
+ Status error;
+ lldb::FileSP file = std::make_shared<NativeFile>(
+ pipe.GetReadFileDescriptor(), File::eOpenOptionReadOnly, false);
+ auto handle = loop.RegisterReadObject(
+ file,
+ [&](MainLoopBase &loop) {
+ callback_count++;
+
+ // Read some data to ensure the handle is not in a readable state.
+ char buf[1024] = {0};
+ size_t len = sizeof(buf);
+ ASSERT_THAT_ERROR(file->Read(buf, len).ToError(), llvm::Succeeded());
+ EXPECT_EQ(len, 2);
+ EXPECT_EQ(buf[0], 'X');
+ EXPECT_EQ(buf[1], 'X');
+ },
+ error);
+ auto cb = [&](MainLoopBase &) {
+ callback_count++;
+ char X = 'X';
+ size_t len = sizeof(X);
+ // Write twice and ensure we coalesce into a single read.
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::HasValue(1));
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::HasValue(1));
+ };
+ // Add a write that triggers a read events.
+ loop.AddCallback(cb, std::chrono::milliseconds(500));
+ loop.AddCallback([](MainLoopBase &loop) { loop.RequestTermination(); },
+ std::chrono::milliseconds(1000));
+ ASSERT_TRUE(error.Success());
+ ASSERT_TRUE(handle);
+
+ // Write between RegisterReadObject / Run should NOT invoke the callback.
+ cb(loop);
+ ASSERT_EQ(1u, callback_count);
+
+ ASSERT_TRUE(loop.Run().Success());
+ ASSERT_EQ(4u, callback_count);
+}
+
+TEST_F(MainLoopTest, NoSelfTriggersDuringPipeHandler) {
+ Pipe pipe;
+
+ ASSERT_TRUE(pipe.CreateNew().Success());
+
+ MainLoop loop;
+
+ Status error;
+ lldb::FileSP file = std::make_shared<NativeFile>(
+ pipe.GetReadFileDescriptor(), File::eOpenOptionReadOnly, false);
+ auto handle = loop.RegisterReadObject(
+ file,
+ [&](MainLoopBase &lop) {
+ callback_count++;
+
+ char X = 'Y';
+ size_t len = sizeof(X);
+ // writes / reads during the handler callback should NOT trigger
itself.
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::HasValue(1));
+
+ char buf[1024] = {0};
+ len = sizeof(buf);
+ ASSERT_THAT_ERROR(file->Read(buf, len).ToError(), llvm::Succeeded());
+ EXPECT_EQ(len, 2);
+ EXPECT_EQ(buf[0], 'X');
+ EXPECT_EQ(buf[1], 'Y');
+
+ if (callback_count == 2)
+ loop.RequestTermination();
+ },
+ error);
+ // Add a write that triggers a read event.
+ loop.AddPendingCallback([&](MainLoopBase &) {
+ char X = 'X';
+ size_t len = sizeof(X);
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::HasValue(1));
+ });
+ loop.AddCallback(
+ [&](MainLoopBase &) {
+ char X = 'X';
+ size_t len = sizeof(X);
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::HasValue(1));
+ },
+ std::chrono::milliseconds(500));
+ ASSERT_TRUE(error.Success());
+ ASSERT_TRUE(handle);
+ ASSERT_TRUE(loop.Run().Success());
+ ASSERT_EQ(2u, callback_count);
+}
+
TEST_F(MainLoopTest, NoSpuriousPipeReads) {
Pipe pipe;
>From 127bfd6efb82a65aeb523e306c4bee4de4a7e8de Mon Sep 17 00:00:00 2001
From: John Harrison <a...@greaterthaninfinity.com>
Date: Tue, 8 Jul 2025 09:00:23 -0700
Subject: [PATCH 2/3] Update lldb/source/Host/windows/MainLoopWindows.cpp
Co-authored-by: Pavel Labath <pa...@labath.sk>
---
lldb/source/Host/windows/MainLoopWindows.cpp | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lldb/source/Host/windows/MainLoopWindows.cpp
b/lldb/source/Host/windows/MainLoopWindows.cpp
index 4e2c1d6de894a..721558e549f57 100644
--- a/lldb/source/Host/windows/MainLoopWindows.cpp
+++ b/lldb/source/Host/windows/MainLoopWindows.cpp
@@ -117,8 +117,10 @@ class PipeEvent : public MainLoopWindows::IOEvent {
continue;
}
- ResetEvent(m_ready);
+ // Notify that data is available on the pipe. It's important to set this
before clearing m_ready to avoid a race with WillPoll.
SetEvent(m_event);
+ // Stop polling until we're told to resume.
+ ResetEvent(m_ready);
// Wait until the current read is consumed before doing the next read.
WaitForSingleObject(m_ready, INFINITE);
>From ff9777b0131796678a27a223bd880c2c2274c7f0 Mon Sep 17 00:00:00 2001
From: John Harrison <a...@greaterthaninfinity.com>
Date: Tue, 8 Jul 2025 09:00:40 -0700
Subject: [PATCH 3/3] Update lldb/source/Host/windows/MainLoopWindows.cpp
Co-authored-by: Pavel Labath <pa...@labath.sk>
---
lldb/source/Host/windows/MainLoopWindows.cpp | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/lldb/source/Host/windows/MainLoopWindows.cpp
b/lldb/source/Host/windows/MainLoopWindows.cpp
index 721558e549f57..ae3d460aacec7 100644
--- a/lldb/source/Host/windows/MainLoopWindows.cpp
+++ b/lldb/source/Host/windows/MainLoopWindows.cpp
@@ -66,11 +66,16 @@ class PipeEvent : public MainLoopWindows::IOEvent {
}
void WillPoll() override {
- // If the m_event is signaled, wait until it is consumed before telling the
- // monitor thread to continue.
- if (WaitForSingleObject(m_event, /*dwMilliseconds=*/0) == WAIT_TIMEOUT &&
- WaitForSingleObject(m_ready, /*dwMilliseconds=*/0) == WAIT_TIMEOUT)
- SetEvent(m_ready);
+ if (WaitForSingleObject(m_event, /*dwMilliseconds=*/0) != WAIT_TIMEOUT) {
+ // The thread has already signalled that the data is available. No need
for further polling until we consume that event.
+ return;
+ }
+ if (WaitForSingleObject(m_ready, /*dwMilliseconds=*/0) != WAIT_TIMEOUT) {
+ // The thread is already waiting for data to become available.
+ return;
+ }
+ // Start waiting.
+ SetEvent(m_ready);
}
void Disarm() override { ResetEvent(m_event); }
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
