[lldb] Make thread safety the responsibility of the log handlers

[JDevlieghere]

Summary

As per Pavel's suggestion in D127922, make thread safety the responsibility of the log handlers.

[labath]

Cool, thanks.

[JDevlieghere]

Landed as 70841b97eb2e5c68d46b70579877e378511a47b4

[clayborg]

Maybe we should leave the option in the command for history purposes??? Taking it out will now make anyone's log enable lines fail when they didn't use to...

lldb/include/lldb/Utility/Log.h

 #include <mutex>
 #include <string>
 #include <type_traits>
 
 namespace llvm {
 class raw_ostream;
 }
 // Logging Options
-#define LLDB_LOG_OPTION_THREADSAFE (1u << 0)
 #define LLDB_LOG_OPTION_VERBOSE (1u << 1)
 #define LLDB_LOG_OPTION_PREPEND_SEQUENCE (1u << 3)
 #define LLDB_LOG_OPTION_PREPEND_TIMESTAMP (1u << 4)
 #define LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD (1u << 5)
 #define LLDB_LOG_OPTION_PREPEND_THREAD_NAME (1U << 6)
 #define LLDB_LOG_OPTION_BACKTRACE (1U << 7)
 #define LLDB_LOG_OPTION_APPEND (1U << 8)
 #define LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION (1U << 9)
 
 // Logging Functions
 namespace lldb_private {
 
 class LogHandler {
 public:
   virtual ~LogHandler() = default;
   virtual void Emit(llvm::StringRef message) = 0;
-  void EmitThreadSafe(llvm::StringRef message);
-
-private:
-  std::mutex m_mutex;
 };
 
 class StreamLogHandler : public LogHandler {
 public:
   StreamLogHandler(int fd, bool should_close, size_t buffer_size = 0);
   ~StreamLogHandler() override;
 
   void Emit(llvm::StringRef message) override;
+  void Flush();
 
 private:
+  std::mutex m_mutex;
   llvm::raw_fd_ostream m_stream;
 };
 
 class CallbackLogHandler : public LogHandler {
 public:
   CallbackLogHandler(lldb::LogOutputCallback callback, void *baton);
 
   void Emit(llvm::StringRef message) override;
public:
   void Emit(llvm::StringRef message) override;
   void Dump(llvm::raw_ostream &stream) const;
 
 private:
   size_t NormalizeIndex(size_t i) const;
   size_t GetNumMessages() const;
   size_t GetFirstMessageIndex() const;
 
+  mutable std::mutex m_mutex;
   std::unique_ptr<std::string[]> m_messages;
   const size_t m_size = 0;
   size_t m_next_index = 0;
   size_t m_total_count = 0;
 };
 
 class Log final {
 public:

lldb/source/Commands/CommandObjectLog.cpp

Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
       case 'f':
         log_file.SetFile(option_arg, FileSpec::Style::native);
         FileSystem::Instance().Resolve(log_file);
         break;
       case 'b':
         error =
             buffer_size.SetValueFromString(option_arg, eVarSetOperationAssign);
         break;
-      case 't':
-        log_options |= LLDB_LOG_OPTION_THREADSAFE;
-        break;
       case 'v':
         log_options |= LLDB_LOG_OPTION_VERBOSE;
         break;
       case 's':
         log_options |= LLDB_LOG_OPTION_PREPEND_SEQUENCE;
         break;
       case 'T':
         log_options |= LLDB_LOG_OPTION_PREPEND_TIMESTAMP;

lldb/source/Commands/Options.td

def history_clear : Option<"clear", "C">, Group<2>,
     Desc<"Clears the current command history.">;
 }
 
 let Command = "log enable" in {
   def log_file : Option<"file", "f">, Group<1>, Arg<"Filename">,
     Desc<"Set the destination file to log to.">;
   def log_buffer_size : Option<"buffer", "b">, Group<1>, Arg<"UnsignedInteger">,
     Desc<"Set the log to be buffered, using the specified buffer size.">;
-  def log_threadsafe : Option<"threadsafe", "t">, Group<1>,
-    Desc<"Enable thread safe logging to avoid interweaved log lines.">;
   def log_verbose : Option<"verbose", "v">, Group<1>,
     Desc<"Enable verbose logging.">;
   def log_sequence : Option<"sequence", "s">, Group<1>,
     Desc<"Prepend all log lines with an increasing integer sequence id.">;
   def log_timestamp : Option<"timestamp", "T">, Group<1>,
     Desc<"Prepend all log lines with a timestamp.">;
   def log_pid_tid : Option<"pid-tid", "p">, Group<1>,
     Desc<"Prepend all log lines with the process and thread ID that generates "

lldb/source/Core/Debugger.cpp

if (!log_handler_sp) {
       log_handler_sp = std::make_shared<StreamLogHandler>(
           (*file)->GetDescriptor(), should_close, buffer_size);
       m_stream_handlers[log_file] = log_handler_sp;
     }
   }
   assert(log_handler_sp);
 
   if (log_options == 0)
-    log_options =
-        LLDB_LOG_OPTION_PREPEND_THREAD_NAME | LLDB_LOG_OPTION_THREADSAFE;
+    log_options = LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
 
   return Log::EnableLogChannel(log_handler_sp, log_options, channel, categories,
                                error_stream);
 }
 
 ScriptInterpreter *
 Debugger::GetScriptInterpreter(bool can_create,
                                llvm::Optional<lldb::ScriptLanguage> language) {

lldb/source/Utility/Log.cpp

 }
 
 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
   auto handler_sp = GetHandler();
   if (!handler_sp)
     return;
