This is an archive of the discontinued LLVM Phabricator instance.

Differential D146266

gn build: Fix Android build.
ClosedPublic

Authored by pcc on Mar 16 2023, 4:40 PM.

Details

Reviewers
aeubanks
thakis
Commits
rG19784825379e: gn build: Fix Android build.
Summary

Remove --unwindlib=none flag that was added by D143598. It is required
to not pass this flag when linking binaries. With this flag, linking
e.g. llvm-symbolizer will fail.

There was a missing dependency from most targets to the implicitly linked
libraries (libunwind and builtins), which was causing the build for those
targets to fail when they were built on their own. I never noticed this
because I always build the implicitly linked libraries together with
their dependencies. Make the dependency explicit by introducing a new
target //llvm/utils/gn/build/libs/implicit representing the platform's
implicitly linked libraries, and depend on that from the LLVM libraries,
so that "normal" binaries like llvm-symbolizer will have all of their
dependencies available.

D143598 set the arch subdirectory to i686 for Android. However, the arch
subdirectory on Android is i386, and has been for a long time.

$ find android-ndk-r* -name i386
android-ndk-r18b/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/7.0.2/lib/linux/i386
android-ndk-r19/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/8.0.2/lib/linux/i386
android-ndk-r20/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/8.0.7/lib/linux/i386
android-ndk-r21-beta1/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/9.0.8/lib/linux/i386
android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/9.0.8/lib/linux/i386
android-ndk-r22/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/11.0.5/lib/linux/i386
android-ndk-r23-beta5/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/12.0.5/lib/linux/i386
android-ndk-r24/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/14.0.1/lib/linux/i386
android-ndk-r25c/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/14.0.7/lib/linux/i386

Using the wrong name prevents Clang from being able to find libunwind.a on
x86. Fix the code to use i386 as the arch subdirectory, consistent with
the NDK.

Bring back -Wl,-z,defs which was removed for Android by D143598, presumably
because of the missing unwind library.

Diff Detail

Event Timeline

pcc created this revision.Mar 16 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 4:40 PM
Herald added subscribers: Enna1, danielkiss, pengfei. · View Herald Transcript
pcc requested review of this revision.Mar 16 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 4:40 PM
pcc updated this revision to Diff 505956.Mar 16 2023, 4:42 PM
pcc edited the summary of this revision. (Show Details)

Fix commit message

aeubanks added a comment.Mar 16 2023, 5:21 PM

yeah this was very confusing when I was looking at this

$ fd i386 ~/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/12.0.5/lib/linux/i386/
$ fd i686 ~/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/
...
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/12.0.5/lib/linux/libclang_rt.ubsan_standalone_cxx-i686-android.a # and lots of other compiler-rt libs
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/i686-linux-android/
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/i686-linux-android/
...

building with this patch, I still get ld.lld: error: unable to find library -l:libunwind.a for all the different arches

perhaps the Android NDK in Chromium is not the standard one? are you using the one bundled in Chromium?

Harbormaster completed remote builds in B219975: Diff 505956.Mar 16 2023, 5:45 PM
srhines added a subscriber: srhines.Mar 16 2023, 6:10 PM
In D146266#4200993, @aeubanks wrote:

yeah this was very confusing when I was looking at this

$ fd i386 ~/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/12.0.5/lib/linux/i386/
$ fd i686 ~/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/
...
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/12.0.5/lib/linux/libclang_rt.ubsan_standalone_cxx-i686-android.a # and lots of other compiler-rt libs
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/i686-linux-android/
/usr/local/google/home/aeubanks/repos/chromium/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/i686-linux-android/
...

building with this patch, I still get ld.lld: error: unable to find library -l:libunwind.a for all the different arches

perhaps the Android NDK in Chromium is not the standard one? are you using the one bundled in Chromium?

I'm pretty sure that Peter is building/using the toolchain with the Android platform, so it might be picking up NDK parts from the platform build. As for the one in Chromium, I see a "12.0.5" in there, which means that it is quite old. You may want to consider updating that in any case. Even NDK r25 from last year is on ~Clang-14. https://github.com/android/ndk/wiki/Changelog-r25#r25c shows a lot of other nice improvements too in terms of repo size, so that might be a real nice win for Chromium devs.

pcc updated this revision to Diff 505968.Mar 16 2023, 6:57 PM
pcc edited the summary of this revision. (Show Details)

After an out of band discussion with Arthur, we figured out the root cause of his problem and I was able to reproduce. This fixes it for me.

