[logging] Make Log::IsEnabled() atomic

With concurrent allocation background threads invoke Log::IsEnabled()
as well. Fix data race here by making is_enabled_ atomic, such that
IsEnabled() remains cheap.

After locking the mutex in MessageBuilder, IsEnabled() needs to be
checked again in case an old value was read. Otherwise we might log
even though logging was already disabled on another thread.

The other direction where a log message isn't logged is deemed
acceptable.

Bug: v8:10315
Change-Id: I32c9dd2e9879fbdb4ca94e080a16ddd875de7c30
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2362948
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69495}

src/logging/log-utils.cc

@@ -4,6 +4,10 @@
 
 #include "src/logging/log-utils.h"
 
+#include <atomic>
+#include <memory>
+
+#include "src/base/platform/mutex.h"
 #include "src/base/platform/platform.h"
 #include "src/common/assert-scope.h"
 #include "src/objects/objects-inl.h"
@@ -33,9 +37,9 @@ FILE* Log::CreateOutputHandle(const char* file_name) {
 }
 
 Log::Log(Logger* logger, const char* file_name)
-    : is_stopped_(false),
-      output_handle_(Log::CreateOutputHandle(file_name)),
+    : output_handle_(Log::CreateOutputHandle(file_name)),
       os_(output_handle_ == nullptr ? stdout : output_handle_),
+      is_enabled_(output_handle_ != nullptr),
       format_buffer_(NewArray<char>(kMessageBufferSize)),
       logger_(logger) {
   // --log-all enables all the log flags.
@@ -64,7 +68,19 @@ Log::Log(Logger* logger, const char* file_name)
   msg.WriteToLogFile();
 }
 
+std::unique_ptr<Log::MessageBuilder> Log::NewMessageBuilder() {
+  std::unique_ptr<Log::MessageBuilder> result(new Log::MessageBuilder(this));
+
+  // Need to check here whether log is still enabled while the mutex is locked
+  // by Log::MessageBuilder. In case IsEnabled() is checked before locking the
+  // mutex we could still read an old value.
+  if (!IsEnabled()) return {};
+
+  return result;
+}
+
 FILE* Log::Close() {
+  base::MutexGuard guard(&mutex_);
   FILE* result = nullptr;
   if (output_handle_ != nullptr) {
     if (strcmp(FLAG_logfile, kLogToTemporaryFile) != 0) {
@@ -77,7 +93,7 @@ FILE* Log::Close() {
 
   format_buffer_.reset();
 
-  is_stopped_ = false;
+  is_enabled_.store(false, std::memory_order_relaxed);
   return result;
 }
 
@@ -206,7 +222,10 @@ void Log::MessageBuilder::AppendRawFormatString(const char* format, ...) {
 
 void Log::MessageBuilder::AppendRawCharacter(char c) { log_->os_ << c; }
 
-void Log::MessageBuilder::WriteToLogFile() { log_->os_ << std::endl; }
+void Log::MessageBuilder::WriteToLogFile() {
+  DCHECK(log_->IsEnabled());
+  log_->os_ << std::endl;
+}
 
 template <>
 Log::MessageBuilder& Log::MessageBuilder::operator<<<const char*>(

src/logging/log-utils.h

@@ -7,7 +7,9 @@
 
 #include <stdio.h>
 
+#include <atomic>
 #include <cstdarg>
+#include <memory>
 
 #include "src/base/compiler-specific.h"
 #include "src/base/optional.h"
@@ -29,8 +31,6 @@ enum class LogSeparator { kSeparator };
 class Log {
  public:
   Log(Logger* log, const char* log_file_name);
-  // Disables logging, but preserves acquired resources.
-  void stop() { is_stopped_ = true; }
 
   static bool InitLogAtStart() {
     return FLAG_log || FLAG_log_api || FLAG_log_code || FLAG_log_handles ||
@@ -47,7 +47,7 @@ class Log {
   FILE* Close();
 
   // Returns whether logging is enabled.
-  bool IsEnabled() { return !is_stopped_ && output_handle_ != nullptr; }
+  bool IsEnabled() { return is_enabled_.load(std::memory_order_relaxed); }
 
   // Size of buffer used for formatting log messages.
   static const int kMessageBufferSize = 2048;
@@ -61,9 +61,6 @@ class Log {
   // and then appends them to the static buffer in Log.
   class MessageBuilder {
    public:
-    // Create a message builder starting from position 0.
-    // This acquires the mutex in the log as well.
-    explicit MessageBuilder(Log* log);
     ~MessageBuilder() = default;
 
     void AppendString(String str,
@@ -87,6 +84,10 @@ class Log {
     void WriteToLogFile();
 
    private:
+    // Create a message builder starting from position 0.
+    // This acquires the mutex in the log as well.
+    explicit MessageBuilder(Log* log);
+
     // Prints the format string into |log_->format_buffer_|. Returns the length
     // of the result, or kMessageBufferSize if it was truncated.
     int PRINTF_FORMAT(2, 0)
@@ -99,8 +100,14 @@ class Log {
 
     Log* log_;
     base::MutexGuard lock_guard_;
+
+    friend class Log;
   };
 
+  // Use this method to create an instance of Log::MessageBuilder. This method
+  // will return null if logging is disabled.
+  std::unique_ptr<Log::MessageBuilder> NewMessageBuilder();
+
  private:
   static FILE* CreateOutputHandle(const char* file_name);
 
@@ -112,14 +119,14 @@ class Log {
     return length;
   }
 
-  // Whether logging is stopped (e.g. due to insufficient resources).
-  bool is_stopped_;
-
   // When logging is active output_handle_ is used to store a pointer to log
   // destination.  mutex_ should be acquired before using output_handle_.
   FILE* output_handle_;
   OFStream os_;
 
+  // Stores whether logging is enabled.
+  std::atomic<bool> is_enabled_;
+
   // mutex_ is a Mutex used for enforcing exclusive
   // access to the formatting buffer and the log file or log memory buffer.
   base::Mutex mutex_;