release/19.x: [libc++] Increase atomic_ref's required alignment for small types (#99654) #101492
Conversation
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
|
@ldionne What do you think about merging this PR to the release branch? |
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: None (llvmbot) ChangesBackport 59ca618 Requested by: @ldionne Full diff: https://github.com/llvm/llvm-project/pull/101492.diff 2 Files Affected:
diff --git a/libcxx/include/__atomic/atomic_ref.h b/libcxx/include/__atomic/atomic_ref.h
index 2849b82e1a3dd..b0180a37ab500 100644
--- a/libcxx/include/__atomic/atomic_ref.h
+++ b/libcxx/include/__atomic/atomic_ref.h
@@ -57,11 +57,6 @@ struct __get_aligner_instance {
template <class _Tp>
struct __atomic_ref_base {
-protected:
- _Tp* __ptr_;
-
- _LIBCPP_HIDE_FROM_ABI __atomic_ref_base(_Tp& __obj) : __ptr_(std::addressof(__obj)) {}
-
private:
_LIBCPP_HIDE_FROM_ABI static _Tp* __clear_padding(_Tp& __val) noexcept {
_Tp* __ptr = std::addressof(__val);
@@ -108,10 +103,14 @@ struct __atomic_ref_base {
friend struct __atomic_waitable_traits<__atomic_ref_base<_Tp>>;
+ // require types that are 1, 2, 4, 8, or 16 bytes in length to be aligned to at least their size to be potentially
+ // used lock-free
+ static constexpr size_t __min_alignment = (sizeof(_Tp) & (sizeof(_Tp) - 1)) || (sizeof(_Tp) > 16) ? 0 : sizeof(_Tp);
+
public:
using value_type = _Tp;
- static constexpr size_t required_alignment = alignof(_Tp);
+ static constexpr size_t required_alignment = alignof(_Tp) > __min_alignment ? alignof(_Tp) : __min_alignment;
// The __atomic_always_lock_free builtin takes into account the alignment of the pointer if provided,
// so we create a fake pointer with a suitable alignment when querying it. Note that we are guaranteed
@@ -218,6 +217,12 @@ struct __atomic_ref_base {
}
_LIBCPP_HIDE_FROM_ABI void notify_one() const noexcept { std::__atomic_notify_one(*this); }
_LIBCPP_HIDE_FROM_ABI void notify_all() const noexcept { std::__atomic_notify_all(*this); }
+
+protected:
+ typedef _Tp _Aligned_Tp __attribute__((aligned(required_alignment)));
+ _Aligned_Tp* __ptr_;
+
+ _LIBCPP_HIDE_FROM_ABI __atomic_ref_base(_Tp& __obj) : __ptr_(std::addressof(__obj)) {}
};
template <class _Tp>
diff --git a/libcxx/test/std/atomics/atomics.ref/is_always_lock_free.pass.cpp b/libcxx/test/std/atomics/atomics.ref/is_always_lock_free.pass.cpp
index acdbf63a24d85..78e46c0397951 100644
--- a/libcxx/test/std/atomics/atomics.ref/is_always_lock_free.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.ref/is_always_lock_free.pass.cpp
@@ -54,7 +54,7 @@ void check_always_lock_free(std::atomic_ref<T> const& a) {
#define CHECK_ALWAYS_LOCK_FREE(T) \
do { \
typedef T type; \
- type obj{}; \
+ alignas(std::atomic_ref<type>::required_alignment) type obj{}; \
std::atomic_ref<type> a(obj); \
check_always_lock_free(a); \
} while (0)
|
ldionne
approved these changes
Aug 1, 2024
…vm#99654) This patch increases the alignment requirement for std::atomic_ref such that we can guarantee lockfree operations more often. Specifically, we require types that are 1, 2, 4, 8, or 16 bytes in size to be aligned to at least their size to be used with std::atomic_ref. This is the case for most types, however a notable exception is `long long` on x86, which is 8 bytes in length but has an alignment of 4. As a result of this patch, one has to be more careful about the alignment of objects used with std::atomic_ref. Failure to provide a properly-aligned object to std::atomic_ref is a precondition violation and is technically UB. On the flipside, this allows us to provide an atomic_ref that is actually lockfree more often, which is an important QOI property. More information in the discussion at llvm#99570 (comment). Co-authored-by: Louis Dionne <[email protected]> (cherry picked from commit 59ca618)
|
@ldionne (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
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.
Backport 59ca618
Requested by: @ldionne