[Lldb-commits] [lldb] 04de24e - [lldb/IOHandler] Improve synchronization between IO handlers.
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Mon Jan 20 11:18:17 PST 2020
Author: Jonas Devlieghere
Date: 2020-01-20T11:17:55-08:00
New Revision: 04de24e690d3ff23bf63bc3901765cd8f07723f3
URL: https://github.com/llvm/llvm-project/commit/04de24e690d3ff23bf63bc3901765cd8f07723f3
DIFF: https://github.com/llvm/llvm-project/commit/04de24e690d3ff23bf63bc3901765cd8f07723f3.diff
LOG: [lldb/IOHandler] Improve synchronization between IO handlers.
The way the IO handlers are currently managed by the debugger is wrong. The
implementation lacks proper synchronization between RunIOHandlerSync and
RunIOHandlers. The latter is meant to be run by the "main thread", while the
former is meant to be run synchronously, potentially from a different thread.
Imagine a scenario where RunIOHandlerSync is called from a different thread
than RunIOHandlers. Both functions manipulate the debugger's IOHandlerStack.
Although the push and pop operations are synchronized, the logic to activate,
deactivate and run IO handlers is not.
While investigating PR44352, I noticed some weird behavior in the Editline
implementation. One of its members (m_editor_status) was modified from another
thread. This happened because the main thread, while running RunIOHandlers
ended up execution the IOHandlerEditline created by the breakpoint callback
thread. Even worse, due to the lack of synchronization within the IO handler
implementation, both threads ended up executing the same IO handler.
Most of the time, the IO handlers don't need to run synchronously. The
exception is sourcing commands from external files, like the .lldbinit file.
I've added a (recursive) mutex to prevent another thread from messing with the
IO handlers wile another thread is running one synchronously. It has to be
recursive, because we might have to source another file when encountering a
command source in the original file.
Differential revision: https://reviews.llvm.org/D72748
Added:
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/Makefile
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/TestBreakpointCallbackCommandSource.py
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/main.c
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/source.lldb
Modified:
lldb/include/lldb/Core/Debugger.h
lldb/source/Core/Debugger.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 26fc0ccb5f8e..d18c82f7b845 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -410,6 +410,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
m_script_interpreters;
IOHandlerStack m_io_handler_stack;
+ std::recursive_mutex m_io_handler_synchronous_mutex;
+
llvm::StringMap<std::weak_ptr<llvm::raw_ostream>> m_log_streams;
std::shared_ptr<llvm::raw_ostream> m_log_callback_stream_sp;
ConstString m_instance_name;
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/Makefile b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/Makefile
new file mode 100644
index 000000000000..10495940055b
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/TestBreakpointCallbackCommandSource.py b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/TestBreakpointCallbackCommandSource.py
new file mode 100644
index 000000000000..fb7ae1dc9fa6
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/TestBreakpointCallbackCommandSource.py
@@ -0,0 +1,35 @@
+"""
+Test completion in our IOHandlers.
+"""
+
+import os
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+
+class BreakpointCallbackCommandSource(PExpectTest):
+
+ mydir = TestBase.compute_mydir(__file__)
+ file_to_source = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'source.lldb')
+
+ # PExpect uses many timeouts internally and doesn't play well
+ # under ASAN on a loaded machine..
+ @skipIfAsan
+ @skipIfEditlineSupportMissing
+ def test_breakpoint_callback_command_source(self):
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+
+ self.launch(exe)
+ self.expect("b main", substrs=["Breakpoint 1"])
+ self.child.send("breakpoint command add -s python\n")
+ self.child.send(
+ "frame.GetThread().GetProcess().GetTarget().GetDebugger().HandleCommand('command source -s true {}')\n"
+ .format(self.file_to_source))
+ self.child.send("DONE\n")
+ self.expect_prompt()
+ self.expect("run", substrs=["Process", "stopped"])
+ self.expect("script print(foo)", substrs=["95126"])
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/main.c b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/main.c
new file mode 100644
index 000000000000..04a06321c49d
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/main.c
@@ -0,0 +1,4 @@
+int main (int argc, char const *argv[])
+{
+ return 0;
+}
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/source.lldb b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/source.lldb
new file mode 100644
index 000000000000..f8f163c441ac
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_callback_command_source/source.lldb
@@ -0,0 +1 @@
+script foo = 95126
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 7fb4baa29390..40c9b1df6cfe 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -895,23 +895,62 @@ void Debugger::ClearIOHandlers() {
}
void Debugger::RunIOHandlers() {
+ IOHandlerSP reader_sp = m_io_handler_stack.Top();
while (true) {
- IOHandlerSP reader_sp(m_io_handler_stack.Top());
if (!reader_sp)
break;
reader_sp->Run();
+ {
+ std::lock_guard<std::recursive_mutex> guard(
+ m_io_handler_synchronous_mutex);
+
+ // Remove all input readers that are done from the top of the stack
+ while (true) {
+ IOHandlerSP top_reader_sp = m_io_handler_stack.Top();
+ if (top_reader_sp && top_reader_sp->GetIsDone())
+ PopIOHandler(top_reader_sp);
+ else
+ break;
+ }
+ reader_sp = m_io_handler_stack.Top();
+ }
+ }
+ ClearIOHandlers();
+}
+
+void Debugger::RunIOHandlerSync(const IOHandlerSP &reader_sp) {
+ std::lock_guard<std::recursive_mutex> guard(m_io_handler_synchronous_mutex);
+
+ PushIOHandler(reader_sp);
+ IOHandlerSP top_reader_sp = reader_sp;
- // Remove all input readers that are done from the top of the stack
+ while (top_reader_sp) {
+ if (!top_reader_sp)
+ break;
+
+ top_reader_sp->Run();
+
+ // Don't unwind past the starting point.
+ if (top_reader_sp.get() == reader_sp.get()) {
+ if (PopIOHandler(reader_sp))
+ break;
+ }
+
+ // If we pushed new IO handlers, pop them if they're done or restart the
+ // loop to run them if they're not.
while (true) {
- IOHandlerSP top_reader_sp = m_io_handler_stack.Top();
- if (top_reader_sp && top_reader_sp->GetIsDone())
+ top_reader_sp = m_io_handler_stack.Top();
+ if (top_reader_sp && top_reader_sp->GetIsDone()) {
PopIOHandler(top_reader_sp);
- else
+ // Don't unwind past the starting point.
+ if (top_reader_sp.get() == reader_sp.get())
+ return;
+ } else {
break;
+ }
}
}
- ClearIOHandlers();
}
bool Debugger::IsTopIOHandler(const lldb::IOHandlerSP &reader_sp) {
@@ -950,28 +989,6 @@ void Debugger::RunIOHandlerAsync(const IOHandlerSP &reader_sp,
PushIOHandler(reader_sp, cancel_top_handler);
}
-void Debugger::RunIOHandlerSync(const IOHandlerSP &reader_sp) {
- PushIOHandler(reader_sp);
-
- IOHandlerSP top_reader_sp = reader_sp;
- while (top_reader_sp) {
- top_reader_sp->Run();
-
- if (top_reader_sp.get() == reader_sp.get()) {
- if (PopIOHandler(reader_sp))
- break;
- }
-
- while (true) {
- top_reader_sp = m_io_handler_stack.Top();
- if (top_reader_sp && top_reader_sp->GetIsDone())
- PopIOHandler(top_reader_sp);
- else
- break;
- }
- }
-}
-
void Debugger::AdoptTopIOHandlerFilesIfInvalid(FileSP &in, StreamFileSP &out,
StreamFileSP &err) {
// Before an IOHandler runs, it must have in/out/err streams. This function
More information about the lldb-commits
mailing list