Disable rosegment for old Android versions.

[danalbert]

Summary

The unwinder used by the crash handler on versions of Android prior to
API 29 did not correctly handle binaries built with rosegment, which is
enabled by default for LLD. Android only supports LLD, so it's not an
issue that this flag is not accepted by other linkers.

[dmajor]

Firefox has a build break from this change. In certain Android configurations we use bfd or gold. The statement in the commit message "Android only supports LLD" is news to me, could you point me to any references for this?

[danalbert]

In D95166#2525131, @dmajor wrote:

Firefox has a build break from this change. In certain Android configurations we use bfd or gold. The statement in the commit message "Android only supports LLD" is news to me, could you point me to any references for this?

https://github.com/android/ndk/wiki/Changelog-r22

GNU binutils is deprecated and will be removed in an upcoming NDK release.

TOT LLVM is the source for the next version of the NDK's compiler, which does remove support for bfd and gold: https://android.googlesource.com/platform/ndk/+/master/docs/changelogs/Changelog-r23.md

[glandium]

"Android only supports lld" might be true now, but it hasn't always been true, which means the change is not backwards compatible with versions of the NDK that don't use lld. Also, -fuse-ld is still a valid flag that can allow to use a different linker than lld.

[vitalybuka]

Our Android build bot is broken after this patch http://lab.llvm.org:8011/#/builders/77/builds/3234

Run Build Command(s):/usr/bin/ninja cmTC_578f8 && [1/2] Building C object CMakeFiles/cmTC_578f8.dir/testCCompiler.c.o
[2/2] Linking C executable cmTC_578f8
FAILED: cmTC_578f8
: && /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang --target=i686-linux-android24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot -gcc-toolchain /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64  -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64  CMakeFiles/cmTC_578f8.dir/testCCompiler.c.o -o cmTC_578f8   && :
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/lib/gcc/i686-linux-android/4.9.x/../../../../i686-linux-android/bin/ld: --no-rosegment: unknown option
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/lib/gcc/i686-linux-android/4.9.x/../../../../i686-linux-android/bin/ld: use the --help option for usage information
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

[danalbert]

In D95166#2527437, @vitalybuka wrote:

Our Android build bot is broken after this patch http://lab.llvm.org:8011/#/builders/77/builds/3234

Shouldn't that build be using lld?

In D95166#2526839, @glandium wrote:

"Android only supports lld" might be true now, but it hasn't always been true, which means the change is not backwards compatible with versions of the NDK that don't use lld.

