mgorny created this revision.
mgorny added reviewers: labath, krytarowski, emaste, jingham.
Herald added a project: All.
mgorny requested review of this revision.
If lots of pending callbacks are added while the main loop has exited
already, the trigger pipe buffer fills in, causing the write to fail
and the related assertion to fail. To avoid this, add a boolean member
indicating whether the callbacks have been triggered already.
If the trigger was done, avoid writing to the pipe until loops proceeds
to run them and resets the variable.
Besides fixing the issue, this also avoids writing to the pipe multiple
times if callbacks are added faster than the loop is able to process
them. Previously, this would lead to the loop performing multiple read
iterations from pipe unnecessarily.
https://reviews.llvm.org/D135516
Files:
lldb/include/lldb/Host/posix/MainLoopPosix.h
lldb/source/Host/posix/MainLoopPosix.cpp
lldb/unittests/Host/MainLoopTest.cpp
Index: lldb/unittests/Host/MainLoopTest.cpp
===================================================================
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -171,6 +171,17 @@
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) {
+ MainLoop loop;
+ Status error;
+ ASSERT_TRUE(loop.Run().Success());
+ // Try to fill the pipe buffer in.
+ for (int i = 0; i < 65536; ++i)
+ loop.AddPendingCallback([&](MainLoopBase &loop) {});
+}
+
#ifdef LLVM_ON_UNIX
TEST_F(MainLoopTest, DetectsEOF) {
Index: lldb/source/Host/posix/MainLoopPosix.cpp
===================================================================
--- lldb/source/Host/posix/MainLoopPosix.cpp
+++ lldb/source/Host/posix/MainLoopPosix.cpp
@@ -222,7 +222,7 @@
}
#endif
-MainLoopPosix::MainLoopPosix() {
+MainLoopPosix::MainLoopPosix() : m_trigger_done(false) {
Status error = m_trigger_pipe.CreateNew(/*child_process_inherit=*/false);
assert(error.Success());
const int trigger_pipe_fd = m_trigger_pipe.GetReadFileDescriptor();
@@ -371,6 +371,7 @@
impl.ProcessEvents();
+ m_trigger_done = false;
ProcessPendingCallbacks();
}
return Status();
@@ -395,10 +396,14 @@
}
void MainLoopPosix::TriggerPendingCallbacks() {
+ if (m_trigger_done)
+ return;
+
char c = '.';
size_t bytes_written;
Status error = m_trigger_pipe.Write(&c, 1, bytes_written);
assert(error.Success());
UNUSED_IF_ASSERT_DISABLED(error);
assert(bytes_written == 1);
+ m_trigger_done = true;
}
Index: lldb/include/lldb/Host/posix/MainLoopPosix.h
===================================================================
--- lldb/include/lldb/Host/posix/MainLoopPosix.h
+++ lldb/include/lldb/Host/posix/MainLoopPosix.h
@@ -13,6 +13,7 @@
#include "lldb/Host/MainLoopBase.h"
#include "lldb/Host/Pipe.h"
#include "llvm/ADT/DenseMap.h"
+#include <atomic>
#include <csignal>
#include <list>
#include <vector>
@@ -87,6 +88,7 @@
llvm::DenseMap<IOObject::WaitableHandle, Callback> m_read_fds;
llvm::DenseMap<int, SignalInfo> m_signals;
Pipe m_trigger_pipe;
+ std::atomic<bool> m_trigger_done;
#if HAVE_SYS_EVENT_H
int m_kqueue;
#endif
Index: lldb/unittests/Host/MainLoopTest.cpp
===================================================================
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -171,6 +171,17 @@
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) {
+ MainLoop loop;
+ Status error;
+ ASSERT_TRUE(loop.Run().Success());
+ // Try to fill the pipe buffer in.
+ for (int i = 0; i < 65536; ++i)
+ loop.AddPendingCallback([&](MainLoopBase &loop) {});
+}
+
#ifdef LLVM_ON_UNIX
TEST_F(MainLoopTest, DetectsEOF) {
Index: lldb/source/Host/posix/MainLoopPosix.cpp
===================================================================
--- lldb/source/Host/posix/MainLoopPosix.cpp
+++ lldb/source/Host/posix/MainLoopPosix.cpp
@@ -222,7 +222,7 @@
}
#endif
-MainLoopPosix::MainLoopPosix() {
+MainLoopPosix::MainLoopPosix() : m_trigger_done(false) {
Status error = m_trigger_pipe.CreateNew(/*child_process_inherit=*/false);
assert(error.Success());
const int trigger_pipe_fd = m_trigger_pipe.GetReadFileDescriptor();
@@ -371,6 +371,7 @@
impl.ProcessEvents();
+ m_trigger_done = false;
ProcessPendingCallbacks();
}
return Status();
@@ -395,10 +396,14 @@
}
void MainLoopPosix::TriggerPendingCallbacks() {
+ if (m_trigger_done)
+ return;
+
char c = '.';
size_t bytes_written;
Status error = m_trigger_pipe.Write(&c, 1, bytes_written);
assert(error.Success());
UNUSED_IF_ASSERT_DISABLED(error);
assert(bytes_written == 1);
+ m_trigger_done = true;
}
Index: lldb/include/lldb/Host/posix/MainLoopPosix.h
===================================================================
--- lldb/include/lldb/Host/posix/MainLoopPosix.h
+++ lldb/include/lldb/Host/posix/MainLoopPosix.h
@@ -13,6 +13,7 @@
#include "lldb/Host/MainLoopBase.h"
#include "lldb/Host/Pipe.h"
#include "llvm/ADT/DenseMap.h"
+#include <atomic>
#include <csignal>
#include <list>
#include <vector>
@@ -87,6 +88,7 @@
llvm::DenseMap<IOObject::WaitableHandle, Callback> m_read_fds;
llvm::DenseMap<int, SignalInfo> m_signals;
Pipe m_trigger_pipe;
+ std::atomic<bool> m_trigger_done;
#if HAVE_SYS_EVENT_H
int m_kqueue;
#endif
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
