Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lldb: get lldb API tests working with newer Android NDKs #106443

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

andrurogerz
Copy link
Contributor

@andrurogerz andrurogerz commented Aug 28, 2024

Purpose

Running the LLDB API tests against a remote Android target with NDK version r22 or later fails to compile the test inferiors. NDK r21 from 2021 is the most recent NDK that still works with the LLDB API tests. This PR updates the Android make rules to support newer Android NDK versions (r19 and later).

Overview

  • Updates and simplifies Android.rules to match the newer Android NDK unified toolchain layout introduced in NDK r19
  • Sets OBJCOPY and ARCHIVER env vars, required by a few test cases, to their llvm- versions in the unified toolchain
  • Drops support for pre-2019 Android NDK versions to keep the rules simple
  • Provides an error message if the tests are run using an incompatible NDK layout

Problem Details

Android introduced a unified tools layout in NDK r19 (2019) and removed the old layout in r22 (2021). Releases r19, r20, and r21 support both the old and new layout side-by-side. More details are in #106270.

Validation

Ran a sub-set of the LLDB API tests against remote Android targets for the four primary architectures i386, x86_64, arm, and aarch64. No validation was done against riscv targets.

For each case, ran the copy of lldb-server from the Android NDK on the device with the latest LLDB test cases in llvm-project

Ran tests with both r19 (the oldest supported) and r26 (more recent, unified layout only) NDK versions.

Example test command for aarch64:

./build/bin/lldb-dotest --out-of-tree-debugserver --arch aarch64 --platform-name remote-android --platform-url connect://localhost:5432 --platform-working-dir /data/local/tmp --compiler=$ANDROID_NDK_ROOT/toolchains/llvm/prebuilt/linux-x86_64/bin/clang lldb/test/API/android/

NOTE: there are a lot of test failures when running the full suite (especially against 32-bit ARM target). These failures occur independent of this change.

Verified the expected error message appears when attempting to run using NDK r18

Build Command Output:
make: Entering directory '/home/andrew/src/llvm/llvm-project/build/lldb-test-build.noindex/android/platform/TestDefaultCacheLineSize.test_cache_line_size'
/home/andrew/src/llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make/Android.rules:16: *** "No unified toolchain sysroot found in /home/andrew/Android/Sdk/ndk/18.1.5063045/toolchains/llvm/prebuilt/linux-x86_64/bin/../../../../... NDK must be r19 or later.".  Stop.
make: Leaving directory '/home/andrew/src/llvm/llvm-project/build/lldb-test-build.noindex/android/platform/TestDefaultCacheLineSize.test_cache_line_size'

Impact

This change explicitly removes support for the pre-2019 NDK structure. Only NDK r19 (from 2019) and later can be used when running the LLDB API tests. If the maintainers object, we can easily support both the old and new NDK toolchain layouts side-by-side at the cost of readability/maintainability. Since this change only impacts tests, I don't see much value in supporting NDKs that are over 5 years old.

Guidance to Reviewers

  • I am not an expert on clang arguments so if anything looks off let me know.
  • While I personally thing supporting 5+ year old NDKs for testing seems unnecessary, please chime-in if you are concerned with dropping that support. I can easily revise to support both old and new layouts side-by-side.
  • If there are any specific tests you'd like me to run I will do my best to accommodate. It doesn't look like there's much (any?) Android LLDB CI coverage.

Android introduced a unified tools layout in NDK r19 (2019) and
removed the old layout in r22 (2021). Running lldb tests with NDK r22
or newer fails when compiling the test inferiors.

This change updates `Android.rules` to match the newer unified tools
structure introduced in NDK r19, which is described in more detail at
android/ndk#780.

NOTE: After this change, ONLY NDK r19c and later can be used when
running the lldb tests. The pre-2019 NDK structure is no longer
supported.
Provide an error message when compiling LLDB API test inferiors against
an older Android NDK without the unified toolchain layout. This message
will help guide developers to install and use an NDK r19 or newer.
@andrurogerz andrurogerz marked this pull request as ready for review August 28, 2024 20:50
@llvmbot llvmbot added the lldb label Aug 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 28, 2024

