[lldb] Don't exit the main loop when in runs out of things to listen on #112565

labath · 2024-10-16T15:15:49Z

This behavior made sense in the beginning as the class was completely single threaded, so if the source count ever reached zero, there was no way to add new ones. In https://reviews.llvm.org/D131160, the class gained the ability to add events (callbacks) from other threads, which means that is no longer the case (and indeed, one possible use case for this class -- acting as a sort of arbiter for multiple threads wanting to run code while making sure it runs serially -- has this class sit in an empty Run call most of the time). I'm not aware of us having a use for such a thing right now, but one of my tests in another patch turned into something similar by accident.

Another problem with the current approach is that, in a distributed/dynamic setup (multiple things using the main loop without a clear coordinator), one can never be sure whether unregistering a specific event will terminate the loop (it depends on whether there are other listeners). We had this problem in lldb-platform.cpp, where we had to add an additional layer of synchronization to avoid premature termination. We can remove this if we can rely on the loop terminating only when we tell it to.

This behavior made sense in the beginning as the class was completely single threaded, so if the source count ever reached zero, there was no way to add new ones. In https://reviews.llvm.org/D131160, the class gained the ability to add events (callbacks) from other threads, which means that is no longer the case (and indeed, one possible use case for this class -- acting as a sort of arbiter for multiple threads wanting to run code while making sure it runs serially -- has this class sit in an empty Run call most of the time). I'm not aware of us having a use for such a thing right now, but one of my tests in another patch turned into something similar by accident. Another problem with the current approach is that, in a distributed/dynamic setup (multiple things using the main loop without a clear coordinator), one can never be sure whether unregistering a specific event will terminate the loop (it depends on whether there are other listeners). We had this problem in lldb-platform.cpp, where we had to add an additional layer of synchronization to avoid premature termination. We can remove this if we can rely on the loop terminating only when we tell it to.

llvmbot · 2024-10-16T15:16:34Z

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

This behavior made sense in the beginning as the class was completely single threaded, so if the source count ever reached zero, there was no way to add new ones. In https://reviews.llvm.org/D131160, the class gained the ability to add events (callbacks) from other threads, which means that is no longer the case (and indeed, one possible use case for this class -- acting as a sort of arbiter for multiple threads wanting to run code while making sure it runs serially -- has this class sit in an empty Run call most of the time). I'm not aware of us having a use for such a thing right now, but one of my tests in another patch turned into something similar by accident.

Another problem with the current approach is that, in a distributed/dynamic setup (multiple things using the main loop without a clear coordinator), one can never be sure whether unregistering a specific event will terminate the loop (it depends on whether there are other listeners). We had this problem in lldb-platform.cpp, where we had to add an additional layer of synchronization to avoid premature termination. We can remove this if we can rely on the loop terminating only when we tell it to.

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

4 Files Affected:

(modified) lldb/source/Host/posix/MainLoopPosix.cpp (+1-4)
(modified) lldb/source/Host/windows/MainLoopWindows.cpp (+1-3)
(modified) lldb/tools/lldb-server/lldb-platform.cpp (+8-15)
(modified) lldb/unittests/Host/MainLoopTest.cpp (+5-6)

