[BUG] Hoist relocation packing flags into the Clang driver

[DanAlbert]

These aren't useful until minSdkVersion >= 28, but we should hoist the -Wl,--pack-dyn-relocs=android+relr and -Wl,--use-android-relr-tags into the driver. So they are used automatically when available.

[enh-google]

why 28? i thought it was 23? that's certainly what's in the documentation --- https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md#relative-relocations-relr

is the confusion that pre-28 you also need -Wl,--use-android-relr-tags? (see internal bug http://b/147452927 "remove the need for -Wl,--use-android-relr-tags".)

[enh-google]

ah, yeah, looks like this bug was filed a year before we learned that pcc@ had implemented the older form of relocation packing in lld. (follow the comments on the internal bug for the gory details.)

[DanAlbert]

I may have filed this before we realized that LLD supported the API 23 stuff natively.

[DanAlbert]

Yes, that :)

[MaskRay]

Drive-by comments (I come across here + I don't know where I should report the problems other than here:) )

--pack-dyn-relocs=android+relr may not be a good combination.

First, non -z options are binary format generic while --pack-dyn-relocs is ELF specific. (GNU ld maintainers will certainly object to it. See the --force-bti -> -z force-bti precedent)
Second, android+relr combines two not-too-related things. (In addition, relr+android does not work. Why?)

-z relr may be good for he --pack-dyn-relocs=relr part.

See https://reviews.llvm.org/D79664 and https://sourceware.org/pipermail/binutils/2020-May/111086.html (I want GNU ld to support at least the REL and RELR features)

[enh-google]

Isn't that another argument for hoisting this into the driver? To avoid this being hard-coded in even more build systems than it already is?

…

On Fri, May 15, 2020, 11:02 Fangrui Song ***@***.***> wrote: Drive-by comments (I come across here + I don't know where I should report the problems other than here:) ) --pack-dyn-relocs=android+relr may not be a good combination. First, non -z options are binary format generic while --pack-dyn-relocs is ELF specific. Second, android+relr combines two not too related things. (In addition, relr+android does not work. Why?) -z relr may be good for he --pack-dyn-relocs=relr part. See https://reviews.llvm.org/D79664 and https://sourceware.org/pipermail/binutils/2020-May/111086.html (I want GNU ld to support at least the REL and RELR features) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#909 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMVLEWDM2OBAY6NNDPKPLITRRV7U5ANCNFSM4GXSQTXQ> .

[DanAlbert]

I think the point is more what we should hoist into the driver. The driver can't currently make very reliable assumptions about what linker is being used.

[DanAlbert]

Optimistically triaging to r23. Here's a patch we can send once we no longer support gold/bfd:

From 133de77b4fd8317979616e7a7d46f5a1ff953f9c Mon Sep 17 00:00:00 2001
From: Dan Albert <danalbert@google.com>
Date: Thu, 25 Jun 2020 16:43:01 -0700
Subject: [PATCH] Pack relocations for Android when possible.

First supported by API 28.
---
 clang/lib/Driver/ToolChains/Linux.cpp | 11 +++++++++++
 clang/test/Driver/linux-ld.c          | 16 ++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/clang/lib/Driver/ToolChains/Linux.cpp b/clang/lib/Driver/ToolChains/Linux.cpp
index 180350476c3..841572e70b0 100644
--- a/clang/lib/Driver/ToolChains/Linux.cpp
+++ b/clang/lib/Driver/ToolChains/Linux.cpp
@@ -232,6 +232,17 @@ Linux::Linux(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
     ExtraOpts.push_back("relro");
   }
 
+  if (Triple.isAndroid() && !Triple.isAndroidVersionLT(28)) {
+    // Android supports relr packing starting with API 28 and had its own flavor
+    // (--pack-dyn-relocs=android) starting in API 23. It's possible to use both
+    // with --pack-dyn-relocs=android+relr, but we need to gather some data on
+    // the impact of that form before we can know if it's a good default.
+    //
+    // On the other hand, relr should always be an improvement.
+    ExtraOpts.push_back("--use-android-relr-tags");
+    ExtraOpts.push_back("--pack-dyn-relocs=relr");
+  }
+
   // 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..cf8752b4a27 100644
--- a/clang/test/Driver/linux-ld.c
+++ b/clang/test/Driver/linux-ld.c
@@ -1092,6 +1092,22 @@
 // CHECK-ANDROID-HASH-STYLE-M: "{{.*}}ld{{(.exe)?}}"
 // CHECK-ANDROID-HASH-STYLE-M: "--hash-style=gnu"
 
+// Check that we pass --pack-dyn-relocs=relr for API 28+ and not before.
+// RUN: %clang %s -### -o %t.o 2>&1 \
+// RUN:     --target=armv7-linux-android27 \
+// RUN:   | FileCheck --check-prefix=CHECK-ANDROID-RELR-27 %s
+// CHECK-ANDROID-RELR-27: "{{.*}}ld{{(.exe)?}}"
+// CHECK-ANDROID-RELR-27-NOT: "--pack-dyn-relocs=relr"
+// CHECK-ANDROID-RELR-27-NOT: "--pack-dyn-relocs=android+relr"
+//
+// RUN: %clang %s -### -o %t.o 2>&1 \
+// RUN:     --target=armv7-linux-android28 \
+// RUN:   | FileCheck --check-prefix=CHECK-ANDROID-RELR-28 %s
+// CHECK-ANDROID-RELR-28: "{{.*}}ld{{(.exe)?}}"
+// CHECK-ANDROID-RELR-28: "--use-android-relr-tags"
+// CHECK-ANDROID-RELR-28: "--pack-dyn-relocs=relr"
+// CHECK-ANDROID-RELR-28-NOT: "--pack-dyn-relocs=android+relr"
+
 // 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