-
-  Flags options = GetOptions();
-  if (options.Test(LLDB_LOG_OPTION_THREADSAFE))
-    handler_sp->EmitThreadSafe(message);
-  else
-    handler_sp->Emit(message);
 }
 
 void Log::Format(llvm::StringRef file, llvm::StringRef function,
                  const llvm::formatv_object_base &payload) {
   std::string message_string;
   llvm::raw_string_ostream message(message_string);
   WriteHeader(message, file, function);
   message << payload << "\n";
   WriteMessage(message.str());
 }
 
-void LogHandler::EmitThreadSafe(llvm::StringRef message) {
-  std::lock_guard<std::mutex> guard(m_mutex);
-  Emit(message);
-}
-
 StreamLogHandler::StreamLogHandler(int fd, bool should_close,
                                    size_t buffer_size)
     : m_stream(fd, should_close, false) {
   if (buffer_size > 0)
     m_stream.SetBufferSize(buffer_size);
 }
 
-StreamLogHandler::~StreamLogHandler() {
+StreamLogHandler::~StreamLogHandler() { Flush(); }
+
+void StreamLogHandler::Flush() {
+  std::lock_guard<std::mutex> guard(m_mutex);
   m_stream.flush();
 }
 
-void StreamLogHandler::Emit(llvm::StringRef message) { m_stream << message; }
+void StreamLogHandler::Emit(llvm::StringRef message) {
+  if (m_stream.GetBufferSize() > 0) {
+    std::lock_guard<std::mutex> guard(m_mutex);
+    m_stream << message;
+  } else {
+    m_stream << message;
+  }
+}
 
 CallbackLogHandler::CallbackLogHandler(lldb::LogOutputCallback callback,
                                        void *baton)
     : m_callback(callback), m_baton(baton) {}
 
 void CallbackLogHandler::Emit(llvm::StringRef message) {
   m_callback(message.data(), m_baton);
 }
 
 RotatingLogHandler::RotatingLogHandler(size_t size)
     : m_messages(std::make_unique<std::string[]>(size)), m_size(size) {}
 
 void RotatingLogHandler::Emit(llvm::StringRef message) {
+  std::lock_guard<std::mutex> guard(m_mutex);
   ++m_total_count;
   const size_t index = m_next_index;
   m_next_index = NormalizeIndex(index + 1);
   m_messages[index] = message.str();
 }
 
 size_t RotatingLogHandler::NormalizeIndex(size_t i) const { return i % m_size; }
 
 size_t RotatingLogHandler::GetNumMessages() const {
   return m_total_count < m_size ? m_total_count : m_size;
 }
 
 size_t RotatingLogHandler::GetFirstMessageIndex() const {
   return m_total_count < m_size ? 0 : m_next_index;
 }
 
 void RotatingLogHandler::Dump(llvm::raw_ostream &stream) const {
+  std::lock_guard<std::mutex> guard(m_mutex);
   const size_t start_idx = GetFirstMessageIndex();
   const size_t stop_idx = start_idx + GetNumMessages();
   for (size_t i = start_idx; i < stop_idx; ++i) {
     const size_t idx = NormalizeIndex(i);
     stream << m_messages[idx];
   }
   stream.flush();
 }