Replace offsetof cache-line check with a well-defined runtime check (#1877) - #2264
Replace offsetof cache-line check with a well-defined runtime check (#1877)#2264devtejasx wants to merge 1 commit into
Conversation
|
I just checked, and i don't believe that diagnostic fires any more on clang-22 on linux. |
fa38102 to
b3be22c
Compare
|
@LebedevRI It does still fire — it just can't be seen on a plain build of Since you asked specifically about clang-22, here it is reduced to #include <cstddef>
class State {
public:
const long max_iterations;
private:
long total_iterations_;
int skipped_;
public:
State() : max_iterations(0), total_iterations_(0), skipped_(0) {
const int cache_line_size = 64;
static_assert(offsetof(State, skipped_) <= (cache_line_size - sizeof(skipped_)), "");
}
};
State s;With That matches clang's current @dmah42 On your objection to #2251 — that moving the check to runtime is a regression because it becomes debug-only — that was fair, and I've fixed it rather than repeating it. Two things: 1. There is no conforming compile-time option. You asked whether there was a better one; I went looking for it, and the answer is no. Fixing the access-control mix isn't enough; the member types themselves rule it out, and the standard guarantees nothing about the layout of any container. A 2. The check is no longer debug-only. This adds Verified in a Release/ Restored to 64, Release and Debug both build clean and the test suite is unchanged from |
b3be22c to
d33268c
Compare
| #if defined(__NVCOMPILER) | ||
| #pragma diagnostic pop | ||
| #endif | ||
| // Ensure commonly accessed data is on the first cache line. |
There was a problem hiding this comment.
we don't need a comment describing what we used to do, only what we do now.
|
so what was wrong with the original version then? it worked, right? |
d33268c to
40101c7
Compare
|
Functionally, yes — the pragma stack on But "compiles clean today" isn't the same as "not broken." It's still moving: in my last comment I posted Rebased onto current |
State is not a standard-layout type, so offsetof(State, skipped_) is only conditionally-supported. Compilers diagnose it, and the static_assert was kept quiet by a stack of vendor pragmas for GCC, clang, ICC, NVCC and NVHPC. That stack does not cover every compiler: google#1877 is clang-18/19 on Windows reporting it anyway. Measure the offset on the object being constructed instead. That is well defined, needs no pragmas, and expresses the same invariant. The check uses a new BM_CHECK_ALWAYS, which is BM_CHECK without the NDEBUG opt-out, so it still holds in a release build. A State is constructed once per benchmark instance and thread, never inside the iteration loop. Verified: with the bound lowered to 8 bytes, a release build fails at run time with src/benchmark.cc:231: State: Check `... <= 8 - ...' failed. the commonly accessed members of State must fit in the first cache line and src/benchmark.cc compiles clean under -Winvalid-offsetof -Wall -Wextra -Werror with both gcc 15 and clang 20.
40101c7 to
87db41a
Compare
|
Comment trimmed, it now only says what the code does. Also worth stating for the record, since #2251 was rejected for making the check debug-only: this version is not. A On "what was wrong with the original version": nothing that shows up today. It compiles clean on current |
Fixes #1877.
The cache-line
static_assertusedoffsetof(State, skipped_).Stateis nota standard-layout type (it has non-static data members with differing access
control), so
offsetof()on it is undefined behavior and is diagnosed by-Winvalid-offsetofon clang (and equivalents on ICC/NVCC/NVHPC). Today this isonly kept quiet by a large stack of compiler-specific
#pragmas, and it stillbreaks
-Werrorbuilds on newer clang (clang-18/19 in the report; also seenwith Apple clang 21).
This replaces the
offsetofcompile-time check with a well-defined runtimecheck that computes the offset from the live object (
&skipped_ - this), guardedby
BM_CHECK. The invariant is therefore still enforced in debug/CI builds andcompiles to nothing under
NDEBUG, and all of theoffsetof-suppression pragmasare removed.
This follows up on @dmah42's request in the issue for a way to "capture the
spirit of the check in another way".
Verification
clang -Winvalid-offsetof -Werrorflags the original pattern andcompiles cleanly after the change (no pragmas needed).
Per AGENTS.md: AI-assisted — the patch was drafted with AI assistance
(Claude) and then reviewed, tested, and understood by me. I take full
responsibility for it.