[libc++][Android] Enable libc++ testing using adb on an x86-64 emulator
Needs ReviewPublic
Actions

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

Details

Reviewers

None

Group Reviewers

Restricted Project
Restricted Project

Summary

Adds a "run-buildbot android-ndk" testing mode that builds libc++
using NDK r25b and runs the tests on an x86-64 API 33 emulator.

A Dockerfile.android builds a docker image based on
ldionne/libcxx-builder, adding the Android SDK and NDK. The Docker
container needs to be started with --privileged so it has access to
KVM.

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	10,400 ms	libcxx CI Clang-cl (no vcruntime exceptions) > llvm-libc++-shared-no-vcruntime-clangcl-cfg-in.std/thread/thread_mutex/thread_mutex_requirements/thread_sharedtimedmutex_requirements/thread_sharedtimedmutex_class::try_lock_shared_for.pass.cpp Script: -- : 'COMPILED WITH'; 'C:/Program Files/LLVM/bin/clang-cl.exe' C:\ws\w4\llvm-project\libcxx-ci\libcxx\test\std\thread\thread.mutex\thread.mutex.requirements\thread.sharedtimedmutex.requirements\thread.sharedtimedmutex.class\try_lock_shared_for.pass.cpp --driver-mode=g++ --target=x86_64-pc-windows-msvc -nostdinc++ -I C:/ws/w4/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/include/c++/v1 -I C:/ws/w4/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/include/c++/v1 -I C:/ws/w4/llvm-project/libcxx-ci/libcxx/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0 -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -nostdlib -L C:/ws/w4/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/lib -lc++ -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w4\llvm-project\libcxx-ci\build\clang-cl-no-vcruntime\test\std\thread\thread.mutex\thread.mutex.requirements\thread.sharedtimedmutex.requirements\thread.sharedtimedmutex.class\Output\try_lock_shared_for.pass.cpp.dir\t.tmp.exe

Event Timeline

rprichard created this revision.Dec 1 2022, 2:50 PM

Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 2:50 PM

Herald added subscribers: danielkiss, pengfei, arichardson. · View Herald Transcript

rprichard requested review of this revision.Dec 1 2022, 2:50 PM

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 1 2022, 2:50 PM

Herald added a reviewer: Restricted Project. · View Herald Transcript

Herald added a subscriber: libcxx-commits. · View Herald Transcript

Harbormaster completed remote builds in B200638: Diff 479443.Dec 1 2022, 2:53 PM

IMO it would be good to also add the Android job with this patch to make sure everything works properly. You should be able to just add a job to libcxx/utils/ci/buildkite-pipeline.yml. See https://libcxx.llvm.org/AddingNewCIJobs.html#addingnewcijobs for more details.

libcxx/cmake/caches/AndroidNDK.cmake
22	Does Android actually use a different ABI namespace, or is this just to avoid name clashes when testing? If it's just fur testing purposes I'd suggest to rename it to something like `__test`.
32	This flag isn't honoured anymore. The experimental library will always be built.
libcxx/utils/ci/run-buildbot-container-android
2	IIUC this allows anybody to run the Android test suite in a docker container? If that's the case, it's really nice.

rprichard added inline comments.Dec 1 2022, 4:39 PM

libcxx/cmake/caches/AndroidNDK.cmake
22	Android has a few different builds of libc++. In the "platform libc++" (`/system/lib[64]/libc++.so`), the namespace is the ordinary `__1`. Apps aren't supposed to use the platform libc++, though, and instead they redistribute a `libc++_shared.so` from the Android NDK. `libc++_shared.so` uses an `__ndk1` namespace. Both the filename and the namespace are supposed to help isolate the NDK libc++ from the platform STL. With recent Android versions, there are also "APEX modules" that have a copy of libc++.so in them. This copy is very similar to the platform libc++.so, but typically built against an older API level. Naming it `__test` for this test configuration seems reasonable.
32	I'll remove this line then. It should be fine to build the library, even though I think the experimental library isn't available in the platform or the NDK currently.
libcxx/utils/ci/run-buildbot-container-android
2	Yes, it should become really easy for others to run the libc++ tests on Android.

Switch namespace from __ndk1 to __test.

Harbormaster completed remote builds in B200666: Diff 479478.Dec 1 2022, 10:07 PM

Add feature LIBCXX-ANDROID-FIXME.

Harbormaster completed remote builds in B201844: Diff 481104.Dec 7 2022, 4:25 PM

In D139147#3965119, @philnik wrote:

IMO it would be good to also add the Android job with this patch to make sure everything works properly. You should be able to just add a job to libcxx/utils/ci/buildkite-pipeline.yml. See https://libcxx.llvm.org/AddingNewCIJobs.html#addingnewcijobs for more details.

+1 That makes reviewing the code easier.

rprichard mentioned this in D139497: [libc++][Android] Bionic also includes + for NAN.Dec 12 2022, 4:07 PM

Reword a comment.

Harbormaster completed remote builds in B202723: Diff 482314.Dec 12 2022, 6:21 PM

It's awesome to see this taking form! This will be the first time that we have an upstream CI bot running via an emulator, and I know there are many more use cases for this so it's great to break trail.

I have a few comments but this LGTM. I agree with others that it would be nice to add a CI runner in the same patch to test that it all works.

libcxx/cmake/caches/AndroidNDK.cmake
22	Ideally, this cache file would reflect the way you actually build for the NDK. The executables in the test suite should reliably use `-nostdlib++` and so on to make sure that we're linking only against the just-built libc++, so I am not sure whether these hoops are necessary. Did you notice that the tests were not being linked/run against the right library? The same comment holds for the `_static` renaming above.
libcxx/utils/ci/run-buildbot
632–635	It would be sweet to be able to do this automatically when we startup the test suite. But this is probably fine to improve incrementally, this is already a giant step in the right direction.
libcxxabi/test/configs/llvm-libc++abi-android-ndk.cfg.in
35	Not attached to this line: You should update our platform support matrix to add Android if it's not already there.