Convert Log class to llvm streams

Summary:
This converts LLDB's logging to use llvm streams instead of
lldb_private::Stream and friends. The changes are mostly
straight-forward and amount to s/lldb_private::Stream/llvm::raw_ostream.

The part worth calling out is the rewrite of the StreamCallback class.
Previously this class contained a per-thread buffer of data written. I
assume this had something to do with it trying to make sure each log
line is delivered as a single event, instead of multiple (possibly
interleaved) events. However, this is no longer relevant as the Log
class already writes things to a temporary buffer and then delivers the
message as a single "write", so I have just removed the code in
question.

Reviewers: zturner, clayborg

Subscribers: emaste, lldb-commits, mgorny

Differential Revision: https://reviews.llvm.org/D29615

llvm-svn: 294736

lldb/include/lldb/Core/Debugger.h

@@ -365,9 +365,8 @@ protected:
   std::unique_ptr<CommandInterpreter> m_command_interpreter_ap;
 
   IOHandlerStack m_input_reader_stack;
-  typedef std::map<std::string, lldb::StreamWP> LogStreamMap;
-  LogStreamMap m_log_streams;
-  lldb::StreamSP m_log_callback_stream_sp;
+  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;
   static LoadPluginCallbackType g_load_plugin_callback;
   typedef std::vector<llvm::sys::DynamicLibrary> LoadedPluginsList;

lldb/include/lldb/Core/Log.h

@@ -50,9 +50,9 @@ public:
   //------------------------------------------------------------------
   typedef void (*DisableCallback)(const char **categories,
                                   Stream *feedback_strm);
