[libc++][Android] Restrict use of mblen/towctrans/wctrans #116147
Conversation
These functions weren't added until API 26 (Android 8.0), but libc++ is supported for API 21 and up. These APIs are undeclared as of r.android.com/3216959.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Ryan Prichard (rprichard) ChangesThese functions weren't added until API 26 (Android 8.0), but libc++ is supported for API 21 and up. These APIs are undeclared as of r.android.com/3216959. Full diff: https://github.com/llvm/llvm-project/pull/116147.diff 4 Files Affected:
diff --git a/libcxx/test/std/depr/depr.c.headers/stdlib_h.pass.cpp b/libcxx/test/std/depr/depr.c.headers/stdlib_h.pass.cpp
index 587c6b6e10ddb6..8199fabe3d1a68 100644
--- a/libcxx/test/std/depr/depr.c.headers/stdlib_h.pass.cpp
+++ b/libcxx/test/std/depr/depr.c.headers/stdlib_h.pass.cpp
@@ -141,7 +141,10 @@ int main(int, char**) {
wchar_t* pw = 0;
const wchar_t* pwc = 0;
char* pc = 0;
+ // mblen was added in Android API 26.
+#if !defined(__ANDROID__) || __ANDROID_API__ >= 26
ASSERT_SAME_TYPE(int, decltype(mblen("",0)));
+#endif
ASSERT_SAME_TYPE(int, decltype(mbtowc(pw,"",0)));
ASSERT_SAME_TYPE(int, decltype(wctomb(pc,L' ')));
ASSERT_SAME_TYPE(size_t, decltype(mbstowcs(pw,"",0)));
diff --git a/libcxx/test/std/depr/depr.c.headers/wctype_h.compile.pass.cpp b/libcxx/test/std/depr/depr.c.headers/wctype_h.compile.pass.cpp
index 35b294532b2bd2..ebbec565c1ab7d 100644
--- a/libcxx/test/std/depr/depr.c.headers/wctype_h.compile.pass.cpp
+++ b/libcxx/test/std/depr/depr.c.headers/wctype_h.compile.pass.cpp
@@ -109,5 +109,8 @@ ASSERT_SAME_TYPE(int, decltype(iswctype(w, wct)));
ASSERT_SAME_TYPE(wctype_t, decltype(wctype("")));
ASSERT_SAME_TYPE(wint_t, decltype(towlower(w)));
ASSERT_SAME_TYPE(wint_t, decltype(towupper(w)));
+// towctrans and wctrans were added in Android API 26.
+#if !defined(__ANDROID__) || __ANDROID_API__ >= 26
ASSERT_SAME_TYPE(wint_t, decltype(towctrans(w, wctr)));
ASSERT_SAME_TYPE(wctrans_t, decltype(wctrans("")));
+#endif
diff --git a/libcxx/test/std/language.support/support.runtime/cstdlib.pass.cpp b/libcxx/test/std/language.support/support.runtime/cstdlib.pass.cpp
index a1f7e1143a1e9b..5f33c72f85bfbe 100644
--- a/libcxx/test/std/language.support/support.runtime/cstdlib.pass.cpp
+++ b/libcxx/test/std/language.support/support.runtime/cstdlib.pass.cpp
@@ -141,7 +141,10 @@ int main(int, char**)
wchar_t* pw = 0;
const wchar_t* pwc = 0;
char* pc = 0;
+ // mblen was added in Android API 26.
+#if !defined(__ANDROID__) || __ANDROID_API__ >= 26
static_assert((std::is_same<decltype(std::mblen("",0)), int>::value), "");
+#endif
static_assert((std::is_same<decltype(std::mbtowc(pw,"",0)), int>::value), "");
static_assert((std::is_same<decltype(std::wctomb(pc,L' ')), int>::value), "");
static_assert((std::is_same<decltype(std::mbstowcs(pw,"",0)), std::size_t>::value), "");
diff --git a/libcxx/test/std/strings/c.strings/cwctype.pass.cpp b/libcxx/test/std/strings/c.strings/cwctype.pass.cpp
index 5bc2531d6f6ac7..7460c36c357e7f 100644
--- a/libcxx/test/std/strings/c.strings/cwctype.pass.cpp
+++ b/libcxx/test/std/strings/c.strings/cwctype.pass.cpp
@@ -111,8 +111,11 @@ int main(int, char**) {
ASSERT_SAME_TYPE(std::wctype_t, decltype(std::wctype("")));
ASSERT_SAME_TYPE(std::wint_t, decltype(std::towlower(w)));
ASSERT_SAME_TYPE(std::wint_t, decltype(std::towupper(w)));
+ // towctrans and wctrans were added in Android API 26.
+#if !defined(__ANDROID__) || __ANDROID_API__ >= 26
ASSERT_SAME_TYPE(std::wint_t, decltype(std::towctrans(w, std::wctrans_t())));
ASSERT_SAME_TYPE(std::wctrans_t, decltype(std::wctrans("")));
+#endif
return 0;
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
I'm fine with this, but I'd like @ldionne to approve as well.
There was a problem hiding this comment.
To understand, these Android API versions are like SDK versions, is that right? Does this basically mean that Android API 21 is the "SDK" released in 2014 as part of Android 5, so basically you'd support using a new libc++ against an Android SDK released in 2014?
What is the intended support policy for libc++ on Android? At what frequency is support for older platforms dropped? I'd just like to make sure that there is some reasonable policy in place to make sure we don't end up having to support ancient SDKs (which often doesn't add much value since mixing a new libc++ with an old SDK is a niche use case). For example, for Apple platforms we officially support only the latest released XCode (with the matching SDK).
It's more like the version of Android itself rather than the version of the SDK (NDK). Android 5.0 is Android Lollipop, which is also API level 21. (Android 5.1 is API level 22 instead.) (See https://apilevels.com.) Each Android release has three similar versions (API level, version number, and codename). The NDK contains a copy of libc++, and the NDK is on a separate release schedule from the Android OS. https://developer.android.com/ndk/downloads/revision_history When the NDK cuts a new release, with a recent version of libc++, the new NDK supports targeting older Android versions. This lets app developers use the latest Clang and libc++ even though they still need to support old versions of Android. I think the Android team would like libc++ to continue supporting an Android OS for as long as the NDK does. Currently the NDK supports API level 21 and up, but we expect to drop old API levels over time. (The minimum supported API level has been steadily marching up over the past several years. e.g. NDK r16 supported Android 4.0 and up -- API level 14.) I'll cc @DanAlbert who I think is more familiar with the policy on dropping support for old Android OSs. |
|
@ldionne I think this PR is still waiting for review. It's needed before I can update the version of Clang used to test libc++ on Android. |
There was a problem hiding this comment.
Sorry this fell between the cracks and I forgot to review again. Thanks for the clear explanation of Android NDK versions.
For these kinds of things in general, we tend to favour using XFAIL: android-ndk-<version> instead, since that prevents the accumulation of #ifdefs in the test suite. Would you be willing to use that instead?
We could use Details: We would need to do XFAIL based on both the target API and the NDK sysroot "version", but that's tricky because AFAIK the sysroot doesn't have a version on it. The problem is that the NDK's sysroot happened to declare these functions even for old API levels, even though they weren't available in libc.so. It was an old hack to make libc++ happy, IIRC. The sysroot was modified to stop declaring the functions in https://r.android.com/3216959. Of course, they still are declared for new enough API levels (26 and up). i.e. This new XFAIL would work with the new sysroot: ... but it would break the libc++ Android CI until the sysroot in the Docker image was updated (because the tests are currently passing for API 21, which is tested in CI). |
I think that should be fine with a
|
We can't use XFAIL yet because the old sysroot used in the Android CI Docker image still declares these functions even for pre-API 26. The new sysroot we want to switch stop drops the declarations, unless the code is targeting API 26 or newer.
rprichard
force-pushed
the
android-mblen-wctrans
branch
from
a014a49 to
a586d0e
Compare
|
Ok, I've updated the tests to be |
There was a problem hiding this comment.
If @ldionne doesn't have any further comments within the next few days I think this should be fine to land.
These functions weren't added until API 26 (Android 8.0), but libc++ is supported for API 21 and up.
These APIs are undeclared as of r.android.com/3216959.