This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Implement P0883R2 ("Fixing Atomic Initialization")
ClosedPublic

Authored by tambre on Jun 6 2021, 9:49 AM.

Diff Detail

Event Timeline

tambre created this revision.Jun 6 2021, 9:49 AM
tambre requested review of this revision.Jun 6 2021, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2021, 9:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Jun 6 2021, 11:03 AM

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.
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.)

libcxx/include/atomic
205–207

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).

1686

> 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.)

1721

Personally I'd prefer to see __base() here. The Standard says T() not T{}:
http://eel.is/c++draft/atomics.types.operations#2
In fact, don't they have visibly different effects when T is struct A { int i; A() = default; }? Or is that different in C++20 because aggregate init, and/or different everywhere now due to some DR? I'm having trouble demonstrating the issue on Godbolt: https://godbolt.org/z/o5aY7E3Eb
I can demonstrate that A a; and auto a = A(); have different behavior (but I don't actually understand why): https://godbolt.org/z/3r75zG8Ms

Ditto below (line 1804 and anywhere I missed).

libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp
29–32 ↗(On Diff #350116)

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 please test test<std::atomic<bool>>(), since it looks like it gets a different specialization?
Also nit: > > is a C++03-ism, and we can use >> here.

Also, doesn't the constructor of throwing need a body? How does this link?

This revision now requires changes to proceed.Jun 6 2021, 11:03 AM
tambre updated this revision to Diff 350226.Jun 7 2021, 3:16 AM
tambre marked 5 inline comments as done.

Address review comments

tambre added a comment.Jun 7 2021, 3:17 AM

Thanks for the lightning fast review!

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.

Added.

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

Commit 06aaf0b3431f29b6debbb96fdd92ada896f336ff adjusted the synopsis "to match what is implemented" so I assumed that part was considered done.
The required change seems to be simply adding _LIBCPP_DEPRECATED_IN_CXX20 so I don't think it's worthy of a separate change. I've added it here.

libcxx/include/atomic
205–207

Done.

1686

Done. Thanks for the background on why it should be used like that.

1721

Changed to the parentheses variant.
T() and T{} are equivalent since they're both variants of value initialization.
T(args) and T{args} (direct initialization) should be equivalent for aggregates since C++20 as a result of P0960R3 (except in case of narrowing).

I can demonstrate that A a; and auto a = A(); have different behavior (but I don't actually understand why): https://godbolt.org/z/3r75zG8Ms

I'm not sure either. P1008R1 did change this to not be aggregate initialization when using {}.

1721

Personally I'd prefer to see __base() here. The Standard says T() not T{}:
http://eel.is/c++draft/atomics.types.operations#2
In fact, don't they have visibly different effects when T is struct A { int i; A() = default; }? Or is that different in C++20 because aggregate init, and/or different everywhere now due to some DR? I'm having trouble demonstrating the issue on Godbolt: https://godbolt.org/z/o5aY7E3Eb
I can demonstrate that A a; and auto a = A(); have different behavior (but I don't actually understand why): https://godbolt.org/z/3r75zG8Ms

Ditto below (line 1804 and anywhere I missed).

libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp
29–32 ↗(On Diff #350116)

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.

Done.

Also please test test<std::atomic<bool>>(), since it looks like it gets a different specialization?

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.

Also nit: > > is a C++03-ism, and we can use >> here.

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.

Also, doesn't the constructor of throwing need a body? How does this link?

Not sure, I've added an empty body. is_nothrow_constructible.pass.cpp similarly has many functions without bodies.

tambre updated this revision to Diff 350227.Jun 7 2021, 3:18 AM

Fix a missed case of parentheses

Quuxplusone added inline comments.Jun 7 2021, 6:57 AM
libcxx/docs/Cxx2aStatusPaperStatus.csv
136

The required change seems to be simply adding _LIBCPP_DEPRECATED_IN_CXX20

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–207

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. :)

1721

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.

2565

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
29–32 ↗(On Diff #350116)

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.

Mordante requested changes to this revision.Jun 7 2021, 10:06 AM
Mordante added a subscriber: Mordante.

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–207

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.)

514–515

Likewise I prefer an until and since version.

1806

Since this code is version guarded you can use constexpr and noexcept directly.

2565

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
29 ↗(On Diff #350227)

Since the test is compile-time only there's no need for a run-time check. Can you make the following changes

  • rename the file to constexpr_noexcept.compile.pass.cpp
  • int main() -> void test()
  • Add the following two lines at the end:
// Required for MSVC internal test runner compatibility.
int main(int, char**) { return 0; }
30 ↗(On Diff #350227)

We typically use the macros ASSERT_NOEXCEPT and ASSERT_NOT_NOEXCEPT from "test_macros.h".

This revision now requires changes to proceed.Jun 7 2021, 10:06 AM
libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp
29 ↗(On Diff #350227)

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
tambre updated this revision to Diff 350847.Jun 9 2021, 4:14 AM
tambre marked 13 inline comments as done.

Address review comments

tambre added inline comments.Jun 9 2021, 4:16 AM
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.
Added the deprecation test, but in the std directory as I don't think this should be a libc++-specific test.

libcxx/include/atomic
205–207

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.

205–207

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. :)

1721

Good idea, done.

2565

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.
Adjusted the test.

Can you update the test test/std/atomics/atomics.flag/default.pass.cpp to validate the new behaviour?

I'm afraid not, it's already testing the behaviour required by P0883, i.e. that the underlying storage is zeroed on creation.

2565

Can you update the test test/std/atomics/atomics.flag/default.pass.cpp to validate the new behaviour?

libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
11

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) {}
      ^
Quuxplusone requested changes to this revision.Jun 9 2021, 9:21 AM
Quuxplusone added a subscriber: jroelofs.

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–207

Ack, LGTM.

1804–1810

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.cpp
11

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.

libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
9–10

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
33–34

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.

This revision now requires changes to proceed.Jun 9 2021, 9:21 AM
jroelofs added inline comments.Jun 9 2021, 9:29 AM
libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
9–10

Now I'm kicking myself for not having filed bugzilla's for them. Oops.

tambre updated this revision to Diff 351659.Jun 12 2021, 7:54 AM
tambre marked 5 inline comments as done.

Address review comments

Thanks for the continued reviewing and sorry for the delay.

libcxx/include/atomic
1804–1810

Good catch and thanks for the tips!

libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
11

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.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange.pass.cpp
33–34

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
53–54

Please leave the original indentation here, and also on line 38 above, and also on all the lines below this one.
If you're doing this because clang-format recommended it, please ignore clang-format.
It looks to me like the only diff that should be in this file is just adding -Wno-psabi on line 11 and all these other diffs are bogus, right?

libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
165–215

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
30

Nice!
Nit: please make main's signature int main(int, char**) and add return 0; at the end. We do this on all mains, for some reason related to testing on embedded platforms.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_add.pass.cpp
74–78

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.

tambre updated this revision to Diff 352391.Jun 16 2021, 4:10 AM
tambre marked 3 inline comments as done.

Address review comments

tambre marked an inline comment as done.Jun 16 2021, 4:22 AM

LGTM % comments at this point; but before landing it please

  • wait for a second OK from the "libc++" reviewer
  • make sure CI is green

Thanks again for the reviews. I will wait for a second OK.

libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
11

I tried replicating the potential ABI breakage, but only managed to get identical binaries.

I'm fairly confident the only real effects are:

  • Users compiling without vector extensions will now get a warning and will have an ABI mismatch if using in translation units with differing vector extensions. Only a potential ABI issue if re-compiled.
  • Translation units with differing vector extensions and where one of them used libc++ internals to do what we do now (initialize std::__cxx_atomic_impl). They would've gotten the warning before for the initialization, and will also now get it in the translation unit where they didn't initialize.
53–54

It looks to me like the only diff that should be in this file is just adding -Wno-psabi on line 11 and all these other diffs are bogus, right?

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/std/atomics/atomics.types.generic/bool.pass.cpp
165–215

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
53–54

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
30

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

Correct this is what I'm fixing in D103983.

tambre updated this revision to Diff 352786.Jun 17 2021, 10:39 AM

Update deprecation test to not use main() since it doesn't need to be run

tambre marked an inline comment as done.Jun 17 2021, 10:41 AM
tambre added inline comments.
libcxx/test/std/atomics/atomics.types.generic/throw.pass.cpp
30

I do seem to have addressed this. Am I missing something?
I've now updated the deprecation test to not use main() since it doesn't need to be run in case if that's what you meant.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 20 2021, 7:37 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
tambre marked an inline comment as done.