[clang-tools-extra] [libc++] Fix inconsistency between is_lock_free and is_always_lock_free (PR #68109)
Louis Dionne via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 17 17:05:58 PDT 2023
https://github.com/ldionne updated https://github.com/llvm/llvm-project/pull/68109
>From 84ae453ad32cad94038bdb1e91b4b5c422f3ceb3 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Tue, 3 Oct 2023 10:06:12 -0400
Subject: [PATCH 1/2] [libc++] Fix inconsistency between is_lock_free and
is_always_lock_free
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
---
libcxx/include/__atomic/atomic_base.h | 2 +-
.../atomics/atomics.align/align.pass.cpp | 1 +
.../isalwayslockfree.pass.cpp | 2 +-
.../atomics.types.generic/address.pass.cpp | 7 +++++--
.../atomics.types.generic/bool.pass.cpp | 21 +++++++++++++------
.../atomics.types.generic/integral.pass.cpp | 7 +++++--
.../atomic_is_lock_free.pass.cpp | 4 ++++
7 files changed, 32 insertions(+), 12 deletions(-)
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);
>From 491a3a128f2e49a56895c62cf9d54f4ddac96cba Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Tue, 17 Oct 2023 17:05:27 -0700
Subject: [PATCH 2/2] Fix CI failures in C++11/03/14
---
.../std/atomics/atomics.types.generic/address.pass.cpp | 3 +++
.../test/std/atomics/atomics.types.generic/bool.pass.cpp | 9 +++++++++
.../std/atomics/atomics.types.generic/integral.pass.cpp | 3 +++
3 files changed, 15 insertions(+)
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 f5119cc74821bf2..0926628a2e9a895 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
@@ -82,8 +82,11 @@ do_test()
assert(obj == T(0));
{
bool lockfree = obj.is_lock_free();
+ (void)lockfree;
+#if TEST_STD_VER >= 17
if (A::is_always_lock_free)
assert(lockfree);
+#endif
}
obj.store(T(0));
assert(obj == T(0));
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 a7ee5d0b78325d4..2609e5b56e798de 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
@@ -63,8 +63,11 @@ int main(int, char**)
assert(obj == true);
{
bool lockfree = obj.is_lock_free();
+ (void)lockfree;
+#if TEST_STD_VER >= 17
if (std::atomic<bool>::is_always_lock_free)
assert(lockfree);
+#endif
}
obj.store(false);
assert(obj == false);
@@ -117,8 +120,11 @@ int main(int, char**)
assert(obj == true);
{
bool lockfree = obj.is_lock_free();
+ (void)lockfree;
+#if TEST_STD_VER >= 17
if (std::atomic<bool>::is_always_lock_free)
assert(lockfree);
+#endif
}
obj.store(false);
assert(obj == false);
@@ -171,8 +177,11 @@ int main(int, char**)
assert(obj == true);
{
bool lockfree = obj.is_lock_free();
+ (void)lockfree;
+#if TEST_STD_VER >= 17
if (std::atomic_bool::is_always_lock_free)
assert(lockfree);
+#endif
}
obj.store(false);
assert(obj == false);
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 1905b1b34071c03..2695ff94da30658 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
@@ -100,8 +100,11 @@ do_test()
assert(obj == T(0));
{
bool lockfree = obj.is_lock_free();
+ (void)lockfree;
+#if TEST_STD_VER >= 17
if (A::is_always_lock_free)
assert(lockfree);
+#endif
}
obj.store(T(0));
assert(obj == T(0));
More information about the cfe-commits
mailing list