[Lldb-commits] [lldb] [lldb] Don't exit the main loop when in runs out of things to listen on (PR #112565)
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Oct 17 07:45:55 PDT 2024
https://github.com/labath updated https://github.com/llvm/llvm-project/pull/112565
>From 8a531630f6b233d83f33740ce9dbbd9b3a778641 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Wed, 16 Oct 2024 16:54:06 +0200
Subject: [PATCH 1/4] [lldb] Don't exit the main loop when in runs out of
things to listen on
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.
---
lldb/source/Host/posix/MainLoopPosix.cpp | 5 +----
lldb/source/Host/windows/MainLoopWindows.cpp | 4 +---
lldb/tools/lldb-server/lldb-platform.cpp | 23 +++++++-------------
lldb/unittests/Host/MainLoopTest.cpp | 11 +++++-----
4 files changed, 15 insertions(+), 28 deletions(-)
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
>From 4749063e775c27f34618e9517b4e597d8cf76d2a Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Wed, 16 Oct 2024 17:30:30 +0200
Subject: [PATCH 2/4] simplify one test
---
lldb/unittests/Host/MainLoopTest.cpp | 3 ---
1 file changed, 3 deletions(-)
diff --git a/lldb/unittests/Host/MainLoopTest.cpp b/lldb/unittests/Host/MainLoopTest.cpp
index 965f0bce82516b..5672d2bfaa8d9f 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -194,9 +194,6 @@ TEST_F(MainLoopTest, PendingCallbackTrigger) {
add_callback2.set_value();
});
Status error;
- auto socket_handle = loop.RegisterReadObject(
- socketpair[1], [](MainLoopBase &) {}, error);
- ASSERT_TRUE(socket_handle);
ASSERT_THAT_ERROR(error.ToError(), llvm::Succeeded());
bool callback2_called = false;
std::thread callback2_adder([&]() {
>From fdeee65b05deb6ed42f24366c6f578c58575e1db Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Wed, 16 Oct 2024 17:31:08 +0200
Subject: [PATCH 3/4] reformat
---
lldb/tools/lldb-server/lldb-platform.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index d702f07deabd31..735a558810da58 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -555,9 +555,9 @@ int main_platform(int argc, char *argv[]) {
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);
+ Status error = spawn_process(progname, sock_up.get(),
+ gdbserver_port, inferior_arguments,
+ log_file, log_channels, main_loop);
if (error.Fail()) {
Log *log = GetLog(LLDBLog::Platform);
LLDB_LOGF(log, "spawn_process failed: %s", error.AsCString());
>From 18227caf30b63eca56d4c445436b7e63441e23f5 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Thu, 17 Oct 2024 16:45:36 +0200
Subject: [PATCH 4/4] more comments
---
lldb/unittests/Host/MainLoopTest.cpp | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lldb/unittests/Host/MainLoopTest.cpp b/lldb/unittests/Host/MainLoopTest.cpp
index 5672d2bfaa8d9f..4688d4fed475b6 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -212,7 +212,11 @@ TEST_F(MainLoopTest, PendingCallbackTrigger) {
TEST_F(MainLoopTest, ManyPendingCallbacks) {
MainLoop loop;
Status error;
- // Try to fill up the pipe buffer and make sure bad things don't happen.
+ // Try to fill up the pipe buffer and make sure bad things don't happen. This
+ // is a regression test for the case where writing to the interrupt pipe
+ // caused a deadlock when the pipe filled up (either because the main loop was
+ // not running, because it was slow, or because it was busy/blocked doing
+ // something else).
for (int i = 0; i < 65536; ++i)
loop.AddPendingCallback(
[&](MainLoopBase &loop) { loop.RequestTermination(); });
More information about the lldb-commits
mailing list