[libc++] Fix inconsistency between is_lock_free and is_always_lock_free #68109
Conversation
This file contains 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
std::atomic is implemented with the following (confusing!) hierarchy of
types:
std::atomic<T> : std::__atomic_base<T> { ... };
std::__atomic_base<T> {
std::__cxx_atomic_impl<T> __impl;
};
std::__cxx_atomic_impl<T> {
_Atomic(T) __val;
};
Inside std::__atomic_base, we implement the is_lock_free() and
is_always_lock_free() functions. However, we used to implement them
inconsistently:
- is_always_lock_free() is based on whether __cxx_atomic_impl<T> is always
lock free (using the builtin), which means that we include any potential
padding added by _Atomic(T) into the determination.
- is_lock_free() was based on whether T is lock free (using the builtin),
which meant that we did not take into account any potential padding
added by _Atomic(T).
It is important to note that the padding added by _Atomic(T) can turn a
type that wouldn't be lock free into a lock free type, for example by
making its size become a power of two.
The inconsistency of how the two functions were implemented could lead
to cases where is_always_lock_free() would return true, but is_lock_free()
would then return false. This is the case for example of the following
type, which is always lock free on arm64 but was incorrectly reported as
!is_lock_free() before this patch:
struct Foo { float x[3]; };
This patch switches the determination of is_lock_free() to be based on
__cxx_atomic_impl<T> instead to match how we determine is_always_lock_free().
rdar://115324353
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Changesstd::atomic is implemented with the following (confusing!) hierarchy of types: Inside std::__atomic_base, we implement the is_lock_free() and is_always_lock_free() functions. However, we used to implement them inconsistently:
It is important to note that the padding added by _Atomic(T) can turn a type that wouldn't be lock free into a lock free type, for example by making its size become a power of two. The inconsistency of how the two functions were implemented could lead to cases where is_always_lock_free() would return true, but is_lock_free() would then return false. This is the case for example of the following type, which is always lock free on arm64 but was incorrectly reported as !is_lock_free() before this patch: This patch switches the determination of is_lock_free() to be based on __cxx_atomic_impl<T> instead to match how we determine is_always_lock_free(). rdar://115324353 Full diff: https://github.com/llvm/llvm-project/pull/68109.diff 7 Files Affected:
diff --git a/libcxx/include/__atomic/atomic_base.h b/libcxx/include/__atomic/atomic_base.h
index 87100ba5d8a50db..775d06d75701833 100644
--- a/libcxx/include/__atomic/atomic_base.h
+++ b/libcxx/include/__atomic/atomic_base.h
@@ -39,7 +39,7 @@ struct __atomic_base // false
_LIBCPP_HIDE_FROM_ABI
bool is_lock_free() const volatile _NOEXCEPT
- {return __cxx_atomic_is_lock_free(sizeof(_Tp));}
+ {return __cxx_atomic_is_lock_free(sizeof(__cxx_atomic_impl<_Tp>));}
_LIBCPP_HIDE_FROM_ABI
bool is_lock_free() const _NOEXCEPT
{return static_cast<__atomic_base const volatile*>(this)->is_lock_free();}
diff --git a/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp b/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
index 495d02fbe5c8d44..f9e01bd5d032bd8 100644
--- a/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
+++ b/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
@@ -100,6 +100,7 @@ int main(int, char**) {
CHECK_ALIGNMENT(struct Empty {});
CHECK_ALIGNMENT(struct OneInt { int i; });
CHECK_ALIGNMENT(struct IntArr2 { int i[2]; });
+ CHECK_ALIGNMENT(struct FloatArr3 { float i[3]; });
CHECK_ALIGNMENT(struct LLIArr2 { long long int i[2]; });
CHECK_ALIGNMENT(struct LLIArr4 { long long int i[4]; });
CHECK_ALIGNMENT(struct LLIArr8 { long long int i[8]; });
diff --git a/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp b/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp
index b2d83f0a6fe8814..6d6e6477bc2511e 100644
--- a/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp
@@ -21,7 +21,6 @@
template <typename T>
void checkAlwaysLockFree() {
if (std::atomic<T>::is_always_lock_free) {
- LIBCPP_ASSERT(sizeof(std::atomic<T>) == sizeof(T)); // technically not required, but libc++ does it that way
assert(std::atomic<T>().is_lock_free());
}
}
@@ -79,6 +78,7 @@ void run()
CHECK_ALWAYS_LOCK_FREE(struct Empty {});
CHECK_ALWAYS_LOCK_FREE(struct OneInt { int i; });
CHECK_ALWAYS_LOCK_FREE(struct IntArr2 { int i[2]; });
+ CHECK_ALWAYS_LOCK_FREE(struct FloatArr3 { float i[3]; });
CHECK_ALWAYS_LOCK_FREE(struct LLIArr2 { long long int i[2]; });
CHECK_ALWAYS_LOCK_FREE(struct LLIArr4 { long long int i[4]; });
CHECK_ALWAYS_LOCK_FREE(struct LLIArr8 { long long int i[8]; });
diff --git a/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
index b3aa1fc47629a3b..f5119cc74821bf2 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
@@ -80,8 +80,11 @@ do_test()
typedef typename std::remove_pointer<T>::type X;
A obj(T(0));
assert(obj == T(0));
- bool b0 = obj.is_lock_free();
- ((void)b0); // mark as unused
+ {
+ bool lockfree = obj.is_lock_free();
+ if (A::is_always_lock_free)
+ assert(lockfree);
+ }
obj.store(T(0));
assert(obj == T(0));
obj.store(T(1), std::memory_order_release);
diff --git a/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
index 78234ae6d96305a..a7ee5d0b78325d4 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
@@ -61,8 +61,11 @@ int main(int, char**)
{
volatile std::atomic<bool> obj(true);
assert(obj == true);
- bool b0 = obj.is_lock_free();
- (void)b0; // to placate scan-build
+ {
+ bool lockfree = obj.is_lock_free();
+ if (std::atomic<bool>::is_always_lock_free)
+ assert(lockfree);
+ }
obj.store(false);
assert(obj == false);
obj.store(true, std::memory_order_release);
@@ -112,8 +115,11 @@ int main(int, char**)
{
std::atomic<bool> obj(true);
assert(obj == true);
- bool b0 = obj.is_lock_free();
- (void)b0; // to placate scan-build
+ {
+ bool lockfree = obj.is_lock_free();
+ if (std::atomic<bool>::is_always_lock_free)
+ assert(lockfree);
+ }
obj.store(false);
assert(obj == false);
obj.store(true, std::memory_order_release);
@@ -163,8 +169,11 @@ int main(int, char**)
{
std::atomic_bool obj(true);
assert(obj == true);
- bool b0 = obj.is_lock_free();
- (void)b0; // to placate scan-build
+ {
+ bool lockfree = obj.is_lock_free();
+ if (std::atomic_bool::is_always_lock_free)
+ assert(lockfree);
+ }
obj.store(false);
assert(obj == false);
obj.store(true, std::memory_order_release);
diff --git a/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
index 058db2dc3ab049f..1905b1b34071c03 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
@@ -98,8 +98,11 @@ do_test()
{
A obj(T(0));
assert(obj == T(0));
- bool b0 = obj.is_lock_free();
- ((void)b0); // mark as unused
+ {
+ bool lockfree = obj.is_lock_free();
+ if (A::is_always_lock_free)
+ assert(lockfree);
+ }
obj.store(T(0));
assert(obj == T(0));
obj.store(T(1), std::memory_order_release);
diff --git a/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp b/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp
index 8b838f62abb1d32..39fa837f4807bf6 100644
--- a/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp
@@ -27,8 +27,12 @@ struct TestFn {
void operator()() const {
typedef std::atomic<T> A;
T t = T();
+
A a(t);
bool b1 = std::atomic_is_lock_free(static_cast<const A*>(&a));
+ if (A::is_always_lock_free)
+ assert(b1);
+
volatile A va(t);
bool b2 = std::atomic_is_lock_free(static_cast<const volatile A*>(&va));
assert(b1 == b2);
|
phyBrackets
reviewed
Oct 3, 2023
...d/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp
Show resolved
Hide resolved
phyBrackets
approved these changes
Oct 3, 2023
|
I'm a bit confused. Is it |
EricWF
reviewed
Oct 3, 2023
| @@ -21,7 +21,6 @@ | |||
| template <typename T> | |||
| void checkAlwaysLockFree() { | |||
| if (std::atomic<T>::is_always_lock_free) { | |||
| LIBCPP_ASSERT(sizeof(std::atomic<T>) == sizeof(T)); // technically not required, but libc++ does it that way | |||
There was a problem hiding this comment.
I assume the FloatArr3 case is what causes this?
Under what architecture? I haven't been able to reproduce the size difference in godbolt.
There was a problem hiding this comment.
Yes, exactly, FloatArr3 showcases this under arm64-apple-macos (and probably any arm64 target). I looked at the assembly and we do not use a lock for FloatArr3 on arm64 so it looks like is_lock_free was really lying.
My understanding is that it's |
|
The CI failure is the bootstrapping build agent dying, definitely not related to this. Shipping. |
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.
std::atomic is implemented with the following (confusing!) hierarchy of types:
Inside std::__atomic_base, we implement the is_lock_free() and is_always_lock_free() functions. However, we used to implement them inconsistently:
It is important to note that the padding added by _Atomic(T) can turn a type that wouldn't be lock free into a lock free type, for example by making its size become a power of two.
The inconsistency of how the two functions were implemented could lead to cases where is_always_lock_free() would return true, but is_lock_free() would then return false. This is the case for example of the following type, which is always lock free on arm64 but was incorrectly reported as !is_lock_free() before this patch:
This patch switches the determination of is_lock_free() to be based on __cxx_atomic_impl instead to match how we determine is_always_lock_free().
rdar://115324353