Condition usage of locale stdlib functions on Android API version

[thomasanderson]

Summary

Some *_l functions were not available in some versions of Bionic. This CL
checks that the NDK version supports the functions, and if not, falls back
on the corresponding functions that don't take a locale.

[thomasanderson]

Peter ptal.

This is likely better fixed in libandroid_support, but for some reason they've been removing these functions:
https://android.googlesource.com/platform/ndk/+/5e6c498d97210342a575640a891be4a4230be602
https://android.googlesource.com/platform/ndk/+/94af4fc04d8a9f1fdc9228a1c737c2899b48f6f9

We could add them back, but then it would require another ndk roll + updating.

[pcc]

It looks like they have been removing them because they are being added to the bionic headers here:
https://android.googlesource.com/platform/bionic/+/master/libc/include/android/legacy_stdlib_inlines.h
and I guess the idea is that bionic headers shipped in the NDK will be consistent with android_support headers.

The strto*_l fallbacks are supposed to already be handled in libc++ by this set of inline definitions:
https://github.com/llvm-mirror/libcxx/blob/master/include/support/xlocale/__strtonum_fallback.h

But then in rL324534 @danalbert removed the #include that brought in that header on Android, presumably because of the conflict between these two sets of inline definitions. So now the libc++ headers depend on an unreleased (?) NDK, which doesn't seem great.

Ideally we should keep platform-specific hacks out of the main codebase, so maybe a better fix would be to bring rL324534 back with a test that the NDK version (__NDK_MAJOR__ in android/ndk-version.h, apparently) is <= 16?

[danalbert]

So now the libc++ headers depend on an unreleased (?) NDK, which doesn't seem great.

¯\_(ツ)_/¯ from the NDK perspective it's also an unreleased version of libc++ (the released versions being those that are included in an NDK release). libc++, libandroid_support, and the bionic headers do need to be kept in sync.

