Fix bug in atomic_ref's calculation of lock_free-ness. #93427
Conversation
The builtin __atomic_always_lock_free takes into account the type of the
pointer provided as the second argument. Because we were passing void*,
rather than T*, the calculation failed. This meant that
atomic_ref<T>::is_always_lock_free was only true for char & bool.
This bug exists elsewhere in the atomic library (when using GCC, we fail
to pass a pointer at all, and we fail to correctly align the atomic like
_Atomic would).
This bug was not initially caught because we don't ever actually expect
a given value for `is_always_lock_free`. This problem is common
throughout atomic, where the tests have been written to assert that
_the value under test_ IS _the value under test_. Which leads to the
admission of bugs like this.
Further work is needed to clean up:
(A) Our detection of has-64-bit-atomics, which uses std::atomic to
determine if std::atomic is supported... (the type `LargeType` may be 64
bits in size, but it's required alignment is only 1 byte). This
configuration test was never intended to provide that information.
(B) The use of __atomic_is_always_lock_free in the GCC atomic
implementation, where we lie about wether a type is always lock free,
when the alignment for the std::atomic<T> is much smaller than required.
For example, struct Counter {int x; int y; };, which _Atomic Counter
aligns to 8 bytes, but our std::atomic<Counter> under GCC only aligns to
4, but still reports that the type is always lock free.
(C) std::atomic_ref<T>::required_alignment should often times be larger
than the natural alignment of the type if the sizeof(T) > alignof(T) and
sizeof(T) 2, 4, 8, or 16. (See the Counter example). In failing to do
so we make many types (Again, see Counter), non-lock free even when
there are atomic instructions on the host that support types of that
size.
(D) We need to actually test against hard coded values throughout our
atomic tests to avoid these sorts of bugs in the future. This probably
means auditing the entire atomic test suite.
This change attempts to start sorting out the testing difficulties
by using the __GCC_ATOMIC_(CHAR|SHORT|INT|LONG|LLONG|POINTER)_IS_LOCK_FREE
predefined macros to establish an expected value for
`is_always_lock_free` and `is_lock_free` for the respective types,
as well as types with matching sizes and compatible alignment values
(Where compatible alignment meants alignof(T) >=
alignof(char|short|int|long|long long) for the matching sized type).
Using these compiler pre-defines we can actually validate that certain
types, like char and int, are actually always lock free like they are
on every platform in the wild(*).
(*) At least for every platform we care about.
Fixing (B) reqires an ABI break where we bump the alignment on the
type std::atomic<T> to match that of _Atomic T (were we under clang).
Fixing (C) also requires an ABI break, but atomic_ref is new enough
that we should consider it ASAP. (Though fixing (C) is arguably more of
a QoI detail, but it's a big one, since we don't want the runtime
alignment of memory to determine the locking behavior of the atomic).
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Eric (EricWF) ChangesThe builtin __atomic_always_lock_free takes into account the type of the This bug exists elsewhere in the atomic library (when using GCC, we fail This bug was not initially caught because we don't ever actually expect Further work is needed to clean up: (A) Our detection of has-64-bit-atomics, which uses std::atomic to (B) The use of __atomic_is_always_lock_free in the GCC atomic (C) std::atomic_ref<T>::required_alignment should often times be larger (D) We need to actually test against hard coded values throughout our This change attempts to start sorting out the testing difficulties Using these compiler pre-defines we can actually validate that certain (*) At least for every platform we care about. Fixing (B) reqires an ABI break where we bump the alignment on the Fixing (C) also requires an ABI break, but atomic_ref is new enough Full diff: https://github.com/llvm/llvm-project/pull/93427.diff 3 Files Affected:
diff --git a/libcxx/include/__atomic/atomic_ref.h b/libcxx/include/__atomic/atomic_ref.h
index 156f1961151c1..b929970edca83 100644
--- a/libcxx/include/__atomic/atomic_ref.h
+++ b/libcxx/include/__atomic/atomic_ref.h
@@ -105,7 +105,7 @@ struct __atomic_ref_base {
// that the pointer is going to be aligned properly at runtime because that is a (checked) precondition
// of atomic_ref's constructor.
static constexpr bool is_always_lock_free =
- __atomic_always_lock_free(sizeof(_Tp), reinterpret_cast<void*>(-required_alignment));
+ __atomic_always_lock_free(sizeof(_Tp), reinterpret_cast<_Tp*>(-required_alignment));
_LIBCPP_HIDE_FROM_ABI bool is_lock_free() const noexcept { return __atomic_is_lock_free(sizeof(_Tp), __ptr_); }
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 94f65e3b4b669..a56455e59a82e 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
@@ -18,9 +18,25 @@
#include <concepts>
#include "test_macros.h"
+#include "atomic_helpers.h"
+
template <typename T>
-void check_always_lock_free(std::atomic_ref<T> const a) {
+void check_always_lock_free_subsumes_is_lock_free(std::atomic_ref<T> const a) {
+ if (is_lock_free_status_known<T>()) {
+ constexpr LockFreeStatus known_status = get_known_atomic_lock_free_status<T>();
+
+ static_assert(std::atomic_ref<T>::is_always_lock_free == (known_status == LockFreeStatus::always),
+ "is_always_lock_free is inconsistent with known lock-free status");
+ if (known_status == LockFreeStatus::always) {
+ assert(a.is_lock_free() && "is_lock_free() is inconsistent with known lock-free status");
+ } else if (known_status == LockFreeStatus::never) {
+ assert(!a.is_lock_free() && "is_lock_free() is inconsistent with known lock-free status");
+ } else {
+ assert(a.is_lock_free() || !a.is_lock_free()); // This is kinda dumb, but we might as well call the function once.
+ }
+
+ }
std::same_as<const bool> decltype(auto) is_always_lock_free = std::atomic_ref<T>::is_always_lock_free;
if (is_always_lock_free) {
std::same_as<bool> decltype(auto) is_lock_free = a.is_lock_free();
@@ -33,18 +49,29 @@ void check_always_lock_free(std::atomic_ref<T> const a) {
do { \
typedef T type; \
type obj{}; \
- check_always_lock_free(std::atomic_ref<type>(obj)); \
+ check_always_lock_free_subsumes_is_lock_free(std::atomic_ref<type>(obj)); \
} while (0)
+void check_always_lock_free_types() {
+
+ static_assert(std::atomic_ref<int>::is_always_lock_free);
+ static_assert(std::atomic_ref<char>::is_always_lock_free);
+}
+
void test() {
+ // While it's hard to portably test the value of is_always_lock_free, since different platforms have different support
+ // for atomic operations, it's still very important to do so. Specifically, it's important to have at least
+ // a few tests that have expected values.
+ check_always_lock_free_types();
+
int i = 0;
- check_always_lock_free(std::atomic_ref<int>(i));
+ check_always_lock_free_subsumes_is_lock_free(std::atomic_ref<int>(i));
float f = 0.f;
- check_always_lock_free(std::atomic_ref<float>(f));
+ check_always_lock_free_subsumes_is_lock_free(std::atomic_ref<float>(f));
int* p = &i;
- check_always_lock_free(std::atomic_ref<int*>(p));
+ check_always_lock_free_subsumes_is_lock_free(std::atomic_ref<int*>(p));
CHECK_ALWAYS_LOCK_FREE(struct Empty{});
CHECK_ALWAYS_LOCK_FREE(struct OneInt { int i; });
diff --git a/libcxx/test/support/atomic_helpers.h b/libcxx/test/support/atomic_helpers.h
index 0266a0961067b..db1aa4d353e6a 100644
--- a/libcxx/test/support/atomic_helpers.h
+++ b/libcxx/test/support/atomic_helpers.h
@@ -11,9 +11,86 @@
#include <cassert>
#include <cstdint>
+#include <cstddef>
+#include <type_traits>
#include "test_macros.h"
+#if defined(TEST_COMPILER_CLANG)
+# define TEST_ATOMIC_CHAR_LOCK_FREE __CLANG_ATOMIC_CHAR_LOCK_FREE
+# define TEST_ATOMIC_SHORT_LOCK_FREE __CLANG_ATOMIC_SHORT_LOCK_FREE
+# define TEST_ATOMIC_INT_LOCK_FREE __CLANG_ATOMIC_INT_LOCK_FREE
+# define TEST_ATOMIC_LONG_LOCK_FREE __CLANG_ATOMIC_LONG_LOCK_FREE
+# define TEST_ATOMIC_LLONG_LOCK_FREE __CLANG_ATOMIC_LLONG_LOCK_FREE
+# define TEST_ATOMIC_POINTER_LOCK_FREE __CLANG_ATOMIC_POINTER_LOCK_FREE
+#elif defined(TEST_COMPILER_GCC)
+# define TEST_ATOMIC_CHAR_LOCK_FREE __GCC_ATOMIC_CHAR_LOCK_FREE
+# define TEST_ATOMIC_SHORT_LOCK_FREE __GCC_ATOMIC_SHORT_LOCK_FREE
+# define TEST_ATOMIC_INT_LOCK_FREE __GCC_ATOMIC_INT_LOCK_FREE
+# define TEST_ATOMIC_LONG_LOCK_FREE __GCC_ATOMIC_LONG_LOCK_FREE
+# define TEST_ATOMIC_LLONG_LOCK_FREE __GCC_ATOMIC_LLONG_LOCK_FREE
+# define TEST_ATOMIC_POINTER_LOCK_FREE __GCC_ATOMIC_POINTER_LOCK_FREE
+#elif TEST_COMPILER_MSVC
+// This is lifted from STL/stl/inc/atomic on github for the purposes of
+// keeping the tests compiling for MSVC's STL. It's not a perfect solution
+// but at least the tests will keep running.
+//
+// Note MSVC's STL never produces a type that is sometimes lock free, but not always lock free.
+template <class T, size_t Size = sizeof(T)>
+constexpr bool msvc_is_lock_free_macro_value() {
+ return (Size <= 8 && (Size & Size - 1) == 0) ? 2 : 0;
+}
+# define TEST_ATOMIC_CHAR_LOCK_FREE ::msvc_is_lock_free_macro_value<char>()
+# define TEST_ATOMIC_SHORT_LOCK_FREE ::msvc_is_lock_free_macro_value<short>()
+# define TEST_ATOMIC_INT_LOCK_FREE ::msvc_is_lock_free_macro_value<int>()
+# define TEST_ATOMIC_LONG_LOCK_FREE ::msvc_is_lock_free_macro_value<long>()
+# define TEST_ATOMIC_LLONG_LOCK_FREE ::msvc_is_lock_free_macro_value<long long>()
+# define TEST_ATOMIC_POINTER_LOCK_FREE ::msvc_is_lock_free_macro_value<void*>()
+#else
+# error "Unknown compiler"
+#endif
+enum class LockFreeStatus { unknown = -1, never = 0, sometimes = 1, always = 2 };
+#define COMPARE_TYPES(T1, T2) \
+ (sizeof(T1) == sizeof(T2) && alignof(T1) >= alignof(T2))
+
+template <class T>
+constexpr inline LockFreeStatus get_known_atomic_lock_free_status() {
+ return LockFreeStatus{COMPARE_TYPES(T, char)
+ ? TEST_ATOMIC_CHAR_LOCK_FREE
+ : (COMPARE_TYPES(T, short)
+ ? TEST_ATOMIC_SHORT_LOCK_FREE
+ : (COMPARE_TYPES(T, int)
+ ? TEST_ATOMIC_INT_LOCK_FREE
+ : (COMPARE_TYPES(T, long)
+ ? TEST_ATOMIC_LONG_LOCK_FREE
+ : (COMPARE_TYPES(T, long long)
+ ? TEST_ATOMIC_LLONG_LOCK_FREE
+ : (COMPARE_TYPES(T, void*) ? TEST_ATOMIC_POINTER_LOCK_FREE
+ : -1)))))};
+}
+
+template <class T>
+constexpr bool is_lock_free_status_known() {
+ return get_known_atomic_lock_free_status<T>() != LockFreeStatus::unknown;
+}
+
+static_assert(is_lock_free_status_known<char>(), "");
+static_assert(is_lock_free_status_known<short>(), "");
+static_assert(is_lock_free_status_known<int>(), "");
+static_assert(is_lock_free_status_known<long>(), "");
+static_assert(is_lock_free_status_known<long long>(), "");
+static_assert(is_lock_free_status_known<void*>(), "");
+
+
+// These macros are somewhat suprising to use, since they take the values 0, 1, or 2.
+// To make the tests clearer, get rid of them in preference of AtomicInfo.
+#undef TEST_ATOMIC_CHAR_LOCK_FREE
+#undef TEST_ATOMIC_SHORT_LOCK_FREE
+#undef TEST_ATOMIC_INT_LOCK_FREE
+#undef TEST_ATOMIC_LONG_LOCK_FREE
+#undef TEST_ATOMIC_LLONG_LOCK_FREE
+#undef TEST_ATOMIC_POINTER_LOCK_FREE
+
struct UserAtomicType {
int i;
@@ -64,6 +141,17 @@ struct LargeUserAtomicType {
}
};
+template <template <class TestArg> class TestFunctor>
+struct TestEachLockFreeKnownIntegralType {
+ void operator()() const {
+ TestFunctor<char>()();
+ TestFunctor<short>()();
+ TestFunctor<int>()();
+ TestFunctor<long long>()();
+ TestFunctor<void*>()();
+ }
+};
+
template <template <class TestArg> class TestFunctor>
struct TestEachIntegralType {
void operator()() const {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Which was a royal pain.
While clang will let us get away with the reinterpret_cast<_Tp*>(-required_alignment); GCC will not. This approach appears to work for both
|
I remember that we had a discussion on this in the original PR but I don’t remember the outcome. I was focusing on the clang bit. (IIRC My concern was that clang builtin had assumption that the address is always _Atomic) |
| static_assert(LockFreeStatusInfo<long long>::status_known, ""); | ||
| static_assert(LockFreeStatusInfo<void*>::status_known, ""); | ||
|
|
||
| // I think these are always supposed to be lock free, and it's worth trying to hardcode expected values. |
There was a problem hiding this comment.
Do you want these assertions both in that helper file and the ones in the atomic_ref is_always_lock_free test?
There was a problem hiding this comment.
Yeah, I do.
Part of the point of this test facility is it provides known & expected value for what has previously been considered unknowable since it's architecture specific.
These assertions are meant to establish that there are atomic operations on all CPU's we support.
|
CC @jyknight |
|
there's some fallout we're seeing internally from #98081 that would be fixed with this, any chance of this going through? |
The builtin __atomic_always_lock_free takes into account the type of the
pointer provided as the second argument. Because we were passing void*,
rather than T*, the calculation failed. This meant that
atomic_ref::is_always_lock_free was only true for char & bool.
This bug exists elsewhere in the atomic library (when using GCC, we fail
to pass a pointer at all, and we fail to correctly align the atomic like
_Atomic would).
This bug was not initially caught because we don't ever actually expect
a given value for
is_always_lock_free. This problem is commonthroughout atomic, where the tests have been written to assert that
the value under test IS the value under test. Which leads to the
admission of bugs like this.
Further work is needed to clean up:
(A) Our detection of has-64-bit-atomics, which uses std::atomic to
determine if std::atomic is supported... (the type
LargeTypemay be 64bits in size, but it's required alignment is only 1 byte). This
configuration test was never intended to provide that information.
(B) The use of __atomic_is_always_lock_free in the GCC atomic
implementation, where we lie about whether a type is always lock free,
when the alignment for the std::atomic is much smaller than required.
For example, struct Counter {int x; int y; };, which _Atomic Counter
aligns to 8 bytes, but our std::atomic under GCC only aligns to
4, but still reports that the type is always lock free.
(C) std::atomic_ref::required_alignment should often times be larger
than the natural alignment of the type if the sizeof(T) > alignof(T) and
sizeof(T) 2, 4, 8, or 16. (See the Counter example). In failing to do
so we make many types (Again, see Counter), non-lock free even when
there are atomic instructions on the host that support types of that
size.
(D) We need to actually test against hard coded values throughout our
atomic tests to avoid these sorts of bugs in the future. This probably
means auditing the entire atomic test suite.
This change attempts to start sorting out the testing difficulties
by using the _GCC_ATOMIC(CHAR|SHORT|INT|LONG|LLONG|POINTER)_IS_LOCK_FREE
predefined macros to establish an expected value for
is_always_lock_freeandis_lock_freefor the respective types,as well as types with matching sizes and compatible alignment values
(Where compatible alignment meants alignof(T) >=
alignof(char|short|int|long|long long) for the matching sized type).
Using these compiler pre-defines we can actually validate that certain
types, like char and int, are actually always lock free like they are
on every platform in the wild(*).
(*) At least for every platform we care about.
Fixing (B) reqires an ABI break where we bump the alignment on the
type std::atomic to match that of _Atomic T (were we under clang).
Fixing (C) also requires an ABI break, but atomic_ref is new enough
that we should consider it ASAP. (Though fixing (C) is arguably more of
a QoI detail, but it's a big one, since we don't want the runtime
alignment of memory to determine the locking behavior of the atomic).