[libc++] Enable C++ stdatomic.h for all C++ versions #95498

rprichard · 2024-06-14T03:46:13Z

This extension is motivated by Android's use of libc++, where <stdatomic.h> has redirected to for many years, long before it was standardized in C++23.

When libc++'s stdatomic.h is included in C translation units, delegate to the next stdatomic.h, which could come from Clang or libc.

This extension is motivated by Android's use of libc++, where <stdatomic.h> has redirected to <atomic> for many years, long before it was standardized in C++23. When libc++'s stdatomic.h is included in C translation units, delegate to the next stdatomic.h, which could come from Clang or libc.

llvmbot · 2024-06-14T03:46:45Z

@llvm/pr-subscribers-libcxx

Author: Ryan Prichard (rprichard)

Changes

This extension is motivated by Android's use of libc++, where <stdatomic.h> has redirected to <atomic> for many years, long before it was standardized in C++23.

When libc++'s stdatomic.h is included in C translation units, delegate to the next stdatomic.h, which could come from Clang or libc.

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

5 Files Affected:

(modified) libcxx/include/atomic (-4)
(modified) libcxx/include/stdatomic.h (+7-7)
(added) libcxx/test/libcxx/atomics/atomics.syn/compatible_with_stdatomic.compile.pass.cpp (+22)
(removed) libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp (-22)
(removed) libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.compile.pass.cpp (-23)

diff --git a/libcxx/include/atomic b/libcxx/include/atomic
index 80a0f9ee373e9..183e8a0d2d2cd 100644
--- a/libcxx/include/atomic
+++ b/libcxx/include/atomic
@@ -589,10 +589,6 @@ template <class T>
 
 #include <__config>
 
-#if _LIBCPP_STD_VER < 23 && defined(_LIBCPP_STDATOMIC_H)
-#  error <atomic> is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
-#endif
-
 #include <__atomic/aliases.h>
 #include <__atomic/atomic.h>
 #include <__atomic/atomic_base.h>
diff --git a/libcxx/include/stdatomic.h b/libcxx/include/stdatomic.h
index 79772eb7fce1f..34fe75536736f 100644
--- a/libcxx/include/stdatomic.h
+++ b/libcxx/include/stdatomic.h
@@ -121,7 +121,7 @@ using std::atomic_signal_fence                         // see below
 #  pragma GCC system_header
 #endif
 
-#if defined(__cplusplus) && _LIBCPP_STD_VER >= 23
+#if defined(__cplusplus)
 
 #  include <atomic>
 #  include <version>
@@ -154,10 +154,14 @@ using std::atomic_long _LIBCPP_USING_IF_EXISTS;
 using std::atomic_ulong _LIBCPP_USING_IF_EXISTS;
 using std::atomic_llong _LIBCPP_USING_IF_EXISTS;
 using std::atomic_ullong _LIBCPP_USING_IF_EXISTS;
+#  ifndef _LIBCPP_HAS_NO_CHAR8_T
 using std::atomic_char8_t _LIBCPP_USING_IF_EXISTS;
+#  endif
 using std::atomic_char16_t _LIBCPP_USING_IF_EXISTS;
 using std::atomic_char32_t _LIBCPP_USING_IF_EXISTS;
+#  ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 using std::atomic_wchar_t _LIBCPP_USING_IF_EXISTS;
+#  endif
 
 using std::atomic_int8_t _LIBCPP_USING_IF_EXISTS;
 using std::atomic_uint8_t _LIBCPP_USING_IF_EXISTS;
@@ -220,16 +224,12 @@ using std::atomic_store_explicit _LIBCPP_USING_IF_EXISTS;
 using std::atomic_signal_fence _LIBCPP_USING_IF_EXISTS;
 using std::atomic_thread_fence _LIBCPP_USING_IF_EXISTS;
 
-#elif defined(_LIBCPP_COMPILER_CLANG_BASED)
+#else
 
-// Before C++23, we include the next <stdatomic.h> on the path to avoid hijacking
-// the header. We do this because Clang has historically shipped a <stdatomic.h>
-// header that would be available in all Standard modes, and we don't want to
-// break that use case.
 #  if __has_include_next(<stdatomic.h>)
 #    include_next <stdatomic.h>
 #  endif
 
