Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[libc++][Android] Enable libc++ testing on Android
ClosedPublic

Authored by rprichard on Dec 1 2022, 2:50 PM.

Details

Reviewers
EricWF
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGd173ce4a670e: [libc++][Android] Support libc++ testing on Android (#69274)
Summary

This patch adds libc++ support for Android L (Android 5.0+) and up,
tested using the Android team's current compiler, a recent version of
the AOSP sysroot, and the x86[-64] Android Emulator.

CMake and Lit Configuration:

Add runtimes/cmake/android/Arch-${ARCH}.cmake files that configure
CMake to cross-compile to Android without using CMake's built-in NDK
support (which only works with an actual packaged NDK).

Add libcxx/cmake/caches/AndroidNDK.cmake that builds and tests libc++
(and libc++abi) for Android. This file configures libc++ to match what
the NDK distributes, e.g.:

  • libc++_shared.so (includes libc++abi objects, there is no libc++abi.so). libunwind is linked statically but not exported.
  • libc++_static.a (does not include libc++abi) and libc++abi.a
  • std::__ndk1 namespace
  • All the libraries are built with __ANDROID_API__=21, even when they are linked to something targeting a higher API level.

(However, when the Android LLVM team builds these components, they do
not use these CMake cache files. Instead they use Python scripts to
configure the builds. See
https://android.googlesource.com/toolchain/llvm_android/.)

Add llvm-libc++[abi].android-ndk.cfg.in files that test the Android
NDK's libc++_shared.so. These files can target old or new Android
devices. The Android LLVM team uses these test files to test libc++ for
both arm/arm64 and x86/x86_64 architectures.

The Android testing mode works by setting %{executor} to adb_run.py,
which uses adb push and adb shell to run tests remotely. adb_run.py
always runs tests as the "shell" user even on an old emulator where
"adb unroot" doesn't work. The script has workarounds for old Android
devices. The script uses a Unix domain socket on the host
(--job-limit-socket) to restrict concurrent adb invocations. Compiling
the tests is a major part of libc++ testing run-time, so it's desirable
to exploit all the host cores without overburdening the test devices,
which can have far fewer cores.

BuildKite CI:

Add a builder to run-buildbot, android-ndk-*, that uses Android Clang
and an Android sysroot to build libc++, then starts an Android emulator
container to run tests.

Run the emulator and an adb server in a separate Docker container
(libcxx-ci-android-emulator), and create a separate Docker image for
each emulator OS system image. Set ADB_SERVER_SOCKET to connect to the
container's adb server. Running the only adb server inside the
container makes cleanup more reliable between test runs, e.g. the adb
client doesn't create a ~/.android directory and the adb server can
be restarted along with the emulator using docker stop/run. (N.B. The
emulator insists on connecting to an adb server and will start one
itself if it can't connect to one.)

The suffix to the android-ndk-* job is a label that concisely specifies
an Android SDK emulator image. Enable CI testing on two configurations:

  • "system-images;android-21;default;x86" ==> 21-def-x86
  • "system-images;android-33;google_apis;x86_64" ==> 33-goog-x86_64

Mark tests XFAIL for Android:

Add three Android lit features:

  • android
  • android-device-api=(21,22,23,...)
  • LIBCXX-ANDROID-FIXME (for failures that need follow-up work)

Mark failing test with XFAIL (and sometimes UNSUPPORTED):

  • Disable various FS tests (because SELinux blocks FIFO and hard linking).
  • Disable various locale tests (because Bionic has limited locale support).

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 8 2023, 1:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard edited the summary of this revision. (Show Details)Jul 8 2023, 1:10 AM
rprichard updated this revision to Diff 538344.Jul 8 2023, 3:24 AM
rprichard edited the summary of this revision. (Show Details)

Run 'docker info' before 'docker image inspect ...' to make sure that Docker is working.

Switch adb_run's job limiting from POSIX semaphore to a Unix domain socket. Instead of needing a SIGINT handler in adb_run.py to release the semaphore, we can instead rely on the OS to close the socket connection.

Remove the cxx_add_android_flags function from libcxx/CMakeLists.txt that was adding -Wno-unused-command-line-argument. Previously, this patch used CMake's NDK support, which was adding an unnecessary -stdlib=libc++. Now that the patch uses just an Android sysroot with an ordinary LLVM build, CMake does not add -stdlib=libc++, so we don't need to pass -Wno-unused-command-line-argument.

rprichard edited the summary of this revision. (Show Details)Jul 14 2023, 12:59 AM
rprichard updated this revision to Diff 540310.Jul 14 2023, 1:08 AM
rprichard edited the summary of this revision. (Show Details)

rebase

rprichard added a comment.EditedJul 14 2023, 1:24 AM

I split off duplicated the Dockerfile changes to D155271.

rprichard updated this revision to Diff 540318.Jul 14 2023, 1:33 AM

In _job_limit_socket_thread, discard the address returned from accept.

rprichard updated this revision to Diff 540323.Jul 14 2023, 1:51 AM

utils/adb_run.py: Print syncing command lines with -v.

utils/libcxx/test/android.py: Add a missing type annotation to make mypy happy.

I copied the two ABI lists into D155341. I think this patch is ready for review now.

rprichard updated this revision to Diff 540580.Jul 14 2023, 3:37 PM

Fix indentation in libcxx/test/support/filesystem_test_helper.h to make git-clang-format happy.

Can you make a stacked review of this patch and have the two split of patches as parent revisions. Then we review what really is intended to land. After the Docker file is approved we need to rebuild the Docker image so it's available for all runners.

rprichard updated this revision to Diff 541233.Jul 17 2023, 2:17 PM

Now that D155271 and D155341 are parent revisions of this revision, remove their edits from this revision. I expect this change to break the Android testing because the Android abilist files will be missing.

ldionne added inline comments.Aug 10 2023, 10:06 AM
libcxx/cmake/caches/AndroidNDK.cmake
26–29

I think what you want here instead is

CMAKE_C_COMPILER_WORKS = True
CMAKE_CXX_COMPILER_WORKS = True

like in https://reviews.llvm.org/D150766. And in the future this shouldn't be required in caches, it should just work out of the box.

40–41

I would suggest treating those as entirely different builds and giving them their own cache files. There are ways to avoid duplication (e.g. include(AndroidBase.cmake)) without adding a ad-hoc knob here.

libcxx/test/configs/llvm-libc++-android.cfg.in
15–23 ↗(On Diff #541233)

Here, you should be able to simply specify --param target_triple=<WHATEVER> when running lit, and get rid of target_api altogether.

libcxx/test/libcxx/modules_include.gen.py
35 ↗(On Diff #541233)

This needs to be updated when you rebase, there's two references in this file now.

libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp
11–13 ↗(On Diff #541233)

Per live review, this comment can be updated.

14 ↗(On Diff #541233)

For now, I would prefer that we keep this LIBCXX-ANDROID-FIXME so we can have a separate discussion about how to handle low-memory environments in the test suite (more generally).

libcxx/utils/ci/buildkite-pipeline.yml
1092 ↗(On Diff #541233)

I think we should make the run-buildbot script stop the emulator unconditionally. It would be nice to keep the property that all the commands in the buildkite pipeline are just direct calls to run-buildbot (with a few exceptions).

1097 ↗(On Diff #541233)

Instead of this, you could run libcxx/utils/ci/run-buildbot android-ndk-<VERSION>, similarly to what we do for the macOS backdeployment?

libcxx/utils/ci/run-buildbot
50

So this goes away.

720–723

Not needed anymore.

726–733

I feel like this is a workaround for not having the equivalent of an apt.llvm.org build of the toolchain. We should make this available on the system we're testing on instead (which probably means building it into the docker image).

libcxx/utils/ci/vendor/android/Dockerfile.emulator
2

Note to self: review stopped here.

rprichard updated this revision to Diff 549826.Aug 14 2023, 1:49 AM
rprichard marked 9 inline comments as done.

Address some of the review comments.

Mark some of the comments as Done.

rprichard updated this revision to Diff 549856.Aug 14 2023, 3:07 AM

Disable flaky notify_one.pass.cpp test for Android, and remove the unnecessary adb socat workaround now that adb -a start-server works.

rprichard edited the summary of this revision. (Show Details)Aug 14 2023, 3:12 AM
ldionne added inline comments.Aug 14 2023, 11:48 AM
libcxx/utils/ci/vendor/android/Dockerfile.emulator.base
2 ↗(On Diff #549856)

Is it necessary to have a .base and a non-base Dockerfile? If so, let's leave a comment to explain why. Otherwise, consider merging both files.

22 ↗(On Diff #549856)

Can we use a URL that points to the latest version so that we get a new version of the command-line tools when we rebuild that docker image?

Or do we want to be intentional about the version we're using here so that we only bump it when we really want to? If so, what is going to be the process for bumping it up? Is that something that you or other Android folks can commit to doing on a regular basis? Otherwise, this will be forgotten forever and will never be updated.

libcxx/utils/ci/vendor/android/build-toolchain.sh
1 ↗(On Diff #549856)

Note to self: This would also go away if we downloaded a pre-built toolchain instead.

libcxx/utils/ci/vendor/android/emulator-wait-for-ready.sh
24
libcxx/utils/ci/vendor/android/start-emulator.sh
28

This assumes that the docker image named $(docker_image_of_emu_img ${EMU_IMG}) is somehow available on whatever's running the CI. We should probably instead publish the emulator's Docker image somewhere (maybe the same place we publish our other docker images) and use that.

libunwind/cmake/caches/Android.cmake
1 ↗(On Diff #549856)

This would go away if we downloaded a pre-built toolchain and used that toolchain's libunwind instead.

rprichard marked 10 inline comments as done.
rprichard edited the summary of this revision. (Show Details)

Address review comments.

rprichard edited the summary of this revision. (Show Details)Aug 15 2023, 10:58 PM
rprichard added inline comments.
libcxx/utils/ci/vendor/android/Dockerfile.emulator.base
22 ↗(On Diff #549856)

Can we use a URL that points to the latest version so that we get a new version of the command-line tools when we rebuild that docker image?

I asked about this in a Google-internal discussion group but haven't heard anything yet. It doesn't look possible at the moment, unless we want to download an XML file and parse it to find the latest stable commandlinetools package.

rprichard updated this revision to Diff 550984.Aug 16 2023, 9:06 PM
rprichard marked an inline comment as done.

Rebase.

rprichard updated this revision to Diff 551399.EditedAug 17 2023, 11:52 PM

The last rebase accounted for about a month's worth of changes in LLVM. Fix the Android tests that were broken by the rebase:

  • fmemopen isn't available until Android M (API 23).
  • The std::system_error string has no OS error component in several std::print tests. I suspect libc++ itself is broken here. See https://reviews.llvm.org/D150044#4597254.
  • The to_std_containers.pass.cpp test is segfaulting on 32-bit x86 Android L. The test needs to be compiled with -mstackrealign to work around an OS bug in pre-N.

For the -mstackrealign issue, see:

smeenai added inline comments.Aug 24 2023, 4:25 PM
libcxx/cmake/caches/AndroidNDK.cmake
14

You aren't caching this, so it won't have any effect. It's also the default though, so you don't need to specify it explicitly.

rprichard updated this revision to Diff 557663.Oct 10 2023, 1:30 AM

Use stdin=subprocess.DEVNULL for adb_run.py subprocesses, except for the main adb shell invocation where we want to forward stdin. This change was necessary to fix stdin forwarding on a Pixel 6 (oriole) device. (However, the API 33 emulator tests passed even without this change. Not sure why.)

Disable three stdin tests on pre-shell_v2 devices for now:

  • libcxx/test/std/input.output/iostream.objects/narrow.stream.objects/cin.sh.cpp
  • libcxx/test/std/input.output/iostream.objects/wide.stream.objects/wcin-imbue.sh.cpp
  • libcxx/test/std/input.output/iostream.objects/wide.stream.objects/wcin.sh.cpp

The tests pass on shell_v2 devices. I _think_ that happens because adb shell <command> uses a pipe for stdin, and this pipe is closed on the device when echo closes the pipe on the host. Closing stdin signals to the test program to stop waiting for more input.

With pre-shell_v2 devices, the test hangs. Pre-shell_v2 devices always allocate a pty, even for adb shell <command>. It appears that the pty is in line editing mode, so it's necessary to send a newline or carriage-return after the 1234 to propagate the line of input into the test program's stdin. Maybe there's some way to fix stdin forwarding in adb_run.py, (e.g. by reading chunks of input in adb_run.py, framing them, then passing each frame through the pty to some shell code on the device that unframes the input and forwards it to the test program via a pipe).

rprichard updated this revision to Diff 557666.Oct 10 2023, 2:08 AM

Cache the LIBCXX_ABI_VERSION variable in AndroidNDK.cmake.

rprichard marked an inline comment as done.Oct 10 2023, 2:12 AM
rprichard added inline comments.
libcxx/cmake/caches/AndroidNDK.cmake
14

Thanks for noticing this! I uploaded a fix.

EricWF requested changes to this revision.Oct 11 2023, 11:02 AM
EricWF added a subscriber: EricWF.

Can we break this up into smaller changes?

Like, lets land the test suite comments first. Because we can land those right away.

Then maybe split out the changes to ci/vendor/android/.
And then finally the rest of the changes in a third review?

This revision now requires changes to proceed.Oct 11 2023, 11:02 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Oct 19 2023, 1:58 PM
This revision was automatically updated to reflect the committed changes.