labath created this revision.

This fixes two threading issues in the logging code. The access to the
mask and options flags had data races when we were trying to
enable/disable logging while another thread was writing to the log.
Since we can log from almost any context, and we want it to be fast, so
I avoided locking primitives and used atomic variables instead. I have
also removed the (unused) setters for the mask and flags to make sure
that the only way to set them is through the enable/disable channel
functions.

I also add tests, which when run under tsan, verify that the use cases
like "doing an LLDB_LOGV while another thread disables logging" are
data-race-free.


https://reviews.llvm.org/D30702

Files:
  include/lldb/Utility/Log.h
  source/Utility/Log.cpp
  unittests/Utility/LogTest.cpp

Index: unittests/Utility/LogTest.cpp
===================================================================
--- unittests/Utility/LogTest.cpp
+++ unittests/Utility/LogTest.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Utility/StreamString.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Threading.h"
+#include <future>
 #include <thread>
 
 using namespace lldb;
@@ -214,3 +215,51 @@
   EXPECT_TRUE(stream_sp->str() == "" || stream_sp->str() == "Hello World\n")
       << "str(): " << stream_sp->str();
 }
+
+TEST_F(LogChannelTest, LogVerboseThread) {
+  // Test that we are able to concurrently check the verbose flag of a log
+  // channel and enable it.
+  std::string message;
+  std::shared_ptr<llvm::raw_string_ostream> stream_sp(
+      new llvm::raw_string_ostream(message));
+  StreamString err;
+  EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
+
+  Log *log = test_channel.GetLogIfAll(FOO);
+
+  // Start logging on one thread. Concurrently, try enabling the log channel
+  // (with different log options).
+  std::thread log_thread([log] { LLDB_LOGV(log, "Hello World"); });
+  EXPECT_TRUE(Log::EnableLogChannel(stream_sp, LLDB_LOG_OPTION_VERBOSE, "chan",
+                                    {}, err));
+  log_thread.join();
+  EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
+
+  // The log thread either managed to write to the log, or it didn't. In either
+  // case, we should not trip any undefined behavior (run the test under TSAN to
+  // verify this).
+  EXPECT_TRUE(stream_sp->str() == "" || stream_sp->str() == "Hello World\n")
+      << "str(): " << stream_sp->str();
+}
+
+TEST_F(LogChannelTest, LogGetLogThread) {
+  // Test that we are able to concurrently get mask of a Log object and disable
+  // it.
+  std::string message;
+  std::shared_ptr<llvm::raw_string_ostream> stream_sp(
+      new llvm::raw_string_ostream(message));
+  StreamString err;
+  EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
+  Log *log = test_channel.GetLogIfAll(FOO);
+
+  // Try fetching the log on one thread. Concurrently, try disabling the log
+  // channel.
+  uint32_t mask;
+  std::thread log_thread([log, &mask] { mask = log->GetMask().Get(); });
+  EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
+  log_thread.join();
+
+  // The mask should be either zero of "FOO". In either case, we should not trip
+  // any undefined behavior (run the test under TSAN to verify this).
+  EXPECT_TRUE(mask == 0 || mask == FOO) << "mask: " << mask;
+}
Index: source/Utility/Log.cpp
===================================================================
--- source/Utility/Log.cpp
+++ source/Utility/Log.cpp
@@ -87,9 +87,10 @@
 void Log::Channel::Enable(Log &log,
                           const std::shared_ptr<llvm::raw_ostream> &stream_sp,
                           uint32_t options, uint32_t flags) {
-  log.GetMask().Set(flags);
-  if (log.GetMask().Get()) {
-    log.GetOptions().Reset(options);
+  uint32_t mask = log.m_mask.load(std::memory_order_acquire) | flags;
+  log.m_mask.store(mask, std::memory_order_release);
+  if (mask) {
+    log.m_options.store(options, std::memory_order_release);
     log.SetStream(stream_sp);
     log_ptr.store(&log, std::memory_order_release);
   }
@@ -99,27 +100,21 @@
   Log *log = log_ptr.load(std::memory_order_acquire);
   if (!log)
     return;
-  log->GetMask().Clear(flags);
-  if (!log->GetMask().Get()) {
+  uint32_t mask = log->m_mask.load(std::memory_order_acquire) & ~flags;
+  log->m_mask.store(mask, std::memory_order_release);
+  if (!mask) {
     log->SetStream(nullptr);
     log_ptr.store(nullptr, std::memory_order_release);
   }
 }
 
-Log::Log() : m_stream_sp(), m_options(0), m_mask_bits(0) {}
-
-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;
-
-Flags &Log::GetOptions() { return m_options; }
-
-const Flags &Log::GetOptions() const { return m_options; }
-
-Flags &Log::GetMask() { return m_mask_bits; }
+const Flags Log::GetOptions() const {
+  return m_options.load(std::memory_order_acquire);
+}
 
-const Flags &Log::GetMask() const { return m_mask_bits; }
+const Flags Log::GetMask() const {
+  return m_mask.load(std::memory_order_acquire);
+}
 
 void Log::PutCString(const char *cstr) { Printf("%s", cstr); }
 void Log::PutString(llvm::StringRef str) { PutCString(str.str().c_str()); }
@@ -153,19 +148,6 @@
 }
 
 //----------------------------------------------------------------------
-// Log only if all of the bits are set
-//----------------------------------------------------------------------
-void Log::LogIf(uint32_t bits, const char *format, ...) {
-  if (!m_options.AllSet(bits))
-    return;
-
-  va_list args;
-  va_start(args, format);
-  VAPrintf(format, args);
-  va_end(args);
-}
-
-//----------------------------------------------------------------------
 // Printing of errors that are not fatal.
 //----------------------------------------------------------------------
 void Log::Error(const char *format, ...) {
@@ -187,7 +169,7 @@
 // enabled.
 //----------------------------------------------------------------------
 void Log::Verbose(const char *format, ...) {
-  if (!m_options.Test(LLDB_LOG_OPTION_VERBOSE))
+  if (!GetVerbose())
     return;
 
   va_list args;
@@ -278,39 +260,42 @@
   for (const auto &channel : *g_channel_map)
     ListCategories(*strm, channel);
 }
-bool Log::GetVerbose() const { return m_options.Test(LLDB_LOG_OPTION_VERBOSE); }
+bool Log::GetVerbose() const {
+  return m_options.load(std::memory_order_acquire) & LLDB_LOG_OPTION_VERBOSE;
+}
 
 void Log::WriteHeader(llvm::raw_ostream &OS, llvm::StringRef file,
                       llvm::StringRef function) {
+  Flags options = GetOptions();
   static uint32_t g_sequence_id = 0;
   // Add a sequence ID if requested
-  if (m_options.Test(LLDB_LOG_OPTION_PREPEND_SEQUENCE))
+  if (options.Test(LLDB_LOG_OPTION_PREPEND_SEQUENCE))
     OS << ++g_sequence_id << " ";
 
   // Timestamp if requested
-  if (m_options.Test(LLDB_LOG_OPTION_PREPEND_TIMESTAMP)) {
+  if (options.Test(LLDB_LOG_OPTION_PREPEND_TIMESTAMP)) {
     auto now = std::chrono::duration<double>(
         std::chrono::system_clock::now().time_since_epoch());
     OS << llvm::formatv("{0:f9} ", now.count());
   }
 
   // Add the process and thread if requested
-  if (m_options.Test(LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD))
+  if (options.Test(LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD))
     OS << llvm::formatv("[{0,0+4}/{1,0+4}] ", getpid(),
                         llvm::get_threadid());
 
   // Add the thread name if requested
-  if (m_options.Test(LLDB_LOG_OPTION_PREPEND_THREAD_NAME)) {
+  if (options.Test(LLDB_LOG_OPTION_PREPEND_THREAD_NAME)) {
     llvm::SmallString<32> thread_name;
     llvm::get_thread_name(thread_name);
     if (!thread_name.empty())
       OS << thread_name;
   }
 
-  if (m_options.Test(LLDB_LOG_OPTION_BACKTRACE))
+  if (options.Test(LLDB_LOG_OPTION_BACKTRACE))
     llvm::sys::PrintStackTrace(OS);
 
-  if (m_options.Test(LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION) &&
+  if (options.Test(LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION) &&
       (!file.empty() || !function.empty())) {
     file = llvm::sys::path::filename(file).take_front(40);
     function = function.take_front(40);
@@ -325,7 +310,8 @@
   if (!stream_sp)
     return;
 
-  if (m_options.Test(LLDB_LOG_OPTION_THREADSAFE)) {
+  Flags options = GetOptions();
+  if (options.Test(LLDB_LOG_OPTION_THREADSAFE)) {
     static std::recursive_mutex g_LogThreadedMutex;
     std::lock_guard<std::recursive_mutex> guard(g_LogThreadedMutex);
     *stream_sp << message;
Index: include/lldb/Utility/Log.h
===================================================================
--- include/lldb/Utility/Log.h
+++ include/lldb/Utility/Log.h
@@ -66,16 +66,20 @@
           default_flags(default_flags) {}
 
     // This function is safe to call at any time
-    // FIXME: Not true yet, mask access is not atomic
+    // If the channel is disabled after (or concurrently with) this function
+    // returning a non-null Log pointer, it is still safe to attempt to write to
+    // the Log object -- the output will be discarded.
     Log *GetLogIfAll(uint32_t mask) {
       Log *log = log_ptr.load(std::memory_order_acquire);
       if (log && log->GetMask().AllSet(mask))
         return log;
       return nullptr;
     }
 
     // This function is safe to call at any time
-    // FIXME: Not true yet, mask access is not atomic
+    // If the channel is disabled after (or concurrently with) this function
+    // returning a non-null Log pointer, it is still safe to attempt to write to
+    // the Log object -- the output will be discarded.
     Log *GetLogIfAny(uint32_t mask) {
       Log *log = log_ptr.load(std::memory_order_acquire);
       if (log && log->GetMask().AnySet(mask))
@@ -115,12 +119,13 @@
 
   //------------------------------------------------------------------
   // Member functions
+  //
+  // These functions are safe to call at any time you have a Log* obtained from
+  // the Channel class. If logging is disabled between you obtaining the Log
+  // object and writing to it, the output will be silently discarded.
   //------------------------------------------------------------------
-  Log();
-
-  Log(const std::shared_ptr<llvm::raw_ostream> &stream_sp);
-
-  ~Log();
+  Log() = default;
+  ~Log() = default;
 
   void PutCString(const char *cstr);
   void PutString(llvm::StringRef str);
@@ -136,26 +141,33 @@
 
   void VAPrintf(const char *format, va_list args);
 
-  void LogIf(uint32_t mask, const char *fmt, ...)
-      __attribute__((format(printf, 3, 4)));
-
   void Error(const char *fmt, ...) __attribute__((format(printf, 2, 3)));
 
   void VAError(const char *format, va_list args);
 
   void Verbose(const char *fmt, ...) __attribute__((format(printf, 2, 3)));
 
   void Warning(const char *fmt, ...) __attribute__((format(printf, 2, 3)));
 
-  Flags &GetOptions();
+  const Flags GetOptions() const;
 
-  const Flags &GetOptions() const;
+  const Flags GetMask() const;
 
-  Flags &GetMask();
+  bool GetVerbose() const;
 
-  const Flags &GetMask() const;
+private:
+  llvm::sys::RWMutex m_stream_mutex; // Protects m_stream_sp
+  std::shared_ptr<llvm::raw_ostream> m_stream_sp;
 
-  bool GetVerbose() const;
+  std::atomic<uint32_t> m_options{0};
+  std::atomic<uint32_t> m_mask{0};
+
+  void WriteHeader(llvm::raw_ostream &OS, llvm::StringRef file,
+                   llvm::StringRef function);
+  void WriteMessage(const std::string &message);
+
+  void Format(llvm::StringRef file, llvm::StringRef function,
+              const llvm::formatv_object_base &payload);
 
   void SetStream(const std::shared_ptr<llvm::raw_ostream> &stream_sp) {
     llvm::sys::ScopedWriter lock(m_stream_mutex);
@@ -167,25 +179,7 @@
     return m_stream_sp;
   }
 
-protected:
-  //------------------------------------------------------------------
-  // Member variables
-  //------------------------------------------------------------------
-  llvm::sys::RWMutex m_stream_mutex; // Protects m_stream_sp
-  std::shared_ptr<llvm::raw_ostream> m_stream_sp;
-
-  Flags m_options;
-  Flags m_mask_bits;
-
-private:
   DISALLOW_COPY_AND_ASSIGN(Log);
-
-  void WriteHeader(llvm::raw_ostream &OS, llvm::StringRef file,
-                   llvm::StringRef function);
-  void WriteMessage(const std::string &message);
-
-  void Format(llvm::StringRef file, llvm::StringRef function,
-              const llvm::formatv_object_base &payload);
 };
 
 } // namespace lldb_private
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to