-  typedef Log *(*EnableCallback)(lldb::StreamSP &log_stream_sp,
-                                 uint32_t log_options, const char **categories,
-                                 Stream *feedback_strm);
+  typedef Log *(*EnableCallback)(
+      const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+      uint32_t log_options, const char **categories, Stream *feedback_strm);
   typedef void (*ListCategoriesCallback)(Stream *strm);
 
   struct Callbacks {
@@ -72,14 +72,15 @@ public:
   static bool GetLogChannelCallbacks(const ConstString &channel,
                                      Log::Callbacks &log_callbacks);
 
-  static bool EnableLogChannel(lldb::StreamSP &log_stream_sp,
-                               uint32_t log_options, const char *channel,
-                               const char **categories, Stream &error_stream);
+  static bool
+  EnableLogChannel(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+                   uint32_t log_options, const char *channel,
+                   const char **categories, Stream &error_stream);
 
-  static void EnableAllLogChannels(lldb::StreamSP &log_stream_sp,
-                                   uint32_t log_options,
-                                   const char **categories,
-                                   Stream *feedback_strm);
+  static void
+  EnableAllLogChannels(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+                       uint32_t log_options, const char **categories,
+                       Stream *feedback_strm);
 
   static void DisableAllLogChannels(Stream *feedback_strm);
 
@@ -100,7 +101,7 @@ public:
   //------------------------------------------------------------------
   Log();
 
-  Log(const lldb::StreamSP &stream_sp);
+  Log(const std::shared_ptr<llvm::raw_ostream> &stream_sp);
 
   ~Log();
 
@@ -143,13 +144,15 @@ public:
 
   bool GetDebug() const;
 
-  void SetStream(const lldb::StreamSP &stream_sp) { m_stream_sp = stream_sp; }
+  void SetStream(const std::shared_ptr<llvm::raw_ostream> &stream_sp) {
+    m_stream_sp = stream_sp;
+  }
 
 protected:
   //------------------------------------------------------------------
   // Member variables
   //------------------------------------------------------------------
-  lldb::StreamSP m_stream_sp;
+  std::shared_ptr<llvm::raw_ostream> m_stream_sp;
   Flags m_options;
   Flags m_mask_bits;
 
@@ -176,7 +179,8 @@ public:
   virtual void Disable(const char **categories, Stream *feedback_strm) = 0;
 
   virtual bool
-  Enable(lldb::StreamSP &log_stream_sp, uint32_t log_options,
+  Enable(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+         uint32_t log_options,
          Stream *feedback_strm, // Feedback stream for argument errors etc
          const char **categories) = 0; // The categories to enable within this
                                        // logging stream, if empty, enable

lldb/include/lldb/Core/Logging.h

@@ -10,11 +10,10 @@
 #ifndef liblldb_Core_Logging_h_
 #define liblldb_Core_Logging_h_
 
-// C Includes
-// C++ Includes
-// Other libraries and framework includes
 // Project includes
 #include "lldb/lldb-private.h"
+// Other libraries and framework includes
+#include "llvm/Support/raw_ostream.h"
 
 //----------------------------------------------------------------------
 // Log Bits specific to logging in lldb
@@ -70,8 +69,9 @@ uint32_t GetLogMask();
 
 void DisableLog(const char **categories, Stream *feedback_strm);
 
-Log *EnableLog(lldb::StreamSP &log_stream_sp, uint32_t log_options,
-               const char **categories, Stream *feedback_strm);
+Log *EnableLog(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+               uint32_t log_options, const char **categories,
+               Stream *feedback_strm);
 
 void ListLogCategories(Stream *strm);
 

lldb/include/lldb/Core/StreamCallback.h

@@ -10,32 +10,23 @@
 #ifndef liblldb_StreamCallback_h_
 #define liblldb_StreamCallback_h_
 
-#include <mutex>
+#include "lldb/lldb-types.h"
+#include "llvm/Support/raw_ostream.h"
 #include <string>
 
-#include "lldb/Utility/Stream.h"
-#include "lldb/Utility/StreamString.h"
-
 namespace lldb_private {
 
-class StreamCallback : public Stream {
+class StreamCallback : public llvm::raw_ostream {
 public:
   StreamCallback(lldb::LogOutputCallback callback, void *baton);
-
-  ~StreamCallback() override;
-
-  void Flush() override;
-
-  size_t Write(const void *src, size_t src_len) override;
+  ~StreamCallback() override = default;
 
 private:
-  typedef std::map<lldb::tid_t, StreamString> collection;
   lldb::LogOutputCallback m_callback;
   void *m_baton;
-  collection m_accumulated_data;
-  std::mutex m_collection_mutex;
 
-  StreamString &FindStreamForThread(lldb::tid_t cur_tid);
+  void write_impl(const char *Ptr, size_t Size) override;
+  uint64_t current_pos() const override;
 };
 
 } // namespace lldb_private

lldb/source/Core/Debugger.cpp

@@ -17,6 +17,7 @@
 // Other libraries and framework includes
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DynamicLibrary.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 
 // Project includes
@@ -1241,25 +1242,34 @@ void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
 bool Debugger::EnableLog(const char *channel, const char **categories,
                          const char *log_file, uint32_t log_options,
                          Stream &error_stream) {
-  StreamSP log_stream_sp;
+  const bool should_close = true;
+  const bool unbuffered = true;
+
+  std::shared_ptr<llvm::raw_ostream> log_stream_sp;
   if (m_log_callback_stream_sp) {
     log_stream_sp = m_log_callback_stream_sp;
     // For now when using the callback mode you always get thread & timestamp.
     log_options |=
         LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
   } else if (log_file == nullptr || *log_file == '\0') {
-    log_stream_sp = GetOutputFile();
+    log_stream_sp = std::make_shared<llvm::raw_fd_ostream>(
+        GetOutputFile()->GetFile().GetDescriptor(), !should_close, unbuffered);
   } else {
-    LogStreamMap::iterator pos = m_log_streams.find(log_file);
+    auto pos = m_log_streams.find(log_file);
     if (pos != m_log_streams.end())
       log_stream_sp = pos->second.lock();
     if (!log_stream_sp) {
-      uint32_t options = File::eOpenOptionWrite | File::eOpenOptionCanCreate |
-                         File::eOpenOptionCloseOnExec | File::eOpenOptionAppend;
-      if (!(log_options & LLDB_LOG_OPTION_APPEND))
-        options |= File::eOpenOptionTruncate;
-
-      log_stream_sp.reset(new StreamFile(log_file, options));
+      llvm::sys::fs::OpenFlags flags = llvm::sys::fs::F_Text;
+      if (log_options & LLDB_LOG_OPTION_APPEND)
+        flags |= llvm::sys::fs::F_Append;
+      int FD;
+      if (std::error_code ec =
+              llvm::sys::fs::openFileForWrite(log_file, FD, flags)) {
+        error_stream.Format("Unable to open log file: {0}", ec.message());
+        return false;
+      }
+      log_stream_sp.reset(
+          new llvm::raw_fd_ostream(FD, should_close, unbuffered));
       m_log_streams[log_file] = log_stream_sp;
     }
   }

lldb/source/Core/Log.cpp

@@ -38,7 +38,7 @@ using namespace lldb_private;
 
 Log::Log() : m_stream_sp(), m_options(0), m_mask_bits(0) {}
 
-Log::Log(const StreamSP &stream_sp)
+Log::Log(const std::shared_ptr<llvm::raw_ostream> &stream_sp)
     : m_stream_sp(stream_sp), m_options(0), m_mask_bits(0) {}
 
 Log::~Log() = default;
@@ -202,9 +202,10 @@ bool Log::GetLogChannelCallbacks(const ConstString &channel,
   return false;
 }
 
-bool Log::EnableLogChannel(lldb::StreamSP &log_stream_sp, uint32_t log_options,
-                           const char *channel, const char **categories,
-                           Stream &error_stream) {
+bool Log::EnableLogChannel(
+    const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+    uint32_t log_options, const char *channel, const char **categories,
+    Stream &error_stream) {
   Log::Callbacks log_callbacks;
   if (Log::GetLogChannelCallbacks(ConstString(channel), log_callbacks)) {
     log_callbacks.enable(log_stream_sp, log_options, categories, &error_stream);
@@ -226,8 +227,9 @@ bool Log::EnableLogChannel(lldb::StreamSP &log_stream_sp, uint32_t log_options,
   }
 }
 
-void Log::EnableAllLogChannels(StreamSP &log_stream_sp, uint32_t log_options,
-                               const char **categories, Stream *feedback_strm) {
+void Log::EnableAllLogChannels(
+    const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+    uint32_t log_options, const char **categories, Stream *feedback_strm) {
   CallbackMap &callback_map = GetCallbackMap();
   CallbackMapIter pos, end = callback_map.end();
 
@@ -355,18 +357,18 @@ void Log::WriteHeader(llvm::raw_ostream &OS, llvm::StringRef file,
 void Log::WriteMessage(const std::string &message) {
   // Make a copy of our stream shared pointer in case someone disables our
   // log while we are logging and releases the stream
-  StreamSP stream_sp(m_stream_sp);
+  auto stream_sp = m_stream_sp;
   if (!stream_sp)
     return;
 
   if (m_options.Test(LLDB_LOG_OPTION_THREADSAFE)) {
     static std::recursive_mutex g_LogThreadedMutex;
     std::lock_guard<std::recursive_mutex> guard(g_LogThreadedMutex);
-    stream_sp->PutCString(message.c_str());
-    stream_sp->Flush();
+    *stream_sp << message;
+    stream_sp->flush();
   } else {
-    stream_sp->PutCString(message.c_str());
-    stream_sp->Flush();
+    *stream_sp << message;
+    stream_sp->flush();
   }
 }
 

lldb/source/Core/Logging.cpp

@@ -167,14 +167,15 @@ void lldb_private::DisableLog(const char **categories, Stream *feedback_strm) {
     }
     log->GetMask().Reset(flag_bits);
     if (flag_bits == 0) {
-      log->SetStream(lldb::StreamSP());
+      log->SetStream(nullptr);
       g_log_enabled = false;
     }
   }
 }
 
-Log *lldb_private::EnableLog(StreamSP &log_stream_sp, uint32_t log_options,
-                             const char **categories, Stream *feedback_strm) {
+Log *lldb_private::EnableLog(
+    const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+    uint32_t log_options, const char **categories, Stream *feedback_strm) {
   // Try see if there already is a log - that way we can reuse its settings.
   // We could reuse the log in toto, but we don't know that the stream is the
   // same.

lldb/source/Core/StreamCallback.cpp

@@ -7,44 +7,15 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include <stdio.h>
-
-#include "lldb/Core/Broadcaster.h"
-#include "lldb/Core/Event.h"
 #include "lldb/Core/StreamCallback.h"
-#include "lldb/Host/Host.h"
-#include "lldb/lldb-private.h"
 
-using namespace lldb;
 using namespace lldb_private;
 
 StreamCallback::StreamCallback(lldb::LogOutputCallback callback, void *baton)
-    : Stream(0, 4, eByteOrderBig), m_callback(callback), m_baton(baton),
-      m_accumulated_data(), m_collection_mutex() {}
+    : llvm::raw_ostream(true), m_callback(callback), m_baton(baton) {}
 
-StreamCallback::~StreamCallback() {}
-
-StreamString &StreamCallback::FindStreamForThread(lldb::tid_t cur_tid) {
-  std::lock_guard<std::mutex> guard(m_collection_mutex);
-  collection::iterator iter = m_accumulated_data.find(cur_tid);
-  if (iter == m_accumulated_data.end()) {
-    std::pair<collection::iterator, bool> ret;
-    ret = m_accumulated_data.insert(
-        std::pair<lldb::tid_t, StreamString>(cur_tid, StreamString()));
-    iter = ret.first;
-  }
-  return (*iter).second;
+void StreamCallback::write_impl(const char *Ptr, size_t Size) {
+  m_callback(std::string(Ptr, Size).c_str(), m_baton);
 }
 
-void StreamCallback::Flush() {
-  lldb::tid_t cur_tid = Host::GetCurrentThreadID();
-  StreamString &out_stream = FindStreamForThread(cur_tid);
-  m_callback(out_stream.GetData(), m_baton);
-  out_stream.Clear();
-}
-
-size_t StreamCallback::Write(const void *s, size_t length) {
-  lldb::tid_t cur_tid = Host::GetCurrentThreadID();
-  FindStreamForThread(cur_tid).Write(s, length);
-  return length;
-}
+uint64_t StreamCallback::current_pos() const { return 0; }

lldb/source/Plugins/Process/POSIX/ProcessPOSIXLog.cpp

@@ -115,8 +115,9 @@ void ProcessPOSIXLog::DisableLog(const char **args, Stream *feedback_strm) {
   return;
 }
 
-Log *ProcessPOSIXLog::EnableLog(StreamSP &log_stream_sp, uint32_t log_options,
-                                const char **args, Stream *feedback_strm) {
+Log *ProcessPOSIXLog::EnableLog(
+    const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+    uint32_t log_options, const char **args, Stream *feedback_strm) {
   // Try see if there already is a log - that way we can reuse its settings.
   // We could reuse the log in toto, but we don't know that the stream is the
   // same.

lldb/source/Plugins/Process/POSIX/ProcessPOSIXLog.h

@@ -62,9 +62,10 @@ public:
   static void DisableLog(const char **args,
                          lldb_private::Stream *feedback_strm);
 
-  static lldb_private::Log *EnableLog(lldb::StreamSP &log_stream_sp,
-                                      uint32_t log_options, const char **args,
-                                      lldb_private::Stream *feedback_strm);
+  static lldb_private::Log *
+  EnableLog(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+            uint32_t log_options, const char **args,
+            lldb_private::Stream *feedback_strm);
 
   static void ListLogCategories(lldb_private::Stream *strm);
 

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.cpp

@@ -115,10 +115,9 @@ void ProcessGDBRemoteLog::DisableLog(const char **categories,
   return;
 }
 
-Log *ProcessGDBRemoteLog::EnableLog(StreamSP &log_stream_sp,
-                                    uint32_t log_options,
-                                    const char **categories,
-                                    Stream *feedback_strm) {
+Log *ProcessGDBRemoteLog::EnableLog(
+    const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+    uint32_t log_options, const char **categories, Stream *feedback_strm) {
   // Try see if there already is a log - that way we can reuse its settings.
   // We could reuse the log in toto, but we don't know that the stream is the
   // same.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h

@@ -45,8 +45,9 @@ public:
 
   static void DisableLog(const char **categories, Stream *feedback_strm);
 
-  static Log *EnableLog(lldb::StreamSP &log_stream_sp, uint32_t log_options,
-                        const char **categories, Stream *feedback_strm);
+  static Log *EnableLog(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+                        uint32_t log_options, const char **categories,
+                        Stream *feedback_strm);
 
   static void ListLogCategories(Stream *strm);
 

lldb/source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp

@@ -95,7 +95,8 @@ void LogChannelDWARF::Disable(const char **categories, Stream *feedback_strm) {
 }
 
 bool LogChannelDWARF::Enable(
-    StreamSP &log_stream_sp, uint32_t log_options,
+    const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+    uint32_t log_options,
     Stream *feedback_strm,  // Feedback stream for argument errors etc
     const char **categories // The categories to enable within this logging
                             // stream, if empty, enable default set

lldb/source/Plugins/SymbolFile/DWARF/LogChannelDWARF.h

@@ -52,7 +52,8 @@ public:
 
   void Delete();
 
-  bool Enable(lldb::StreamSP &log_stream_sp, uint32_t log_options,
+  bool Enable(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
+              uint32_t log_options,
               lldb_private::Stream
                   *feedback_strm, // Feedback stream for argument errors etc
               const char **categories) override; // The categories to enable

lldb/tools/lldb-server/LLDBServerUtilities.cpp

@@ -16,25 +16,32 @@
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
 
 using namespace lldb;
 using namespace lldb_private::lldb_server;
 using namespace llvm;
 
+static std::shared_ptr<raw_ostream> GetLogStream(StringRef log_file) {
+  if (!log_file.empty()) {
+    std::error_code EC;
+    std::shared_ptr<raw_ostream> stream_sp = std::make_shared<raw_fd_ostream>(
+        log_file, EC, sys::fs::F_Text | sys::fs::F_Append);
+    if (!EC)
+      return stream_sp;
+    errs() << llvm::formatv(
+        "Failed to open log file `{0}`: {1}\nWill log to stderr instead.\n",
+        log_file, EC.message());
+  }
+  // No need to delete the stderr stream.
+  return std::shared_ptr<raw_ostream>(&errs(), [](raw_ostream *) {});
+}
+
 bool LLDBServerUtilities::SetupLogging(const std::string &log_file,
                                        const StringRef &log_channels,
                                        uint32_t log_options) {
-  lldb::StreamSP log_stream_sp;
-  if (log_file.empty()) {
-    log_stream_sp.reset(new StreamFile(stdout, false));
-  } else {
-    uint32_t options = File::eOpenOptionWrite | File::eOpenOptionCanCreate |
-                       File::eOpenOptionCloseOnExec | File::eOpenOptionAppend;
-    if (!(log_options & LLDB_LOG_OPTION_APPEND))
-      options |= File::eOpenOptionTruncate;
 
-    log_stream_sp.reset(new StreamFile(log_file.c_str(), options));
-  }
+  auto log_stream_sp = GetLogStream(log_file);
 
   SmallVector<StringRef, 32> channel_array;
   log_channels.split(channel_array, ":", /*MaxSplit*/ -1, /*KeepEmpty*/ false);

lldb/unittests/Core/CMakeLists.txt

@@ -7,6 +7,7 @@ add_lldb_unittest(LLDBCoreTests
   LogTest.cpp
   ScalarTest.cpp
   StateTest.cpp
+  StreamCallbackTest.cpp
   StructuredDataTest.cpp
   TimerTest.cpp
 

lldb/unittests/Core/LogTest.cpp

@@ -18,12 +18,14 @@ using namespace lldb_private;
 
 static std::string GetLogString(uint32_t log_options, const char *format,
                                 int arg) {
-  std::shared_ptr<StreamString> stream_sp(new StreamString());
+  std::string stream_string;
+  std::shared_ptr<llvm::raw_string_ostream> stream_sp(
+      new llvm::raw_string_ostream(stream_string));
   Log log_(stream_sp);
   log_.GetOptions().Reset(log_options);
   Log *log = &log_;
   LLDB_LOG(log, format, arg);
-  return stream_sp->GetString();
+  return stream_sp->str();
 }
 
 TEST(LogTest, LLDB_LOG_nullptr) {

lldb/unittests/Core/StreamCallbackTest.cpp

@@ -0,0 +1,28 @@
+//===-- StreamCallbackTest.cpp ----------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Core/StreamCallback.h"
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+static char test_baton;
+static size_t callback_count = 0;
+static void TestCallback(const char *data, void *baton) {
+  EXPECT_STREQ("Foobar", data);
+  EXPECT_EQ(&test_baton, baton);
+  ++callback_count;
+}
+
+TEST(StreamCallbackTest, Callback) {
+  StreamCallback stream(TestCallback, &test_baton);
+  stream << "Foobar";
+  EXPECT_EQ(1u, callback_count);
+}