[BOLT][AArch64] Provide createDummyReturnFunction

[paschalis-mpeis]

On AArch64, this method is needed when trying to instrument a static
binary.

Sample commands:

clang -Wl,-q test.c -static -o out
llvm-bolt -instrument -instrumentation-sleep-time=5 out -o out.instr

---

Unrelated runtime crash with static binaries

Currently, on AArch64 there is an unrelated runtime crash (at binary loading time to be specific) with any rewritten > static binaries. Trying to invoke any of such binaries (including ones with instrumentation) will result to:

Unexpected reloc type in static binary.

This will be a separate patch:

• [WORKAROUND][BOLT][AArch64] Static binary patching for ELF. #97710

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Paschalis Mpeis (paschalis-mpeis)

Changes

On AArch64, this method is needed when trying to instrument a static
binary.

Sample commands:

clang -Wl,-q test.c -static -o out
llvm-bolt -instrument -instrumentation-sleep-time=5 out -o out.instr

---

This is a Stacked Pull Request on top of:

• #83394

Reviewing instructions:

• Please consider only the commits that are chronologically after: a0796e6 (see commits tab)
• any previous commits belong to the stacked PR (#<!-- -->83394), which is an external fork. So this was the only way to keep this PR part of llvm/llvm-project repo
• Eventually, once parrent PR is merged, this will be rebased on top of main

---

Full diff: https://github.com/llvm/llvm-project/pull/96626.diff

4 Files Affected:

• (modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+33-2)
• (added) bolt/test/AArch64/dummy-return.test (+10)
• (added) bolt/test/AArch64/test-indirect-branch.s (+110)
• (added) bolt/test/Inputs/main.c (+3)

diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index a74eda8e4a566..4388b796252dd 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -706,8 +706,20 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     unsigned ShiftVal = AArch64_AM::getArithShiftValue(OperandExtension);
     AArch64_AM::ShiftExtendType ExtendType =
         AArch64_AM::getArithExtendType(OperandExtension);
-    if (ShiftVal != 2)
-      llvm_unreachable("Failed to match indirect branch! (fragment 2)");
+    if (ShiftVal != 2) {
+      // TODO: Handle the patten where ShiftVal != 2.
+      // The following code sequence below has no shift amount,
+      // the range could be 0 to 4.
+      // The pattern comes from libc, it occurs when the binary is static.
+      //   adr     x6, 0x219fb0 <sigall_set+0x88>
+      //   add     x6, x6, x14, lsl #2
+      //   ldr     w7, [x6]
+      //   add     x6, x6, w7, sxtw => no shift amount
+      //   br      x6
+      errs() << "BOLT-WARNING: "
+                "Failed to match indirect branch: ShiftVAL != 2 \n";
+      return false;
+    }
 
     if (ExtendType == AArch64_AM::SXTB)
       ScaleValue = 1LL;
@@ -752,6 +764,19 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
       return true;
     }
 
+    if (DefJTBaseAdd->getOpcode() == AArch64::ADR) {
+      // TODO: Handle the pattern where there is no adrp/add pair.
+      // It also occurs when the binary is static.
+      //  adr     x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
+      //  ldrh    w13, [x13, w12, uxtw #1]
+      //  adr     x12, 0x247b30 <__gettextparse+0x5b0>
+      //  add     x13, x12, w13, sxth #2
+      //  br      x13
+      errs() << "BOLT-WARNING: Failed to match indirect branch: "
+                "nop/adr instead of adrp/add \n";
+      return false;
+    }
+
     assert(DefJTBaseAdd->getOpcode() == AArch64::ADDXri &&
            "Failed to match jump table base address pattern! (1)");
 
@@ -1582,6 +1607,12 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     return Instrs;
   }
 