@llvm/pr-subscribers-lldb

Author: Andrew Rogers (andrurogerz)

Changes

Purpose

Running the LLDB API tests against a remote Android target with NDK version r22 or later fails to compile the test inferiors. NDK r21 from 2021 is the most recent NDK that still works with the LLDB API tests. This PR updates the Android make rules to support newer Android NDK versions (r19 and later).

Overview

  • Updates and simplifies Android.rules to match the newer Android NDK unified toolchain layout introduced in NDK r19
  • Modernizes the clang arguments for improved readability/maintainability
  • Drops support for pre-2019 Android NDK versions to keep the rules simple
  • Provides an error message if the tests are run using an incompatible NDK layout

Problem Details

Android introduced a unified tools layout in NDK r19 (2019) and removed the old layout in r22 (2021). Releases r19, r20, and r21 support both the old and new layout side-by-side. More details are in #106270.

Validation

Ran a sub-set of the LLDB API tests against remote Android targets for the four primary architectures i386, x86_64, arm, and aarch64. No validation was done against riscv targets.

For each case, ran the copy of lldb-server from the Android NDK on the device with the latest LLDB test cases in llvm-project

Ran tests with both r19 (the oldest supported) and r26 (more recent, unified layout only) NDK versions.

Example test command for aarch64:

./build/bin/lldb-dotest --out-of-tree-debugserver --arch aarch64 --platform-name remote-android --platform-url connect://localhost:5432 --platform-working-dir /data/local/tmp --compiler=$ANDROID_NDK_ROOT/toolchains/llvm/prebuilt/linux-x86_64/bin/clang --exclude /home/user/src/ds2/Support/Testing/Excluded/upstream/android-x86_64.excluded --env OBJCOPY=/home/user/src/llvm/llvm-project/build/bin/llvm-objcopy --env ARCHIVER=/home/user/src/llvm/llvm-project/build/bin/llvm-ar lldb/test/API/android/

NOTE: there are a lot of test failures when running the full suite (especially against 32-bit ARM target). These failures occur independent of this change.

Verified the expected error message appears when attempting to run using NDK r18

Build Command Output:
make: Entering directory '/home/andrew/src/llvm/llvm-project/build/lldb-test-build.noindex/android/platform/TestDefaultCacheLineSize.test_cache_line_size'
/home/andrew/src/llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make/Android.rules:16: *** "No unified toolchain sysroot found in /home/andrew/Android/Sdk/ndk/18.1.5063045/toolchains/llvm/prebuilt/linux-x86_64/bin/../../../../... NDK must be r19 or later.".  Stop.
make: Leaving directory '/home/andrew/src/llvm/llvm-project/build/lldb-test-build.noindex/android/platform/TestDefaultCacheLineSize.test_cache_line_size'

Impact

This change explicitly removes support for the pre-2019 NDK structure. Only NDK r19 (from 2019) and later can be used when running the LLDB API tests. If the maintainers object, we can easily support both the old and new NDK toolchain layouts side-by-side at the cost of readability/maintainability. Since this change only impacts tests, I don't see much value in supporting NDKs that are over 5 years old.

Guidance to Reviewers

  • I am not an expert on clang arguments so if anything looks off let me know.
  • While I personally thing supporting 5+ year old NDKs for testing seems unnecessary, please chime-in if you are concerned with dropping that support. I can easily revise to support both old and new layouts side-by-side.
  • If there are any specific tests you'd like me to run I will do my best to accommodate. It doesn't look like there's much (any?) Android LLDB CI coverage.

Full diff: https://github.com/llvm/llvm-project/pull/106443.diff

1 Files Affected:

  • (modified) lldb/packages/Python/lldbsuite/test/make/Android.rules (+29-55)
diff --git a/lldb/packages/Python/lldbsuite/test/make/Android.rules b/lldb/packages/Python/lldbsuite/test/make/Android.rules
index cd7d8ae74d6bf3..3bece0d0ae8045 100644
--- a/lldb/packages/Python/lldbsuite/test/make/Android.rules
+++ b/lldb/packages/Python/lldbsuite/test/make/Android.rules
@@ -1,81 +1,55 @@
 NDK_ROOT := $(shell dirname $(CC))/../../../../..
 
