Details
- Reviewers
ldionne • Quuxplusone Mordante jfb - Group Reviewers
Restricted Project - Commits
- rG56aac567acfd: [libcxx] Implement P0883R2 ("Fixing Atomic Initialization")
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM % comments, but several comments are major.
libcxx/docs/Cxx2aStatusPaperStatus.csv | ||
---|---|---|
136 | You also need to change the value of __cpp_lib_atomic_value_initialization and re-run libcxx/utils/generate_feature_test_macro_components.py. Also, p0883r2 asks to deprecate atomic_init. That could easily be a separate PR, but I don't think we should mark p0883 "Complete" until that's complete. (p0883 is also supposed to affect atomic<shared_ptr<T>> and atomic<weak_ptr<T>>, but libc++ does not implement those specializations (yet). So I think that part is OK.) | |
libcxx/include/atomic | ||
205–206 | 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.) | |
1677 | > 17. (C++2b behaves more like 23 than like 20. We don't actually expect to see e.g. 18 here anymore, but if we did, it'd mean C++2a, which should behave more like 20 than like 17. This is a general libc++ style no matter which three-year cycle we're talking about.) | |
1712 | Personally I'd prefer to see __base() here. The Standard says T() not T{}: Ditto below (line 1804 and anywhere I missed). | |
libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp | ||
30–33 | static_assert(test<std::atomic<int>>()); etc., and make the constexpr function test end with return true;. This way you make sure it's actually happening at compile time. Also, doesn't the constructor of throwing need a body? How does this link? |
Thanks for the lightning fast review!
libcxx/docs/Cxx2aStatusPaperStatus.csv | ||
---|---|---|
136 |
Added.
Commit 06aaf0b3431f29b6debbb96fdd92ada896f336ff adjusted the synopsis "to match what is implemented" so I assumed that part was considered done. | |
libcxx/include/atomic | ||
205–206 | Done. | |
1677 | Done. Thanks for the background on why it should be used like that. | |
1712 | Changed to the parentheses variant.
I'm not sure either. P1008R1 did change this to not be aggregate initialization when using {}. | |
1712 |
| |
libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp | ||
30–33 |
Done.
I think this should be covered by the trivial variant since the general case is used if "integral and not bool", but I've added it.
Fixed. I trusted clang-format here. This is probably the result of Standard: Cpp03 in libcxx's .clang-format. Had to use --nolint for arc diff.
Not sure, I've added an empty body. is_nothrow_constructible.pass.cpp similarly has many functions without bodies. |
libcxx/docs/Cxx2aStatusPaperStatus.csv | ||
---|---|---|
136 |
Code change LGTM. We usually want to see a "deprecation warning test" added as well; I think you should add a libcxx/test/libcxx/depr/depr.atomics.general/atomic_init.depr_in_cxx20.verify.cpp along the same lines as libcxx/test/libcxx/depr/depr.function.objects/adaptors.depr_in_cxx11.verify.cpp. (My impression is that we're not very consistent about these tests, so I suggest copying-and-modifying adaptors.depr_in_cxx11.verify.cpp in the obvious way, but if @ldionne asks for some other testing strategy, you should listen to him not me.) | |
libcxx/include/atomic | ||
205–206 | 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. :) | |
1712 | Bah, I missed the first time around that this is initializing __atomic_base<_Tp, false> __base, not _Tp __a_, anyway! The important parens-not-braces is on line 1679; I now agree it definitely doesn't matter which one we use here. Can't we eliminate this member-initializer entirely, because __atomic_base<_Tp, false>'s default ctor (line 1679) will do the right thing? I think the whole #if can be removed, and all you need to change here is to add _LIBCPP_CONSTEXPR_AFTER_CXX17: _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 __atomic_base() _NOEXCEPT _LIBCPP_DEFAULT Ditto lines 1800, 1832. | |
2566 | Since __a_ is not literally bool, I would feel slightly safer if this explicitly said __a_(false); compare line 2573. Sorry for not noticing this the first time around. | |
libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp | ||
30–33 | Ah, I get it now: you instantiate test<std::atomic<trivial>> but not test<std::atomic<throwing>>. Instantiating test<std::atomic<throwing>> would require a body for trivial's constructor. |
Thanks for working this. (Actually I was considering to look at it, but happy I didn't start ;-))
Can you investigate the CI errors that are introduced with this patch?
libcxx/docs/Cxx2aStatusPaperStatus.csv | ||
---|---|---|
136 | Can you add a note that floating-point and shared_ptr aren't done yet? | |
libcxx/include/atomic | ||
205–206 |
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.) | |
513 | Likewise I prefer an until and since version. | |
1802 | Since this code is version guarded you can use constexpr and noexcept directly. | |
2566 | Can you update the test test/std/atomics/atomics.flag/default.pass.cpp to validate the new behaviour? | |
libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp | ||
30 | Since the test is compile-time only there's no need for a run-time check. Can you make the following changes
// Required for MSVC internal test runner compatibility. int main(int, char**) { return 0; } | |
31 | We typically use the macros ASSERT_NOEXCEPT and ASSERT_NOT_NOEXCEPT from "test_macros.h". |
libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp | ||
---|---|---|
30 | I'm in agreement on the first two bullet points, but on the third point, I believe we have an unwritten policy that .compile.pass.cpp tests specifically must not contain int main, because otherwise there's a hazard that people will unwittingly add code to main expecting it to run. We have lots of .compile.pass.cpp tests these days that don't contain a main: $ find libcxx/test -name \*.compile.pass.cpp | xargs grep -l main | wc -l 31 $ find libcxx/test -name \*.compile.pass.cpp | xargs grep -L main | wc -l 185 |
libcxx/docs/Cxx2aStatusPaperStatus.csv | ||
---|---|---|
136 | Added note that shared_ptr and floating-point changes haven't been applied as they themselves aren't implemented yet. | |
libcxx/include/atomic | ||
205–206 |
I took the leading constexpr by example from <numerics> synopsis. | |
205–206 |
| |
1712 | Good idea, done. | |
2566 | Good catch, the false is necessary due to the indirection through __cxx_atomic_impl. The rest weren't actually getting properly initialized either. Should be implemented correctly now.
I'm afraid not, it's already testing the behaviour required by P0883, i.e. that the underlying storage is zeroed on creation. | |
2566 |
| |
libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp | ||
11 ↗ | (On Diff #350847) | 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 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.
libcxx/include/atomic | ||
---|---|---|
205–206 | Ack, LGTM. | |
1800–1806 | 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. | |
libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp | ||
11 ↗ | (On Diff #350847) | 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. |
libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp | ||
10 ↗ | (On Diff #350847) | 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. |
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange.pass.cpp | ||
35–36 ↗ | (On Diff #350847) | 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. |
libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp | ||
---|---|---|
10 ↗ | (On Diff #350847) | Now I'm kicking myself for not having filed bugzilla's for them. Oops. |
Thanks for the continued reviewing and sorry for the delay.
libcxx/include/atomic | ||
---|---|---|
1800–1806 | Good catch and thanks for the tips! | |
libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp | ||
11 ↗ | (On Diff #350847) | 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. 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. |
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange.pass.cpp | ||
35–36 ↗ | (On Diff #350847) | 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. |
LGTM % comments at this point; but before landing it please
- wait for a second OK from the "libc++" reviewer
- make sure CI is green
libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp | ||
---|---|---|
47–50 ↗ | (On Diff #351659) | Please leave the original indentation here, and also on line 38 above, and also on all the lines below this one. |
libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp | ||
165–215 ↗ | (On Diff #351659) | FWIW, one could actually replace these 50 lines with static_assert(std::is_same<std::atomic_bool, std::atomic<bool> >::value, ""); You don't have to do that in this PR, but I wouldn't fault you if you did. :) |
libcxx/test/std/atomics/atomics.types.generic/throw.pass.cpp | ||
29 ↗ | (On Diff #351659) | Nice! |
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_add.pass.cpp | ||
74–78 ↗ | (On Diff #351659) | Attn: @Mordante! These bogusly-template-argumented calls are relevant to D103983, right? (This diff LGTM. Just cross-referencing it with @Mordante's PR, which will merge-conflict with it.) These tests are utterly bogus, btw. Basically what they're doing is initializing an atomic<int*> with (int*)(4), and then fetch_add'ing 2 to it, and then asserting that the result is (int*)(12). This is entirely UB. They should be rewritten to use an array of T or something. But that's out of scope for this PR. |
Thanks again for the reviews. I will wait for a second OK.
libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp | ||
---|---|---|
47–50 ↗ | (On Diff #351659) |
Correct, done. Would be nice if someone either ran clang-format on the codebase or the configuration better matched the actual mandated style. |
libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp | ||
11 ↗ | (On Diff #350847) | I tried replicating the potential ABI breakage, but only managed to get identical binaries. I'm fairly confident the only real effects are:
|
libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp | ||
165–215 ↗ | (On Diff #351659) | I'll do that separately. |
LGTM, but there's one comment not addressed yet. Please fix that before landing the patch.
libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp | ||
---|---|---|
47–50 ↗ | (On Diff #351659) | The problem is the version of clang-format used can't properly format the entire code base. In C++03 mode it breaks C++20 and in C++20 mode it breaks C++03. Probably we can use clang-format on our codebase once we use clang-format 13. |
libcxx/test/std/atomics/atomics.types.generic/throw.pass.cpp | ||
29 ↗ | (On Diff #351659) | This comment hasn't been addressed yet. |
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_add.pass.cpp | ||
74–78 ↗ | (On Diff #351659) | Correct this is what I'm fixing in D103983. |
libcxx/test/std/atomics/atomics.types.generic/throw.pass.cpp | ||
---|---|---|
29 ↗ | (On Diff #351659) | I do seem to have addressed this. Am I missing something? |
You also need to change the value of __cpp_lib_atomic_value_initialization and re-run libcxx/utils/generate_feature_test_macro_components.py.
Also, p0883r2 asks to deprecate atomic_init. That could easily be a separate PR, but I don't think we should mark p0883 "Complete" until that's complete.
http://eel.is/c++draft/depr.atomics.general
(p0883 is also supposed to affect atomic<shared_ptr<T>> and atomic<weak_ptr<T>>, but libc++ does not implement those specializations (yet). So I think that part is OK.)