[lldb] Synchronize the debugger's stdout and stderr streams #126630
Conversation
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis patch moves the synchronization for the Debugger's output and error into StreamFile. Currently, the debugger's output is synchronized at the IOHandler level. Because Debugger::PrintAsync first gives the top IOHandler a chance to print output, that works most of the time, except when we fall back to printing to the output and error streams directly. By storing the mutex in the new SynchronizedStreamFile, everyone from the Debugger level down has access to it. This should make it easier to do the right thing. It also means we don't have to pass the mutex around separately (e.g. between IOHandler and Editline). Locking at the StreamFile level also eliminates a few lock guard around calls to StreamFile::Write. I resisted the urge to simplify the code to aid reviewing. There's definitely some repetition in Editline that could be solved by extracting some helper methods. I'm planning on doing that in a follow-up. Patch is 39.28 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126630.diff 15 Files Affected:
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 70f4c4216221c65..67069b9eb819c35 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -133,9 +133,13 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
lldb::FileSP GetInputFileSP() { return m_input_file_sp; }
- lldb::StreamFileSP GetOutputStreamSP() { return m_output_stream_sp; }
+ lldb::SynchronizedStreamFileSP GetOutputStreamSP() {
+ return m_output_stream_sp;
+ }
- lldb::StreamFileSP GetErrorStreamSP() { return m_error_stream_sp; }
+ lldb::SynchronizedStreamFileSP GetErrorStreamSP() {
+ return m_error_stream_sp;
+ }
File &GetInputFile() { return *m_input_file_sp; }
@@ -206,8 +210,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
// If any of the streams are not set, set them to the in/out/err stream of
// the top most input reader to ensure they at least have something
void AdoptTopIOHandlerFilesIfInvalid(lldb::FileSP &in,
- lldb::StreamFileSP &out,
- lldb::StreamFileSP &err);
+ lldb::SynchronizedStreamFileSP &out,
+ lldb::SynchronizedStreamFileSP &err);
/// Run the given IO handler and return immediately.
void RunIOHandlerAsync(const lldb::IOHandlerSP &reader_sp,
@@ -693,8 +697,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
// these should never be NULL
lldb::FileSP m_input_file_sp;
- lldb::StreamFileSP m_output_stream_sp;
- lldb::StreamFileSP m_error_stream_sp;
+ lldb::SynchronizedStreamFileSP m_output_stream_sp;
+ lldb::SynchronizedStreamFileSP m_error_stream_sp;
/// Used for shadowing the input file when capturing a reproducer.
repro::DataRecorder *m_input_recorder;
diff --git a/lldb/include/lldb/Core/IOHandler.h b/lldb/include/lldb/Core/IOHandler.h
index cb14d7241320919..5eacc87dc899d75 100644
--- a/lldb/include/lldb/Core/IOHandler.h
+++ b/lldb/include/lldb/Core/IOHandler.h
@@ -58,8 +58,9 @@ class IOHandler {
IOHandler(Debugger &debugger, IOHandler::Type type);
IOHandler(Debugger &debugger, IOHandler::Type type,
- const lldb::FileSP &input_sp, const lldb::StreamFileSP &output_sp,
- const lldb::StreamFileSP &error_sp, uint32_t flags);
+ const lldb::FileSP &input_sp,
+ const lldb::SynchronizedStreamFileSP &output_sp,
+ const lldb::SynchronizedStreamFileSP &error_sp, uint32_t flags);
virtual ~IOHandler();
@@ -117,17 +118,11 @@ class IOHandler {
int GetErrorFD();
- FILE *GetInputFILE();
-
- FILE *GetOutputFILE();
-
- FILE *GetErrorFILE();
-
lldb::FileSP GetInputFileSP();
- lldb::StreamFileSP GetOutputStreamFileSP();
+ lldb::SynchronizedStreamFileSP GetOutputStreamFileSP();
- lldb::StreamFileSP GetErrorStreamFileSP();
+ lldb::SynchronizedStreamFileSP GetErrorStreamFileSP();
Debugger &GetDebugger() { return m_debugger; }
@@ -160,14 +155,11 @@ class IOHandler {
virtual void PrintAsync(const char *s, size_t len, bool is_stdout);
- std::recursive_mutex &GetOutputMutex() { return m_output_mutex; }
-
protected:
Debugger &m_debugger;
lldb::FileSP m_input_sp;
- lldb::StreamFileSP m_output_sp;
- lldb::StreamFileSP m_error_sp;
- std::recursive_mutex m_output_mutex;
+ lldb::SynchronizedStreamFileSP m_output_sp;
+ lldb::SynchronizedStreamFileSP m_error_sp;
Predicate<bool> m_popped;
Flags m_flags;
Type m_type;
@@ -335,8 +327,9 @@ class IOHandlerEditline : public IOHandler {
IOHandlerEditline(Debugger &debugger, IOHandler::Type type,
const lldb::FileSP &input_sp,
- const lldb::StreamFileSP &output_sp,
- const lldb::StreamFileSP &error_sp, uint32_t flags,
+ const lldb::SynchronizedStreamFileSP &output_sp,
+ const lldb::SynchronizedStreamFileSP &error_sp,
+ uint32_t flags,
const char *editline_name, // Used for saving history files
llvm::StringRef prompt, llvm::StringRef continuation_prompt,
bool multi_line, bool color,
@@ -350,9 +343,10 @@ class IOHandlerEditline : public IOHandler {
IOHandlerDelegate &) = delete;
IOHandlerEditline(Debugger &, IOHandler::Type, const lldb::FileSP &,
- const lldb::StreamFileSP &, const lldb::StreamFileSP &,
- uint32_t, const char *, const char *, const char *, bool,
- bool, uint32_t, IOHandlerDelegate &) = delete;
+ const lldb::SynchronizedStreamFileSP &,
+ const lldb::SynchronizedStreamFileSP &, uint32_t,
+ const char *, const char *, const char *, bool, bool,
+ uint32_t, IOHandlerDelegate &) = delete;
~IOHandlerEditline() override;
diff --git a/lldb/include/lldb/Host/Editline.h b/lldb/include/lldb/Host/Editline.h
index 27b863870090cb7..85225cc5fbff938 100644
--- a/lldb/include/lldb/Host/Editline.h
+++ b/lldb/include/lldb/Host/Editline.h
@@ -151,8 +151,9 @@ using namespace line_editor;
/// facility. Both single- and multi-line editing are supported.
class Editline {
public:
- Editline(const char *editor_name, FILE *input_file, FILE *output_file,
- FILE *error_file, bool color, std::recursive_mutex &output_mutex);
+ Editline(const char *editor_name, FILE *input_file,
+ lldb::SynchronizedStreamFileSP output_stream_sp,
+ lldb::SynchronizedStreamFileSP error_stream_sp, bool color);
~Editline();
@@ -392,8 +393,8 @@ class Editline {
volatile std::sig_atomic_t m_terminal_size_has_changed = 0;
std::string m_editor_name;
FILE *m_input_file;
- FILE *m_output_file;
- FILE *m_error_file;
+ lldb::SynchronizedStreamFileSP m_output_stream_sp;
+ lldb::SynchronizedStreamFileSP m_error_stream_sp;
ConnectionFileDescriptor m_input_connection;
IsInputCompleteCallbackType m_is_input_complete_callback;
@@ -411,7 +412,6 @@ class Editline {
std::string m_suggestion_ansi_suffix;
std::size_t m_previous_autosuggestion_size = 0;
- std::recursive_mutex &m_output_mutex;
};
}
diff --git a/lldb/include/lldb/Host/StreamFile.h b/lldb/include/lldb/Host/StreamFile.h
index 2c96e13565a0079..0d3cdfd989a82c3 100644
--- a/lldb/include/lldb/Host/StreamFile.h
+++ b/lldb/include/lldb/Host/StreamFile.h
@@ -52,6 +52,39 @@ class StreamFile : public Stream {
const StreamFile &operator=(const StreamFile &) = delete;
};
+class SynchronizedStreamFile : public StreamFile {
+public:
+ SynchronizedStreamFile(uint32_t flags, uint32_t addr_size,
+ lldb::ByteOrder byte_order)
+ : StreamFile(flags, addr_size, byte_order) {}
+
+ SynchronizedStreamFile(int fd, bool transfer_ownership)
+ : StreamFile(fd, transfer_ownership) {}
+
+ SynchronizedStreamFile(
+ const char *path, File::OpenOptions options,
+ uint32_t permissions = lldb::eFilePermissionsFileDefault)
+ : StreamFile(path, options, permissions) {}
+
+ SynchronizedStreamFile(FILE *fh, bool transfer_ownership)
+ : StreamFile(fh, transfer_ownership) {}
+
+ SynchronizedStreamFile(std::shared_ptr<File> file) : StreamFile(file) {}
+
+ ~SynchronizedStreamFile() override;
+
+ std::recursive_mutex &GetMutex() { return m_mutex; }
+
+protected:
+ size_t WriteImpl(const void *s, size_t length) override;
+ std::recursive_mutex m_mutex;
+
+private:
+ SynchronizedStreamFile(const SynchronizedStreamFile &) = delete;
+ const SynchronizedStreamFile &
+ operator=(const SynchronizedStreamFile &) = delete;
+};
+
} // namespace lldb_private
#endif // LLDB_HOST_STREAMFILE_H
diff --git a/lldb/include/lldb/Interpreter/ScriptInterpreter.h b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
index 2c2bd6f232e0943..34f92816337a300 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -128,8 +128,8 @@ class ScriptInterpreterIORedirect {
ScriptInterpreterIORedirect(Debugger &debugger, CommandReturnObject *result);
lldb::FileSP m_input_file_sp;
- lldb::StreamFileSP m_output_file_sp;
- lldb::StreamFileSP m_error_file_sp;
+ lldb::SynchronizedStreamFileSP m_output_file_sp;
+ lldb::SynchronizedStreamFileSP m_error_file_sp;
ThreadedCommunication m_communication;
bool m_disconnect;
};
@@ -478,7 +478,7 @@ class ScriptInterpreter : public PluginInterface {
dest.clear();
return false;
}
-
+
virtual StructuredData::ObjectSP
GetOptionsForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
return {};
@@ -488,9 +488,9 @@ class ScriptInterpreter : public PluginInterface {
GetArgumentsForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
return {};
}
-
+
virtual bool SetOptionValueForCommandObject(
- StructuredData::GenericSP cmd_obj_sp, ExecutionContext *exe_ctx,
+ StructuredData::GenericSP cmd_obj_sp, ExecutionContext *exe_ctx,
llvm::StringRef long_option, llvm::StringRef value) {
return false;
}
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
index fc7456a4b9a32e6..c684e13daf5a9db 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -215,6 +215,7 @@ class StoppointCallbackContext;
class Stream;
class StreamFile;
class StreamString;
+class SynchronizedStreamFile;
class StringList;
class StringTableReader;
class StructuredDataImpl;
@@ -432,6 +433,8 @@ typedef std::unique_ptr<lldb_private::StackFrameRecognizerManager>
typedef std::shared_ptr<lldb_private::StopInfo> StopInfoSP;
typedef std::shared_ptr<lldb_private::Stream> StreamSP;
typedef std::shared_ptr<lldb_private::StreamFile> StreamFileSP;
+typedef std::shared_ptr<lldb_private::SynchronizedStreamFile>
+ SynchronizedStreamFileSP;
typedef std::shared_ptr<lldb_private::StringSummaryFormat>
StringTypeSummaryImplSP;
typedef std::unique_ptr<lldb_private::StructuredDataImpl> StructuredDataImplUP;
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 2df2aeb20aa26a1..0a38f2baf0966d7 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -875,8 +875,10 @@ Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
: UserID(g_unique_id++),
Properties(std::make_shared<OptionValueProperties>()),
m_input_file_sp(std::make_shared<NativeFile>(stdin, false)),
- m_output_stream_sp(std::make_shared<StreamFile>(stdout, false)),
- m_error_stream_sp(std::make_shared<StreamFile>(stderr, false)),
+ m_output_stream_sp(
+ std::make_shared<SynchronizedStreamFile>(stdout, false)),
+ m_error_stream_sp(
+ std::make_shared<SynchronizedStreamFile>(stderr, false)),
m_input_recorder(nullptr),
m_broadcaster_manager_sp(BroadcasterManager::MakeBroadcasterManager()),
m_terminal_state(), m_target_list(*this), m_platform_list(),
@@ -1084,12 +1086,12 @@ void Debugger::SetInputFile(FileSP file_sp) {
void Debugger::SetOutputFile(FileSP file_sp) {
assert(file_sp && file_sp->IsValid());
- m_output_stream_sp = std::make_shared<StreamFile>(file_sp);
+ m_output_stream_sp = std::make_shared<SynchronizedStreamFile>(file_sp);
}
void Debugger::SetErrorFile(FileSP file_sp) {
assert(file_sp && file_sp->IsValid());
- m_error_stream_sp = std::make_shared<StreamFile>(file_sp);
+ m_error_stream_sp = std::make_shared<SynchronizedStreamFile>(file_sp);
}
void Debugger::SaveInputTerminalState() {
@@ -1226,8 +1228,9 @@ void Debugger::RunIOHandlerAsync(const IOHandlerSP &reader_sp,
PushIOHandler(reader_sp, cancel_top_handler);
}
-void Debugger::AdoptTopIOHandlerFilesIfInvalid(FileSP &in, StreamFileSP &out,
- StreamFileSP &err) {
+void Debugger::AdoptTopIOHandlerFilesIfInvalid(FileSP &in,
+ SynchronizedStreamFileSP &out,
+ SynchronizedStreamFileSP &err) {
// Before an IOHandler runs, it must have in/out/err streams. This function
// is called when one ore more of the streams are nullptr. We use the top
// input reader's in/out/err streams, or fall back to the debugger file
@@ -1253,7 +1256,7 @@ void Debugger::AdoptTopIOHandlerFilesIfInvalid(FileSP &in, StreamFileSP &out,
out = GetOutputStreamSP();
// If there is nothing, use stdout
if (!out)
- out = std::make_shared<StreamFile>(stdout, false);
+ out = std::make_shared<SynchronizedStreamFile>(stdout, false);
}
// If no STDERR has been set, then set it appropriately
if (!err || !err->GetFile().IsValid()) {
@@ -1263,7 +1266,7 @@ void Debugger::AdoptTopIOHandlerFilesIfInvalid(FileSP &in, StreamFileSP &out,
err = GetErrorStreamSP();
// If there is nothing, use stderr
if (!err)
- err = std::make_shared<StreamFile>(stderr, false);
+ err = std::make_shared<SynchronizedStreamFile>(stderr, false);
}
}
diff --git a/lldb/source/Core/IOHandler.cpp b/lldb/source/Core/IOHandler.cpp
index ca06b52b874db68..ecf3da82b7ef827 100644
--- a/lldb/source/Core/IOHandler.cpp
+++ b/lldb/source/Core/IOHandler.cpp
@@ -53,18 +53,20 @@ using namespace lldb_private;
using llvm::StringRef;
IOHandler::IOHandler(Debugger &debugger, IOHandler::Type type)
- : IOHandler(debugger, type,
- FileSP(), // Adopt STDIN from top input reader
- StreamFileSP(), // Adopt STDOUT from top input reader
- StreamFileSP(), // Adopt STDERR from top input reader
- 0 // Flags
+ : IOHandler(
+ debugger, type,
+ FileSP(), // Adopt STDIN from top input reader
+ SynchronizedStreamFileSP(), // Adopt STDOUT from top input reader
+ SynchronizedStreamFileSP(), // Adopt STDERR from top input reader
+ 0 // Flags
) {}
IOHandler::IOHandler(Debugger &debugger, IOHandler::Type type,
const lldb::FileSP &input_sp,
- const lldb::StreamFileSP &output_sp,
- const lldb::StreamFileSP &error_sp, uint32_t flags)
+ const lldb::SynchronizedStreamFileSP &output_sp,
+ const lldb::SynchronizedStreamFileSP &error_sp,
+ uint32_t flags)
: m_debugger(debugger), m_input_sp(input_sp), m_output_sp(output_sp),
m_error_sp(error_sp), m_popped(false), m_flags(flags), m_type(type),
m_user_data(nullptr), m_done(false), m_active(false) {
@@ -88,24 +90,16 @@ int IOHandler::GetErrorFD() {
return (m_error_sp ? m_error_sp->GetFile().GetDescriptor() : -1);
}
-FILE *IOHandler::GetInputFILE() {
- return (m_input_sp ? m_input_sp->GetStream() : nullptr);
-}
+FileSP IOHandler::GetInputFileSP() { return m_input_sp; }
-FILE *IOHandler::GetOutputFILE() {
- return (m_output_sp ? m_output_sp->GetFile().GetStream() : nullptr);
+SynchronizedStreamFileSP IOHandler::GetOutputStreamFileSP() {
+ return m_output_sp;
}
-FILE *IOHandler::GetErrorFILE() {
- return (m_error_sp ? m_error_sp->GetFile().GetStream() : nullptr);
+SynchronizedStreamFileSP IOHandler::GetErrorStreamFileSP() {
+ return m_error_sp;
}
-FileSP IOHandler::GetInputFileSP() { return m_input_sp; }
-
-StreamFileSP IOHandler::GetOutputStreamFileSP() { return m_output_sp; }
-
-StreamFileSP IOHandler::GetErrorStreamFileSP() { return m_error_sp; }
-
bool IOHandler::GetIsInteractive() {
return GetInputFileSP() ? GetInputFileSP()->GetIsInteractive() : false;
}
@@ -119,7 +113,6 @@ void IOHandler::SetPopped(bool b) { m_popped.SetValue(b, eBroadcastOnChange); }
void IOHandler::WaitForPop() { m_popped.WaitForValueEqualTo(true); }
void IOHandler::PrintAsync(const char *s, size_t len, bool is_stdout) {
- std::lock_guard<std::recursive_mutex> guard(m_output_mutex);
lldb::StreamFileSP stream = is_stdout ? m_output_sp : m_error_sp;
stream->Write(s, len);
stream->Flush();
@@ -228,19 +221,20 @@ IOHandlerEditline::IOHandlerEditline(
llvm::StringRef prompt, llvm::StringRef continuation_prompt,
bool multi_line, bool color, uint32_t line_number_start,
IOHandlerDelegate &delegate)
- : IOHandlerEditline(debugger, type,
- FileSP(), // Inherit input from top input reader
- StreamFileSP(), // Inherit output from top input reader
- StreamFileSP(), // Inherit error from top input reader
- 0, // Flags
- editline_name, // Used for saving history files
- prompt, continuation_prompt, multi_line, color,
- line_number_start, delegate) {}
+ : IOHandlerEditline(
+ debugger, type,
+ FileSP(), // Inherit input from top input reader
+ SynchronizedStreamFileSP(), // Inherit output from top input reader
+ SynchronizedStreamFileSP(), // Inherit error from top input reader
+ 0, // Flags
+ editline_name, // Used for saving history files
+ prompt, continuation_prompt, multi_line, color, line_number_start,
+ delegate) {}
IOHandlerEditline::IOHandlerEditline(
Debugger &debugger, IOHandler::Type type, const lldb::FileSP &input_sp,
- const lldb::StreamFileSP &output_sp, const lldb::StreamFileSP &error_sp,
- uint32_t flags,
+ const lldb::SynchronizedStreamFileSP &output_sp,
+ const lldb::SynchronizedStreamFileSP &error_sp, uint32_t flags,
const char *editline_name, // Used for saving history files
llvm::StringRef prompt, llvm::StringRef continuation_prompt,
bool multi_line, bool color, uint32_t line_number_start,
@@ -256,15 +250,12 @@ IOHandlerEditline::IOHandlerEditline(
SetPrompt(prompt);
#if LLDB_ENABLE_LIBEDIT
- bool use_editline = false;
-
- use_editline = GetInputFILE() && GetOutputFILE() && GetErrorFILE() &&
- m_input_sp && m_input_sp->GetIsRealTerminal();
-
+ const bool use_editline = m_input_sp && m_output_sp && m_error_sp &&
+ m_input_sp->GetIsRealTerminal();
if (use_editline) {
- m_editline_up = std::make_unique<Editline>(editline_name, GetInputFILE(),
- GetOutputFILE(), GetErrorFILE(),
- m_color, GetOutputMutex());
+ m_editline_up = std::make_unique<Editline>(
+ editline_name, m_input_sp ? m_input_sp->GetStream() : nullptr,
+ m_output_sp, m_error_sp, m_color);
m_editline_up->SetIsInputCompleteCallback(
[this](Editline *editline, StringList &lines) {
return this->IsInputCompleteCallback(editline, lines);
@@ -380,7 +371,7 @@ bool IOHandlerEditline::GetLine(std::string &line, bool &interrupted) {
return false;
}
- FILE *in = GetInputFILE();
+ FILE *in = m_input_sp ? m_input_sp->GetStream() : nullptr;
char buffer[256];
if (!got_line && !in && m_input_sp) {
@@ -630,7 +621,6 @@ void IOHandlerEditline::GotEOF() {
void IOHandlerEditline::PrintAsync(const char *s, size_t len, bool is_stdout) {
#if LLDB_ENABLE_LIBEDIT
if (m_editline_up) {
- std::lock_guard<std::recursive_mutex> guard(m_output_mutex);
lldb::StreamFileSP stream = is_stdout ? m_output_sp : m_error_sp;
m_editline_up->PrintAsync(stream.get(), s, len);
} else
diff --git a/lldb/source/Core/IOHandlerCursesGUI.cpp b/lldb/source/Core/IOHandlerCursesGUI.cpp
index 456ce7d16e102d9..b51e946bb9afe0f 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -7574,7 +7574,9 @@ IOHandlerCursesGUI::IOHandlerCursesGUI(Debugger &debugger)
void IOHandlerCursesGUI::Activate() {
IOHandler::Activate();
if (!m_app_ap) {
- m_app_ap = std::make_unique<Application>(GetInputFILE(), GetOutputFILE());
+ m_app_ap = std::make_unique<Application>(
+ m_input_sp ? m_input_sp->GetStream() : nullptr,
+ m_output_sp ? m_input_sp->GetStream() : nullptr);
// This is both a window and a menu delegate
std::shared_ptr<ApplicationDelegate> app_delegate_sp(
diff --git a/lldb/source/Host/common/E...
[truncated]
|
lldb/source/Host/common/Editline.cpp
Outdated
| @@ -393,7 +394,7 @@ void Editline::MoveCursor(CursorLocation from, CursorLocation to) { | |||
| int fromLine = GetLineIndexForLocation(from, editline_cursor_row); | |||
| int toLine = GetLineIndexForLocation(to, editline_cursor_row); | |||
| if (toLine != fromLine) { | |||
| fprintf(m_output_file, | |||
| fprintf(m_output_stream_sp->GetFile().GetStream(), | |||
There was a problem hiding this comment.
There are a lot of unguarded calls to fprintf and fputs in this file. Are there some guarantees about the state of the synchronized stream when these functions are executed? Or do we just have tons of unsynchronized writes to the output stream? :D
There was a problem hiding this comment.
So, the problem I see here is that this is enforcing mutual exclusion at the level of most basic write operations. That's fine if what you want to achieve is to protect the internal state of the stream object. However, I'm not sure if that's your goal, since it doesn't e.g. prevent someone from printing something in the middle of a statusline update. It also may be unnecessary since I believe that both file descriptors and FILE* objects are synchronized internally (one by the OS, the other by the C library).
To achieve proper output synchronization, the exclusion needs to happen at a higher level (e.g. "a single statusline update", or "writing of one line of text", etc.). In theory, that can be done using the provided GetMutex accessor, but it's also very easy to forget doing that.
To better guarantee that, I am wondering if a slightly different pattern wouldn't be better. We could have one object (let's call it LockableStream here, just so it differs from what you have in this PR), which holds the real stream (and a mutex) as a member, and doesn't allow you to access it without locking it. Then, when you want write something, you call a Lock function or something (it could also be constructing a guard object and passing the LockableStream to its constructor), and this returns something that allows you to access the underlying stream. As long as you hold on to the returned object (which contains a lock_guard on the stream mutex internally), you are permitted to write to the stream. The destruction of the object can automatically flush the output stream.
WDYT?
Technically it does, because the statusline is printed as a single call to write, but I agree that's fragile and one could argue that works by accident rather than by design. I really like the idea of introducing an RAII object as it (1) makes the locking more explicit and (2) makes it easy to do the right thing and hard to do the wrong thing. |
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See llvm#126630 for more details.
JDevlieghere
force-pushed
the
SynchronizedStreamFile
branch
from
094a7f7 to
af22e24
Compare
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See llvm#126630 for more details.
JDevlieghere
force-pushed
the
SynchronizedStreamFile
branch
from
af22e24 to
d7b6704
Compare
JDevlieghere
changed the title
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See #126630 for more details.
JDevlieghere
force-pushed
the
SynchronizedStreamFile
branch
from
d7b6704 to
55cc3a5
Compare
|
Rebased. @labath can you confirm this is what you had in mind? |
There was a problem hiding this comment.
Mostly yes. It's not as clean as I would have liked, though I suppose that is to be expected of an attempt to retrofit a significant change like this. The existence of Debugger::Get{Output,Error}File in particular is a very large hole in the RAII locking scheme. From the looks of things, none of the uses of these functions (GetErrorFile is unused already) is particularly load-bearing and they could be replaced by something else (but don't do that here, as the patch is big enough already).
The RAII thing doesn't make things much safer than the original implementation did because the Printf currently translate to a single Write call anyway, but they are more future-proof, as I can easily see someone "modernising" them into a Format call and then someone else "optimizing" our Format implementation to use the llvm raw_ostream method (which does not use a single write call). The current implementation will work even with those changes.
I do want to spend some time discussing the relationship between stdout and stderr. The current implementation uses a separate mutex for each stream, which is not unreasonable, but I can see at least two other possible philosophies:
- Since both of the streams will be going to the same terminal most of the time, it might make sense to use a single mutex for both of them, as that's sort of the only way to ensure consistent output. (Imagine the situation of printing the "stderr" of a CLI command while the status line is being updated)
- Alternatively, we could say that stderr is for immediate and exceptional output where it is more important to get the message to the user than it being formatted beautifully. In this world, we could keep "stderr" as a regular unlocked stream. In specific cases, where synchronizing the output is particularly important to us (like the beforementioned case of "stderr" of a CLI command) we could synchronize its output by holding the "stdout" mutex while writing it.
What do you think of that? I'm sort of leaning towards option two since stderr might be used in contexts where holding a mutex might not be completely safe (and the first option would make that even worse), and the current implementation with separate mutexes doesn't really guarantee "reasonable" stdout/err sequencing.
lldb/source/Host/common/Editline.cpp
Outdated
| @@ -574,10 +581,10 @@ int Editline::GetCharacter(EditLineGetCharType *c) { | |||
| // indefinitely. This gives a chance for someone to interrupt us. After | |||
| // Read returns, immediately lock the mutex again and check if we were | |||
| // interrupted. | |||
| m_output_mutex.unlock(); | |||
| m_output_stream_sp->GetMutex().unlock(); | |||
There was a problem hiding this comment.
What would you say if, instead of exposing rather non-RAII method on the generic class, we worked around this issue (the issue being us wanting to put libedit code in a critical section) locally, by having something like std::optional<LockedStream> m_locked_output member on the editline class? GetLine could set this member when doing its work, and here we would clear it before blocking, and re-set it immediately afterwards.
There was a problem hiding this comment.
I see a thumbs up but no change? Any reason for not doing it. Or are you waiting until the other stuff is sorted out first?
There was a problem hiding this comment.
Just hadn't gotten around to it. I've pushed a commit that adds this.
| @@ -700,12 +707,14 @@ unsigned char Editline::EndOrAddLineCommand(int ch) { | |||
| } | |||
| } | |||
| MoveCursor(CursorLocation::EditingCursor, CursorLocation::BlockEnd); | |||
| fprintf(m_output_file, "\n"); | |||
| LockedStreamFile locked_stream = m_output_stream_sp->Lock(); | |||
There was a problem hiding this comment.
And I suspect most/all of these callbacks are called from GetLine which means that they could reuse the locked_stream member instead of locking it once again.
| io_handler.GetOutputStreamFileSP()) { | ||
| LockedStreamFile locked_stream = output_sp->Lock(); | ||
| locked_stream.PutCString(g_reader_instructions); | ||
| } |
There was a problem hiding this comment.
Optional, but I think it would be reasonable to write all of these "simple" lock calls as one-liners like this
| io_handler.GetOutputStreamFileSP()) { | |
| LockedStreamFile locked_stream = output_sp->Lock(); | |
| locked_stream.PutCString(g_reader_instructions); | |
| } | |
| io_handler.GetOutputStreamFileSP()) | |
| output_sp->Lock().PutCString(g_reader_instructions); |
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See llvm#126630 for more details.
I had a feeling this was going to come up... The tricky part is that, as you pointed out, most of the time the output is going to same terminal. But on the other hand, we totally support having those two streams go to totally separate files and it feels wrong to force them to be synchronized. How about a compromise between (1) and (2) where we use the same mutex when the debugger is creating the StreamFile for stdout and stderr, but we use separate murexes when they're set through the setter (and the SB API)? Conceptually I'd also lean towards (2) but I worry about losing the benefits the current approach brings. Deciding whether "the output is particularly important" is a judgement call that we can't enforce at the API level. I like how the current implementation makes it easy to do the right thing and hard to do the wrong thing (or at least make you think about locking). If the default is to not lock the stdout (the alternative of always locking is equivalent to (1)) then I'm skeptical that we won't just end up with the same outcome that we have with two mutexes. |
How would that work? We'd check the value of the passed-in
But with two mutexes you still don't get stdout<=>stderr synchronization, which means that stderr output can corrupt things you're writing to stdout. What the stderr mutex buys you is stderr<=>stderr synchronization, which is... maybe nice, but maybe also not necessary? FWIW, there is some precedent for unconditional stdout-stderr interaction: writes to |
What I had in mind was much simpler than that: it would only apply when the debugger is the one creating the streams in its constructor. I was specifically referring to: While not doing this when someone calls
Yes, I think we're saying the same thing here.
Fair enough. What I'm hearing is that you think it's important to synchronize the two somehow and I'd rather do it consistently rather than doing it only sometimes which makes it hard to know what the expectations both for us as the people writing that code and for our users. It should be fairly straightforward to support tying the two streams together. I'll see how that goes and update the PR. |
I see. Well.. that's slightly cleaner, but it's currently a no-op because even our own driver calls SetOutputFileHandle. I guess we could change that, but I also don't particularly like the inaccessibility of this feature.
I guess so, but I think we're drawing different conclusions. My take on this is that if the only thing we get from locking stderr is the ability to mutually exclude two stderr accesses, then maybe we don't need to bother with locking stderr at all.
Yes, I think that tying them always is better than tying them sometimes. However, I don't really have a clear preference between the other options flying around. The options being:
If it sounds like I'm in favor of one of them, it's because I'm comparing its (dis)advantages relative to some other approach. However, I think all of these approaches have their (dis)advantages.
A random idea, I don't know if it's a good one: have one object (LockableStreamPair?) that holds the two streams and their mutex. One less argument to pass around? |
That's what I understood and I agree. We don't use the error stream all that much, so I don't feel strongly either way. If it were up to me I would've gone with doing it anyway for parity with stdout.
Ack, I'm sorry if I was reading too much into it. Given that neither of us has a clear preference, and given that we need to make a decision, I'll suggest that we go with (1) as that's really the only thing that guarantees we don't mess up the statusline, which was the motivation behind this PR. The other two options will probably still do the right thing most of the time so it's really about picking a tie-breaker.
Sounds good, that simplifies the signatures (which this PR is already touching) and keeps the mutex abstracted away. |
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See llvm#126630 for more details.
JDevlieghere
force-pushed
the
SynchronizedStreamFile
branch
from
55cc3a5 to
0716b13
Compare
I did implement this and I'm not happy with the result: even though you only pass one object, you still need to get the output or error stream out of it which ends up being more verbose. And because of the |
- Remove _ap (auto_ptr) suffix with _up (unique_ptr) suffix - Move forward declaration from IOHandler.h to IOHandlerCursesGUI.h - Move curses namespace under lldb_private Motivated by Alex' comment in #126630.
JDevlieghere
force-pushed
the
SynchronizedStreamFile
branch
from
0716b13 to
b6578a2
Compare
The async streams buffer until they're flushed (or more commonly destroyed) and they delegate to the IOHandler instead of writing directly to the debugger's output or error stream. The problem is that while the IOHandler can protect the write, as is the case for
Yes, with this patch protecting the underlying streams, I don't think we need the asynchronous streams and most uses could be converted to lock the stream. That said, there are still uses cases that benefit from the buffering, like running the expressions. As you pointed out, we don't want to lock the streams for the duration of running the expression. But maybe we should rename them to "BufferedOutputStream" but still remove the |
|
The buffering doesn't sound like a problem to me. Basically it means you have to use the stream like an RAII object, which is sort of what we have here anyway. We also have the same problem with not being able to get everything to use the RAII locker object from this patch (due to legacy reasons..) That said, not needing to replicate the locking logic in every IOHandler would definitely be nice. I'm not a mind reader nor a time traveller, but I have a feeling this is the reason why the async functionality is implemented the way it is. Maybe that's something we don't need? I think I could live without it. But if that is the case, I would like to see it dismantled, at least partially. For example, do you think it would be possible to change Debugger::PrintAsync to print the output directly (after locking the stream and such) -- without going through the IOHandlers and stuff? |
Are you saying this is how it could work? Looking at
I ran the test suite with this change: I'm not sure how representative it is to rely on the test suite for something like this, but I got one failure ( @labath After reading your last message I'm not sure where you want to take this. I understand the desire to not have two ways of doing things, but I don't necessarily see how they have to be tied together. I'm convinced that protecting the underlying streams is a net benefit. We were doing that in an ad-hoc way in the IOHandler previously and this moves it up a level. If that means we can improve other things (like the asynchronous IOHandler) then I'm happy to continue chipping away at this, but your earlier message sounds like you're proposing that as an alternative and I'm not sure I fully understand what you have in mind. |
Correct. This is not how IOHandlerProcessSTDIO works right now (but IOHandlerEditline does). I'm using that as an example of something that would not be possible if one relied solely on the locking mechanism. The curses gui iohandler might be an even better example of that (although I'd much rather see it burn than improve it), as there the output might need to go to some particular curses window in order to avoid completely corrupting the curses window layout.
Thanks for trying it out. You're right, it probably isn't representative, but this test failure probably is a symptom of the problem -- this implementation would not trigger the prompt redrawing code I linked to above.
I'm sorry about the confusion. I think that's a reflection of my ambiguity. I wouldn't really say I want to take this in a specific direction. I'm more of in the "asking questions" stage, where I'm turning the problem over in order to understand it. Yesterday, I wasn't sure if we need the async stream mechanism. Today, I think we do (because of the prompt redrawing thingy). I think the two questions (async print and stream protection), but are separate in a way (the iohandlers still need, and the editline iohandler actually uses a mutex), but they are also kind of related in that they have to work together to achieve desired outcome ("nice" console output?) -- which may impact how they're implemented. At this point, I think the main question I am trying to answer is about a some guideline/rule about the usage of the various printing mechanism. Imagine I'm writing (or reviewing) a piece of code trying to print something. How do I know which method to use. If I can get a hold of a Debugger, I have the choice between GetOutputStreamSP, GetOutputFile and GetAsyncOutputStream. This patch doesn't really change that (which is why I think you say it's a net benefit), but it might make it seem like using GetOutputStreamSP (after locking it) is safe/okay -- which I think is the part that's bothering me. So to try to propose an answer to this question: would it be correct to say that (even after this patch, as it is right now), one should approximately always use GetAsyncOutputStream for printing stuff out (at least in cases where one doesn't have the CommandReturnObject around), and that the other methods exist as implementation details of that (supporting locking in iohandlers, which is necessary to produce output correctly), or due legacy code we don't know how to get rid of? |
I think so too. When I started writing that message yesterday I was under the impression that we could get rid of the asynchronous streams altogether, but the prompt redrawing makes me doubt that.
Yes. I now think that everyone should use the asynchronous streams by default. The lockable streams should only be used if the buffering poses a problem or you need the underlying stream for legacy reasons (e.g. the SB API). Let me try to confirm that assumption by converting all the uses that don't fall in the exception categories in a separate PR. If that works out, we can think about a way to enforce this at the API level (e.g. making the getters for the lockable stream private and relying on friends) and rebase this PR on top. |
JDevlieghere
force-pushed
the
SynchronizedStreamFile
branch
from
54d5bc9 to
4c7990f
Compare
|
Rebased on top of #127682 |
|
As I had hoped, all the remaining uses of |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This patch improves the synchronization of the debugger's output and error streams using two new abstractions: LockableStreamFile and LockedStreamFile. - LockableStreamFile is a wrapper around a StreamFile and a mutex. Client cannot use the StreamFile without calling Lock, which returns a LockedStreamFile. - LockedStreamFile is an RAII object that locks the stream for the duration of its existence. As long as you hold on to the returned object you are permitted to write to the stream. The destruction of the object automatically flush the output stream.
JDevlieghere
force-pushed
the
SynchronizedStreamFile
branch
from
4c7990f to
8471301
Compare
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See llvm#126630 for more details.
Similar to llvm#126821, in support of llvm#126630.
- Remove _ap (auto_ptr) suffix with _up (unique_ptr) suffix - Move forward declaration from IOHandler.h to IOHandlerCursesGUI.h - Move curses namespace under lldb_private Motivated by Alex' comment in llvm#126630.
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See llvm#126630 for more details. (cherry picked from commit eff3c34)
Similar to llvm#126821, in support of llvm#126630. (cherry picked from commit ed32d85)
- Remove _ap (auto_ptr) suffix with _up (unique_ptr) suffix - Move forward declaration from IOHandler.h to IOHandlerCursesGUI.h - Move curses namespace under lldb_private Motivated by Alex' comment in llvm#126630. (cherry picked from commit 776fa2d)
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in preparation for replacing both with a new variant that needs to be locked and hence can't be handed out like we do right now. The patch replaces most uses with GetAsyncOutputStream and GetAsyncErrorStream respectively. There methods return new StreamSP objects that automatically get flushed on destruction. See llvm#126630 for more details. (cherry picked from commit eff3c34)
Similar to llvm#126821, in support of llvm#126630. (cherry picked from commit ed32d85)
- Remove _ap (auto_ptr) suffix with _up (unique_ptr) suffix - Move forward declaration from IOHandler.h to IOHandlerCursesGUI.h - Move curses namespace under lldb_private Motivated by Alex' comment in llvm#126630. (cherry picked from commit 776fa2d)
This patch improves the synchronization of the debugger's output and
error streams using two new abstractions: LockableStreamFile and
LockedStreamFile.
LockableStreamFile is a wrapper around a StreamFile and a mutex.
Client cannot use the StreamFile without calling Lock, which returns
a LockedStreamFile.
LockedStreamFile is an RAII object that locks the stream for the
duration of its existence. As long as you hold on to the returned
object you are permitted to write to the stream. The destruction of
the object automatically flush the output stream.