From b3be22c5482b9f1df541b1f83dfbbf69a51a5271 Mon Sep 17 00:00:00 2001 From: devtejasx Date: Fri, 31 Jul 2026 04:42:10 +0530 Subject: [PATCH] Replace offsetof cache-line check with a well-defined runtime check (#1877) The cache-line static_assert used offsetof(State, skipped_). State is not a standard-layout type, so offsetof() on it is only conditionally-supported and is diagnosed by -Winvalid-offsetof on clang and by the equivalent diagnostics on ICC, NVCC and NVHPC. Keeping it quiet required a stack of vendor-specific pragmas, and it still breaks -Werror builds. State cannot be made standard-layout to fix this properly: that requires every non-static data member to be of standard-layout type, and `counters` is a std::map, which is not. So there is no conforming way to obtain this offset at compile time. Compute the offset from the live object instead, which is well defined, and drop all of the offsetof-suppression pragmas. Add BM_CHECK_ALWAYS so the check stays enabled under NDEBUG rather than becoming debug-only: a State is constructed once per benchmark instance and thread, never inside the iteration loop, so an unconditional compare costs nothing measurable. --- src/benchmark.cc | 55 +++++++++++++++++------------------------------- src/check.h | 13 ++++++++---- 2 files changed, 28 insertions(+), 40 deletions(-) diff --git a/src/benchmark.cc b/src/benchmark.cc index 91280295e8..fa7eb7485b 100644 --- a/src/benchmark.cc +++ b/src/benchmark.cc @@ -222,42 +222,25 @@ State::State(std::string name, IterationCount max_iters, } } - // Note: The use of offsetof below is technically undefined until C++17 - // because State is not a standard layout type. However, all compilers - // currently provide well-defined behavior as an extension (which is - // demonstrated since constexpr evaluation must diagnose all undefined - // behavior). However, GCC and Clang also warn about this use of offsetof, - // which must be suppressed. -#if defined(__INTEL_COMPILER) -#pragma warning push -#pragma warning(disable : 1875) -#elif defined(__GNUC__) || defined(__clang__) -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Winvalid-offsetof" -#endif -#if defined(__NVCC__) -#pragma nv_diagnostic push -#pragma nv_diag_suppress 1427 -#endif -#if defined(__NVCOMPILER) -#pragma diagnostic push -#pragma diag_suppress offset_in_non_POD_nonstandard -#endif - // Offset tests to ensure commonly accessed data is on the first cache line. - const int cache_line_size = 64; - static_assert( - offsetof(State, skipped_) <= (cache_line_size - sizeof(skipped_)), ""); -#if defined(__INTEL_COMPILER) -#pragma warning pop -#elif defined(__GNUC__) || defined(__clang__) -#pragma GCC diagnostic pop -#endif -#if defined(__NVCC__) -#pragma nv_diagnostic pop -#endif -#if defined(__NVCOMPILER) -#pragma diagnostic pop -#endif + // Ensure commonly accessed data is on the first cache line. + // + // offsetof() cannot be used for this: State is not a standard-layout type, so + // offsetof() on it is only conditionally-supported and is diagnosed by + // -Winvalid-offsetof (and its equivalents on ICC/NVCC/NVHPC), which had to be + // silenced with a stack of vendor-specific pragmas. State also cannot be made + // standard-layout -- that would require every non-static data member to be of + // standard-layout type, and `counters` is a std::map, which is not. + // + // So compute the offset from the live object, which is well defined. Unlike + // BM_CHECK this stays enabled under NDEBUG: a State is constructed once per + // benchmark instance and thread, never inside the iteration loop, so the + // check costs nothing measurable and the invariant holds in every build. + BM_CHECK_ALWAYS(reinterpret_cast(&skipped_) - + reinterpret_cast(this) <= + /*cache_line_size=*/64 - + static_cast(sizeof(skipped_))) + << "the commonly accessed members of State must fit in the first cache " + "line"; } void State::PauseTiming() { diff --git a/src/check.h b/src/check.h index aa8c78c92f..b6fc22ff0b 100644 --- a/src/check.h +++ b/src/check.h @@ -79,15 +79,20 @@ class CheckHandler { } // end namespace internal } // end namespace benchmark -// The BM_CHECK macro returns a std::ostream object that can have extra -// information written to it. -#ifndef NDEBUG -#define BM_CHECK(b) \ +// Like BM_CHECK, but enabled in every build configuration. Reserved for +// invariants that are cheap enough to verify unconditionally and that must not +// be violated silently in a release build. +#define BM_CHECK_ALWAYS(b) \ (b ? ::benchmark::internal::GetNullLogInstance() \ : ::benchmark::internal::CheckHandler( \ std::string_view(#b), std::string_view(__FILE__), \ std::string_view(__func__), __LINE__) \ .GetLog()) + +// The BM_CHECK macro returns a std::ostream object that can have extra +// information written to it. +#ifndef NDEBUG +#define BM_CHECK(b) BM_CHECK_ALWAYS(b) #else #define BM_CHECK(b) ::benchmark::internal::GetNullLogInstance() #endif