[PATCH] D64427: LLVM Test-Suite: Support Cross-Compilation and Cross-execution targeting arm64-linux-android

Thu Jul 11 11:47:22 PDT 2019

danalbert added inline comments.

================
Comment at: CMakeLists.txt:186
 if(NOT DEFINED TARGET_OS)
-  message(STATUS "Check target operating system - ${CMAKE_SYSTEM_NAME}")
-  set(TARGET_OS ${CMAKE_SYSTEM_NAME})
+  # FIXME: Currently there is a compatibility issue between Android NDK and cmake
+  # Therefore, we check for TEST_SUITE_REMOTE_CLIENT==adb to know whether
----------------
Got a link to a bug? `CMAKE_SYSTEM_NAME` should be `Android` for Android.

================
Comment at: External/SPEC/CFP2006/CMakeLists.txt:10
+
+  if(NOT TARGET_OS STREQUAL "Android")
+    cpu2006_subdir(447.dealII)
----------------
Everything that's excluded should have a comment with a link to a bug explaining what we need to fix, and should probably also include `message(WARNING "Skipping <test name> on Android: <bug number>")`

================
Comment at: External/SPEC/CINT2006/462.libquantum/CMakeLists.txt:11
   #  somehow copy+fix the reference outputs?)
-  llvm_test_verify(RUN_TYPE ${run_type} WORKDIR ${CMAKE_CURRENT_BINARY_DIR}
-    diff --strip-trailing-cr
-    data/${run_type}/output/${run_type}.out
-    ${run_type}.out
-  )
+  # diff in Android doesn't have --strip-trailing-cr, use -w instead
+  if(NOT TARGET_OS STREQUAL "Android")
----------------
Have you filed a bug? That sounds trivial to add to toybox.

================
Comment at: MultiSource/Benchmarks/CMakeLists.txt:32
 if(NOT "${ARCH}" STREQUAL "XCore")
-  add_subdirectory(7zip)
+  if(NOT TEST_SUITE_REMOTE_CLIENT STREQUAL "adb")
+    add_subdirectory(7zip)
----------------
ziangwan wrote:
> kristof.beyls wrote:
> > I'm wondering if (NOT "${TARGET_OS}" STREQUAL "Android") wouldn't be a better test?
> > Isn't the reason many of these programs fail to build that some header file isn't provided on the Android platform?
> > Also, that would make the test more similar to other tests already in here that remove specific benchmarks for specific architectures or OSs.
> Most of the build failures are due to missing header files in the Android platform. There is an issue with the data format of Microbenchmark. I didn't attempt to fix that since I didn't care much about Microbenchmarks. I simply disabled them. There are a few tests/benchmarks I disabled because of bugs that prevent compilation. I didn't try to fix them either.
Missing platform stuff, or missing third-party stuff? i.e. is libc missing something, or is the problem that Android doesn't include the 7zip development package? The former needs to be fixed and/or have bugs filed.

================
Comment at: cmake/caches/target-arm64-android-template.sh:6
+# REQUIRED: path to the test-suite source file
+TEST_SUITE_LOCATION="/data/local/tmp/devspace/test-suite"
+# REQUIRED: path to android ndk that contains necessary header files and libraries
----------------
ziangwan wrote:
> fhahn wrote:
> > This setting and the ones below seems very system/setup specific, and related to general test-suite setup, not android related.
> Exactly, there is a requirement on host system setup for llvm-test-suite to correctly execute:
> > Special Requirement for build directory: Due to the current state of the build system, the tests are generated using the absolute path on the host machine. For example, if you build the test-suite at /aa/bb/cc on the host machine, all the build files should also be stored at /aa/bb/cc on the target android device for the tests to correctly execute. Therefore, we need to create a build directory, whose absolute path is also a writable location on the target Android device, on the host machine. For example, if you want the tests to run on /data/local/tmp/build on the Android device, you should build the test-suite inside /data/local/tmp/build as well. You specifically cannot use arbitrary path on the host machine since Android's SeLinux prevents creating directories at sysroot.
Probably worth a comment referencing whatever doc you're quoting.

================
Comment at: cmake/caches/target-arm64-android-template.sh:8
+# REQUIRED: path to android ndk that contains necessary header files and libraries
+ANDROID_NDK_LOCATION="/data/local/tmp/docspace/android-ndk-r20"
+# REQUIRED: path to llvm build bin
----------------
(why is this "doc" space?)

