[libcxx-commits] [PATCH] D103769: [libcxx] Implement P0883R2 ("Fixing Atomic Initialization")
Raul Tambre via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Jun 12 07:55:18 PDT 2021
tambre added a comment.
Thanks for the continued reviewing and sorry for the delay.
================
Comment at: libcxx/include/atomic:1797-1803
+#if _LIBCPP_STD_VER > 17
+ _LIBCPP_INLINE_VISIBILITY constexpr
+ atomic() noexcept(is_nothrow_default_constructible_v<_Tp>) _LIBCPP_DEFAULT
+#else
_LIBCPP_INLINE_VISIBILITY
atomic() _NOEXCEPT _LIBCPP_DEFAULT
+#endif
----------------
Quuxplusone wrote:
> I recommend replacing lines 1797-1803 with
> ```
> #if _LIBCPP_STD_VER > 17
> _LIBCPP_INLINE_VISIBILITY
> atomic() = default;
> #else
> _LIBCPP_INLINE_VISIBILITY
> atomic() _NOEXCEPT _LIBCPP_DEFAULT
> #endif
> ```
> And then, please add a new test case:
> ```
> struct Throwing { Throwing() { throw 42; } };
> int main(int, char**) {
> try {
> std::atomic<Throwing> a;
> (void)a;
> assert(false);
> } catch (int x) {
> assert(x == 42);
> }
> return 0;
> }
> ```
> I believe that this test case will fail with your current PR, because you incorrectly mark `__atomic_base`'s constructor as unconditionally noexcept. Try to use `noexcept`/`_NOEXCEPT` as sparingly as possible — let the compiler infer it for you whenever possible, a la the Rule of Zero. By defaulting our constructor here, we get "noexcept auto" semantics: the compiler will infer `noexcept` exactly when it's appropriate (and then we just have to write tests to verify that the compiler is getting it right). Ditto for "constexpr auto" semantics: just write `=default` (or follow the Rule of Zero) and the compiler will sort it out for you.
Good catch and thanks for the tips!
================
Comment at: libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp:11
// REQUIRES: is-lockfree-runtime-function
+// ADDITIONAL_COMPILE_FLAGS: -Wno-psabi
----------------
Quuxplusone wrote:
> tambre wrote:
> > Since we're now initializing we otherwise get:
> > ```
> > In file included from /opt/deb/llvm/libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp:25:
> > /opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(int)))) int' (vector of 16 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > __atomic_base() _NOEXCEPT : __a_(_Tp()) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(int)))) int' (vector of 16 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > : _Base(value) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(int)))) int' (vector of 32 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > __atomic_base() _NOEXCEPT : __a_(_Tp()) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(int)))) int' (vector of 32 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > : _Base(value) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(float)))) float' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > __atomic_base() _NOEXCEPT : __a_(_Tp()) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(float)))) float' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > : _Base(value) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(float)))) float' (vector of 32 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > __atomic_base() _NOEXCEPT : __a_(_Tp()) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(float)))) float' (vector of 32 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > : _Base(value) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(4 * sizeof(double)))) double' (vector of 4 'double' values) without 'avx' enabled changes the ABI [-Werror,-Wpsabi]
> > __atomic_base() _NOEXCEPT : __a_(_Tp()) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(4 * sizeof(double)))) double' (vector of 4 'double' values) without 'avx' enabled changes the ABI [-Werror,-Wpsabi]
> > : _Base(value) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(double)))) double' (vector of 16 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > __atomic_base() _NOEXCEPT : __a_(_Tp()) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(double)))) double' (vector of 16 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > : _Base(value) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(double)))) double' (vector of 32 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > __atomic_base() _NOEXCEPT : __a_(_Tp()) {}
> > ^
> > /opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(double)))) double' (vector of 32 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
> > : _Base(value) {}
> > ^
> > ```
> I suspect (from `git grep psabi libcxx/` and `git log libcxx/ | grep psabi`) that this is not the appropriate solution to whatever the problem is here. However, `grep vector_size libcxx/` didn't turn up anything likely-looking either.
> I think it's now important to figure out whether this PR is actually ABI-breaking in some way.
>
I think the reason is the ABI for passing a default-initialized T for the vector types to `__cxx_atomic_impl` ends up different depending whether using architecture extensions or not.
Maybe could this be an issue when linking a static library with one using a different set of extensions compared to the other? Otherwise I'm not sure how one could end up compiling `__cxx_atomic_impl` with different architecture extensions than `__atomic_base` and using them together.
Note having the constructor as `atomic_test() : std__atomic_base<T>(T())` before would've given the same warning before.
PS. I renamed remove the double `pass` in the filename.
================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange.pass.cpp:35-36
typedef std::atomic<T> A;
A t;
std::atomic_init(&t, T(1));
assert(std::atomic_exchange(&t, T(2)) == T(1));
----------------
Quuxplusone wrote:
> Rather than disable the deprecation warning, could we just rewrite this as
> ```
> A t(T(1));
> ~~~
> volatile A vt(T(3));
> ```
> and likewise in the other affected files? The point of these tests doesn't seem related to `atomic_init`; it seems like whoever wrote them was just using `atomic_init` incidentally to set up the inputs for the //actual// test of `atomic_exchange` or whatever.
Good idea. I actually thought about this, but disregarded it because I for some reason thought initializing with a value is a libc++ extension, but that's only for `std::atomic_flag`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103769/new/
https://reviews.llvm.org/D103769
More information about the libcxx-commits
mailing list