If I'm understanding correctly, Firefox is building a custom toolchain for use with the NDK sysroot? That works as long as you're building the whole toolchain (though we can't guarantee that even that will work out of the box, because these things are developed in tandem and other configurations are not tested), but you'll need to build lld, llvm-ar, etc and use those as well.

Also, -fuse-ld is still a valid flag that can allow to use a different linker than lld.

We could check -fuse-ld for something other than ld or lld and not pass these flags in those cases. Would that be suitable? I see Clang did recently gain a LinkerIsLLD out param for Toolchain::GetLinkerPath. It's wrong for Android toolchains (because it assumes ld is not lld), but we could fix that. It'd be wrong whenever someone is using any toolchain other than the default NDK configuration (which would include Firefox), but at least it'd work for the common case.

A better solution IMO would be to let each target have its own default linker. We currently have this at the OS level, but that means that Android and GNU systems are assumed to have the same default linker. That's false. I'd tried fixing this with https://reviews.llvm.org/D76452 but the patch was not accepted. I'd be happy to revive and submit that if folks agree that it is the better choice.

Though TBH my favorite solution is still to make it clear that Android no longer supports anything but lld and skip all the special cases that will never get tested in practice. gold and bfd just barely worked for Android even when they were our defaults. One of the other fixes I suggest might fix your build today, but it's doubtful to me that it'd help you for long.

[dmajor]

It does feel kind of awkward to me that _this_ is the patch that ends up breaking the builds, versus something at the cmake level that says "you are explicitly unsupported".

Also it's unfortunate that this landed just hours before 12.0.0 branched. Had it landed slightly later, release users could have six months to plan for the change. You say that the next NDK compiler is based on ToT, in that case would it make sense to revert this on the 12.x branch, and add a warning to the 12.0.0 release notes that lld will be assumed in 13?

[glandium]

Firefox CI is using a custom clang, but uses a NDK otherwise (an old-ish one, clearly older than r22 which is the first that defaults to lld). And we do pass -fuse-ld=bfd at the moment for $reasons.
If clang _really_ wants to assume lld as the linker for android, then it should make using -fuse-ld=somethingelse an error and invoke ld.lld rather than ld, if it doesn't already do that.

[danalbert]

In D95166#2529412, @dmajor wrote:

It does feel kind of awkward to me that _this_ is the patch that ends up breaking the builds, versus something at the cmake level that says "you are explicitly unsupported".

If you're using our CMake toolchain file you're automatically migrated to LLD unless you pass -DANDROID_LD=deprecated starting with NDK r22. If you're not using our toolchain file there unfortunately was no way for us to have any kind of input to that process.

r22 is fairly new, but this patch isn't expected to be in a stable NDK until several months from now. It takes us quite a while to move from a patch being submitted to clang to being shipped in a stable NDK, so that's why we're doing this now. We do try pretty hard to adhere to a deprecation window so people have time to adapt, but we're now working on Clang features that are beyond the end of the window (features we'd have preferred to land last summer, but didn't specifically to avoid breaking binutils compatibility). Clang is the last place that migration changes land, so using TOT clang but an older NDK means you started getting incompatible features before you got any of the warnings from the tools.

Also it's unfortunate that this landed just hours before 12.0.0 branched. Had it landed slightly later, release users could have six months to plan for the change. You say that the next NDK compiler is based on ToT, in that case would it make sense to revert this on the 12.x branch, and add a warning to the 12.0.0 release notes that lld will be assumed in 13?

Would have waited if I'd known :( No objection to reverting in the 12.x release branch. We don't ship from the release branches anyway so reverting in 12.x keeps your next release the way you want it keeps ours the way we want it. Feel free to do that, or I can do it tomorrow.

FWIW, we gave much more than six months notice that Android was dropping support for binutils: https://github.com/android/ndk/wiki/Changelog-r19-beta1. We did even communicate it specifically to Firefox, I believe: https://github.com/android/ndk/issues/1063.

In D95166#2529432, @glandium wrote:

If clang _really_ wants to assume lld as the linker for android, then it should make using -fuse-ld=somethingelse an error and invoke ld.lld rather than ld, if it doesn't already do that.

The other patch I linked does the second part of that and would make it possible to continue working with gold/bfd (even if Android has no intention to support that workflow). I'll rebase that and restore the review to try again. I'd also much prefer to be able to do this in a way that doesn't so forcibly break compatibility but that wasn't an option without that patch.

[thakis]

This breaks us too: https://bugs.chromium.org/p/chromium/issues/detail?id=1172291

Given that this breaks at least 3 distinct build configs, let's revert this for now until we've figured out a path forward.

[thakis]

Landed revert in 1608ba09462d877111230e9461b895f696f8fcb1. Someone should file a PR to make sure that gets merged to the 12.0 branch.

We can then reland on trunk once there are clear instructions for folks building with trunk clang but old NDK but without the toolchain file (either "use toolchain file" or "set LLVM_USE_LLD" or something like that, I'm guessing?)

[thakis]

Alternatively, there's LinkerIsLLD in the driver code, maybe the driver wants to check that in some way (if only to emit a better diag if it's false, I guess)

[danalbert]

In D95166#2530791, @thakis wrote:

Landed revert in 1608ba09462d877111230e9461b895f696f8fcb1. Someone should file a PR to make sure that gets merged to the 12.0 branch.

We can then reland on trunk once there are clear instructions for folks building with trunk clang but old NDK but without the toolchain file (either "use toolchain file" or "set LLVM_USE_LLD" or something like that, I'm guessing?)

The instructions would be "use the whole toolchain, don't mix and match". Where should that be documented?

[thakis]

In D95166#2531194, @danalbert wrote:

In D95166#2530791, @thakis wrote:

Landed revert in 1608ba09462d877111230e9461b895f696f8fcb1. Someone should file a PR to make sure that gets merged to the 12.0 branch.

We can then reland on trunk once there are clear instructions for folks building with trunk clang but old NDK but without the toolchain file (either "use toolchain file" or "set LLVM_USE_LLD" or something like that, I'm guessing?)

The instructions would be "use the whole toolchain, don't mix and match". Where should that be documented?

I don't know what "use the whole toolchain, don't mix and match" means.

In our case, we were building compiler-rt for android by running

cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON "-DLLVM_ENABLE_PROJECTS=clang;compiler-rt;lld;chrometools;clang-tools-extra" "-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;Mips;PowerPC;SystemZ;WebAssembly;X86" -DLLVM_ENABLE_PIC=OFF -DLLVM_ENABLE_UNWIND_TABLES=OFF -DLLVM_ENABLE_TERMINFO=OFF -DLLVM_ENABLE_Z3_SOLVER=OFF -DCLANG_PLUGIN_SUPPORT=OFF -DCLANG_ENABLE_STATIC_ANALYZER=OFF -DCLANG_ENABLE_ARCMT=OFF "-DBUG_REPORT_URL=https://crbug.com and run tools/clang/scripts/process_crashreports.py (only works inside Google) which will upload a report" -DLLVM_INCLUDE_GO_TESTS=OFF -DENABLE_X86_RELAX_RELOCATIONS=NO -DLLVM_ENABLE_DIA_SDK=OFF -DLLVM_LOCAL_RPATH=$HOME/src/chrome/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/lib64 "-DCOMPILER_RT_TEST_COMPILER_CFLAGS=--gcc-toolchain=$HOME/src/chrome/src/third_party/llvm-build-tools/gcc-10.2.0-trusty -Wl,-rpath,$HOME/src/chrome/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/lib64 -Wl,-rpath,$HOME/src/chrome/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/lib32" -DLLVM_ENABLE_LIBXML2=FORCE_ON -DCMAKE_C_COMPILER=$HOME/src/chrome/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/bin/gcc -DCMAKE_CXX_COMPILER=$HOME/src/chrome/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/bin/g++ -DCMAKE_C_COMPILER=$HOME/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang -DCMAKE_CXX_COMPILER=$HOME/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang++ -DLLVM_CONFIG_PATH=$HOME/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/llvm-config "-DCMAKE_C_FLAGS=--target=aarch64-linux-android21 --sysroot=$HOME/src/chrome/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=$HOME/src/chrome/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64" "-DCMAKE_CXX_FLAGS=--target=aarch64-linux-android21 --sysroot=$HOME/src/chrome/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=$HOME/src/chrome/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64" "-DCMAKE_ASM_FLAGS=--target=aarch64-linux-android21 --sysroot=$HOME/src/chrome/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=$HOME/src/chrome/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64" -DCOMPILER_RT_BUILD_BUILTINS=OFF -DCOMPILER_RT_BUILD_CRT=OFF -DCOMPILER_RT_BUILD_LIBFUZZER=OFF -DCOMPILER_RT_BUILD_MEMPROF=OFF -DCOMPILER_RT_BUILD_PROFILE=ON -DCOMPILER_RT_BUILD_SANITIZERS=ON -DCOMPILER_RT_BUILD_XRAY=OFF -DSANITIZER_CXX_ABI=libcxxabi -DCMAKE_SHARED_LINKER_FLAGS=-Wl,-u__cxa_demangle -DANDROID=1

in an android-aarch64 subdir of a linux llvm build dir (and similar for other archs).

We've since added -fuse-ld=lld to the three CMAKE_*_FLAGS so I think we're likely set on our end.

If there's some way to build android compiler-rt runtimes as part of a regular linux llvm build, we'd love to learn about that :)

[danalbert]

We've since added -fuse-ld=lld to the three CMAKE_*_FLAGS so I think we're likely set on our end.

https://reviews.llvm.org/D76452 not being accepted means that Android toolchains must have LLD installed as ld. I'm guessing this thread has shown that the patch is worth doing so I can fix this in a more compatible way.

If there's some way to build android compiler-rt runtimes as part of a regular linux llvm build, we'd love to learn about that :)

afaik CMake makes this impossible because one build has exactly one target :(

[thakis]

In D95166#2540399, @danalbert wrote:

We've since added -fuse-ld=lld to the three CMAKE_*_FLAGS so I think we're likely set on our end.

https://reviews.llvm.org/D76452 not being accepted means that Android toolchains must have LLD installed as ld.

If you pass -fuse-ld=lld everwhere, that's not needed, right (?)

If there's some way to build android compiler-rt runtimes as part of a regular linux llvm build, we'd love to learn about that :)

afaik CMake makes this impossible because one build has exactly one target :(

[danalbert]

In D95166#2540705, @thakis wrote:

In D95166#2540399, @danalbert wrote:

We've since added -fuse-ld=lld to the three CMAKE_*_FLAGS so I think we're likely set on our end.

https://reviews.llvm.org/D76452 not being accepted means that Android toolchains must have LLD installed as ld.

If you pass -fuse-ld=lld everwhere, that's not needed, right (?)

Yes, but the defaults should work for the NDK.

clang/lib/Driver/ToolChains/Linux.cpp

Linux::Linux(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
   }
 
   if (Distro.IsOpenSUSE() || Distro.IsUbuntu() || Distro.IsAlpineLinux() ||
       Triple.isAndroid()) {
     ExtraOpts.push_back("-z");
     ExtraOpts.push_back("relro");
   }
 
+  if (Triple.isAndroid() && Triple.isAndroidVersionLT(29)) {
+    // https://github.com/android/ndk/issues/1196
+    // The unwinder used by the crash handler on versions of Android prior to
+    // API 29 did not correctly handle binaries built with rosegment, which is
+    // enabled by default for LLD. Android only supports LLD, so it's not an
+    // issue that this flag is not accepted by other linkers.
+    ExtraOpts.push_back("--no-rosegment");
+  }
+
   // Android ARM/AArch64 use max-page-size=4096 to reduce VMA usage. Note, lld
   // from 11 onwards default max-page-size to 65536 for both ARM and AArch64.
   if ((Triple.isARM() || Triple.isAArch64()) && Triple.isAndroid()) {
     ExtraOpts.push_back("-z");
     ExtraOpts.push_back("max-page-size=4096");
   }
 
   if (GCCInstallation.getParentLibPath().find("opt/rh/devtoolset") !=

clang/test/Driver/linux-ld.c

 // CHECK-ANDROID-HASH-STYLE-L: "--hash-style=both"
 //
 // RUN: %clang %s -### -o %t.o 2>&1 \
 // RUN:     --target=armv7-linux-android23 \
 // RUN:   | FileCheck --check-prefix=CHECK-ANDROID-HASH-STYLE-M %s
 // CHECK-ANDROID-HASH-STYLE-M: "{{.*}}ld{{(.exe)?}}"
 // CHECK-ANDROID-HASH-STYLE-M: "--hash-style=gnu"
 
+// Check that we pass --no-rosegment for pre-29 Android versions and do not for
+// 29+.
+// RUN: %clang %s -### -o %t.o 2>&1 \
+// RUN:     --target=armv7-linux-android28 \
+// RUN:   | FileCheck --check-prefix=CHECK-ANDROID-ROSEGMENT-28 %s
+// CHECK-ANDROID-ROSEGMENT-28: "{{.*}}ld{{(.exe)?}}"
+// CHECK-ANDROID-ROSEGMENT-28: "--no-rosegment"
+//
+// RUN: %clang %s -### -o %t.o 2>&1 \
+// RUN:     --target=armv7-linux-android29 \
+// RUN:   | FileCheck --check-prefix=CHECK-ANDROID-ROSEGMENT-29 %s
+// CHECK-ANDROID-ROSEGMENT-29: "{{.*}}ld{{(.exe)?}}"
+// CHECK-ANDROID-ROSEGMENT-29-NOT: "--no-rosegment"
+
 // RUN: %clang %s -### -o %t.o 2>&1 \
 // RUN:     --target=armv7-linux-android21 \
 // RUN:   | FileCheck --check-prefix=CHECK-ANDROID-NOEXECSTACK %s
 // CHECK-ANDROID-NOEXECSTACK: "{{.*}}ld{{(.exe)?}}"
 // CHECK-ANDROID-NOEXECSTACK: "-z" "noexecstack"
 // CHECK-ANDROID-NOEXECSTACK-NOT: "-z" "execstack"
 // CHECK-ANDROID-NOEXECSTACK-NOT: "-z,execstack"
 // CHECK-ANDROID-NOEXECSTACK-NOT: "-zexecstack"