[libc++][Android] Always redirect <stdatomic.h> to <atomic> #143036
Conversation
When targeting Android, enable the <stdatomic.h> to <atomic> redirection for C++ language dialects before C++23, because this redirection historically existed for Android's C++ toolchain. Currently, the Android toolchain achieves this redirection with a stdatomic.h in the sysroot (from Bionic) that defines the C API either directly or by aliasing std identifiers from <atomic>. The Android team's LLVM build scripts then copy Bionic's header over the one in the Clang resource directory. Hopefully, by moving Android's redirection into LLVM itself, the Android stdatomic.h situation can be simplified eventually.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Ryan Prichard (rprichard) ChangesWhen targeting Android, enable the <stdatomic.h> to <atomic> redirection for C++ language dialects before C++23, because this redirection historically existed for Android's C++ toolchain. Currently, the Android toolchain achieves this redirection with a stdatomic.h in the sysroot (from Bionic) that defines the C API either directly or by aliasing std identifiers from <atomic>. The Android team's LLVM build scripts then copy Bionic's header over the one in the Clang resource directory. Hopefully, by moving Android's redirection into LLVM itself, the Android stdatomic.h situation can be simplified eventually. Full diff: https://github.com/llvm/llvm-project/pull/143036.diff 5 Files Affected:
diff --git a/libcxx/include/atomic b/libcxx/include/atomic
index 75af5de33ca4c..fba7040f6900c 100644
--- a/libcxx/include/atomic
+++ b/libcxx/include/atomic
@@ -598,7 +598,10 @@ template <class T>
# define _LIBCPP_STDATOMIC_H_HAS_DEFINITELY_BEEN_INCLUDED 0
# endif
-# if _LIBCPP_STD_VER < 23 && _LIBCPP_STDATOMIC_H_HAS_DEFINITELY_BEEN_INCLUDED
+// The Android LLVM toolchain has historically allowed combining the <atomic>
+// and <stdatomic.h> headers in dialects before C++23, so for backwards
+// compatibility, preserve that ability when targeting Android.
+# if _LIBCPP_STD_VER < 23 && !defined(__ANDROID__) && _LIBCPP_STDATOMIC_H_HAS_DEFINITELY_BEEN_INCLUDED
# error <atomic> is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
# endif
diff --git a/libcxx/include/stdatomic.h b/libcxx/include/stdatomic.h
index 2991030eee456..dc1a955b69cb6 100644
--- a/libcxx/include/stdatomic.h
+++ b/libcxx/include/stdatomic.h
@@ -126,7 +126,10 @@ using std::atomic_signal_fence // see below
# pragma GCC system_header
# endif
-# if defined(__cplusplus) && _LIBCPP_STD_VER >= 23
+// The Android LLVM toolchain has historically allowed combining the <atomic>
+// and <stdatomic.h> headers in dialects before C++23, so for backwards
+// compatibility, preserve that ability when targeting Android.
+# if defined(__cplusplus) && (_LIBCPP_STD_VER >= 23 || defined(__ANDROID__))
# include <atomic>
# include <version>
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 349dc51aaa0e6..c997e4b792459 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,7 +7,10 @@
//===----------------------------------------------------------------------===//
// UNSUPPORTED: no-threads
-// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// On Android, libc++'s <stdatomic.h> header always redirects to <atomic>, even before C++23.
+// UNSUPPORTED: c++03
+// UNSUPPORTED: (c++11 || c++14 || c++17 || c++20) && !android
// This test verifies that <stdatomic.h> redirects to <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
index a788ea32dddc8..7d14fa308bf06 100644
--- a/libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp
+++ b/libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp
@@ -9,6 +9,9 @@
// UNSUPPORTED: no-threads
// REQUIRES: c++03 || c++11 || c++14 || c++17 || c++20
+// On Android, libc++'s <stdatomic.h> header always redirects to <atomic>, even before C++23.
+// XFAIL: android
+
// No diagnostic gets emitted when we build with modules.
// XFAIL: clang-modules-build
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
index a8a99e6937f31..5c966c6bca6e2 100644
--- 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
@@ -21,6 +21,9 @@
// 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
+// On Android, libc++'s <stdatomic.h> header always redirects to <atomic>, even before C++23.
+// XFAIL: android
+
#include <atomic>
#include <stdatomic.h>
#include <type_traits>
|
|
Some more context:
I'm trying to make the Android situation less clumsy, by making the upstream LLVM headers acceptable to use as-is, for the Android toolchain team's build of LLVM. I think our team would actually prefer a way to enable the backport even when we target non-Android, but I don't know if this desire warrants adding a config flag (e.g. to I'm wondering if upstream libc++ has any thoughts on restoring the backport, but only when targeting Android. |
| // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20 | ||
|
|
||
| // On Android, libc++'s <stdatomic.h> header always redirects to <atomic>, even before C++23. | ||
| // UNSUPPORTED: c++03 |
There was a problem hiding this comment.
why is c++03 on a separate line now?
There was a problem hiding this comment.
It probably shouldn't be on a separate line -- it looks like libc++ does support <atomic> for C++03.
Though now I'm wondering if the UNSUPPORTED directive ought to be XFAIL instead. I think the libc++ project usually prefers XFAIL when we know that a test should fail.
There was a problem hiding this comment.
I suppose UNSUPPORTED might be fine, though, because for previous language dialects, we have the incompatible_with_stdatomic.verify.cpp test already.
There was a problem hiding this comment.
I think I'd move back c++03 on the same line as the other ones. I'm neutral on XFAIL vs UNSUPPORTED -- as you mention we already have a dedicated test for the inverse behavior.
There was a problem hiding this comment.
I do have a bit of a concern with making this part of upstream: that makes libc++ non-conforming in pre-C++23 modes (in particular, libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.cxx23.compile.pass.cpp doesn't pass).
Have you folks looked into removing that historical extension on the Android side? If that's something Android would be open to, I could suggest the following:
- We take this patch and upstream your support, but we also add a deprecation warning and a release note that this will be dropped in the future
- In 1-2 releases (there's flexibility here), we remove the extension, while optionally providing an escape hatch to bring it back.
- After one more release, we remove the escape hatch.
That solution:
- Gives plenty of time for Android users to migrate / fix their code (between 1 year and 1.5 years)
- Allows Android to upstream this diff
- Allows libc++ to eventually drop this debt and ensure conformance on all platforms, like we aim to
Would something like this be reasonable from Android's side?
| // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20 | ||
|
|
||
| // On Android, libc++'s <stdatomic.h> header always redirects to <atomic>, even before C++23. | ||
| // UNSUPPORTED: c++03 |
There was a problem hiding this comment.
I think I'd move back c++03 on the same line as the other ones. I'm neutral on XFAIL vs UNSUPPORTED -- as you mention we already have a dedicated test for the inverse behavior.
our problem is (as you guessed) that this is exposed to app developers and partners (both SoC vendors and device manufacturers). we've tried things like you suggest in the past, and the trouble is that people fall into one of two groups: the -Werror group (who you've now broken) and the "ain't nobody got time for warnings" group (who won't do anything until/unless you break them). given that there's no advantage to breaking anyone, it's hard to justify this. (don't get me started on the transitive include "cleanup" in c++23 ... that's going to have everyone mad at me :-( ) for /<stdatomic.h> is there a way we can move the hackery out into our header which does an
(ah, the joys of working for apple :-) ) |
As a historic note, P0943 stemmed from our attempt to standardize the Android-specific extension. I agree with enh about not breaking our users. Moreover, spending effort to fix Android codebase for the pre-C++-23 mode, while it is expected to work in C++23 and later, doesn't seem like a valuable use of resources. Upgrading to C++23 is the viable strategy here. We can foresee updating Android platform (and vendors) to C++-23 in the next 1-2 years, but breaking app developers is something we want to avoid. |
When targeting Android, enable the <stdatomic.h> to redirection for C++ language dialects before C++23, because this redirection historically existed for Android's C++ toolchain. Currently, the Android toolchain achieves this redirection with a stdatomic.h in the sysroot (from Bionic) that defines the C API either directly or by aliasing std identifiers from . The Android team's LLVM build scripts then copy Bionic's header over the one in the Clang resource directory.
Hopefully, by moving Android's redirection into LLVM itself, the Android stdatomic.h situation can be simplified eventually.