From cafba580ec4faeeb8e61a97f91819fd0db3205a0 Mon Sep 17 00:00:00 2001 From: Sergiu Deitsch Date: Thu, 4 Jan 2024 01:18:36 +0100 Subject: [PATCH] feat: use standard atomics and call_once (#1026) --- CMakeLists.txt | 7 ---- src/config.h.cmake.in | 3 -- src/raw_logging.cc | 8 ++--- src/signalhandler.cc | 69 ++++++++++++++++----------------------- src/utilities.cc | 15 +++++---- src/utilities.h | 28 ---------------- src/utilities_unittest.cc | 7 ---- 7 files changed, 40 insertions(+), 97 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2124557ee..576e7122e 100644 --- a/CMakeLists.txt +++ b/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. diff --git a/src/config.h.cmake.in b/src/config.h.cmake.in index 430959539..b02bc0dbe 100644 --- a/src/config.h.cmake.in +++ b/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 diff --git a/src/raw_logging.cc b/src/raw_logging.cc index 49b959131..75bf0541d 100644 --- a/src/raw_logging.cc +++ b/src/raw_logging.cc @@ -31,10 +31,10 @@ // // logging_unittest.cc covers the functionality herein -#include #include #include #include +#include #include #include #include @@ -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() } } diff --git a/src/signalhandler.cc b/src/signalhandler.cc index 2aa1dad68..2e15df21d 100644 --- a/src/signalhandler.cc +++ b/src/signalhandler.cc @@ -34,6 +34,7 @@ #include #include #include +#include #include #include @@ -266,50 +267,19 @@ void InvokeDefaultSignalHandler(int signal_number) { #endif } -// This variable is used for protecting FailureSignalHandler() from -// dumping stuff while another thread is doing it. Our policy is to let -// the first thread dump stuff and let other threads wait. +// This variable is used for protecting FailureSignalHandler() from dumping +// stuff while another thread is doing it. Our policy is to let the first +// 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 -// 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) +static void HandleSignal(int signal_number +#if !defined(GLOG_OS_WINDOWS) + , + 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(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_ { diff --git a/src/utilities.cc b/src/utilities.cc index 00eecbc12..b736d5285 100644 --- a/src/utilities.cc +++ b/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 #include #include #include +#include +#include "base/googleinit.h" #include "config.h" + #ifdef HAVE_SYS_TIME_H # include #endif -#include #if defined(HAVE_SYSCALL_H) # include // for syscall() #elif defined(HAVE_SYS_SYSCALL_H) @@ -58,8 +61,6 @@ # include #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 g_reason{nullptr}; void SetCrashReason(const CrashReason* r) { - sync_val_compare_and_swap(&g_reason, reinterpret_cast(0), - r); + const CrashReason* expected = nullptr; + g_reason.compare_exchange_strong(expected, r); } void InitGoogleLoggingUtilities(const char* argv0) { diff --git a/src/utilities.h b/src/utilities.h index 87bdf9b09..50b4bece3 100644 --- a/src/utilities.h +++ b/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 -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 { diff --git a/src/utilities_unittest.cc b/src/utilities_unittest.cc index e095d1f19..18184795a 100644 --- a/src/utilities_unittest.cc +++ b/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"), ""); }