teemperor updated this revision to Diff 153129.
teemperor edited the summary of this revision.
teemperor added a comment.
- Fixed that we only pass a nullptr as a debugger to the real editline object
(which now fixes the deadlock in the code completion patch).
https://reviews.llvm.org/D48463
Files:
include/lldb/Core/Debugger.h
include/lldb/Host/Editline.h
source/Core/Debugger.cpp
source/Core/IOHandler.cpp
source/Host/common/Editline.cpp
unittests/Editline/EditlineTest.cpp
Index: unittests/Editline/EditlineTest.cpp
===================================================================
--- unittests/Editline/EditlineTest.cpp
+++ unittests/Editline/EditlineTest.cpp
@@ -123,9 +123,9 @@
return;
// Create an Editline instance.
- _editline_sp.reset(new lldb_private::Editline("gtest editor", *_el_slave_file,
- *_el_slave_file,
- *_el_slave_file, false));
+ _editline_sp.reset(new lldb_private::Editline(
+ "gtest editor", *_el_slave_file, *_el_slave_file, *_el_slave_file, false,
+ nullptr));
_editline_sp->SetPrompt("> ");
// Hookup our input complete callback.
Index: source/Host/common/Editline.cpp
===================================================================
--- source/Host/common/Editline.cpp
+++ source/Host/common/Editline.cpp
@@ -11,6 +11,7 @@
#include <iostream>
#include <limits.h>
+#include "lldb/Core/Debugger.h"
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/Editline.h"
#include "lldb/Host/Host.h"
@@ -507,9 +508,15 @@
// indefinitely. This gives a chance for someone to interrupt us. After
// Read returns, immediately lock the mutex again and check if we were
// interrupted.
+ int read_count;
m_output_mutex.unlock();
- int read_count = m_input_connection.Read(&ch, 1, llvm::None, status, NULL);
+ {
+ // We also disable the message delaying while we have the mutex unlocked.
+ lldb_private::Debugger::DisableMessageDelayScope guard(m_debugger);
+ read_count = m_input_connection.Read(&ch, 1, llvm::None, status, NULL);
+ }
m_output_mutex.lock();
+
if (m_editor_status == EditorStatus::Interrupted) {
while (read_count > 0 && status == lldb::eConnectionStatusSuccess)
read_count = m_input_connection.Read(&ch, 1, llvm::None, status, NULL);
@@ -1128,10 +1135,12 @@
}
Editline::Editline(const char *editline_name, FILE *input_file,
- FILE *output_file, FILE *error_file, bool color_prompts)
+ FILE *output_file, FILE *error_file, bool color_prompts,
+ Debugger *debugger)
: m_editor_status(EditorStatus::Complete), m_color_prompts(color_prompts),
m_input_file(input_file), m_output_file(output_file),
- m_error_file(error_file), m_input_connection(fileno(input_file), false) {
+ m_error_file(error_file), m_input_connection(fileno(input_file), false),
+ m_debugger(debugger) {
// Get a shared history instance
m_editor_name = (editline_name == nullptr) ? "lldb-tmp" : editline_name;
m_history_sp = EditlineHistory::GetHistory(m_editor_name);
@@ -1264,6 +1273,12 @@
m_input_lines = std::vector<EditLineStringType>();
m_input_lines.insert(m_input_lines.begin(), EditLineConstString(""));
+ // While we own the output mutex, we delay all messages that are printed via
+ // PrintAsync until we release the mutex again. This prevents dead locks that
+ // are caused when someone calls PrintAsync (which also needs to aquire
+ // the output mutex). Note that Editline::GetCharacter unlocks the guarded
+ // mutex from a nested call and also temporarily disables the message delay.
+ Debugger::MessageDelayScope msg_guard(m_debugger);
std::lock_guard<std::mutex> guard(m_output_mutex);
lldbassert(m_editor_status != EditorStatus::Editing);
@@ -1309,6 +1324,9 @@
m_input_lines = std::vector<EditLineStringType>();
m_input_lines.insert(m_input_lines.begin(), EditLineConstString(""));
+ // We delay all message printing until we release the output mutex. See
+ // Editline::GetLine for a more detailed explanation.
+ Debugger::MessageDelayScope msg_guard(m_debugger);
std::lock_guard<std::mutex> guard(m_output_mutex);
// Begin the line editing loop
DisplayInput();
Index: source/Core/IOHandler.cpp
===================================================================
--- source/Core/IOHandler.cpp
+++ source/Core/IOHandler.cpp
@@ -313,7 +313,7 @@
if (use_editline) {
m_editline_ap.reset(new Editline(editline_name, GetInputFILE(),
GetOutputFILE(), GetErrorFILE(),
- m_color_prompts));
+ m_color_prompts, &m_debugger));
m_editline_ap->SetIsInputCompleteCallback(IsInputCompleteCallback, this);
m_editline_ap->SetAutoCompleteCallback(AutoCompleteCallback, this);
// See if the delegate supports fixing indentation
Index: source/Core/Debugger.cpp
===================================================================
--- source/Core/Debugger.cpp
+++ source/Core/Debugger.cpp
@@ -797,6 +797,29 @@
SetUseColor(false);
}
+void Debugger::TryFlushingDelayedMessages() {
+ // We copy the buffer here. We do *not* want to call PrintAsync (or
+ // anything else) while we own this lock.
+ std::vector<DelayedMessage> buffer_to_send;
+ {
+ std::lock_guard<std::mutex> guard(m_delayed_output_mutex);
+ buffer_to_send = std::move(m_delayed_output);
+ m_delayed_output.clear();
+ // It's an error if we call this method before EnableDelayedPrinting.
+ assert(m_delayed_output_counter > 0);
+ --m_delayed_output_counter;
+ // 0 means that all users we have have consented that we can now forward
+ // our delayed messages to the IOHandlers. Other values mean we that can't
+ // forward the delayed messages yet.
+ if (m_delayed_output_counter != 0)
+ 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);
+ }
+}
+
Debugger::~Debugger() { Clear(); }
void Debugger::Clear() {
@@ -985,6 +1008,16 @@
}
void Debugger::PrintAsync(const char *s, size_t len, bool is_stdout) {
+ // 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);
+ return;
+ }
+ }
lldb::StreamFileSP stream = is_stdout ? GetOutputFile() : GetErrorFile();
m_input_reader_stack.PrintAsync(stream.get(), s, len);
}
Index: include/lldb/Host/Editline.h
===================================================================
--- include/lldb/Host/Editline.h
+++ include/lldb/Host/Editline.h
@@ -58,6 +58,7 @@
#include "lldb/Utility/FileSpec.h"
namespace lldb_private {
+class Debugger;
namespace line_editor {
// type alias's to help manage 8 bit and wide character versions of libedit
@@ -146,7 +147,8 @@
class Editline {
public:
Editline(const char *editor_name, FILE *input_file, FILE *output_file,
- FILE *error_file, bool color_prompts);
+ FILE *error_file, bool color_prompts,
+ lldb_private::Debugger *debugger);
~Editline();
@@ -359,6 +361,9 @@
CompleteCallbackType m_completion_callback = nullptr;
void *m_completion_callback_baton = nullptr;
+ /// The debugger that is using this Editline instance for input/output. May
+ /// be a nullptr if there is no debugger using this instance.
+ lldb_private::Debugger *m_debugger = nullptr;
std::mutex m_output_mutex;
};
}
Index: include/lldb/Core/Debugger.h
===================================================================
--- include/lldb/Core/Debugger.h
+++ include/lldb/Core/Debugger.h
@@ -14,6 +14,7 @@
#include <stdint.h>
// C++ Includes
+#include <cassert>
#include <memory>
#include <vector>
@@ -204,6 +205,64 @@
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.
+ ///
+ /// If a nullptr is passed as a debugger object, this guard will perform no
+ /// actions.
+ ///
+ /// @see EnableDelayedPrinting()
+ /// @see TryFlushingDelayedMessages()
+ /// @see DisableMessageDelayScope
+ //------------------------------------------------------------------
+ struct MessageDelayScope {
+ Debugger *m_debugger;
+ explicit MessageDelayScope(Debugger *d) : m_debugger(d) {
+ if (m_debugger)
+ m_debugger->EnableDelayedPrinting();
+ }
+ ~MessageDelayScope() {
+ if (m_debugger)
+ m_debugger->TryFlushingDelayedMessages();
+ }
+ DISALLOW_COPY_AND_ASSIGN(MessageDelayScope);
+ };
+
+ //------------------------------------------------------------------
+ /// Guard class that reverts the effect of one existing MessageDelayScope
+ /// during its lifetime. This guard can only be instantiated on a debugger
+ /// with at least one active MessageDelayScope. If a nullptr is passed as a
+ /// debugger, this guard will perform no actions.
+ ///
+ /// @note This class is intended to make existing code compatible with
+ /// MessageDelayScope and shouldn't be used in new code if possible.
+ ///
+ /// @see MessageDelayScope
+ //------------------------------------------------------------------
+ struct DisableMessageDelayScope {
+ Debugger *m_debugger;
+ // TODO: This constructor should not take a debugger object, but rather an
+ // existing MessageDelayScope to prevent using it in a spot where no active
+ // MessageDelayScope exists. However, we currently don't have access to the
+ // current MessageDelayScope object in all places where we need this.
+ explicit DisableMessageDelayScope(Debugger *d) : m_debugger(d) {
+ if (m_debugger)
+ m_debugger->TryFlushingDelayedMessages();
+ }
+ ~DisableMessageDelayScope() {
+ if (m_debugger)
+ m_debugger->EnableDelayedPrinting();
+ }
+ DISALLOW_COPY_AND_ASSIGN(DisableMessageDelayScope);
+ };
+
void PrintAsync(const char *s, size_t len, bool is_stdout);
ConstString GetTopIOHandlerControlSequence(char ch);
@@ -417,6 +476,54 @@
// 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();
+
+ /// 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()
+ /// @see 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