Herald added a subscriber: hiraditya. · View Herald TranscriptMar 16 2023, 6:57 PM
Harbormaster completed remote builds in B219987: Diff 505968.Mar 16 2023, 8:22 PM
aeubanks added inline comments.Mar 17 2023, 10:33 AM
llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn
7 ↗(On Diff #505968)

why put this in Support rather than something like executable (which you had mentioned trying)?

pcc added inline comments.Mar 17 2023, 10:59 AM
llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn
7 ↗(On Diff #505968)

Unfortunately that wouldn't work, because the deps attribute is not allowed on config.

https://gn.googlesource.com/gn/+/main/docs/reference.md#func_config

pcc updated this revision to Diff 506150.Mar 17 2023, 11:18 AM
pcc edited the summary of this revision. (Show Details)

Bring back -Wl,-z,defs for Android

Harbormaster completed remote builds in B220116: Diff 506150.Mar 17 2023, 12:07 PM
aeubanks added a comment.Mar 17 2023, 2:56 PM

I'd like @thakis to review this change, he probably has more thoughts

pcc added inline comments.Mar 20 2023, 1:51 PM
llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn
7 ↗(On Diff #505968)

I just remembered how this is done in Chromium, they define template wrappers around the target types that invoke the linker: https://source.chromium.org/chromium/chromium/src/+/main:build/config/BUILDCONFIG.gn;l=512

That's a much better approach, so I'll switch to that.

pcc updated this revision to Diff 506729.Mar 20 2023, 2:23 PM

Switch to template wrappers

Harbormaster completed remote builds in B220550: Diff 506729.Mar 20 2023, 4:21 PM
aeubanks added a comment.Apr 19 2023, 9:56 AM

ping @thakis

pcc added a comment.Apr 28 2023, 2:57 PM

Ping; still needed to build for Android.

thakis accepted this revision.Jul 10 2023, 5:10 AM

lg

(aeubanks, feel free to approve changes like this!)

llvm/utils/gn/build/BUILDCONFIG.gn
53 ↗(On Diff #506729)

This is a bit yucky. Maybe we could do this if (current_os == "android") for now, since we at least currently only need it there? (If so, let group("implicit") only exist on android too)

This revision is now accepted and ready to land.Jul 10 2023, 5:10 AM
aeubanks added a comment.Jul 28 2023, 10:31 AM

(reverse ping)

Closed by commit rG19784825379e: gn build: Fix Android build. (authored by pcc). · Explain WhyAug 1 2023, 5:41 PM
This revision was automatically updated to reflect the committed changes.
pcc added a commit: rG19784825379e: gn build: Fix Android build..
pcc marked an inline comment as done.Aug 1 2023, 5:42 PM

Revision Contents

PathSize
llvm/
utils/
gn/
build/
toolchain/
5 lines
secondary/
compiler-rt/
6 lines
libunwind/
src/
2 lines

Diff 505956

llvm/utils/gn/build/toolchain/target_flags.gni

Show All 9 Lines
target_ldflags = [] target_ldflags = []
if (current_os == "android") { if (current_os == "android") {
target_flags += [ target_flags += [
"--target=$llvm_current_triple", "--target=$llvm_current_triple",
"--sysroot=$android_ndk_path/toolchains/llvm/prebuilt/linux-x86_64/sysroot", "--sysroot=$android_ndk_path/toolchains/llvm/prebuilt/linux-x86_64/sysroot",
"--gcc-toolchain=$android_ndk_path/toolchains/llvm/prebuilt/linux-x86_64", "--gcc-toolchain=$android_ndk_path/toolchains/llvm/prebuilt/linux-x86_64",
] ]
target_ldflags += [ target_ldflags += [ "-static-libstdc++" ]
"-static-libstdc++",
"--unwindlib=none",
]
if (current_cpu == "arm") { if (current_cpu == "arm") {
target_flags += [ "-march=armv7-a" ] target_flags += [ "-march=armv7-a" ]
} }
} else if (current_os == "ios") { } else if (current_os == "ios") {
if (current_cpu == "arm64") { if (current_cpu == "arm64") {
target_flags += [ target_flags += [
"-arch", "-arch",
"arm64", "arm64",
Show All 22 Lines

llvm/utils/gn/secondary/compiler-rt/target.gni

import("//clang/resource_dir.gni") import("//clang/resource_dir.gni")
import("//clang/runtimes.gni") import("//clang/runtimes.gni")
if (current_cpu == "x86") { if (current_cpu == "x86") {
if (current_os == "android") {
crt_current_target_arch = "i686"
} else {
crt_current_target_arch = "i386" crt_current_target_arch = "i386"
}
} else if (current_cpu == "x64") { } else if (current_cpu == "x64") {
crt_current_target_arch = "x86_64" crt_current_target_arch = "x86_64"
} else if (current_cpu == "arm") { } else if (current_cpu == "arm") {
crt_current_target_arch = "arm" crt_current_target_arch = "arm"
} else if (current_cpu == "arm64") { } else if (current_cpu == "arm64") {
crt_current_target_arch = "aarch64" crt_current_target_arch = "aarch64"
} else if (current_cpu == "ppc64") { } else if (current_cpu == "ppc64") {
crt_current_target_arch = "powerpc64le" crt_current_target_arch = "powerpc64le"
Show All 26 Lines

llvm/utils/gn/secondary/libunwind/src/BUILD.gn

Show First 20 Lines • Show All 52 Lines • ▼ Show 20 Lines
if (current_os == "android") { if (current_os == "android") {
if (current_cpu == "arm64") { if (current_cpu == "arm64") {
unwind_output_dir = "$crt_current_out_dir/aarch64" unwind_output_dir = "$crt_current_out_dir/aarch64"
} else if (current_cpu == "arm") { } else if (current_cpu == "arm") {
unwind_output_dir = "$crt_current_out_dir/arm" unwind_output_dir = "$crt_current_out_dir/arm"
} else if (current_cpu == "x64") { } else if (current_cpu == "x64") {
unwind_output_dir = "$crt_current_out_dir/x86_64" unwind_output_dir = "$crt_current_out_dir/x86_64"
} else if (current_cpu == "x86") { } else if (current_cpu == "x86") {
unwind_output_dir = "$crt_current_out_dir/i686" unwind_output_dir = "$crt_current_out_dir/i386"
} }
} else { } else {
unwind_output_dir = runtimes_dir unwind_output_dir = runtimes_dir
} }
config("unwind_config") { config("unwind_config") {
cflags = [] cflags = []
cflags_c = [ "-std=c99" ] cflags_c = [ "-std=c99" ]
▲ Show 20 Lines • Show All 85 Lines • Show Last 20 Lines