[FR] disable rosegment in the driver once we no longer support bfd/gold

[DanAlbert]

Once we no longer support gold and bfd we can hoist the rosegment disabling logic from the build systems into the driver so third-party build systems don't need to worry about this. Can't send this patch until then because gold and bfd don't understand the argument and there's no way for us to figure out which linker is used within the clang driver and LLD doesn't know it's targeting Android, let alone the Android version.

Don't know exactly when gold/bfd will be gone, but triaging to r23.

From debe246ff031a715732b4a1af270e6bc68c7a467 Mon Sep 17 00:00:00 2001
From: Dan Albert <danalbert@google.com>
Date: Thu, 25 Jun 2020 16:25:51 -0700
Subject: [PATCH] Disable rosegment for old Android versions.

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.
---
 clang/lib/Driver/ToolChains/Linux.cpp |  9 +++++++++
 clang/test/Driver/linux-ld.c          | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/clang/lib/Driver/ToolChains/Linux.cpp b/clang/lib/Driver/ToolChains/Linux.cpp
index 180350476c3..b0e62330ead 100644
--- a/clang/lib/Driver/ToolChains/Linux.cpp
+++ b/clang/lib/Driver/ToolChains/Linux.cpp
@@ -232,6 +232,15 @@ Linux::Linux(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
     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()) {
diff --git a/clang/test/Driver/linux-ld.c b/clang/test/Driver/linux-ld.c
index ec539522c25..afe7418cded 100644
--- a/clang/test/Driver/linux-ld.c
+++ b/clang/test/Driver/linux-ld.c
@@ -1092,6 +1092,20 @@
 // 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
-- 
2.27.0.111.gc72c7da667-goog

[rprichard]

It might be possible to hoist the option even before removing ld.bfd / ld.gold. I think it's uncommon to use a path with -fuse-ld= versus specifying just the type of linker, and upstream LLVM deprecated the use of a path with -fuse-ld= in favor of a new --ld-path= argument.

A path is still accepted but it's deprecated:

$ /x/llvm-upstream/stage1/bin/clang -fuse-ld=/x/llvm-upstream/stage1/bin/ld.lld hello.c -Weverything
clang-12: warning: '-fuse-ld=' taking a path is deprecated. Use '--ld-path=' instead [-Wfuse-ld-path]

(Even -Wall doesn't turn on -Wfuse-ld-path, not sure why.)

The ToolChain::GetLinkerPath function in upsteam LLVM now has a bool *LinkerIsLLD argument that's intended to let the driver know when it's targeting the bundled LLD. The current AOSP llvm-project (master-legacy) has the new --ld-path= argument but not the new LinkerIsLLD parameter yet.

• https://reviews.llvm.org/D83015
• https://reviews.llvm.org/D91884
• https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93645

[DanAlbert]

The trouble is that Clang doesn't know why type of linker the binary that is installed as ld is. Without an explicit -fuse-ld=lld that's what ends up being run (at least that's true for the version of Clang I'm currently using). Looks like that new API will incorrectly identify ld which is LLD as not LLD.

I'd tried introducing a "what is the default linker for this target" API for Clang that would solve that, but it was rejected. Maybe worth trying to push that again, but I think since r23 has already dropped gold/bfd we don't need to worry too much and I should just push the change above.

[DanAlbert]

Sent for review: https://reviews.llvm.org/D95166

[DanAlbert]

Merged. Unsure if we'll be updating the toolchain again before r23 goes out or not, but will keep this triaged here for now.

[DanAlbert]

Was reverted.