[lldb] Improving synchronization of MainLoopWindows. #147438

ashgti · 2025-07-08T00:46:56Z

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.

I believe this should fix #147291 but it is hard to ensure a race condition is fixed without running the CI on multiple machines/configurations.

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.

llvmbot · 2025-07-08T00:52:34Z

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

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.

I believe this should fix #147291 but it is hard to ensure a race condition is fixed without running the CI on multiple machines/configurations.

Full diff: https://github.com/llvm/llvm-project/pull/147438.diff

2 Files Affected:

(modified) lldb/source/Host/windows/MainLoopWindows.cpp (+16-8)
(modified) lldb/unittests/Host/MainLoopTest.cpp (+140-1)

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;

labath

Thanks. I like how this means we no longer create the thread lazily.

I think I see one more race, see inline comment. LGTM assuming you agree with my analysis.

lldb/source/Host/windows/MainLoopWindows.cpp

Co-authored-by: Pavel Labath <[email protected]>

github-actions · 2025-07-08T16:04:12Z

✅ With the latest revision this PR passed the C/C++ code formatter.

slydiman

LGTM.
But I agree that

it is hard to ensure a race condition is fixed without running the CI on multiple machines/configurations.

ashgti · 2025-07-08T20:14:53Z

I kicked off a build on lldb-x86_64-win, once that passes I'll submit and we can see if there are any failures in the CI jobs.

ashgti · 2025-07-08T20:34:55Z

Hmm I think I didn't specify the commit correctly for the CI job.

I'll merge this and we can revert it if needed later.

slydiman · 2025-07-09T08:05:39Z

So far everything looks good. Thanks.

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. I believe this should fix llvm#147291 but it is hard to ensure a race condition is fixed without running the CI on multiple machines/configurations. --------- Co-authored-by: Pavel Labath <[email protected]>

ashgti requested review from labath and slydiman July 8, 2025 00:48

ashgti force-pushed the lldb-dap-windowsloop branch from 177d68f to 8a60d1c Compare July 8, 2025 00:50

ashgti added the lldb label Jul 8, 2025

ashgti marked this pull request as ready for review July 8, 2025 01:12

ashgti requested a review from JDevlieghere as a code owner July 8, 2025 01:12

labath approved these changes Jul 8, 2025

View reviewed changes

lldb/source/Host/windows/MainLoopWindows.cpp Outdated Show resolved Hide resolved

lldb/source/Host/windows/MainLoopWindows.cpp Outdated Show resolved Hide resolved

ashgti and others added 2 commits July 8, 2025 09:00

Update lldb/source/Host/windows/MainLoopWindows.cpp

127bfd6

Co-authored-by: Pavel Labath <[email protected]>

Update lldb/source/Host/windows/MainLoopWindows.cpp

ff9777b

Co-authored-by: Pavel Labath <[email protected]>

Applying clang-format.

46aea6e

slydiman approved these changes Jul 8, 2025

View reviewed changes

ashgti merged commit 9be7194 into llvm:main Jul 8, 2025
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] Improving synchronization of MainLoopWindows. #147438

[lldb] Improving synchronization of MainLoopWindows. #147438

Uh oh!

ashgti commented Jul 8, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jul 8, 2025

Uh oh!

labath left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 8, 2025 •

edited

Loading

Uh oh!

slydiman left a comment

Uh oh!

ashgti commented Jul 8, 2025

Uh oh!

ashgti commented Jul 8, 2025

Uh oh!

Uh oh!

slydiman commented Jul 9, 2025

Uh oh!

Uh oh!

[lldb] Improving synchronization of MainLoopWindows. #147438

[lldb] Improving synchronization of MainLoopWindows. #147438

Uh oh!

Conversation

ashgti commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 8, 2025

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slydiman left a comment

Choose a reason for hiding this comment

Uh oh!

ashgti commented Jul 8, 2025

Uh oh!

ashgti commented Jul 8, 2025

Uh oh!

Uh oh!

slydiman commented Jul 9, 2025

Uh oh!

Uh oh!

ashgti commented Jul 8, 2025 •

edited

Loading

github-actions bot commented Jul 8, 2025 •

edited

Loading