[libc++] Don't implement <stdatomic.h> before C++23 #123130
Conversation
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.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) Changes#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. Full diff: https://github.com/llvm/llvm-project/pull/123130.diff 6 Files Affected:
diff --git a/libcxx/include/atomic b/libcxx/include/atomic
index 80f9e437bfaab0..0c641c7a68b58c 100644
--- a/libcxx/include/atomic
+++ b/libcxx/include/atomic
@@ -592,6 +592,10 @@ template <class T>
#else
# 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_flag.h>
diff --git a/libcxx/include/stdatomic.h b/libcxx/include/stdatomic.h
index a0b46e3b7bc173..ea57a1fb067020 100644
--- a/libcxx/include/stdatomic.h
+++ b/libcxx/include/stdatomic.h
@@ -126,7 +126,7 @@ using std::atomic_signal_fence // see below
# pragma GCC system_header
# endif
-# if defined(__cplusplus)
+# if defined(__cplusplus) && _LIBCPP_STD_VER >= 23
# include <atomic>
# include <version>
@@ -233,11 +233,14 @@ using std::atomic_thread_fence _LIBCPP_USING_IF_EXISTS;
# else
+// Before C++23, we include the next <stdatomic.h> on the path to avoid hijacking
+// the header, since Clang and some platforms have been providing this header for
+// a long time and users rely on it.
# if __has_include_next(<stdatomic.h>)
# include_next <stdatomic.h>
# endif
-# endif // defined(__cplusplus)
+# endif // defined(__cplusplus) && _LIBCPP_STD_VER >= 23
#endif // defined(__cplusplus) && __cplusplus < 201103L && defined(_LIBCPP_USE_FROZEN_CXX03_HEADERS)
#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
index 323072da14463b..30e9672a256830 100644
--- 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
@@ -7,15 +7,16 @@
//===----------------------------------------------------------------------===//
// UNSUPPORTED: no-threads
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// XFAIL: FROZEN-CXX03-HEADERS-FIXME
-// This test verifies that <stdatomic.h> redirects to <atomic>. As an extension,
-// libc++ enables this redirection even before C++23.
+// This test verifies that <stdatomic.h> redirects to <atomic>.
-// 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.
+// Before C++23, <stdatomic.h> can be included after <atomic>, but including it
+// first doesn't work because its macros break <atomic>. Fixing that is the point
+// of the C++23 change that added <stdatomic.h> to C++. Thus, this test verifies
+// that <stdatomic.h> can be included first.
#include <stdatomic.h>
#include <atomic>
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
new file mode 100644
index 00000000000000..ca092d9c602753
--- /dev/null
+++ b/libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.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
+// 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
new file mode 100644
index 00000000000000..6df80daf9414e5
--- /dev/null
+++ b/libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.compile.pass.cpp
@@ -0,0 +1,24 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 (e.g. by providing
+// an empty header) even when compiling before C++23, since some users were using the
+// Clang or platform provided header before libc++ added its own.
+
+// 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;
+}
diff --git a/libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.cxx23.compile.pass.cpp b/libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.cxx23.compile.pass.cpp
new file mode 100644
index 00000000000000..9a30b439c96c9b
--- /dev/null
+++ b/libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.cxx23.compile.pass.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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-threads
+// XFAIL: FROZEN-CXX03-HEADERS-FIXME
+
+// This test verifies that <stdatomic.h> DOES NOT redirect to <atomic> before C++23,
+// since doing so is a breaking change. Several things can break when that happens,
+// because the type of _Atomic(T) changes from _Atomic(T) to std::atomic<T>.
+//
+// For example, redeclarations can become invalid depending on whether they
+// have been declared with <stdatomic.h> in scope or not.
+
+// REQUIRES: c++03 || c++11 || c++14 || c++17 || c++20
+
+#include <atomic>
+#include <stdatomic.h>
+#include <type_traits>
+
+static_assert(!std::is_same<_Atomic(int), std::atomic<int> >::value, "");
|
|
Another example of valid code that works without the extension and breaks with the extension: // REQUIRES: c++03 || c++11 || c++14 || c++17 || c++20
struct Foo {
_Atomic(int) f() const;
};
#include <stdatomic.h>
_Atomic(int) Foo::f() const; // This declaration would differ from the previous one in C++23 |
|
Yeah, removing the extension from upstream seems OK to me. I suspect the Android toolchain situation around stdatomic.h will continue to be complicated, but maybe we can upgrade enough stuff to C++23 and drop Android's historic extension?
FWIW, I think this code is accepted by Clang prior to the stdatomic.h extension PR, but not by other compilers. i.e. It's a Clang extension to allow C11-style atomics in a C++ language dialect (as long as |
libcxx/include/atomic
Outdated
| @@ -592,6 +592,10 @@ template <class T> | |||
| #else | |||
| # include <__config> | |||
|
|
|||
| # if _LIBCPP_STD_VER < 23 && defined(_LIBCPP_STDATOMIC_H) | |||
There was a problem hiding this comment.
FWIW: Android might be better served by checking for a macro like kill_dependency or atomic_load instead of _LIBCPP_STDATOMIC_H. (The previous version of this compatibility check looked for kill_dependency. #83351) Using kill_dependency or atomic_load, the Bionic stdatomic.h and the libc++ atomic headers could be included simultaneously.
The Android/Bionic stdatomic.h never defines kill_dependency. It does define atomic_load for the C dialect, but for C++, it does not define atomic_load, because it delegates to atomic instead and has a using std::atomic_load; declaration. (The Bionic stdatomic.h has delegated to atomic since 2014 and was the inspiration for the current C++23 behavior.)
Maybe it's possible for Bionic to drop its C++ support from its stdatomic.h, by switching users to C++23 instead. That seems like a good goal in the long-run. I think Android can carry a local LLVM patch, though, too, if we need to.
There was a problem hiding this comment.
Please let me know what you think about the latest update.
|
Still debugging, but I wanted to give a heads up that we're hitting build issues after this, e.g. I think that before this change, we got the |
|
That would make sense: this change goes back to the status quo that C's |
#95498 implemented a libc++ extension where <stdatomic.h> would forward to 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. 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.