teemperor created this revision.
While working on the `expr` completion, I've encountered the issue that
sometimes lldb
deadlocks when doing input/output. The underlying cause for this is that we
seem to expect
that we can always call `Debugger::PrintAsync` from any point of lldb and that
this call will
always return at some point.
However this is not always the case. For example, when calling the completion
handler, we
obviously hold locks that make the console IO thread safe. The expression
parsing
code then tries to parse the user expression to provide possible completions.
It's possible
that while parsing our expression, we hit a case where some code decides to
print
information to the debugger output (for example to inform the user that some
functionality
has failed unexpectedly). If this call now happens from another thread, then
even our
recursive_mutex won't protect us and we get a deadlock.
To prevent this issue in the future, this patch introduces the notion of
delayed output scopes,
that essentially act as safe zones in which all calls to PrintAsync for a given
debugger
are safe independently of the locking inside the IOHandlers or related issues.
While printing in
this scope, we queue messages and then actually print them once we leave the
scope.
So far I only use this feature in the Editline code (because that's the case
where it solves my problem
with the deadlocked expr completion), but it might be more fitting in another
place.
https://reviews.llvm.org/D48463
Files:
include/lldb/Core/Debugger.h
source/Core/Debugger.cpp
source/Core/IOHandler.cpp
Index: source/Core/IOHandler.cpp
===================================================================
--- source/Core/IOHandler.cpp
+++ source/Core/IOHandler.cpp
@@ -348,6 +348,15 @@
}
bool IOHandlerEditline::GetLine(std::string &line, bool &interrupted) {
+ // For safety reasons let's delay all messages that might be printed
+ // to the output until we are done with this method. Printing to the
+ // standard output while we are working with EditLine could cause
+ // problems. Especially any held locks that are needed for printing
+ // to the console are problematic. We might take those locks here
+ // because we want to do console output and they might be requested by any
+ // lldb code that will be called from here (possibly in another thread).
+ Debugger::MessageDelayScope buffer_scope(m_debugger);
+
#ifndef LLDB_DISABLE_LIBEDIT
if (m_editline_ap) {
return m_editline_ap->GetLine(line, interrupted);
Index: source/Core/Debugger.cpp
===================================================================
--- source/Core/Debugger.cpp
+++ source/Core/Debugger.cpp
@@ -985,6 +985,21 @@
}
void Debugger::PrintAsync(const char *s, size_t len, bool is_stdout) {
+ bool ShouldForward = false;
+ {
+ // We check if any user requested to delay output to a later time
+ // (which is designated by m_delayed_output_counter being not 0).
+ std::lock_guard<std::mutex> guard(m_delayed_output_mutex);
+ if (m_delayed_output_counter != 0) {
+ // We want to delay messages, so push them to the buffer.
+ m_delayed_output.emplace_back(std::string(s, len), is_stdout);
+ } else {
+ // Allow direct forwarding to the IOHandlers.
+ ShouldForward = true;
+ }
+ }
+ if (!ShouldForward)
+ return;
lldb::StreamFileSP stream = is_stdout ? GetOutputFile() : GetErrorFile();
m_input_reader_stack.PrintAsync(stream.get(), s, len);
}
Index: include/lldb/Core/Debugger.h
===================================================================
--- include/lldb/Core/Debugger.h
+++ include/lldb/Core/Debugger.h
@@ -204,6 +204,26 @@
bool CheckTopIOHandlerTypes(IOHandler::Type top_type,
IOHandler::Type second_top_type);
+ //------------------------------------------------------------------
+ /// Guard class that delays the output printing from the given debugger
+ /// until the end of the scope. Useful if you aquire any IOHandler locks
+ /// in the current scope and also call code that might print via an IOHandler
+ /// (which could lead to deadlocks).
+ ///
+ /// This guard can be used in nested scopes. Multiple guards on the
+ /// same debugger behave the same as if only the top-most guard
+ /// requested to delay the messages.
+ ///
+ /// @see EnableDelayedPrinting() and TryFlushingDelayedMessages().
+ //------------------------------------------------------------------
+ struct MessageDelayScope {
+ Debugger &m_debugger;
+ MessageDelayScope(Debugger &d) : m_debugger(d) {
+ m_debugger.EnableDelayedPrinting();
+ }
+ ~MessageDelayScope() { m_debugger.TryFlushingDelayedMessages(); }
+ };
+
void PrintAsync(const char *s, size_t len, bool is_stdout);
ConstString GetTopIOHandlerControlSequence(char ch);
@@ -417,6 +437,74 @@
// object
Debugger(lldb::LogOutputCallback m_log_callback, void *baton);
+ //------------------------------------------------------------------
+ /// A message sent via PrintAsync that we store to delay the actual
+ /// call until later.
+ //------------------------------------------------------------------
+ struct DelayedMessage {
+ DelayedMessage(std::string data, bool for_stdout)
+ : m_data(data), m_for_stdout(for_stdout) {}
+ std::string m_data;
+ bool m_for_stdout;
+ };
+
+ //------------------------------------------------------------------
+ /// Starts delaying any calls to PrintAsync until a matching call
+ /// to FlushDelayedMessages occurs. This function should only be
+ /// called before a matching call to FlushDelayedMessages.
+ ///
+ /// This method is intentionally private. Use MessageDelayScope to
+ /// call this method (and the matching FlushDelayedMessage()) method
+ /// below.
+ //------------------------------------------------------------------
+ void EnableDelayedPrinting() {
+ std::lock_guard<std::mutex> guard(m_delayed_output_mutex);
+ ++m_delayed_output_counter;
+ }
+
+ //------------------------------------------------------------------
+ /// Tries to flush any delayed messages to PrintAsync. This might
+ /// not be possible if it was requested by multiple users to
+ /// delay messages and not all have given consent at this point for
+ /// forwarding the messages.
+ //------------------------------------------------------------------
+ void TryFlushingDelayedMessages() {
+ // We copy the buffer here. We do *not* want to call PrintAsync (or
+ // actually anything else) while we own this lock.
+ std::vector<DelayedMessage> buffer_to_send;
+ bool should_print_delayed = false;
+ {
+ std::lock_guard<std::mutex> guard(m_delayed_output_mutex);
+ buffer_to_send = std::move(m_delayed_output);
+ m_delayed_output.clear();
+ --m_delayed_output_counter;
+ // 0 means that all users we have have consented that we can now foward
+ // our delayed messages to the IOHandlers.
+ if (m_delayed_output_counter == 0)
+ should_print_delayed = true;
+ }
+ if (!should_print_delayed)
+ return;
+ // Forward all cached messages to the IOHandlers.
+ for (const auto &msg : buffer_to_send) {
+ PrintAsync(msg.m_data.data(), msg.m_data.size(), msg.m_for_stdout);
+ }
+ }
+
+ /// This mutex protects *only* m_async_buffer and m_should_buffer_async.
+ std::mutex m_delayed_output_mutex;
+ /// The list of delayed messages in the order in which they arrived.
+ /// @note Protected by m_delayed_output_mutex.
+ std::vector<DelayedMessage> m_delayed_output;
+ /// This counter keeps track of how many users have requested that we delay
+ /// the forwarding of calls to PrintAsync to the IOHandlers. If the counter
+ /// is 0, then it's safe to directly forward all calls to PrintAsync to the
+ /// IOHandlers. If the counter is *not* 0, then all calls to PrintAsync
+ /// should be cached until the counter is 0 again.
+ /// @see EnableDelayedPrinting() and TryFlushingDelayedMessages().
+ /// @note Protected by m_delayed_output_mutex.
+ unsigned m_delayed_output_counter = 0;
+
DISALLOW_COPY_AND_ASSIGN(Debugger);
};
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