Not all of Android's/the NDK's (I don't think we have a different set of non-xfail patches between the two, but they update on a different schedule and therefore branch, so idr for certain) patches have been upstreamed to libc++ (mostly haven't found time), so it's inadvisable to use the LLVM upstream as-is on Android since that's the less tested path (not to mention you'll potentially make yourself incompatible with other Android binaries).

If upstream wants to maintain support for obsolete NDK versions (I certainly don't), doing like @pcc says and moving this check out of the callsites and instead using it to decide whether or not to include the fallback header would be preferable.

[thomasanderson]

Add back include to support/xlocale/__strtonum_fallback.h

[pcc]

LGTM

[thomasanderson]

Thanks, Peter. Could you land this for me? Or are we waiting for additional reviewers?

[pcc]

It looks like this patch conflicts with rL330045. That patch would seem to cause the same conflict with the unreleased NDK that rL324524 was trying to fix, so probably the right thing to do would to make this patch revert that one at the same time?

[pcc]

In D46558#1094919, @pcc wrote:

It looks like this patch conflicts with rL330045. That patch would seem to cause the same conflict with the unreleased NDK that rL324524 was trying to fix, so probably the right thing to do would to make this patch revert that one at the same time?

(To be clear, by "that one" I meant rL330045)

[thomasanderson]

Reverted https://reviews.llvm.org/rL330045

[thomasanderson]

More context in diff

[thomasanderson]

Hm.. It looks like ToT libc++ is only missing 3 functions: strtoll_l, strtoull_l, and strtold_l. We could just add these in locale_bionic.h after line 33 (all of them were added in ANDROID_API 21 https://android.googlesource.com/platform/bionic/+/master/libc/include/stdlib.h#194). These functions are not in __posix_l_fallback.h

Should we do this or just land the CL as-is, which reverts https://reviews.llvm.org/rL330045 and includes __strtonum_fallback.h. I think I prefer the former.

[pcc]

Either would work for me, but I think the former would break the unreleased NDK (or, at least, break it further since it looks like things are currently broken due to conflicting definitions) so let's see what @danalbert thinks. Your current change LGTM.

[thomasanderson]

Ah, ok. In that case, I think I prefer this change as-is, so long as @danalbert is ok with it.

[danalbert]

include/__locale

27

Except when it gained them in 26. strtof_l, strtod_l, and strtol_l are all needed until __ANDROID_API__ >= 26. I don't think this patch works if you're targeting 21 <= __ANDROID_API__ < 26.

include/support/android/locale_bionic.h

30

Probably deserves a comment.

[mclow.lists]

I'm fine with this, since it is localized (sorry) inside the Android bits.
I have no opinion on the >= 21 or >= 26 discussion.

[thomasanderson]

Fix the case where 21 <= ANDROID_API < 26 and NDK_VERSION <= 16

[thomasanderson]

I hope my understanding is correct -- in the latest patch, the behavior is:

• if NDK_MAJOR > 16
  • do nothing
• otherwise
  • if ANDROID_API < 21
    • Fallback to __strtonum_fallback.h which provides 7 locale stdlib functions
  • otherwise if ANDROID_API < 26
    • Add only 3 missing locale stdlib functions

[pcc]

LGTM

I will commit.

[eugenis]

libcxx/trunk/include/support/android/locale_bionic.h

30 ↗

(On Diff #147206)

This requires NDK r16 or higher, the header is not present in r15.
I'm now updating the buildbot to r16, but I imagine not all users can upgrade easily.
Is there a way to preserve compatibility with older releases?

[pcc]

libcxx/trunk/include/support/android/locale_bionic.h

30 ↗

(On Diff #147206)

I guess we could use the __libcpp_has_include macro here. If the header is not present, we can assume that the NDK version is old.

[danalbert]

libcxx/trunk/include/support/android/locale_bionic.h

30 ↗

(On Diff #147206)

That will conflict with a (not yet upstreamed) patch that we needed to get this to build in AOSP: https://android-review.googlesource.com/c/platform/external/libcxx/+/709923.

There isn't a good a way to differentiate between Android platform and pre-r16 NDK that I know of.

Has chrome updated to r17 yet? When they do we can just revert this patch.

[thomasanderson]

libcxx/trunk/include/support/android/locale_bionic.h

30 ↗

(On Diff #147206)

Chrome is still using r16. To get it updated, I'd bug as agrieve@

include/__locale

 #elif defined(_AIX)
 # include <support/ibm/xlocale.h>
 #elif defined(__ANDROID__)
-# include <support/android/locale_bionic.h>
+// Android gained the locale aware functions in L (API level 21)
+# include <android/api-level.h>
+# if __ANDROID_API__ <= 20
+#  include <support/android/locale_bionic.h>
+# endif
 #elif defined(__sun__)
 # include <xlocale.h>
 # include <support/solaris/xlocale.h>

include/support/android/locale_bionic.h

 #ifndef _LIBCPP_SUPPORT_ANDROID_LOCALE_BIONIC_H
 #define _LIBCPP_SUPPORT_ANDROID_LOCALE_BIONIC_H
 
+#include <android/ndk-version.h>
+
 #if defined(__BIONIC__)
 
 #ifdef __cplusplus

 }
 #endif
 
-#if defined(__ANDROID__)
-
-#include <android/api-level.h>
-
-// Android gained most locale aware functions in L (API level 21)
-#if __ANDROID_API__ < 21
 #include <support/xlocale/__posix_l_fallback.h>
+#if __NDK_MAJOR__ <= 16
+#include <support/xlocale/__strtonum_fallback.h>
 #endif
 
-// The strto* family was added in O (API Level 26)
-#if __ANDROID_API__ < 26
-
-#if defined(__cplusplus)
-extern "C" {
-#endif
-
-inline _LIBCPP_ALWAYS_INLINE float strtof_l(const char* __nptr, char** __endptr,
-                                            locale_t) {
-  return ::strtof(__nptr, __endptr);
-}
-
-inline _LIBCPP_ALWAYS_INLINE double strtod_l(const char* __nptr,
-                                             char** __endptr, locale_t) {
-  return ::strtod(__nptr, __endptr);
-}
-
-inline _LIBCPP_ALWAYS_INLINE long strtol_l(const char* __nptr, char** __endptr,
-                                           int __base, locale_t) {
-  return ::strtol(__nptr, __endptr, __base);
-}
-
-#if defined(__cplusplus)
-}
-#endif
-
-#endif // __ANDROID_API__ < 26
-
-#endif // defined(__ANDROID__)
-
 #endif // defined(__BIONIC__)
 #endif // _LIBCPP_SUPPORT_ANDROID_LOCALE_BIONIC_H