-#endif // defined(__cplusplus) && _LIBCPP_STD_VER >= 23
+#endif // defined(__cplusplus)
 
 #endif // _LIBCPP_STDATOMIC_H
diff --git a/libcxx/test/libcxx/atomics/atomics.syn/compatible_with_stdatomic.compile.pass.cpp b/libcxx/test/libcxx/atomics/atomics.syn/compatible_with_stdatomic.compile.pass.cpp
new file mode 100644
index 0000000000000..210bb333ad715
--- /dev/null
+++ b/libcxx/test/libcxx/atomics/atomics.syn/compatible_with_stdatomic.compile.pass.cpp
@@ -0,0 +1,22 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-threads
+
+// This test verifies that <stdatomic.h> redirects to <atomic>. As an extension,
+// libc++ enables this redirection even before C++23.
+
+// Ordinarily, <stdatomic.h> can be included after <atomic>, but including it
+// first doesn't work because its macros break <atomic>. Verify that
+// <stdatomic.h> can be included first.
+#include <stdatomic.h>
+#include <atomic>
+
+#include <type_traits>
+
+static_assert(std::is_same<atomic_int, std::atomic<int>>::value, "");
diff --git a/libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp b/libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp
deleted file mode 100644
index ca092d9c60275..0000000000000
--- a/libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp
+++ /dev/null
@@ -1,22 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-// UNSUPPORTED: no-threads
-// REQUIRES: c++03 || c++11 || c++14 || c++17 || c++20
-
-// This test ensures that we issue a reasonable diagnostic when including <atomic> after
-// <stdatomic.h> has been included. Before C++23, this otherwise leads to obscure errors
-// because <atomic> may try to redefine things defined by <stdatomic.h>.
-
-// Ignore additional weird errors that happen when the two headers are mixed.
-// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error -Xclang -verify-ignore-unexpected=warning
-
-#include <stdatomic.h>
-#include <atomic>
-
-// expected-error@*:* {{<atomic> is incompatible with <stdatomic.h> before C++23.}}
diff --git a/libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.compile.pass.cpp b/libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.compile.pass.cpp
deleted file mode 100644
index f35eddfb35335..0000000000000
--- a/libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.compile.pass.cpp
+++ /dev/null
@@ -1,23 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-// UNSUPPORTED: no-threads
-
-// This test ensures that we don't hijack the <stdatomic.h> header even when compiling
-// before C++23, since Clang used to provide that header before libc++ provided one.
-
-// On GCC, the compiler-provided <stdatomic.h> is not C++ friendly, so including <stdatomic.h>
-// doesn't work at all if we don't use the <stdatomic.h> provided by libc++ in C++23 and above.
-// XFAIL: (c++11 || c++14 || c++17 || c++20) && gcc
-
-#include <stdatomic.h>
-
-void f() {
-  atomic_int i; // just make sure the header isn't empty
-  (void)i;
-}

rprichard · 2024-06-14T04:01:42Z

See #83351 (comment) for the some of the Android context. The Bionic version of <stdatomic.h> to <atomic> redirection was added in 2014 by https://r.android.com/104366. (The current version is at https://android.googlesource.com/platform/bionic/+/main/libc/include/stdatomic.h.)

ldionne · 2024-06-14T12:52:07Z

libcxx/include/stdatomic.h

@@ -154,10 +154,14 @@ using std::atomic_long _LIBCPP_USING_IF_EXISTS;
 using std::atomic_ulong _LIBCPP_USING_IF_EXISTS;
 using std::atomic_llong _LIBCPP_USING_IF_EXISTS;
 using std::atomic_ullong _LIBCPP_USING_IF_EXISTS;
+#  ifndef _LIBCPP_HAS_NO_CHAR8_T


The using-if-exists attribute should handle this, is there a reason why you're adding this #if?

It looks like GCC doesn't have the __using_if_exists__ attribute, so the generic-gcc-cxx11 test failed until I added the #if.

Right. char8_t is not supported before C++20 so this starts failing when we don't have the attribute. Ok, I think that's acceptable.

ldionne · 2024-06-14T12:57:50Z

libcxx/include/atomic

@@ -589,10 +589,6 @@ template <class T>

 #include <__config>

