Replace offsetof cache-line check with a well-defined runtime check (#1877) - #2264
Open
devtejasx wants to merge 1 commit into
Open
Replace offsetof cache-line check with a well-defined runtime check (#1877)#2264devtejasx wants to merge 1 commit into
devtejasx wants to merge 1 commit into
Conversation
…oogle#1877) The static_assert used offsetof(State, skipped_) to verify that commonly accessed data stays on the first cache line. State is not a 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-offsetof on clang (and equivalents elsewhere). This forced a stack of compiler-specific pragmas (GCC/Clang, ICC, NVCC, NVHPC) just to silence the warning, and it still broke -Werror builds on newer clang. Compute the offset from the live object instead (address of the member minus address of the object), which is well defined. The check now runs at construction time via BM_CHECK, so it is active in debug/CI builds and is a no-op under NDEBUG. This removes all the offsetof-suppression pragmas. AI-assisted: patch drafted with AI assistance (Claude); reviewed, tested, and understood by me. I take full responsibility for it.
Collaborator
|
I just checked, and i don't believe that diagnostic fires any more on clang-22 on linux. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.