diff --git a/lldb/source/Host/posix/MainLoopPosix.cpp b/lldb/source/Host/posix/MainLoopPosix.cpp
index 816581e70294a7..6f8eaa55cfdf09 100644
--- a/lldb/source/Host/posix/MainLoopPosix.cpp
+++ b/lldb/source/Host/posix/MainLoopPosix.cpp
@@ -365,10 +365,7 @@ Status MainLoopPosix::Run() {
   Status error;
   RunImpl impl(*this);
 
-  // run until termination or until we run out of things to listen to
-  // (m_read_fds will always contain m_trigger_pipe fd, so check for > 1)
-  while (!m_terminate_request &&
-         (m_read_fds.size() > 1 || !m_signals.empty())) {
+  while (!m_terminate_request) {
     error = impl.Poll();
     if (error.Fail())
       return error;
diff --git a/lldb/source/Host/windows/MainLoopWindows.cpp b/lldb/source/Host/windows/MainLoopWindows.cpp
index 88d929535ab6c5..c9aa6d339d8f48 100644
--- a/lldb/source/Host/windows/MainLoopWindows.cpp
+++ b/lldb/source/Host/windows/MainLoopWindows.cpp
@@ -116,9 +116,7 @@ Status MainLoopWindows::Run() {
 
   Status error;
 
-  // run until termination or until we run out of things to listen to
-  while (!m_terminate_request && !m_read_fds.empty()) {
-
+  while (!m_terminate_request) {
     llvm::Expected<size_t> signaled_event = Poll();
     if (!signaled_event)
       return Status::FromError(signaled_event.takeError());
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 2ef780578d0a28..d702f07deabd31 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -260,8 +260,7 @@ static void client_handle(GDBRemoteCommunicationServerPlatform &platform,
 static Status spawn_process(const char *progname, const Socket *conn_socket,
                             uint16_t gdb_port, const lldb_private::Args &args,
                             const std::string &log_file,
-                            const StringRef log_channels, MainLoop &main_loop,
-                            std::promise<void> &child_exited) {
+                            const StringRef log_channels, MainLoop &main_loop) {
   Status error;
   SharedSocket shared_socket(conn_socket, error);
   if (error.Fail())
@@ -301,12 +300,10 @@ static Status spawn_process(const char *progname, const Socket *conn_socket,
   if (g_server)
     launch_info.SetMonitorProcessCallback([](lldb::pid_t, int, int) {});
   else
-    launch_info.SetMonitorProcessCallback(
-        [&child_exited, &main_loop](lldb::pid_t, int, int) {
-          main_loop.AddPendingCallback(
-              [](MainLoopBase &loop) { loop.RequestTermination(); });
-          child_exited.set_value();
-        });
+    launch_info.SetMonitorProcessCallback([&main_loop](lldb::pid_t, int, int) {
+      main_loop.AddPendingCallback(
+          [](MainLoopBase &loop) { loop.RequestTermination(); });
+    });
 
   // Copy the current environment.
   launch_info.GetEnvironment() = Host::GetEnvironment();
@@ -550,27 +547,24 @@ int main_platform(int argc, char *argv[]) {
     return socket_error;
   }
 
-  std::promise<void> child_exited;
   MainLoop main_loop;
   {
     llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> platform_handles =
         platform_sock->Accept(
             main_loop, [progname, gdbserver_port, &inferior_arguments, log_file,
-                        log_channels, &main_loop, &child_exited,
+                        log_channels, &main_loop,
                         &platform_handles](std::unique_ptr<Socket> sock_up) {
               printf("Connection established.\n");
               Status error = spawn_process(
                   progname, sock_up.get(), gdbserver_port, inferior_arguments,
-                  log_file, log_channels, main_loop, child_exited);
+                  log_file, log_channels, main_loop);
               if (error.Fail()) {
                 Log *log = GetLog(LLDBLog::Platform);
                 LLDB_LOGF(log, "spawn_process failed: %s", error.AsCString());
                 WithColor::error()
                     << "spawn_process failed: " << error.AsCString() << "\n";
-                if (!g_server) {
+                if (!g_server)
                   main_loop.RequestTermination();
-                  child_exited.set_value();
-                }
               }
               if (!g_server)
                 platform_handles->clear();
@@ -592,7 +586,6 @@ int main_platform(int argc, char *argv[]) {
 
     main_loop.Run();
   }
-  child_exited.get_future().get();
 
   fprintf(stderr, "lldb-server exiting...\n");
 
diff --git a/lldb/unittests/Host/MainLoopTest.cpp b/lldb/unittests/Host/MainLoopTest.cpp
index b8417c9f00aa86..965f0bce82516b 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -212,15 +212,14 @@ TEST_F(MainLoopTest, PendingCallbackTrigger) {
   ASSERT_TRUE(callback2_called);
 }
 
-// Regression test for assertion failure if a lot of callbacks end up
-// being queued after loop exits.
-TEST_F(MainLoopTest, PendingCallbackAfterLoopExited) {
+TEST_F(MainLoopTest, ManyPendingCallbacks) {
   MainLoop loop;
   Status error;
-  ASSERT_TRUE(loop.Run().Success());
-  // Try to fill the pipe buffer in.
+  // Try to fill up the pipe buffer and make sure bad things don't happen.
   for (int i = 0; i < 65536; ++i)
-    loop.AddPendingCallback([&](MainLoopBase &loop) {});
+    loop.AddPendingCallback(
+        [&](MainLoopBase &loop) { loop.RequestTermination(); });
+  ASSERT_TRUE(loop.Run().Success());
 }
 
 #ifdef LLVM_ON_UNIX

github-actions · 2024-10-16T15:19:43Z

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

Jlalond · 2024-10-16T23:46:39Z

lldb/source/Host/posix/MainLoopPosix.cpp

@@ -365,10 +365,7 @@ Status MainLoopPosix::Run() {
  Status error;
  RunImpl impl(*this);

-  // run until termination or until we run out of things to listen to
-  // (m_read_fds will always contain m_trigger_pipe fd, so check for > 1)
-  while (!m_terminate_request &&


What changed to make this comment above (about the pipe never being empty) incorrect?

Then again it basically sounds like a no-op

It's not about the pipe being empty or not, but rather about the (internal, used for interrupts) pipe FD being present in the list of FDs we are listening on. Since we want(ed) to exit when the last user/external FD went away, we needed to skip/ignore the internal pipe fd. Removing this is another reason why I like this patch :)

Jlalond · 2024-10-16T23:47:44Z

lldb/unittests/Host/MainLoopTest.cpp

  MainLoop loop;
  Status error;
-  ASSERT_TRUE(loop.Run().Success());
-  // Try to fill the pipe buffer in.
+  // Try to fill up the pipe buffer and make sure bad things don't happen.


The prior comment I think better explained the purpose of the test (ensuring we terminate even if the pipe buffer is full).

I'll expand on that a bit. I had to refactor the test as it was no longer possible to run in in this way. Although it was testing very a specific scenario (adding callbacks after the loop has exited, the overall problem was wider). The original implementation was writing one character to the pipe for each interrupt/callback, which meant that you could fill up the pipe (and block) if noone was draining it. That could happen because the loop was busy doing something, not just because it has exited.

mgorny

I guess LGTM but I haven't given it really deep thought.

labath · 2024-10-17T14:41:59Z

Thanks for the reviews.

labath requested review from mgorny and slydiman October 16, 2024 15:15

labath requested a review from JDevlieghere as a code owner October 16, 2024 15:15

llvmbot added the lldb label Oct 16, 2024

labath added 2 commits October 16, 2024 17:30

simplify one test

4749063

reformat

fdeee65

Jlalond reviewed Oct 16, 2024

View reviewed changes

Jlalond approved these changes Oct 16, 2024

View reviewed changes

mgorny approved these changes Oct 17, 2024

View reviewed changes

more comments

18227ca

labath merged commit 98b419c into llvm:main Oct 17, 2024
7 checks passed

labath deleted the loop branch October 17, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] Don't exit the main loop when in runs out of things to listen on #112565

[lldb] Don't exit the main loop when in runs out of things to listen on #112565

Uh oh!

labath commented Oct 16, 2024

Uh oh!

llvmbot commented Oct 16, 2024

Uh oh!

github-actions bot commented Oct 16, 2024 •

edited

Loading

Uh oh!

Jlalond Oct 16, 2024

Uh oh!

labath Oct 17, 2024

Uh oh!

Jlalond Oct 16, 2024

Uh oh!

labath Oct 17, 2024

Uh oh!

mgorny left a comment

Uh oh!

labath commented Oct 17, 2024

Uh oh!

Uh oh!

Uh oh!

[lldb] Don't exit the main loop when in runs out of things to listen on #112565

[lldb] Don't exit the main loop when in runs out of things to listen on #112565

Uh oh!

Conversation

labath commented Oct 16, 2024

Uh oh!

llvmbot commented Oct 16, 2024

Uh oh!

github-actions bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jlalond Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

labath Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Jlalond Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

labath Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

mgorny left a comment

Choose a reason for hiding this comment

Uh oh!

labath commented Oct 17, 2024

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 16, 2024 •

edited

Loading