+  InstructionListType createDummyReturnFunction(MCContext *Ctx) const override {
+    InstructionListType Insts(1);
+    createReturn(Insts[0]);
+    return Insts;
+  }
+
   std::vector<MCInst> createSymbolTrampoline(const MCSymbol *TgtSym,
                                              MCContext *Ctx) override {
     std::vector<MCInst> Insts;
diff --git a/bolt/test/AArch64/dummy-return.test b/bolt/test/AArch64/dummy-return.test
new file mode 100644
index 0000000000000..76f12cf9d4f09
--- /dev/null
+++ b/bolt/test/AArch64/dummy-return.test
@@ -0,0 +1,10 @@
+// Tests that AArch64 is able to instrument static binaries in relocation mode.
+
+REQUIRES: system-linux
+
+RUN: %clang %p/../Inputs/main.c -o %t -Wl,-q -static
+RUN: llvm-bolt -instrument -instrumentation-sleep-time=1 %t -o %t.instr 2>&1 | FileCheck %s
+RUN: llvm-nm -n %t.instr | FileCheck %s -check-prefix=CHECK-SYM
+
+CHECK: BOLT-INFO: output linked against instrumentation runtime library
+CHECK-SYM: __bolt_fini_trampoline
\ No newline at end of file
diff --git a/bolt/test/AArch64/test-indirect-branch.s b/bolt/test/AArch64/test-indirect-branch.s
new file mode 100644
index 0000000000000..cb9e325654774
--- /dev/null
+++ b/bolt/test/AArch64/test-indirect-branch.s
@@ -0,0 +1,110 @@
+// Test how BOLT handles indirect branch sequence of instructions in
+// AArch64MCPlus builder.
+// This test checks the pattern where there is no shift amount after add
+// instruction. The pattern come from libc, it can be reproduced with
+// a 'static' built binary.
+//
+//   adr     x6, 0x219fb0 <sigall_set+0x88>
+//   add     x6, x6, x14, lsl #2
+//   ldr     w7, [x6]
+//   add     x6, x6, w7, sxtw => no shift amount
+//   br      x6
+//
+// It also tests another case where there is no adrp/add pair.
+// The pattern also come from libc, and it only represents in the binary
+// if the lld linker is used to create the static binary.
+// It doesn't occur with GCC ld linker.
+//
+//  nop   => nop/adr instead of adrp/add
+//  adr     x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
+//  ldrh    w13, [x13, w12, uxtw #1]
+//  adr     x12, 0x247b30 <__gettextparse+0x5b0>
+//  add     x13, x12, w13, sxth #2
+//  br      x13
+
+// clang-format off
+
+// REQUIRES: system-linux
+// RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+// RUN: %clang %cflags --target=aarch64-unknown-linux %t.o -o %t.exe -Wl,-q
+// RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg \
+// RUN:  -v=1 2>&1 | FileCheck %s
+
+// CHECK: BOLT-WARNING: Failed to match indirect branch: nop/adr instead of adrp/add
+// CHECK: BOLT-WARNING: Failed to match indirect branch: ShiftVAL != 2
+
+
+  .section .text
+  .align 4
+  .globl _start
+  .type  _start, %function
+_start:
+  bl bar
+  bl end
+  mov x0, #4
+  mov w8, #93
+  svc #0
+
+bar:
+  mov     w1, #3
+  cmp     x1, #0
+  b.eq    end
+  nop
+  adr     x3, jump_table
+  ldrh    w3, [x3, x1, lsl #1]
+  adr     x1, .case0
+  add     x3, x1, w3, sxth #2
+  br      x3
+.case0:
+  mov     w0, #1
+  ret
+.case1:
+  mov     w0, #2
+  ret
+.case3:
+  mov     w0, #3
+  ret
+.case4:
+  nop
+  mov     x1, #0
+  adr     x3, datatable
+  add     x3, x3, x1, lsl #2
+  ldr     w2, [x3]
+  add     x3, x3, w2, sxtw
+  br      x3
+  nop
+  mov     w0, #4
+  ret
+.case7:
+  mov     w0, #4
+  ret
+
+foo1:
+   ret
+
+foo2:
+   add    w0, w0, #3
+   ret
+
+foo3:
+   add    w0, w0, #3
+   ret
+
+end:
+  add     x0, x0, #99
+  ret
+
+  .section .rodata,"a",@progbits
+jump_table:
+  .hword  (.case0-.case0)>>2
+  .hword  (.case1-.case0)>>2
+  .hword  (.case3-.case0)>>2
+  .hword  (.case4-.case0)>>2
+  .hword  (.case7-.case0)>>2
+
+
+datatable:
+  .word foo1-datatable
+  .word foo2-datatable
+  .word foo3-datatable
+  .word 20
diff --git a/bolt/test/Inputs/main.c b/bolt/test/Inputs/main.c
new file mode 100644
index 0000000000000..c05bedd98dba0
--- /dev/null
+++ b/bolt/test/Inputs/main.c
@@ -0,0 +1,3 @@
+// dummy function just for emitting relocations  to the linker.
+int foo() { return 0; }
+int main(int argc, char **argv) { return foo() + 1; }

[paschalis-mpeis]

Hi @samolisov,
Thanks for your review; I hope I've addressed your concerns.

Since you requested some minor generic refactoring, I renamed the function to createDummyReturn, as we are simply populating the function body in this case. Let me know if you think it needs more adjustments.

Also, I've added you as a reviewer to #83394 a few days back (the parent PR this patch is stacked upon), in case you wanted to have a look. In either case, it looks like there is a consensus and that patch is going forward soon.

---

This is still a stacked pull-requests, showing more changes in diff view.

[yota9]

Hi please rebase the PR

[paschalis-mpeis]

Now that #83394 is merged, I was able to rebase to main.

@yota9, can you please have a look now, sharing your thoughts on @samolisov concerns here as well?

Thanks in advance,
Paschalis

[samolisov]

Thanks for addressing, LGTM.

[yota9]

Thanks!

[aaupov]

Thanks for working on this. Left a comment inline. Please also retitle as an imperative statement: "[BOLT][AArch64] Provide createDummyReturnFunction".

[paschalis-mpeis]

Force-pushed history:

Adding %cflags interfered with my previous c test dummy-return.test. I preferred not to expand that test (ie adding a _start method and letting the linker know about the entry method), but instead I've written a minimal assembly test.

I force-pushed, as I wanted to introduce the asm test on the initial 'showcase-commit'.
The last commit drops dummy-return.test and Inputs/main.c.