-ifeq "$(findstring 64, $(ARCH))" "64"
-	# lowest 64-bit API level
-	API_LEVEL := 21
-else ifeq "$(ARCH)" "i386"
-	# clone(2) declaration is present only since this api level
-	API_LEVEL := 17
+ifeq "$(HOST_OS)" "Linux"
+	HOST_TAG := linux-x86_64
+else ifeq "$(HOST_OS)" "Darwin"
+	HOST_TAG := darwin-x86_64
 else
-	# lowest supported 32-bit API level
-	API_LEVEL := 16
+	HOST_TAG := windows-x86_64
+endif
+
+TOOLCHAIN_SYSROOT := $(NDK_ROOT)/toolchains/llvm/prebuilt/$(HOST_TAG)/sysroot
+
+ifeq "$(wildcard $(TOOLCHAIN_SYSROOT)/.)" ""
+# Compiling test inferiors for Android requires an NDK with the unified
+# toolchain introduced in version r19.
+$(error "No unified toolchain sysroot found in $(NDK_ROOT). NDK must be r19 or later.")
 endif
 
 ifeq "$(ARCH)" "arm"
-	SYSROOT_ARCH := arm
-	STL_ARCH := armeabi-v7a
 	TRIPLE := armv7-none-linux-androideabi
 	ARCH_CFLAGS += -march=armv7-a -mfloat-abi=softfp -mfpu=vfpv3-d16 -marm
 else ifeq "$(ARCH)" "aarch64"
-	SYSROOT_ARCH := arm64
-	STL_ARCH := arm64-v8a
 	TRIPLE := aarch64-none-linux-android
 else ifeq "$(ARCH)" "i386"
-	SYSROOT_ARCH := x86
-	STL_ARCH := x86
 	TRIPLE := i686-none-linux-android
 else
-	SYSROOT_ARCH := $(ARCH)
-	STL_ARCH := $(ARCH)
 	TRIPLE := $(ARCH)-none-linux-android
 endif
 
-ifeq "$(findstring 86,$(ARCH))" "86"
-	TOOLCHAIN_DIR := $(STL_ARCH)-4.9
-else ifeq "$(ARCH)" "arm"
-	TOOLCHAIN_DIR := arm-linux-androideabi-4.9
-else
-	TOOLCHAIN_DIR := $(subst -none,,$(TRIPLE))-4.9
-endif
+# lowest 64-bit API level
+API_LEVEL := 21
 
 ifeq "$(ARCH)" "arm"
-	TOOL_PREFIX := arm-linux-androideabi
-else
-	TOOL_PREFIX := $(subst -none,,$(TRIPLE))
-endif
-
-ifeq "$(HOST_OS)" "Linux"
-	HOST_TAG := linux-x86_64
-else ifeq "$(HOST_OS)" "Darwin"
-	HOST_TAG := darwin-x86_64
+	ARCH_DIR := arm-linux-androideabi
 else
-	HOST_TAG := windows-x86_64
-endif
-
-GCC_TOOLCHAIN = $(NDK_ROOT)/toolchains/$(TOOLCHAIN_DIR)/prebuilt/$(HOST_TAG)
-
-OBJCOPY ?= $(GCC_TOOLCHAIN)/bin/$(TOOL_PREFIX)-objcopy
-ARCHIVER ?= $(GCC_TOOLCHAIN)/bin/$(TOOL_PREFIX)-ar
-
-ifeq "$(findstring clang,$(CC))" "clang"
-	ARCH_CFLAGS += -target $(TRIPLE) --gcc-toolchain=$(GCC_TOOLCHAIN)
-	ARCH_LDFLAGS += -target $(TRIPLE) --gcc-toolchain=$(GCC_TOOLCHAIN)
+	ARCH_DIR := $(subst -none,,$(TRIPLE))
 endif
 
