[libcxx-commits] [PATCH] D103769: [libcxx] Implement P0883R2 ("Fixing Atomic Initialization")
Raul Tambre via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jun 16 04:23:00 PDT 2021
tambre marked an inline comment as done.
tambre added a comment.
In D103769#2820085 <https://reviews.llvm.org/D103769#2820085>, @Quuxplusone wrote:
> 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.
================
Comment at: libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp:47-50
+#define CHECK_ALIGNMENT(T) \
+ do { \
+ typedef T type; \
+ atomic_test<type> t; \
----------------
Quuxplusone wrote:
> 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?
> 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.
================
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:
> 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.
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.
================
Comment at: libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp:165-215
{
std::atomic_bool obj(true);
assert(obj == true);
- std::atomic_init(&obj, false);
- assert(obj == false);
- std::atomic_init(&obj, true);
- assert(obj == true);
bool b0 = obj.is_lock_free();
(void)b0; // to placate scan-build
obj.store(false);
assert(obj == false);
----------------
Quuxplusone wrote:
> 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. :)
I'll do that separately.
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