Should probably keep everything under a common subdirectory to avoid any collisions. `/data/local/tmp/llvm-test-suite/...`

Is it actually necessary to push the NDK to the device? None of the toolchains are built to run on Android. I can't even think of what use the sysroot would be.

================
Comment at: cmake/caches/target-arm64-android-template.sh:10
+# REQUIRED: path to llvm build bin
+COMPILER_BIN="/data/local/tmp/docspace/llvm-build/bin"
+# OPTIONAL: path to SPEC CPU2006 folder
----------------
Same. Why are we running the compiler on the device?

================
Comment at: cmake/caches/target-arm64-android-template.sh:32
+-D BIONIC_IOCTL_NO_SIGNEDNESS_OVERLOAD \
+-L $ANDROID_NDK_LOCATION/toolchains/llvm/prebuilt/$HOST_SYSTEM/lib/gcc/aarch64-linux-android/4.9.x/"
+
----------------
This is needed because the compiler being used isn't the NDK toolchain, right? It's just the bare compiler without the sysroot and all that? If so, you *might* be able to use `-gcc-toolchain $ANDROID_NDK_LOCATION/toolchains/llvm/prebuilt/$HOST_SYSTEM` instead of specifying the internal location of libgcc. (If not, fixing the driver to make that work would probably be good)

================
Comment at: cmake/caches/target-arm64-android-template.sh:42
+# Push the shared library libc++_shared.so onto the device.
+adb -s $DEVICE_SERIAL push $ANDROID_NDK_LOCATION/toolchains/llvm/prebuilt/\
+$HOST_SYSTEM/sysroot/usr/lib/aarch64-linux-android/libc++_shared.so \
----------------
fhahn wrote:
> Can this be handled by the general sync logic, that copies over the other artifacts? Also, is using a libcxx built alongside clang supported?
> Also, is using a libcxx built alongside clang supported?

Not really. I haven't had the time to upstream all of our patches. It might work if this only needs to support new devices, but we have a number of things we have to do to build libc++ for Jelly Bean.

================
Comment at: cmake/caches/target-arm64-android-template.sh:43
+adb -s $DEVICE_SERIAL push $ANDROID_NDK_LOCATION/toolchains/llvm/prebuilt/\
+$HOST_SYSTEM/sysroot/usr/lib/aarch64-linux-android/libc++_shared.so \
+$EXTRA_LIB_LOCATION/libc++_shared.so
----------------
nit: indent continuation lines

================
Comment at: litsupport/modules/android.py:1
+"""Test module to execute a benchmark through adb on a connected Android
+device. This assumes all relevant directories and files are present on the remote
----------------
Nit: docstring convention is:

```
"""Short summary sentence.

Additional description if needed.
"""
```

(also, is LLVM Python 3 yet?)

================
Comment at: litsupport/modules/android.py:4
+device."""
+from litsupport import testplan
+import logging
----------------
nit: stdlib imports come first and are separated by a blank line

```
import logging
import os
import subprocess

from litsupport import testplan
```

================
Comment at: litsupport/modules/android.py:22
+
+    # append `adb [-s remote_host] shell` in front of each command
+    # append LD_LIBRARY_PATH to resolve libc++_shared.so
----------------
prepend

================
Comment at: litsupport/modules/android.py:24
+    # append LD_LIBRARY_PATH to resolve libc++_shared.so
+    def append_adb_shell(s_in):
+        s_out = "adb {} shell \"{} {}\"".format(
----------------
It's doing more than that. Maybe `make_adb_command`?

================
Comment at: utils/rsync_android.sh:1
+#!/bin/bash
+
----------------
This isn't running rsync, so don't call it that. Just `sync_android.sh` or `push_android.sh`.

================
Comment at: utils/rsync_android.sh:11
+# into actual directories by making a copy
+find $TEST_SUITE_BUILD -type l -exec bash -c 'cp -r $(readlink -m "$0") "$0.new"; rm "$0"; mv "$0.new" "$0"' {} \;
+
----------------
+jmgao: should `adb push` have an `--unsymlink` option?

================
Comment at: utils/rsync_android.sh:31
+do
+    adb -s $TARGET push $file $file
+done
----------------
I'm guessing it's slower to run a lot of pushes instead of a single push command... but idk if jmgao likes the idea of `adb push --filter $PATTERN`

Repository:
  rT test-suite

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64427/new/

https://reviews.llvm.org/D64427