-#if _LIBCPP_STD_VER < 23 && defined(_LIBCPP_STDATOMIC_H)
-#  error <atomic> is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.


Why do we claim that <atomic> is incompatible with <stdatomic.h> before C++23? Can you refresh my memory? In other words, what changed in C++23 that made the header compatible?

C++ versions before C++23 don't specify a <stdatomic.h>, so including <stdatomic.h> in a C++ file typically finds <stdatomic.h> from the C library (or maybe from Clang's resource directory). That header typically implements <stdatomic.h> from C11, which typically implements type-generic operations using macros. For example, atomic_is_lock_free is a macro that works with any _Atomic-qualified object:

From clang/lib/Headers/stdatomic.h:

#define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj)))

If <stdatomic.h> is included first, then this macro will break the parsing of std::atomic_is_lock_free when <atomic> is included later (e.g. libcxx/include/__atomic/atomic.h):

template <class _Tp> _LIBCPP_HIDE_FROM_ABI bool atomic_is_lock_free(const volatile atomic<_Tp>* __o) _NOEXCEPT { return __o->is_lock_free(); }

It can be possible to use the C11 <stdatomic.h> in a C++ file, as long as <atomic> is included first (or not at all), and only if the C++ compiler and <stdatomic.h> are compatible. For example:

GCC can't include its C11 <stdatomic.h> in C++ mode because GCC only provides the _Atomic qualifier in C mode, not C++.

Clang can include its C11 <stdatomic.h> in C++ mode.

Other STL headers that use the C11 <stdatomic.h> identifiers can also be an issue. For example, including <memory> after <stdatomic.h> also hits the incompatibility:

$ cat test.cpp #include <stdatomic.h> #include <memory> $ clang++-19 -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES test.cpp -c -stdlib=libc++ -std=c++23 # works fine -- libc++ provides the C++23 stdatomic.h $ clang++-19 -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES test.cpp -c -stdlib=libc++ -std=c++20 In file included from test.cpp:2: In file included from /usr/lib/llvm-19/bin/../include/c++/v1/memory:937: /usr/lib/llvm-19/bin/../include/c++/v1/__memory/shared_ptr.h:1587:35: error: expected expression 1587 | inline _LIBCPP_HIDE_FROM_ABI bool atomic_is_lock_free(const shared_ptr<_Tp>*) { | ^ /usr/lib/llvm-19/lib/clang/19/include/stdatomic.h:87:73: note: expanded from macro 'atomic_is_lock_free' 87 | #define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj))) | ^

Including <memory> defines std::atomic_* function overloads for std::shared_ptr (until C++26, when they are removed). https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic

klausler · 2024-06-14T15:09:16Z

why did you ask me to review this PR?

ldionne · 2024-06-14T16:28:39Z

why did you ask me to review this PR?

It is possible that @rprichard meant to ping @philnik777 , whose last name is similar to yours.

rprichard · 2024-06-14T20:16:03Z

why did you ask me to review this PR?

@klausler Sorry, I had meant to add @philnik777 instead.

ldionne · 2024-07-15T17:55:49Z

libcxx/include/stdatomic.h

@@ -121,7 +121,7 @@ using std::atomic_signal_fence                         // see below
 #  pragma GCC system_header
 #endif

-#if defined(__cplusplus) && _LIBCPP_STD_VER >= 23
+#if defined(__cplusplus)


Since this header from libc++ is supposed to only be included in C++ mode, we should always take this branch and the #else should be unnecessary. Do you agree? The #else was previously useful because it handled the pre-C++23 behavior, but it wouldn't be useful anymore with this patch.

Many of the libc++ C headers also have defined(__cplusplus). (For example, stdlib.h.) It has mattered in the past for Android's Soong build system because IIRC it adds libc++ headers to the include path even when compiling a C source file.

Nevermind, I was under the wrong impression that stdatomic.h was only provided in C++. It's also a C header, so of course it would need this #if defined(__cplusplus).

ldionne · 2024-07-15T17:59:28Z

libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp

-// UNSUPPORTED: no-threads
-// REQUIRES: c++03 || c++11 || c++14 || c++17 || c++20
-
-// This test ensures that we issue a reasonable diagnostic when including <atomic> after


