[lldb] Synchronize Debugger's stdout and stderr at the StreamFile level

This 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.

Author: JDevlieghere

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;

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;
 

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;
 };
 }
 

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

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;
   }

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;

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);
   }
 }