This is an archive of the discontinued LLVM Phabricator instance.

gn build: Fix Android build.
ClosedPublic

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

Details

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
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

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?

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.

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

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

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

(reverse ping)

This revision was automatically updated to reflect the committed changes.
pcc marked an inline comment as done.Aug 1 2023, 5:42 PM