[libcxx-commits] [PATCH] D103769: [libcxx] Implement P0883R2 ("Fixing Atomic Initialization")
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jun 9 09:21:05 PDT 2021
Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: jroelofs.
Quuxplusone added a comment.
This revision now requires changes to proceed.
I did not realize this was going to be such a rabbit hole. :P But I think at least we're making forward progress in each round.
================
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
----------------
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.
================
Comment at: libcxx/include/atomic:205-206
- atomic() noexcept = default;
+ atomic() noexcept = default; // until C++17
+ constexpr atomic() noexcept; // since C++17
constexpr atomic(T* desr) noexcept;
----------------
tambre wrote:
> tambre wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > tambre wrote:
> > > > > Quuxplusone wrote:
> > > > > > Nit: IMHO you could/should make this
> > > > > > ```
> > > > > > atomic() noexcept; // constexpr since C++20
> > > > > > ```
> > > > > > and likewise below. (I'm guessing this one should say "C++20" like the others, too.)
> > > > > > But I do see that C++20 dropped the `=default`, and maybe that's significant enough to call out in the synopsis (but IMHO it's not).
> > > > > Done.
> > > > Nit: I think the leading `constexpr` should be dropped:
> > > > ```
> > > > atomic() noexcept; // constexpr since C++20
> > > > ```
> > > > but if anyone disagrees, I don't care that much. These synopses are all pretty ad-hoc. :)
> > > > But I do see that C++20 dropped the `=default`, and maybe that's significant enough to call out in the synopsis (but IMHO it's not).
> > > I think it would be better to show the difference. (Actually I already wrote a comment about it, only to discover these remarks were a line below.)
> > >
> > >
> > > Nit: I think the leading `constexpr` should be dropped:
> >
> > I took the leading `constexpr` by example from `<numerics>` synopsis.
> > But I've now returned this to be separate as it was originally.
> > Nit: I think the leading `constexpr` should be dropped:
> > ```
> > atomic() noexcept; // constexpr since C++20
> > ```
> > but if anyone disagrees, I don't care that much. These synopses are all pretty ad-hoc. :)
>
>
Ack, LGTM.
================
Comment at: libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp:11
// REQUIRES: is-lockfree-runtime-function
+// ADDITIONAL_COMPILE_FLAGS: -Wno-psabi
----------------
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.
================
Comment at: libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp:10
// UNSUPPORTED: libcpp-has-no-threads
// ... test case crashes clang.
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
----------------
I know line 10 is a pre-existing comment, but it makes no sense to me. Looks like 5 comments just like this were added by @jroelofs in b3fcc67f8f13cd95d36ed29d0ae1308decb9e099 (D3969) back in 2014. I propose that someone should delete these comments. Could even be in this PR, since you are having to touch all 5 of these test files already.
================
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));
----------------
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.
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