This patch trades flexibility for diagnostic quality, IIUC. When we happen to include <stdatomic.h> on an implementation where the headers are not compatible, we'll now get the confusing errors mentioned in this comment.

Is that right? Do you see a way that we can somehow detect whether the implementations are compatible and issue a nice diagnostic if they are not compatible?

The <stdatomic.h> header had originally been introduced with strict standards conformance and simplicity in mind because the situation is already very confusing for users. The intent was to fail very clearly when users do something they're not supposed to do. Unfortunately, I fear that this patch loses that property. Am I mistaken?

I believe that with this patch, <stdatomic.h> and <atomic> would always come from libc++ and have the C++ behavior, so they should be compatible. This new libc++ <stdatomic.h> never delegates to the libc/system/C <stdatomic.h> header, in C++ mode.

Yes, rereading this, I think I agree now.

thevinster · 2024-09-17T06:40:41Z

At Meta, we build libc++ from source when targeting Android and we've ran into the issue where sources include <stdatomic.h>. This eventually ends up including the Android NDK's <stdatomic.h> which includes <atomic> (noted in the summary). I believe this patch solves this use case without resorting to a workaround of gating <stdatomic.h> under #if !defined(__cplusplus) in user code. Is there any progress on this?

ldionne · 2024-09-19T13:38:30Z

libcxx/include/stdatomic.h

@@ -121,7 +121,7 @@ using std::atomic_signal_fence                         // see below
 #  pragma GCC system_header
 #endif

-#if defined(__cplusplus) && _LIBCPP_STD_VER >= 23
+#if defined(__cplusplus)


Nevermind, I was under the wrong impression that stdatomic.h was only provided in C++. It's also a C header, so of course it would need this #if defined(__cplusplus).

ldionne · 2024-09-19T13:40:36Z

libcxx/include/stdatomic.h

@@ -154,10 +154,14 @@ using std::atomic_long _LIBCPP_USING_IF_EXISTS;
 using std::atomic_ulong _LIBCPP_USING_IF_EXISTS;
 using std::atomic_llong _LIBCPP_USING_IF_EXISTS;
 using std::atomic_ullong _LIBCPP_USING_IF_EXISTS;
+#  ifndef _LIBCPP_HAS_NO_CHAR8_T


Right. char8_t is not supported before C++20 so this starts failing when we don't have the attribute. Ok, I think that's acceptable.

ldionne · 2024-09-19T13:42:04Z

libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp

-// UNSUPPORTED: no-threads
-// REQUIRES: c++03 || c++11 || c++14 || c++17 || c++20
-
-// This test ensures that we issue a reasonable diagnostic when including <atomic> after


Yes, rereading this, I think I agree now.

thevinster · 2024-09-19T21:16:04Z

Would it make sense to cherry-pick this to release/19.x? I'm not sure if release/19.x is still accepting patches.

This extension is motivated by Android's use of libc++, where <stdatomic.h> has redirected to <atomic> for many years, long before it was standardized in C++23. When libc++'s stdatomic.h is included in C translation units, delegate to the next stdatomic.h, which could come from Clang or libc.

ldionne · 2024-09-20T16:09:17Z

IMO that's too close to a "new feature" for cherry-picking. It's unfortunate that we missed the branch by a bit, but I think it would make more sense to leave this for LLVM 20.

alexfh · 2024-09-30T12:25:32Z

FYI: this breaks users of _Atomic as a type qualifier. I don't know if that should work in C++ at all, but it used to before this commit: https://gcc.godbolt.org/z/8PoxdP8rz

Depending on how marginal of a use case is this, maybe it's worth mentioning this aspect in the release notes.

ldionne · 2024-09-30T12:50:37Z

Thanks for the heads up! I think this breakage is inherent with the usage of <stdatomic.h>. See for example this where both libc++ and libstdc++ behave the same when <stdatomic.h> is "enabled": https://gcc.godbolt.org/z/ae9E9z8ah

llvm#95498 implemented a libc++ extension where <stdatomic.h> would forward to <atomic> even before C++23. Unfortunately, this was found to be a breaking change (with fairly widespread impact) since that changes whether _Atomic(T) is a C style atomic or std::atomic<T>. In principle, this can even be an ABI break. We generally don't implement extensions in libc++ because they cause so many problems, and that extension had been accepted because it was deemed pretty small and only a quality of life improvement. Since it has widespread impact on valid C++20 (and before) code, this patch removes the extension before we ship it in any public release.

