From 96b358f67b523f343497d3e32822f821e3564bcc Mon Sep 17 00:00:00 2001 From: Eric Fiselier Date: Sun, 26 May 2024 17:07:32 -0400 Subject: [PATCH 1/3] [libc++] Fix bug in atomic_ref's calculation of lock_free-ness. 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 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 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_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 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). --- libcxx/include/__atomic/atomic_ref.h | 15 +- .../atomics.lockfree/is_always_lock_free.cpp | 165 ++++++++++++++++++ .../isalwayslockfree.pass.cpp | 120 ------------- .../atomics.ref/is_always_lock_free.pass.cpp | 34 +++- libcxx/test/support/atomic_helpers.h | 93 ++++++++++ 5 files changed, 302 insertions(+), 125 deletions(-) create mode 100644 libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp delete mode 100644 libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp diff --git a/libcxx/include/__atomic/atomic_ref.h b/libcxx/include/__atomic/atomic_ref.h index 156f1961151c1..2849b82e1a3dd 100644 --- a/libcxx/include/__atomic/atomic_ref.h +++ b/libcxx/include/__atomic/atomic_ref.h @@ -42,6 +42,19 @@ _LIBCPP_BEGIN_NAMESPACE_STD #if _LIBCPP_STD_VER >= 20 +// These types are required to make __atomic_is_always_lock_free work across GCC and Clang. +// The purpose of this trick is to make sure that we provide an object with the correct alignment +// to __atomic_is_always_lock_free, since that answer depends on the alignment. +template +struct __alignment_checker_type { + alignas(_Alignment) char __data; +}; + +template +struct __get_aligner_instance { + static constexpr __alignment_checker_type<_Alignment> __instance{}; +}; + template struct __atomic_ref_base { protected: @@ -105,7 +118,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(-required_alignment)); + __atomic_always_lock_free(sizeof(_Tp), &__get_aligner_instance::__instance); _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.lockfree/is_always_lock_free.cpp b/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp new file mode 100644 index 0000000000000..2dc7f5c765419 --- /dev/null +++ b/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp @@ -0,0 +1,165 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// UNSUPPORTED: c++03, c++11, c++14 + +// +// +// template +// class atomic; +// +// static constexpr bool is_always_lock_free; + +#include +#include +#include + +#include "test_macros.h" +#include "atomic_helpers.h" + +template +void check_always_lock_free(std::atomic const& a) { + using InfoT = LockFreeStatusInfo; + + constexpr std::same_as decltype(auto) is_always_lock_free = std::atomic::is_always_lock_free; + + // If we know the status of T for sure, validate the exact result of the function. + if constexpr (InfoT::status_known) { + constexpr LockFreeStatus known_status = InfoT::value; + if constexpr (known_status == LockFreeStatus::always) { + static_assert(is_always_lock_free, "is_always_lock_free is inconsistent with known lock-free status"); + assert(a.is_lock_free() && "is_lock_free() is inconsistent with known lock-free status"); + } else if constexpr (known_status == LockFreeStatus::never) { + static_assert(!is_always_lock_free, "is_always_lock_free is inconsistent with known lock-free status"); + 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. + } + } + + // In all cases, also sanity-check it based on the implication always-lock-free => lock-free. + if (is_always_lock_free) { + std::same_as decltype(auto) is_lock_free = a.is_lock_free(); + assert(is_lock_free); + } + ASSERT_NOEXCEPT(a.is_lock_free()); +} + +#define CHECK_ALWAYS_LOCK_FREE(T) \ + do { \ + typedef T type; \ + type obj{}; \ + std::atomic a(obj); \ + check_always_lock_free(a); \ + } while (0) + +void test() { + char c = 'x'; + check_always_lock_free(std::atomic(c)); + + int i = 0; + check_always_lock_free(std::atomic(i)); + + float f = 0.f; + check_always_lock_free(std::atomic(f)); + + int* p = &i; + check_always_lock_free(std::atomic(p)); + + CHECK_ALWAYS_LOCK_FREE(bool); + CHECK_ALWAYS_LOCK_FREE(char); + CHECK_ALWAYS_LOCK_FREE(signed char); + CHECK_ALWAYS_LOCK_FREE(unsigned char); +#if TEST_STD_VER > 17 && defined(__cpp_char8_t) + CHECK_ALWAYS_LOCK_FREE(char8_t); +#endif + CHECK_ALWAYS_LOCK_FREE(char16_t); + CHECK_ALWAYS_LOCK_FREE(char32_t); + CHECK_ALWAYS_LOCK_FREE(wchar_t); + CHECK_ALWAYS_LOCK_FREE(short); + CHECK_ALWAYS_LOCK_FREE(unsigned short); + CHECK_ALWAYS_LOCK_FREE(int); + CHECK_ALWAYS_LOCK_FREE(unsigned int); + CHECK_ALWAYS_LOCK_FREE(long); + CHECK_ALWAYS_LOCK_FREE(unsigned long); + CHECK_ALWAYS_LOCK_FREE(long long); + CHECK_ALWAYS_LOCK_FREE(unsigned long long); + CHECK_ALWAYS_LOCK_FREE(std::nullptr_t); + CHECK_ALWAYS_LOCK_FREE(void*); + CHECK_ALWAYS_LOCK_FREE(float); + CHECK_ALWAYS_LOCK_FREE(double); + CHECK_ALWAYS_LOCK_FREE(long double); +#if __has_attribute(vector_size) && defined(_LIBCPP_VERSION) + CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(1 * sizeof(int))))); + CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(2 * sizeof(int))))); + CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(4 * sizeof(int))))); + CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(16 * sizeof(int))))); + CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(32 * sizeof(int))))); + CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(1 * sizeof(float))))); + CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(2 * sizeof(float))))); + CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(4 * sizeof(float))))); + CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(16 * sizeof(float))))); + CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(32 * sizeof(float))))); + CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(1 * sizeof(double))))); + CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(2 * sizeof(double))))); + CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(4 * sizeof(double))))); + CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(16 * sizeof(double))))); + CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(32 * sizeof(double))))); +#endif // __has_attribute(vector_size) && defined(_LIBCPP_VERSION) + 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]; }); + CHECK_ALWAYS_LOCK_FREE(struct LLIArr16 { long long int i[16]; }); + CHECK_ALWAYS_LOCK_FREE(struct Padding { + char c; /* padding */ + long long int i; + }); + CHECK_ALWAYS_LOCK_FREE(union IntFloat { + int i; + float f; + }); + CHECK_ALWAYS_LOCK_FREE(enum class CharEnumClass : char{foo}); + + // C macro and static constexpr must be consistent. + enum class CharEnumClass : char { foo }; + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_BOOL_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); +#if TEST_STD_VER > 17 && defined(__cpp_char8_t) + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR8_T_LOCK_FREE), ""); +#endif + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR16_T_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR32_T_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_WCHAR_T_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_SHORT_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_SHORT_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_INT_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_INT_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LONG_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LONG_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LLONG_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LLONG_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_POINTER_LOCK_FREE), ""); + static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_POINTER_LOCK_FREE), ""); + +#if TEST_STD_VER >= 20 + static_assert(std::atomic_signed_lock_free::is_always_lock_free, ""); + static_assert(std::atomic_unsigned_lock_free::is_always_lock_free, ""); +#endif +} + +int main(int, char**) { + test(); + return 0; +} diff --git a/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp b/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp deleted file mode 100644 index 6d6e6477bc251..0000000000000 --- a/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp +++ /dev/null @@ -1,120 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// -// -// UNSUPPORTED: c++03, c++11, c++14 - -// - -// static constexpr bool is_always_lock_free; - -#include -#include -#include - -#include "test_macros.h" - -template -void checkAlwaysLockFree() { - if (std::atomic::is_always_lock_free) { - assert(std::atomic().is_lock_free()); - } -} - -void run() -{ -// structs and unions can't be defined in the template invocation. -// Work around this with a typedef. -#define CHECK_ALWAYS_LOCK_FREE(T) \ - do { \ - typedef T type; \ - checkAlwaysLockFree(); \ - } while (0) - - CHECK_ALWAYS_LOCK_FREE(bool); - CHECK_ALWAYS_LOCK_FREE(char); - CHECK_ALWAYS_LOCK_FREE(signed char); - CHECK_ALWAYS_LOCK_FREE(unsigned char); -#if TEST_STD_VER > 17 && defined(__cpp_char8_t) - CHECK_ALWAYS_LOCK_FREE(char8_t); -#endif - CHECK_ALWAYS_LOCK_FREE(char16_t); - CHECK_ALWAYS_LOCK_FREE(char32_t); - CHECK_ALWAYS_LOCK_FREE(wchar_t); - CHECK_ALWAYS_LOCK_FREE(short); - CHECK_ALWAYS_LOCK_FREE(unsigned short); - CHECK_ALWAYS_LOCK_FREE(int); - CHECK_ALWAYS_LOCK_FREE(unsigned int); - CHECK_ALWAYS_LOCK_FREE(long); - CHECK_ALWAYS_LOCK_FREE(unsigned long); - CHECK_ALWAYS_LOCK_FREE(long long); - CHECK_ALWAYS_LOCK_FREE(unsigned long long); - CHECK_ALWAYS_LOCK_FREE(std::nullptr_t); - CHECK_ALWAYS_LOCK_FREE(void*); - CHECK_ALWAYS_LOCK_FREE(float); - CHECK_ALWAYS_LOCK_FREE(double); - CHECK_ALWAYS_LOCK_FREE(long double); -#if __has_attribute(vector_size) && defined(_LIBCPP_VERSION) - CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(1 * sizeof(int))))); - CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(2 * sizeof(int))))); - CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(4 * sizeof(int))))); - CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(16 * sizeof(int))))); - CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(32 * sizeof(int))))); - CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(1 * sizeof(float))))); - CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(2 * sizeof(float))))); - CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(4 * sizeof(float))))); - CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(16 * sizeof(float))))); - CHECK_ALWAYS_LOCK_FREE(float __attribute__((vector_size(32 * sizeof(float))))); - CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(1 * sizeof(double))))); - CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(2 * sizeof(double))))); - CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(4 * sizeof(double))))); - CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(16 * sizeof(double))))); - CHECK_ALWAYS_LOCK_FREE(double __attribute__((vector_size(32 * sizeof(double))))); -#endif // __has_attribute(vector_size) && defined(_LIBCPP_VERSION) - 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]; }); - CHECK_ALWAYS_LOCK_FREE(struct LLIArr16 { long long int i[16]; }); - CHECK_ALWAYS_LOCK_FREE(struct Padding { char c; /* padding */ long long int i; }); - CHECK_ALWAYS_LOCK_FREE(union IntFloat { int i; float f; }); - CHECK_ALWAYS_LOCK_FREE(enum class CharEnumClass : char { foo }); - - // C macro and static constexpr must be consistent. - enum class CharEnumClass : char { foo }; - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_BOOL_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR_LOCK_FREE), ""); -#if TEST_STD_VER > 17 && defined(__cpp_char8_t) - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR8_T_LOCK_FREE), ""); -#endif - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR16_T_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_CHAR32_T_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_WCHAR_T_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_SHORT_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_SHORT_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_INT_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_INT_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LONG_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LONG_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LLONG_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_LLONG_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_POINTER_LOCK_FREE), ""); - static_assert(std::atomic::is_always_lock_free == (2 == ATOMIC_POINTER_LOCK_FREE), ""); - -#if TEST_STD_VER >= 20 - static_assert(std::atomic_signed_lock_free::is_always_lock_free, ""); - static_assert(std::atomic_unsigned_lock_free::is_always_lock_free, ""); -#endif -} - -int main(int, char**) { run(); return 0; } 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..acdbf63a24d85 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 @@ -9,7 +9,10 @@ // UNSUPPORTED: c++03, c++11, c++14, c++17 // - +// +// template +// class atomic_ref; +// // static constexpr bool is_always_lock_free; // bool is_lock_free() const noexcept; @@ -18,10 +21,29 @@ #include #include "test_macros.h" +#include "atomic_helpers.h" template -void check_always_lock_free(std::atomic_ref const a) { - std::same_as decltype(auto) is_always_lock_free = std::atomic_ref::is_always_lock_free; +void check_always_lock_free(std::atomic_ref const& a) { + using InfoT = LockFreeStatusInfo; + + constexpr std::same_as decltype(auto) is_always_lock_free = std::atomic_ref::is_always_lock_free; + + // If we know the status of T for sure, validate the exact result of the function. + if constexpr (InfoT::status_known) { + constexpr LockFreeStatus known_status = InfoT::value; + if constexpr (known_status == LockFreeStatus::always) { + static_assert(is_always_lock_free, "is_always_lock_free is inconsistent with known lock-free status"); + assert(a.is_lock_free() && "is_lock_free() is inconsistent with known lock-free status"); + } else if constexpr (known_status == LockFreeStatus::never) { + static_assert(!is_always_lock_free, "is_always_lock_free is inconsistent with known lock-free status"); + 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. + } + } + + // In all cases, also sanity-check it based on the implication always-lock-free => lock-free. if (is_always_lock_free) { std::same_as decltype(auto) is_lock_free = a.is_lock_free(); assert(is_lock_free); @@ -33,10 +55,14 @@ void check_always_lock_free(std::atomic_ref const a) { do { \ typedef T type; \ type obj{}; \ - check_always_lock_free(std::atomic_ref(obj)); \ + std::atomic_ref a(obj); \ + check_always_lock_free(a); \ } while (0) void test() { + char c = 'x'; + check_always_lock_free(std::atomic_ref(c)); + int i = 0; check_always_lock_free(std::atomic_ref(i)); diff --git a/libcxx/test/support/atomic_helpers.h b/libcxx/test/support/atomic_helpers.h index 0266a0961067b..e5d07bab9f616 100644 --- a/libcxx/test/support/atomic_helpers.h +++ b/libcxx/test/support/atomic_helpers.h @@ -11,9 +11,102 @@ #include #include +#include +#include #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 +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() +# define TEST_ATOMIC_SHORT_LOCK_FREE ::msvc_is_lock_free_macro_value() +# define TEST_ATOMIC_INT_LOCK_FREE ::msvc_is_lock_free_macro_value() +# define TEST_ATOMIC_LONG_LOCK_FREE ::msvc_is_lock_free_macro_value() +# define TEST_ATOMIC_LLONG_LOCK_FREE ::msvc_is_lock_free_macro_value() +# define TEST_ATOMIC_POINTER_LOCK_FREE ::msvc_is_lock_free_macro_value() +#else +# error "Unknown compiler" +#endif + +#ifdef TEST_COMPILER_CLANG +# pragma clang diagnostic push +# pragma clang diagnostic ignored "-Wc++11-extensions" +#endif + +// The entire LockFreeStatus exists entirely to work around the support for C++03, which many of our atomic tests +// run under. This is a bit of a hack, but it's the best we can do. +enum class LockFreeStatus : int { unknown = -1, never = 0, sometimes = 1, always = 2 }; +#define COMPARE_TYPES(T1, T2) (sizeof(T1) == sizeof(T2) && TEST_ALIGNOF(T1) >= TEST_ALIGNOF(T2)) + +template +struct LockFreeStatusInfo { + static const LockFreeStatus value = 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)))))); + + static const bool status_known = LockFreeStatusInfo::value != LockFreeStatus::unknown; +}; + +// This doesn't work in C++03 due to issues with scoped enumerations. Just disable the test. +#if TEST_STD_VER >= 11 +static_assert(LockFreeStatusInfo::status_known, ""); +static_assert(LockFreeStatusInfo::status_known, ""); +static_assert(LockFreeStatusInfo::status_known, ""); +static_assert(LockFreeStatusInfo::status_known, ""); +static_assert(LockFreeStatusInfo::status_known, ""); +static_assert(LockFreeStatusInfo::status_known, ""); + +// Those should always be lock free: hardcode some expected values to make sure our tests are actually +// testing something meaningful. +static_assert(LockFreeStatusInfo::value == LockFreeStatus::always, ""); +static_assert(LockFreeStatusInfo::value == LockFreeStatus::always, ""); +static_assert(LockFreeStatusInfo::value == LockFreeStatus::always, ""); +#endif + +// 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 LockFreeStatusInfo. +#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 + +#ifdef TEST_COMPILER_CLANG +# pragma clang diagnostic pop +#endif + struct UserAtomicType { int i; From 271c27be62e43561c0b7a1fa864e020a8dd8c85d Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Thu, 25 Jul 2024 14:43:04 -0400 Subject: [PATCH 2/3] Try to fix issues with long long --- libcxx/test/support/atomic_helpers.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/libcxx/test/support/atomic_helpers.h b/libcxx/test/support/atomic_helpers.h index e5d07bab9f616..fd5ed69a30b92 100644 --- a/libcxx/test/support/atomic_helpers.h +++ b/libcxx/test/support/atomic_helpers.h @@ -55,10 +55,15 @@ constexpr bool msvc_is_lock_free_macro_value() { # pragma clang diagnostic ignored "-Wc++11-extensions" #endif -// The entire LockFreeStatus exists entirely to work around the support for C++03, which many of our atomic tests -// run under. This is a bit of a hack, but it's the best we can do. enum class LockFreeStatus : int { unknown = -1, never = 0, sometimes = 1, always = 2 }; -#define COMPARE_TYPES(T1, T2) (sizeof(T1) == sizeof(T2) && TEST_ALIGNOF(T1) >= TEST_ALIGNOF(T2)) + +// We should really be checking whether the alignment of T is greater-than-or-equal-to the alignment required +// for T to be atomic, but this is basically impossible to implement portably. Instead, we assume that any type +// aligned to at least its size is going to be atomic if there exists atomic operations for that size at all, +// which is true on most platforms. This technically reduces our test coverage in the sense that if a type has +// an alignment requirement less than its size but could still be made lockfree, LockFreeStatusInfo will report +// that we don't know whether it is lockfree or not. +#define COMPARE_TYPES(T, FundamentalT) (sizeof(T) == sizeof(FundamentalT) && TEST_ALIGNOF(T) >= sizeof(T)) template struct LockFreeStatusInfo { @@ -78,6 +83,8 @@ struct LockFreeStatusInfo { static const bool status_known = LockFreeStatusInfo::value != LockFreeStatus::unknown; }; +#undef COMPARE_TYPES + // This doesn't work in C++03 due to issues with scoped enumerations. Just disable the test. #if TEST_STD_VER >= 11 static_assert(LockFreeStatusInfo::status_known, ""); From af791c9dfea842ce69066664fec14d0be414b272 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Fri, 26 Jul 2024 11:00:28 -0400 Subject: [PATCH 3/3] Hopefully fix test on x86 --- libcxx/test/support/atomic_helpers.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libcxx/test/support/atomic_helpers.h b/libcxx/test/support/atomic_helpers.h index fd5ed69a30b92..d2f2b751cb47d 100644 --- a/libcxx/test/support/atomic_helpers.h +++ b/libcxx/test/support/atomic_helpers.h @@ -91,9 +91,12 @@ static_assert(LockFreeStatusInfo::status_known, ""); static_assert(LockFreeStatusInfo::status_known, ""); static_assert(LockFreeStatusInfo::status_known, ""); static_assert(LockFreeStatusInfo::status_known, ""); -static_assert(LockFreeStatusInfo::status_known, ""); static_assert(LockFreeStatusInfo::status_known, ""); +// long long is a bit funky: on some platforms, its alignment is 4 bytes but its size is +// 8 bytes. In that case, atomics may or may not be lockfree based on their address. +static_assert(alignof(long long) == sizeof(long long) ? LockFreeStatusInfo::status_known : true, ""); + // Those should always be lock free: hardcode some expected values to make sure our tests are actually // testing something meaningful. static_assert(LockFreeStatusInfo::value == LockFreeStatus::always, "");