Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a53163
[lldb] Don't exit the main loop when in runs out of things to listen on
labath Oct 16, 2024
4749063
simplify one test
labath Oct 16, 2024
fdeee65
reformat
labath Oct 16, 2024
18227ca
more comments
labath Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions lldb/source/Host/posix/MainLoopPosix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Then again it basically sounds like a no-op

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

(m_read_fds.size() > 1 || !m_signals.empty())) {
while (!m_terminate_request) {
error = impl.Poll();
if (error.Fail())
return error;
Expand Down
4 changes: 1 addition & 3 deletions lldb/source/Host/windows/MainLoopWindows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
27 changes: 10 additions & 17 deletions lldb/tools/lldb-server/lldb-platform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
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());
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();
Expand All @@ -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");

Expand Down
18 changes: 9 additions & 9 deletions lldb/unittests/Host/MainLoopTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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([&]() {
Expand All @@ -212,15 +209,18 @@ 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. 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.AddPendingCallback(
[&](MainLoopBase &loop) { loop.RequestTermination(); });
ASSERT_TRUE(loop.Run().Success());
}

#ifdef LLVM_ON_UNIX
Expand Down
Loading