JDevlieghere created this revision.
JDevlieghere added reviewers: labath, jingham, friss.
Herald added a subscriber: abidh.
Herald added a project: LLDB.
JDevlieghere added a comment.
I didn't get a chance to write a test case yet, but you can reproduce the
problem with the following setup:
$ echo "script foo = 1" > test.lldb
$ lldb ./a.out
(lldb) b main
(lldb) breakpoint command add -s python
frame.GetThread().GetProcess().GetTarget().GetDebugger().HandleCommand("command
source -s true ./test.lldb")
DONE
(lldb) run
The way the IO handlers are currently managed by the debugger is wrong. The
implementation lacks proper synchronization between `RunIOHandler` and
`ExecuteIOHandlers`. 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 `RunIOHandler` is called from a different
thread than `ExecuteIOHandlers`. 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 https://llvm.org/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 `ExecuteIOHandlers` 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.
Given the way the IO handlers work, I don't see the need to have execute them
synchronously. When an IO handler is pushed, it will interrupt the current
handler unless specified otherwise. One exception where being able to run a
handler synchronously is the sourcing of the `.lldbinit` file in the home and
current working directory. This takes place *before* `ExecuteIOHandlers` is
started from `RunCommandInterpreter`, which means that in the new scheme these
two IO handlers end up at the bottom of the stack. To work around this specific
problem, I've added an option to run the IO handler synchronously if needed
(i.e. `ExecuteIOHandlers` is not running yet). When that's the case, any other
threads are prevented (blocked) from starting to execute the IO handlers. I
don't think this workaround is necessary for any other handlers.
I've been starting at this for quite a bit and tried a few different
approaches, but it's totally possible that I missed some. I'm more than open to
suggestions for more elegant solutions!
Repository:
rLLDB LLDB
https://reviews.llvm.org/D72748
Files:
lldb/include/lldb/Core/Debugger.h
lldb/include/lldb/Interpreter/CommandInterpreter.h
lldb/source/Commands/CommandObjectBreakpointCommand.cpp
lldb/source/Commands/CommandObjectCommands.cpp
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Commands/CommandObjectType.cpp
lldb/source/Commands/CommandObjectWatchpointCommand.cpp
lldb/source/Core/Debugger.cpp
lldb/source/Interpreter/CommandInterpreter.cpp
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -1248,14 +1248,14 @@
CommandReturnObject &result) {
m_active_io_handler = eIOHandlerBreakpoint;
m_debugger.GetCommandInterpreter().GetPythonCommandsFromIOHandler(
- " ", *this, true, &bp_options_vec);
+ " ", *this, &bp_options_vec);
}
void ScriptInterpreterPythonImpl::CollectDataForWatchpointCommandCallback(
WatchpointOptions *wp_options, CommandReturnObject &result) {
m_active_io_handler = eIOHandlerWatchpoint;
m_debugger.GetCommandInterpreter().GetPythonCommandsFromIOHandler(
- " ", *this, true, wp_options);
+ " ", *this, wp_options);
}
Status ScriptInterpreterPythonImpl::SetBreakpointCommandCallbackFunction(
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===================================================================
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1854,7 +1854,7 @@
IOHandlerConfirm *confirm =
new IOHandlerConfirm(m_debugger, message, default_answer);
IOHandlerSP io_handler_sp(confirm);
- m_debugger.RunIOHandler(io_handler_sp);
+ m_debugger.PushIOHandler(io_handler_sp);
return confirm->GetResponse();
}
@@ -2477,7 +2477,11 @@
m_command_source_depth++;
- debugger.RunIOHandler(io_handler_sp);
+ const bool cancel_top_handler = true;
+ const bool asynchronous_if_needed = true;
+ debugger.PushIOHandler(io_handler_sp, cancel_top_handler,
+ asynchronous_if_needed);
+
if (!m_command_source_flags.empty())
m_command_source_flags.pop_back();
m_command_source_depth--;
@@ -2839,8 +2843,7 @@
}
void CommandInterpreter::GetLLDBCommandsFromIOHandler(
- const char *prompt, IOHandlerDelegate &delegate, bool asynchronously,
- void *baton) {
+ const char *prompt, IOHandlerDelegate &delegate, void *baton) {
Debugger &debugger = GetDebugger();
IOHandlerSP io_handler_sp(
new IOHandlerEditline(debugger, IOHandler::Type::CommandList,
@@ -2855,16 +2858,12 @@
if (io_handler_sp) {
io_handler_sp->SetUserData(baton);
- if (asynchronously)
- debugger.PushIOHandler(io_handler_sp);
- else
- debugger.RunIOHandler(io_handler_sp);
+ debugger.PushIOHandler(io_handler_sp);
}
}
void CommandInterpreter::GetPythonCommandsFromIOHandler(
- const char *prompt, IOHandlerDelegate &delegate, bool asynchronously,
- void *baton) {
+ const char *prompt, IOHandlerDelegate &delegate, void *baton) {
Debugger &debugger = GetDebugger();
IOHandlerSP io_handler_sp(
new IOHandlerEditline(debugger, IOHandler::Type::PythonCode,
@@ -2879,10 +2878,7 @@
if (io_handler_sp) {
io_handler_sp->SetUserData(baton);
- if (asynchronously)
- debugger.PushIOHandler(io_handler_sp);
- else
- debugger.RunIOHandler(io_handler_sp);
+ debugger.PushIOHandler(io_handler_sp);
}
}
Index: lldb/source/Core/Debugger.cpp
===================================================================
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -708,9 +708,10 @@
m_source_manager_up(), m_source_file_cache(),
m_command_interpreter_up(
std::make_unique<CommandInterpreter>(*this, false)),
- m_input_reader_stack(), m_instance_name(), m_loaded_plugins(),
- m_event_handler_thread(), m_io_handler_thread(),
- m_sync_broadcaster(nullptr, "lldb.debugger.sync"),
+ m_input_reader_stack(),
+ m_synchronous_reader_lock(m_synchronous_reader_mutex, std::defer_lock),
+ m_instance_name(), m_loaded_plugins(), m_event_handler_thread(),
+ m_io_handler_thread(), m_sync_broadcaster(nullptr, "lldb.debugger.sync"),
m_forward_listener_sp(), m_clear_once() {
char instance_cstr[256];
snprintf(instance_cstr, sizeof(instance_cstr), "debugger_%d", (int)GetID());
@@ -895,6 +896,11 @@
}
void Debugger::ExecuteIOHandlers() {
+ // Prevent anyone from executing the IO handlers synchronously while they're
+ // running here.
+ std::lock_guard<std::unique_lock<std::mutex>> guard(
+ m_synchronous_reader_lock);
+
while (true) {
IOHandlerSP reader_sp(m_input_reader_stack.Top());
if (!reader_sp)
@@ -941,28 +947,6 @@
return m_input_reader_stack.GetTopIOHandlerHelpPrologue();
}
-void Debugger::RunIOHandler(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_input_reader_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
@@ -1005,7 +989,8 @@
}
void Debugger::PushIOHandler(const IOHandlerSP &reader_sp,
- bool cancel_top_handler) {
+ bool cancel_top_handler,
+ bool asynchronous_if_needed) {
if (!reader_sp)
return;
@@ -1029,6 +1014,12 @@
if (cancel_top_handler)
top_reader_sp->Cancel();
}
+
+ // Support running the IO handlers synchronously on the current thread if
+ // they aren't running yet.
+ if (asynchronous_if_needed && !m_synchronous_reader_lock.owns_lock()) {
+ ExecuteIOHandlers();
+ }
}
bool Debugger::PopIOHandler(const IOHandlerSP &pop_reader_sp) {
Index: lldb/source/Commands/CommandObjectWatchpointCommand.cpp
===================================================================
--- lldb/source/Commands/CommandObjectWatchpointCommand.cpp
+++ lldb/source/Commands/CommandObjectWatchpointCommand.cpp
@@ -243,7 +243,6 @@
m_interpreter.GetLLDBCommandsFromIOHandler(
"> ", // Prompt
*this, // IOHandlerDelegate
- true, // Run IOHandler in async mode
wp_options); // Baton for the "io_handler" that will be passed back into
// our IOHandlerDelegate functions
}
Index: lldb/source/Commands/CommandObjectType.cpp
===================================================================
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -1332,7 +1332,6 @@
m_interpreter.GetPythonCommandsFromIOHandler(
" ", // Prompt
*this, // IOHandlerDelegate
- true, // Run IOHandler in async mode
options); // Baton for the "io_handler" that will be passed back into
// our IOHandlerDelegate functions
result.SetStatus(eReturnStatusSuccessFinishNoResult);
@@ -2232,7 +2231,6 @@
m_interpreter.GetPythonCommandsFromIOHandler(
" ", // Prompt
*this, // IOHandlerDelegate
- true, // Run IOHandler in async mode
options); // Baton for the "io_handler" that will be passed back into our
// IOHandlerDelegate functions
result.SetStatus(eReturnStatusSuccessFinishNoResult);
Index: lldb/source/Commands/CommandObjectTarget.cpp
===================================================================
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4689,7 +4689,6 @@
m_interpreter.GetLLDBCommandsFromIOHandler(
"> ", // Prompt
*this, // IOHandlerDelegate
- true, // Run IOHandler in async mode
nullptr); // Baton for the "io_handler" that will be passed back
// into our IOHandlerDelegate functions
}
Index: lldb/source/Commands/CommandObjectCommands.cpp
===================================================================
--- lldb/source/Commands/CommandObjectCommands.cpp
+++ lldb/source/Commands/CommandObjectCommands.cpp
@@ -1652,7 +1652,6 @@
m_interpreter.GetPythonCommandsFromIOHandler(
" ", // Prompt
*this, // IOHandlerDelegate
- true, // Run IOHandler in async mode
nullptr); // Baton for the "io_handler" that will be passed back
// into our IOHandlerDelegate functions
} else {
Index: lldb/source/Commands/CommandObjectBreakpointCommand.cpp
===================================================================
--- lldb/source/Commands/CommandObjectBreakpointCommand.cpp
+++ lldb/source/Commands/CommandObjectBreakpointCommand.cpp
@@ -256,7 +256,6 @@
m_interpreter.GetLLDBCommandsFromIOHandler(
"> ", // Prompt
*this, // IOHandlerDelegate
- true, // Run IOHandler in async mode
&bp_options_vec); // Baton for the "io_handler" that will be passed back
// into our IOHandlerDelegate functions
}
Index: lldb/include/lldb/Interpreter/CommandInterpreter.h
===================================================================
--- lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -429,12 +429,10 @@
void RunCommandInterpreter(bool auto_handle_events, bool spawn_thread,
CommandInterpreterRunOptions &options);
void GetLLDBCommandsFromIOHandler(const char *prompt,
- IOHandlerDelegate &delegate,
- bool asynchronously, void *baton);
+ IOHandlerDelegate &delegate, void *baton);
void GetPythonCommandsFromIOHandler(const char *prompt,
- IOHandlerDelegate &delegate,
- bool asynchronously, void *baton);
+ IOHandlerDelegate &delegate, void *baton);
const char *GetCommandPrefix();
Index: lldb/include/lldb/Core/Debugger.h
===================================================================
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -191,13 +191,11 @@
lldb::StreamFileSP &err);
void PushIOHandler(const lldb::IOHandlerSP &reader_sp,
- bool cancel_top_handler = true);
+ bool cancel_top_handler = true,
+ bool asynchronous_if_needed = false);
bool PopIOHandler(const lldb::IOHandlerSP &reader_sp);
- // Synchronously run an input reader until it is done
- void RunIOHandler(const lldb::IOHandlerSP &reader_sp);
-
bool IsTopIOHandler(const lldb::IOHandlerSP &reader_sp);
bool CheckTopIOHandlerTypes(IOHandler::Type top_type,
@@ -403,6 +401,9 @@
m_script_interpreters;
IOHandlerStack m_input_reader_stack;
+ std::mutex m_synchronous_reader_mutex;
+ std::unique_lock<std::mutex> m_synchronous_reader_lock;
+
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;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
