Repository navigation
Make the cached hardware support atomic in the C++ build too - #950
Merged
Merged
Conversation
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>
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. It is_Atomiconly for the C11 atomic implementation, so compiling the sources as C++ leaves a data race (reported by ThreadSanitizer).This takes the approach of #949 (a
croaring_atomic_inttypedef:std::atomic<int>in C++,_Atomic intin C11, plainintotherwise) and fixes its C++11/14 build:std::atomic<int> x = 0xFFFFFFF;is copy-initialization, which needs the deleted copy constructor before C++17. The initializer is now a macro that expands to{0xFFFFFFF}in C++ (copy-list-initialization, which calls theconstexprconstructor directly) and to0xFFFFFFFin C (clang rejects braces around an_Atomicinitializer).Compilers without atomics keep the plain
int, as before.Checked with
-Wall -Wextra -Werror: C++11/14/17/20 (clang, g++-15), C11 (clang, gcc-15), and C99 withCROARING_ATOMIC_IMPL=CROARING_ATOMIC_IMPL_NONE, on arm64, x86_64 and x86_64 with AVX2.Supersedes #949. Thanks to @moshap-firebolt for the diagnosis and the original fix.