Repository navigation
Make the cached hardware support atomic in the C++ build too - #949
Closed
moshap-firebolt wants to merge 1 commit into
Closed
moshap-firebolt wants to merge 1 commit into
moshap-firebolt wants to merge 1 commit into
Conversation
croaring_hardware_support() caches its result in a function-local static that every thread calling it for the first time writes. It is _Atomic only when compiled as C, so building the sources as C++ leaves a data race on a plain int, which ThreadSanitizer reports. Use std::atomic<int> there, as the refcounts already do. The read still compiles to a plain load.
Member
|
Will go with #950 thanks, and we credit you. |
lemire
added a commit
that referenced
this pull request
Nov 3, 2026
* Make the cached hardware support atomic in the C++ build too croaring_hardware_support() caches the detected CPU features in a function-local static int that was _Atomic only for the C11 atomic implementation, leaving a data race when the sources are compiled as C++. Use std::atomic<int> in C++, initialized with braces so that it compiles before C++17 (std::atomic has no copy constructor). Based on #949. Co-authored-by: moshap <moshap@firebolt.io> * Apply clang-format --------- Co-authored-by: moshap <moshap@firebolt.io>
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
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.
croaring_hardware_support()caches the detected CPU features in a function-localstatic int, which every thread calling it for the first time reads and writes. It is declared_Atomiconly whenCROARING_ATOMIC_IMPL == CROARING_ATOMIC_IMPL_C, so a build that compiles the sources as C++ (whereCROARING_ATOMIC_IMPL_CPPis selected) leaves a data race on a plainint. ThreadSanitizer reports it when several threads reach roaring code for the first time together; we hit it in a server running under TSan.This adds a
croaring_atomic_inttypedef next to the other atomic selections:std::atomic<int>for the C++ implementation,_Atomic intfor C as before, and a plainintwhere no atomics are available. All three copies of the cache (x64 AVX2, x64 generic, arm64) use it.The read still compiles to a plain
movon x86-64 (and a load on arm64); only the one-time store becomes anxchg.Testing: a small TSan program calling
croaring_hardware_support()from 8 threads, withisadetection.ccompiled as C++, reports the race on 5 of 5 runs before this change and on none after, with and without-mavx2. The file still compiles as C11, and for aarch64 as both C11 and C++17.