-ARCH_CFLAGS += --sysroot=$(NDK_ROOT)/sysroot \
-	-isystem $(NDK_ROOT)/sysroot/usr/include/$(TOOL_PREFIX) \
-	-D__ANDROID_API__=$(API_LEVEL) \
-	-isystem $(NDK_ROOT)/platforms/android-$(API_LEVEL)/arch-$(SYSROOT_ARCH)/usr/include
-
-ARCH_LDFLAGS += --sysroot=$(NDK_ROOT)/platforms/android-$(API_LEVEL)/arch-$(SYSROOT_ARCH) -lm
+ARCH_CFLAGS += \
+	--target=$(TRIPLE) \
+	--sysroot=$(TOOLCHAIN_SYSROOT) \
+	-D __ANDROID_API__=$(API_LEVEL) \
 
 ARCH_CXXFLAGS += \
-	-isystem $(NDK_ROOT)/sources/cxx-stl/llvm-libc++/include \
-	-isystem $(NDK_ROOT)/sources/android/support/include \
-	-isystem $(NDK_ROOT)/sources/cxx-stl/llvm-libc++abi/include
+	-isystem $(TOOLCHAIN_SYSROOT)/usr/include/c++/v1 \
 
 ARCH_LDFLAGS += \
-	-L$(NDK_ROOT)/sources/cxx-stl/llvm-libc++/libs/$(STL_ARCH) \
-	$(NDK_ROOT)/sources/cxx-stl/llvm-libc++/libs/$(STL_ARCH)/libc++_static.a \
+	--target=$(TRIPLE) \
+	--sysroot=$(TOOLCHAIN_SYSROOT) \
+	--prefix=$(TOOLCHAIN_SYSROOT)/usr/lib/$(ARCH_DIR)/$(API_LEVEL) \
+	--library-directory=$(TOOLCHAIN_SYSROOT)/usr/lib/$(ARCH_DIR)/$(API_LEVEL) \
+	$(TOOLCHAIN_SYSROOT)/usr/lib/$(ARCH_DIR)/libc++_static.a \
+	-lm \
 	-lc++abi \
-	-nostdlib++
+	-nostdlib++ \

Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I don't think anyone will object to removing old ndk support, but let's leave this PR open for a while to give people a chance to notice.

lldb/packages/Python/lldbsuite/test/make/Android.rules Outdated Show resolved Hide resolved
lldb/packages/Python/lldbsuite/test/make/Android.rules Outdated Show resolved Hide resolved
The NDK unified toolchain layout includes `llvm-objcopy` and `llvm-ar`
host executables. A few of the lldb API tests rely on these tools.
Previously, the GCC verions of these tools were used from their old NDK
location which no longer exists in post-r22 NDKs. Instead, use the llvm
versions of these tools which are included in the unified toolchain.
@andrurogerz
Copy link
Contributor Author

andrurogerz commented Aug 29, 2024

I applied feedback from @labath (thank you!) and I am happy to leave this PR up for a while in case anyone else has thoughts.

I also made one improvement:

It now sets OBJCOPY= and ARCHIVER= to llvm-objcopy and llvm-ar included since NDK r19. These vars were previously pointed at the binutils versions that were removed in NDK r22. A small number of the tests rely on objcopy and ar, so explicitly pointing these vars at the llvm- variants in the NDK avoids requiring them be passed to lldb-dotest via --env command line args.

@andrurogerz
Copy link
Contributor Author

@labath there have been no additional comments; is there anyone specifically you'd like me to seek feedback from?

@JDevlieghere would you have a moment to take a look at this PR soon? I really appreciate it.

@compnerd FYI

@labath
Copy link
Collaborator

labath commented Sep 19, 2024

@labath there have been no additional comments; is there anyone specifically you'd like me to seek feedback from?

No, this is fine. Thanks for reminding me.

@compnerd
Copy link
Member

Going to go ahead and merge this since there/s not been any additional feedback.

@compnerd compnerd merged commit 18b9d49 into llvm:main Sep 25, 2024
7 checks passed
@andrurogerz andrurogerz deleted the lldb-tests-unified-tools-ndk branch October 10, 2024 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants