From 9881ea5969ecda1867e39345a32c2baa45d21d25 Mon Sep 17 00:00:00 2001 From: Marco Date: Tue, 23 Feb 2021 05:17:25 +0800 Subject: [PATCH] Fix race conditions in LOG_EVERY_N (#492) * Use CMakeLists.txt to check the availability of std::atomic<> * Declare the static integer counters as std::atomic instead of int if C++11's atomic is available * implement fallback mechanism for pre C++11 compilers * src/glog/logging.h.in: fix wrong atomic increment * src/glog/logging.h.in: make modulo operations atomic --- CMakeLists.txt | 6 +++ src/config.h.cmake.in | 3 ++ src/glog/logging.h.in | 101 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 106 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7649b2b..e4c9341 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -223,6 +223,12 @@ std::aligned_storage::type data; int main() { } " HAVE_ALIGNED_STORAGE) +check_cxx_source_compiles (" +#include +std::atomic i; +int main() { } +" HAVE_CXX11_ATOMIC) + if (WITH_TLS) # Cygwin does not support the thread attribute. Don't bother. if (HAVE_GCC_TLS) diff --git a/src/config.h.cmake.in b/src/config.h.cmake.in index 4f014a1..d309993 100644 --- a/src/config.h.cmake.in +++ b/src/config.h.cmake.in @@ -198,6 +198,9 @@ /* Check whether aligned_storage and alignof present */ #cmakedefine HAVE_ALIGNED_STORAGE ${HAVE_ALIGNED_STORAGE} +/* Check whether C++11 atomic is available */ +#cmakedefine HAVE_CXX11_ATOMIC ${HAVE_CXX11_ATOMIC} + /* Version number of package */ #cmakedefine VERSION diff --git a/src/glog/logging.h.in b/src/glog/logging.h.in index 357390c..1f68183 100644 --- a/src/glog/logging.h.in +++ b/src/glog/logging.h.in @@ -86,6 +86,12 @@ #include #endif +#if HAVE_CXX11_ATOMIC +#include +#elif defined(OS_WINDOWS) +#include +#endif + @ac_google_start_namespace@ #if @ac_cv_have_uint16_t@ // the C99 format @@ -905,6 +911,7 @@ PLOG_IF(FATAL, GOOGLE_PREDICT_BRANCH_NOT_TAKEN((invocation) == -1)) \ #define LOG_OCCURRENCES LOG_EVERY_N_VARNAME(occurrences_, __LINE__) #define LOG_OCCURRENCES_MOD_N LOG_EVERY_N_VARNAME(occurrences_mod_n_, __LINE__) + #if defined(__has_feature) #define _GLOG_HAS_FEATURE(...) __has_feature(__VA_ARGS__) #else @@ -936,8 +943,9 @@ extern "C" void AnnotateBenignRaceSized( namespace google { #endif +#ifdef HAVE_CXX11_ATOMIC #define SOME_KIND_OF_LOG_EVERY_N(severity, n, what_to_do) \ - static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + static std::atomic LOG_OCCURRENCES(0), LOG_OCCURRENCES_MOD_N(0); \ _GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \ _GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES_MOD_N, sizeof(int), "")); \ ++LOG_OCCURRENCES; \ @@ -948,7 +956,7 @@ namespace google { &what_to_do).stream() #define SOME_KIND_OF_LOG_IF_EVERY_N(severity, condition, n, what_to_do) \ - static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + static std::atomic LOG_OCCURRENCES(0), LOG_OCCURRENCES_MOD_N(0); \ _GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \ _GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES_MOD_N, sizeof(int), "")); \ ++LOG_OCCURRENCES; \ @@ -959,7 +967,7 @@ namespace google { &what_to_do).stream() #define SOME_KIND_OF_PLOG_EVERY_N(severity, n, what_to_do) \ - static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + static std::atomic LOG_OCCURRENCES(0), LOG_OCCURRENCES_MOD_N(0); \ _GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \ _GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES_MOD_N, sizeof(int), "")); \ ++LOG_OCCURRENCES; \ @@ -970,7 +978,7 @@ namespace google { &what_to_do).stream() #define SOME_KIND_OF_LOG_FIRST_N(severity, n, what_to_do) \ - static int LOG_OCCURRENCES = 0; \ + static std::atomic LOG_OCCURRENCES(0); \ _GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \ if (LOG_OCCURRENCES <= n) \ ++LOG_OCCURRENCES; \ @@ -979,6 +987,91 @@ namespace google { __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ &what_to_do).stream() +#elif defined(OS_WINDOWS) + +#define SOME_KIND_OF_LOG_EVERY_N(severity, n, what_to_do) \ + static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + InterlockedIncrement(&LOG_OCCURRENCES); \ + if (InterlockedIncrement(&LOG_OCCURRENCES_MOD_N) > n) \ + InterlockedExchangeSubtract(&LOG_OCCURRENCES_MOD_N, n); \ + if (LOG_OCCURRENCES_MOD_N == 1) \ + @ac_google_namespace@::LogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() + +#define SOME_KIND_OF_LOG_IF_EVERY_N(severity, condition, n, what_to_do) \ + static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + InterlockedIncrement(&LOG_OCCURRENCES); \ + if (condition && \ + (InterlockedIncrement(&LOG_OCCURRENCES_MOD_N) || true) && \ + ((LOG_OCCURRENCES_MOD_N >= n && InterlockedExchangeAdd(&LOG_OCCURRENCES_MOD_N, n)) || true) && \ + LOG_OCCURRENCES_MOD_N == (1 % n)) \ + @ac_google_namespace@::LogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() + +#define SOME_KIND_OF_PLOG_EVERY_N(severity, n, what_to_do) \ + static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + InterlockedIncrement(&LOG_OCCURRENCES); \ + if (InterlockedIncrement(&LOG_OCCURRENCES_MOD_N) > n) \ + InterlockedExchangeSubtract(&LOG_OCCURRENCES_MOD_N, n); \ + if (LOG_OCCURRENCES_MOD_N == 1) \ + @ac_google_namespace@::ErrnoLogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() + +#define SOME_KIND_OF_LOG_FIRST_N(severity, n, what_to_do) \ + static int LOG_OCCURRENCES = 0; \ + if (LOG_OCCURRENCES <= n) \ + InterlockedIncrement(&LOG_OCCURRENCES); \ + if (LOG_OCCURRENCES <= n) \ + @ac_google_namespace@::LogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() + +#else + +#define SOME_KIND_OF_LOG_EVERY_N(severity, n, what_to_do) \ + static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + __sync_add_and_fetch(&LOG_OCCURRENCES, 1); \ + if (__sync_add_and_fetch(&LOG_OCCURRENCES_MOD_N, 1) > n) \ + __sync_sub_and_fetch(&LOG_OCCURRENCES_MOD_N, n); \ + if (LOG_OCCURRENCES_MOD_N == 1) \ + @ac_google_namespace@::LogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() + +#define SOME_KIND_OF_LOG_IF_EVERY_N(severity, condition, n, what_to_do) \ + static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + __sync_add_and_fetch(&LOG_OCCURRENCES, 1); \ + if (condition && \ + (__sync_add_and_fetch(&LOG_OCCURRENCES_MOD_N, 1) || true) && \ + ((LOG_OCCURRENCES_MOD_N >= n && __sync_sub_and_fetch(&LOG_OCCURRENCES_MOD_N, n)) || true) && \ + LOG_OCCURRENCES_MOD_N == (1 % n)) \ + @ac_google_namespace@::LogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() + +#define SOME_KIND_OF_PLOG_EVERY_N(severity, n, what_to_do) \ + static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \ + __sync_add_and_fetch(&LOG_OCCURRENCES, 1); \ + if (__sync_add_and_fetch(&LOG_OCCURRENCES_MOD_N, 1) > n) \ + __sync_sub_and_fetch(&LOG_OCCURRENCES_MOD_N, n); \ + if (LOG_OCCURRENCES_MOD_N == 1) \ + @ac_google_namespace@::ErrnoLogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() + +#define SOME_KIND_OF_LOG_FIRST_N(severity, n, what_to_do) \ + static int LOG_OCCURRENCES = 0; \ + if (LOG_OCCURRENCES <= n) \ + __sync_add_and_fetch(&LOG_OCCURRENCES, 1); \ + if (LOG_OCCURRENCES <= n) \ + @ac_google_namespace@::LogMessage( \ + __FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \ + &what_to_do).stream() +#endif + namespace glog_internal_namespace_ { template struct CompileAssert {