ldionne · 2025-01-15T22:27:47Z

Sadly, this caused fairly widespread breakage over here and we discovered that it broke valid C++20-and-before code. I am proposing a revert here: #123130

#95498 implemented a libc++ extension where <stdatomic.h> would forward to <atomic> even before C++23. Unfortunately, this was found to be a breaking change (with fairly widespread impact) since that changes whether _Atomic(T) is a C style atomic or std::atomic<T>. In principle, this can even be an ABI break. We generally don't implement extensions in libc++ because they cause so many problems, and that extension had been accepted because it was deemed pretty small and only a quality of life improvement. Since it has widespread impact on valid C++20 (and before) code, this patch removes the extension before we ship it in any public release.

llvm/llvm-project#95498 implemented a libc++ extension where <stdatomic.h> would forward to <atomic> even before C++23. Unfortunately, this was found to be a breaking change (with fairly widespread impact) since that changes whether _Atomic(T) is a C style atomic or std::atomic<T>. In principle, this can even be an ABI break. We generally don't implement extensions in libc++ because they cause so many problems, and that extension had been accepted because it was deemed pretty small and only a quality of life improvement. Since it has widespread impact on valid C++20 (and before) code, this patch removes the extension before we ship it in any public release.

rprichard requested a review from a team as a code owner June 14, 2024 03:46

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jun 14, 2024

rprichard requested review from mordante, daltenty and klausler June 14, 2024 03:49

ldionne reviewed Jun 14, 2024

View reviewed changes

klausler removed their request for review June 14, 2024 16:30

rprichard requested a review from philnik777 June 15, 2024 00:11

Add a space between >> so generic-cxx03 test passes

7b24c13

ldionne added this to the LLVM 19.X Release milestone Jul 9, 2024

ldionne reviewed Jul 15, 2024

View reviewed changes

ldionne removed this from the LLVM 19.X Release milestone Sep 19, 2024

ldionne approved these changes Sep 19, 2024

View reviewed changes

ldionne merged commit 2d26fc8 into llvm:main Sep 19, 2024
57 of 58 checks passed

ldionne mentioned this pull request Jan 15, 2025

[libc++] Don't implement <stdatomic.h> before C++23 #123130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Enable C++ stdatomic.h for all C++ versions #95498

[libc++] Enable C++ stdatomic.h for all C++ versions #95498

rprichard commented Jun 14, 2024

llvmbot commented Jun 14, 2024

rprichard commented Jun 14, 2024

ldionne Jun 14, 2024

rprichard Jun 15, 2024

ldionne Sep 19, 2024

ldionne Jun 14, 2024

rprichard Jun 15, 2024

klausler commented Jun 14, 2024

ldionne commented Jun 14, 2024 •

edited

Loading

rprichard commented Jun 14, 2024

ldionne Jul 15, 2024

rprichard Sep 17, 2024

ldionne Sep 19, 2024

ldionne Jul 15, 2024

rprichard Sep 17, 2024

ldionne Sep 19, 2024

thevinster commented Sep 17, 2024

ldionne Sep 19, 2024

ldionne Sep 19, 2024

ldionne Sep 19, 2024

thevinster commented Sep 19, 2024

ldionne commented Sep 20, 2024

alexfh commented Sep 30, 2024 •

edited

Loading

ldionne commented Sep 30, 2024

ldionne commented Jan 15, 2025

[libc++] Enable C++ stdatomic.h for all C++ versions #95498

[libc++] Enable C++ stdatomic.h for all C++ versions #95498

Conversation

rprichard commented Jun 14, 2024

llvmbot commented Jun 14, 2024

rprichard commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klausler commented Jun 14, 2024

ldionne commented Jun 14, 2024 • edited Loading

rprichard commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thevinster commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thevinster commented Sep 19, 2024

ldionne commented Sep 20, 2024

alexfh commented Sep 30, 2024 • edited Loading

ldionne commented Sep 30, 2024

ldionne commented Jan 15, 2025

ldionne commented Jun 14, 2024 •

edited

Loading

alexfh commented Sep 30, 2024 •

edited

Loading