feat: use standard atomics and call_once (#1026)

CMakeLists.txt

@@ -163,13 +163,6 @@ set (CMAKE_REQUIRED_LIBRARIES dbghelp)
 check_cxx_symbol_exists (UnDecorateSymbolName "windows.h;dbghelp.h" HAVE_DBGHELP)
 cmake_pop_check_state ()
 
-check_cxx_source_compiles ("
-int main(void)
-{
-  int a; if (__sync_val_compare_and_swap(&a, 0, 1)) return 1; return 0;
-}
-" HAVE___SYNC_VAL_COMPARE_AND_SWAP)
-
 if (WITH_FUZZING STREQUAL none)
   # Disable compiler demangler if fuzzing is active; we only want to use the
   # glog demangler then.

src/config.h.cmake.in

@@ -82,9 +82,6 @@
 /* define if you have libunwind */
 #cmakedefine HAVE_LIBUNWIND
 
-/* define if your compiler has __sync_val_compare_and_swap */
-#cmakedefine HAVE___SYNC_VAL_COMPARE_AND_SWAP
-
 /* define if symbolize support is available */
 #cmakedefine HAVE_SYMBOLIZE
 

src/raw_logging.cc

@@ -31,10 +31,10 @@
 //
 // logging_unittest.cc covers the functionality herein
 
-#include <cerrno>
 #include <cstdarg>
 #include <cstdio>
 #include <iomanip>
+#include <mutex>
 #include <ostream>
 #include <streambuf>
 #include <thread>
@@ -124,7 +124,7 @@ inline static bool VADoRawLog(char** buf, size_t* size, const char* format,
 }
 
 static const int kLogBufSize = 3000;
-static bool crashed = false;
+static std::once_flag crashed;
 static CrashReason crash_reason;
 static char crash_buf[kLogBufSize + 1] = {0};  // Will end in '\0'
 
@@ -195,7 +195,7 @@ void RawLog__(LogSeverity severity, const char* file, int line,
   // We write just once to avoid races with other invocations of RawLog__.
   safe_write(STDERR_FILENO, buffer, strlen(buffer));
   if (severity == GLOG_FATAL) {
-    if (!sync_val_compare_and_swap(&crashed, false, true)) {
+    std::call_once(crashed, [file, line, msg_start, msg_size] {
       crash_reason.filename = file;
       crash_reason.line_number = line;
       memcpy(crash_buf, msg_start, msg_size);  // Don't include prefix
@@ -207,7 +207,7 @@ void RawLog__(LogSeverity severity, const char* file, int line,
       crash_reason.depth = 0;
 #endif
       SetCrashReason(&crash_reason);
-    }
+    });
     LogMessage::Fail();  // abort()
   }
 }

src/signalhandler.cc

@@ -34,6 +34,7 @@
 #include <algorithm>
 #include <csignal>
 #include <ctime>
+#include <mutex>
 #include <sstream>
 #include <thread>
 
@@ -266,50 +267,19 @@ void InvokeDefaultSignalHandler(int signal_number) {
 #endif
 }
 
-// This variable is used for protecting FailureSignalHandler() from
+// This variable is used for protecting FailureSignalHandler() from dumping
-// dumping stuff while another thread is doing it.  Our policy is to let
+// stuff while another thread is doing it.  Our policy is to let the first
-// the first thread dump stuff and let other threads wait.
+// thread dump stuff and let other threads do nothing.
 // See also comments in FailureSignalHandler().
-static std::thread::id* g_entered_thread_id_pointer = nullptr;
+static std::once_flag signaled;
 
-// Dumps signal and stack frame information, and invokes the default
+static void HandleSignal(int signal_number
-// signal handler once our job is done.
+#if !defined(GLOG_OS_WINDOWS)
-#if defined(GLOG_OS_WINDOWS)
+                         ,
-void FailureSignalHandler(int signal_number)
+                         siginfo_t* signal_info, void* ucontext
-#else
-void FailureSignalHandler(int signal_number, siginfo_t* signal_info,
-                          void* ucontext)
 #endif
-{
+) {
-  // First check if we've already entered the function.  We use an atomic
-  // compare and swap operation for platforms that support it.  For other
-  // platforms, we use a naive method that could lead to a subtle race.
 
-  std::thread::id my_thread_id = std::this_thread::get_id();
-  // NOTE: We could simply use std::thread::id rather than std::thread::id* for
-  // this, if std::thread::get_id() is guaranteed to return non-zero value for
-  // thread ids, but there is no such guarantee.  We need to distinguish if the
-  // old value (value returned from __sync_val_compare_and_swap) is
-  // different from the original value (in this case nullptr).
-  std::thread::id* old_thread_id_pointer =
-      glog_internal_namespace_::sync_val_compare_and_swap(
-          &g_entered_thread_id_pointer, static_cast<std::thread::id*>(nullptr),
-          &my_thread_id);
-  if (old_thread_id_pointer != nullptr) {
-    // We've already entered the signal handler.  What should we do?
-    if (my_thread_id == *g_entered_thread_id_pointer) {
-      // It looks the current thread is reentering the signal handler.
-      // Something must be going wrong (maybe we are reentering by another
-      // type of signal?).  Kill ourself by the default signal handler.
-      InvokeDefaultSignalHandler(signal_number);
-    }
-    // Another thread is dumping stuff.  Let's wait until that thread
-    // finishes the job and kills the process.
-    while (true) {
-      using namespace std::chrono_literals;
-      std::this_thread::sleep_for(1s);
-    }
-  }
   // This is the first time we enter the signal handler.  We are going to
   // do some interesting stuff from here.
   // TODO(satorux): We might want to set timeout here using alarm(), but
@@ -359,6 +329,23 @@ void FailureSignalHandler(int signal_number, siginfo_t* signal_info,
   InvokeDefaultSignalHandler(signal_number);
 }
 
+// Dumps signal and stack frame information, and invokes the default
+// signal handler once our job is done.
+#if defined(GLOG_OS_WINDOWS)
+void FailureSignalHandler(int signal_number)
+#else
+void FailureSignalHandler(int signal_number, siginfo_t* signal_info,
+                          void* ucontext)
+#endif
+{
+  std::call_once(signaled, &HandleSignal, signal_number
+#if !defined(GLOG_OS_WINDOWS)
+                 ,
+                 signal_info, ucontext
+#endif
+  );
+}
+
 }  // namespace
 
 namespace glog_internal_namespace_ {

src/utilities.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, Google Inc.
+// Copyright (c) 2024, Google Inc.
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
@@ -31,15 +31,18 @@
 
 #include "utilities.h"
 
+#include <atomic>
 #include <csignal>
 #include <cstdio>
 #include <cstdlib>
+#include <ctime>
 
+#include "base/googleinit.h"
 #include "config.h"
+
 #ifdef HAVE_SYS_TIME_H
 #  include <sys/time.h>
 #endif
-#include <ctime>
 #if defined(HAVE_SYSCALL_H)
 #  include <syscall.h>  // for syscall()
 #elif defined(HAVE_SYS_SYSCALL_H)
@@ -58,8 +61,6 @@
 #  include <android/log.h>
 #endif
 
-#include "base/googleinit.h"
-
 using std::string;
 
 namespace google {
@@ -254,11 +255,11 @@ void DumpStackTraceToString(string* stacktrace) {
 
 // We use an atomic operation to prevent problems with calling CrashReason
 // from inside the Mutex implementation (potentially through RAW_CHECK).
-static const CrashReason* g_reason = nullptr;
+static std::atomic<const CrashReason*> g_reason{nullptr};
 
 void SetCrashReason(const CrashReason* r) {
-  sync_val_compare_and_swap(&g_reason, reinterpret_cast<const CrashReason*>(0),
+  const CrashReason* expected = nullptr;
-                            r);
+  g_reason.compare_exchange_strong(expected, r);
 }
 
 void InitGoogleLoggingUtilities(const char* argv0) {

src/utilities.h

@@ -173,34 +173,6 @@ const std::string& MyUserName();
 // (Doesn't modify filepath, contrary to basename() in libgen.h.)
 const char* const_basename(const char* filepath);
 
-// Wrapper of __sync_val_compare_and_swap. If the GCC extension isn't
-// defined, we try the CPU specific logics (we only support x86 and
-// x86_64 for now) first, then use a naive implementation, which has a
-// race condition.
-template <typename T>
-inline T sync_val_compare_and_swap(T* ptr, T oldval, T newval) {
-#if defined(HAVE___SYNC_VAL_COMPARE_AND_SWAP)
-  return __sync_val_compare_and_swap(ptr, oldval, newval);
-#elif defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))
-  T ret;
-  __asm__ __volatile__("lock; cmpxchg %1, (%2);"
-                       : "=a"(ret)
-                       // GCC may produces %sil or %dil for
-                       // constraint "r", but some of apple's gas
-                       // doesn't know the 8 bit registers.
-                       // We use "q" to avoid these registers.
-                       : "q"(newval), "q"(ptr), "a"(oldval)
-                       : "memory", "cc");
-  return ret;
-#else
-  T ret = *ptr;
-  if (ret == oldval) {
-    *ptr = newval;
-  }
-  return ret;
-#endif
-}
-
 void DumpStackTraceToString(std::string* stacktrace);
 
 struct CrashReason {

src/utilities_unittest.cc

@@ -40,13 +40,6 @@ using namespace GFLAGS_NAMESPACE;
 
 using namespace google;
 
-TEST(utilities, sync_val_compare_and_swap) {
-  bool now_entering = false;
-  EXPECT_FALSE(sync_val_compare_and_swap(&now_entering, false, true));
-  EXPECT_TRUE(sync_val_compare_and_swap(&now_entering, false, true));
-  EXPECT_TRUE(sync_val_compare_and_swap(&now_entering, false, true));
-}
-
 TEST(utilities, InitGoogleLoggingDeathTest) {
   ASSERT_DEATH(InitGoogleLogging("foobar"), "");
 }