This is an archive of the discontinued LLVM Phabricator instance.

[libc++][Android] Adjust FP conversion tests for Bionic
Needs ReviewPublic

Authored by rprichard on Dec 6 2022, 7:12 PM.

Details

Reviewers
danalbert
Group Reviewers
Restricted Project
Summary
  1. Bionic also includes '+' for NAN

However, it did not do so for Android L (APIs 21 and 22), so define a
compiler macro, ANDROID_PRINTF_NAN_NO_SIGN, to adjust the behavior of
the facet.num.put.members/put_long_double.pass.cpp test.

  1. Detect broken strtold of NAN on LP64

Converting the string "NAN" to a long double is broken on 64-bit
Android prior to Android O (API 26), so define
ANDROID_BROKEN_STRTOLD_NAN to disable parts of two tests.

See https://android-review.googlesource.com/q/Id7d46ac2d8acb8770b5e8c445e87cfabfde6f111

Diff Detail

Event Timeline

rprichard created this revision.Dec 6 2022, 7:12 PM
rprichard requested review of this revision.Dec 6 2022, 7:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 7:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard updated this revision to Diff 480743.Dec 6 2022, 7:21 PM

L MR1 (5.1) is API 22.

rprichard edited the summary of this revision. (Show Details)Dec 6 2022, 7:22 PM

I think we can use a simpler test, WDYT?

libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp
10736

Would this work too?

rprichard added inline comments.Dec 12 2022, 4:07 PM
libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp
10736

It ties the expected test output to the build API level of the test, which may not be what we want.

i.e. There are three API levels:

  • the build API level of libc++ itself
  • the build API of the libc++ tests (or the application code)
  • the runtime API level of the device

Currently for the NDK, the libc++ build API is typically 19 or 21, the app build API is typically low-ish (maybe 21/23?), and the runtime API steadily increases as people buy newer devices. It's normal for the build API for application code to be low (to target all the old devices in the wild), and the NDK only packages a single libc++_{shared.so,static.a} per CPU architecture.

This use of __ANDROID_API__ would break the test config in D139147, where the build API for libc++ and the tests is 21 (AndroidNDK.cmake, llvm-libc++abi-android-ndk.cfg.in), but the runtime API is 33 (Dockerfile.android).

A few nits and I'm happy.

libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp
10736

I see, can you add the gist of this information to the comment in features.py.

10736

We typically only use _LIBCPP for configuration options in libc++ itself and not for the test suite.

rprichard updated this revision to Diff 526857.May 30 2023, 5:44 PM
rprichard edited the summary of this revision. (Show Details)

Add a comment explaning why we don't just check __ANDROID_API__.

Also disable tests that convert a "NAN" string to long double on 64-bit targets. This bug was fixed in Android O. See https://android-review.googlesource.com/q/Id7d46ac2d8acb8770b5e8c445e87cfabfde6f111.

Remove the __LIBCPP_TESTING_ prefix from the macros in favor of:

  • ANDROID_PRINTF_NAN_NO_SIGN
  • ANDROID_BROKEN_STRTOLD_NAN
rprichard retitled this revision from [libc++][Android] Bionic also includes + for NAN to [libc++][Android] Adjust FP conversion tests for Bionic.May 30 2023, 5:45 PM
rprichard edited the summary of this revision. (Show Details)

Can you rebase this patch so we can see the CI run. (Mainly to make sure it doesn't break things unexpectedly.)

libcxx/test/std/strings/string.conversions/stold.pass.cpp
48
115
libcxx/utils/libcxx/test/features.py
169
175

This is the prefix we use in our test suite.

188

How about testing this macro at line 181 insted?

189