Replace offsetof cache-line check with a well-defined runtime check (#1877)

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<const char*>(&skipped_) -
+                      reinterpret_cast<const char*>(this) <=
+                  /*cache_line_size=*/64 -
+                      static_cast<std::ptrdiff_t>(sizeof(skipped_)))
+      << "the commonly accessed members of State must fit in the first cache "
+         "line";
 }
 
 void State::PauseTiming() {

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