Page MenuHomePhabricator

Condition usage of locale stdlib functions on Android API version
ClosedPublic

Authored by thomasanderson on May 7 2018, 4:00 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

thomasanderson created this revision.May 7 2018, 4:00 PM

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 added a subscriber: danalbert.May 7 2018, 4:50 PM

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?

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.

Add back include to support/xlocale/__strtonum_fallback.h

pcc accepted this revision.May 9 2018, 3:46 PM
pcc added reviewers: mclow.lists, EricWF.
pcc added a subscriber: llvm-commits.

LGTM

This revision is now accepted and ready to land.May 9 2018, 3:46 PM
thomasanderson edited the summary of this revision. (Show Details)May 9 2018, 4:50 PM
thomasanderson added a reviewer: thakis.

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

danalbert accepted this revision.May 10 2018, 11:29 AM
pcc added a subscriber: compnerd.May 10 2018, 11:57 AM

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 added a comment.May 10 2018, 11:59 AM
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)

More context in diff

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 added a comment.May 11 2018, 3:57 PM

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.

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

danalbert added inline comments.May 15 2018, 11:18 AM
include/__locale
27 ↗(On Diff #146241)

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 ↗(On Diff #146241)

Probably deserves a comment.

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

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

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
danalbert accepted this revision.May 16 2018, 3:33 PM
pcc accepted this revision.May 16 2018, 3:41 PM

LGTM

I will commit.

This revision was automatically updated to reflect the committed changes.
eugenis added inline comments.
libcxx/trunk/include/support/android/locale_bionic.h
30

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 added inline comments.Jun 28 2018, 2:07 PM
libcxx/trunk/include/support/android/locale_bionic.h
30

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 added inline comments.Jul 9 2018, 11:40 AM
libcxx/trunk/include/support/android/locale_bionic.h
30

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 added inline comments.Jul 9 2018, 11:55 AM
libcxx/trunk/include/support/android/locale_bionic.h
30

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