[libc++] Make _Atomic(T) directly use an alias template

[frederick-vs-ja]

Previously, libc++'s _Atomic(T) directly expanded to ::std::atomic<T>, while the standard wording specified that it uses an alias template. The divergence was observable and made libc++ accept some invalid code like struct _Atomic(T) t;.

This patch makes libc++'s _Atomic(T) directly use an internal alias template to eliminate such divergence.

Fixes #168579.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

Previously, libc++'s _Atomic(T) directly expanded to ::std::atomic<T>, while the standard wording specified that it uses an alias template. The divergence was observable and made libc++ accept some invalid code like struct _Atomic(T) t;.

This patch makes libc++'s _Atomic(T) directly use an internal alias template to eliminate such divergence.

Fixes #168579.

---

Full diff: https://github.com/llvm/llvm-project/pull/168679.diff

2 Files Affected:

• (modified) libcxx/include/stdatomic.h (+6-1)
• (added) libcxx/test/std/atomics/stdatomic.h.syn/alias.template.verify.cpp (+25)

diff --git a/libcxx/include/stdatomic.h b/libcxx/include/stdatomic.h
index 2991030eee456..f27c8936ce790 100644
--- a/libcxx/include/stdatomic.h
+++ b/libcxx/include/stdatomic.h
@@ -135,7 +135,12 @@ using std::atomic_signal_fence                         // see below
 #      undef _Atomic
 #    endif
 
-#    define _Atomic(_Tp) ::std::atomic<_Tp>
+_LIBCPP_BEGIN_NAMESPACE_STD
+template <class _Tp>
+using __libcpp_atomic_alias _LIBCPP_NODEBUG = atomic<_Tp>;
+_LIBCPP_END_NAMESPACE_STD
+
+#    define _Atomic(_Tp) ::std::__libcpp_atomic_alias<_Tp>
 
 using std::memory_order _LIBCPP_USING_IF_EXISTS;
 using std::memory_order_relaxed _LIBCPP_USING_IF_EXISTS;
diff --git a/libcxx/test/std/atomics/stdatomic.h.syn/alias.template.verify.cpp b/libcxx/test/std/atomics/stdatomic.h.syn/alias.template.verify.cpp
new file mode 100644
index 0000000000000..34b542d540f6e
--- /dev/null
+++ b/libcxx/test/std/atomics/stdatomic.h.syn/alias.template.verify.cpp
@@ -0,0 +1,25 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: std-at-least-c++23
+// UNSUPPORTED: no-threads
+
+// <stdatomic.h>
+
+// template<class T>
+//   using std-atomic = std::atomic<T>;        // exposition only
+//
+// #define _Atomic(T) std-atomic<T>
+
+// Verify that _Atomic(T) directly uses an alias template but not the std::atomic class template.
+// See also https://llvm.org/PR168579.
+
+#include <stdatomic.h>
+
+struct _Atomic(int) x;
+// expected-error-re@-1{{error: alias template '{{.*}}' cannot be referenced with the 'struct' specifier}}

[philnik777]

IMO we should just update the standard here. What the alias wanted to achieve didn't actually work out, and I don't see a good reason this should be rejected (not that anybody should ever write that code, but I don't care to reject it either). If people really want to make it possible to reject such code it can be worded that way.

[eisenwave]

If people really want to make it possible to reject such code it can be worded that way.

It already is worded that way. Whether intentional or not, I find it to be a convenient consequence of the alias template that you cannot write struct _Atomic(int). This also increase the symmetry with C.

Maybe we could achieve the same in wording by saying that it expands to struct ::std::atomic<int>; the added struct before _Atomic would then also be disallowed because there would be two structs. That's arguably an evolutionary change though, rather than fixing any kind of defect in the wording.

[philnik777]

If people really want to make it possible to reject such code it can be worded that way.

It already is worded that way. Whether intentional or not, I find it to be a convenient consequence of the alias template that you cannot write struct _Atomic(int). This also increase the symmetry with C.

Then it should be mentioned in the wording IMO. as-is it's not at all clear what the intention is - and at least two out of three implementation have done the obvious thing. AFAICT this effect was never intended.

Maybe we could achieve the same in wording by saying that it expands to struct ::std::atomic<int>; the added struct before _Atomic would then also be disallowed because there would be two structs. That's arguably an evolutionary change though, rather than fixing any kind of defect in the wording.

[frederick-vs-ja]

Then it should be mentioned in the wording IMO. as-is it's not at all clear what the intention is - and at least two out of three implementation have done the obvious thing. AFAICT this effect was never intended.

I'm sure that it was intended to use something other than plain std::atomic<T>, as this was the changed part between P0943R5 and P0943R6. But the motivation is not clear to me. It's possibly related to:

It is unspecified whether <stdatomic.h> makes available any declarations in namespace std.

(now in [stdatomic.h.syn]/1), but the relation is not very clear, either.

[philnik777]

Then it should be mentioned in the wording IMO. as-is it's not at all clear what the intention is - and at least two out of three implementation have done the obvious thing. AFAICT this effect was never intended.

I'm sure that it was intended to use something other than plain std::atomic<T>, as this was the changed part between P0943R5 and P0943R6. But the motivation is not clear to me. It's possibly related to:

According to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122738#c2 it's to avoid a dependency on <atomic>, however that's supposed to work.

[frederick-vs-ja]

Then it should be mentioned in the wording IMO. as-is it's not at all clear what the intention is - and at least two out of three implementation have done the obvious thing. AFAICT this effect was never intended.

I'm sure that it was intended to use something other than plain std::atomic<T>, as this was the changed part between P0943R5 and P0943R6. But the motivation is not clear to me. It's possibly related to:

According to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122738#c2 it's to avoid a dependency on <atomic>, however that's supposed to work.

I've checked that we can avoid including the whole <atomic> in <stdatomic.h>, although this doesn't seem to save compilation time a lot. Do you think it's worth doing this? However, it seems unavoidable to expose std::atomic, unless we seal it into a module.

[philnik777]

Then it should be mentioned in the wording IMO. as-is it's not at all clear what the intention is - and at least two out of three implementation have done the obvious thing. AFAICT this effect was never intended.

I'm sure that it was intended to use something other than plain std::atomic<T>, as this was the changed part between P0943R5 and P0943R6. But the motivation is not clear to me. It's possibly related to:

According to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122738#c2 it's to avoid a dependency on <atomic>, however that's supposed to work.

I've checked that we can avoid including the whole <atomic> in <stdatomic.h>, although this doesn't seem to save compilation time a lot. Do you think it's worth doing this? However, it seems unavoidable to expose std::atomic, unless we seal it into a module.

I interpreted it more as "we don't need to provide std::atomic". We're allowed to not actually include <atomic> even if it's #define _Atomic(T) ::std::atomic<T> according to the standard, no?

[huixie90]

I think I am a bit confused. Currently our std::atomic contains a member _Atomic(_Tp) . and now i see we have _Atomic(_Tp) is an alias to std::atomic. How did we manage not to get into an infinite loop?