[BOLT][AArch64] Fix error message for failed ADR relaxation #142533

maksfb · 2025-06-03T05:57:10Z

Do not recommend the strict mode to the user when ADR relaxation fails on a non-simple function, i.e. a function with unknown CFG.

We cannot rely on relocations to reconstruct compiler-generated jump tables for AArch64, hence strict mode does not work as intended.

Do not recommend the strict mode to the user when ADR relaxation fails on a non-simple function, i.e. a function with unknown CFG. We cannot rely on relocations to reconstruct compiler-generated jump tables for AArch64, hence strict mode does not work as intended.

llvmbot · 2025-06-03T05:57:31Z

@llvm/pr-subscribers-bolt

Author: Maksim Panchenko (maksfb)

Changes

Do not recommend the strict mode to the user when ADR relaxation fails on a non-simple function, i.e. a function with unknown CFG.

We cannot rely on relocations to reconstruct compiler-generated jump tables for AArch64, hence strict mode does not work as intended.

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

4 Files Affected:

(modified) bolt/lib/Passes/ADRRelaxationPass.cpp (+3-5)
(added) bolt/test/AArch64/adr-relaxation-fail.s (+35)
(modified) bolt/test/AArch64/r_aarch64_prelxx.s (+5-4)
(modified) bolt/test/AArch64/test-indirect-branch.s (+9-2)

diff --git a/bolt/lib/Passes/ADRRelaxationPass.cpp b/bolt/lib/Passes/ADRRelaxationPass.cpp
index 4b37a061ac12d..c3954c94a7f92 100644
--- a/bolt/lib/Passes/ADRRelaxationPass.cpp
+++ b/bolt/lib/Passes/ADRRelaxationPass.cpp
@@ -81,17 +81,15 @@ void ADRRelaxationPass::runOnFunction(BinaryFunction &BF) {
         It = BB.eraseInstruction(std::prev(It));
       } else if (std::next(It) != BB.end() && BC.MIB->isNoop(*std::next(It))) {
         BB.eraseInstruction(std::next(It));
-      } else if (!opts::StrictMode && !BF.isSimple()) {
+      } else if (!BF.isSimple()) {
         // If the function is not simple, it may contain a jump table undetected
         // by us. This jump table may use an offset from the branch instruction
         // to land in the desired place. If we add new instructions, we
         // invalidate this offset, so we have to rely on linker-inserted NOP to
         // replace it with ADRP, and abort if it is not present.
         auto L = BC.scopeLock();
-        BC.errs() << formatv(
-            "BOLT-ERROR: Cannot relax adr in non-simple function "
-            "{0}. Use --strict option to override\n",
-            BF.getOneName());
+        BC.errs() << "BOLT-ERROR: cannot relax ADR in non-simple function "
+                  << BF << '\n';
         PassFailed = true;
         return;
       }
diff --git a/bolt/test/AArch64/adr-relaxation-fail.s b/bolt/test/AArch64/adr-relaxation-fail.s
new file mode 100644
index 0000000000000..320d662be4d58
--- /dev/null
+++ b/bolt/test/AArch64/adr-relaxation-fail.s
@@ -0,0 +1,35 @@
+## Check that llvm-bolt generates a proper error message when ADR instruction
+## cannot be relaxed.
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -static
+# RUN: not llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
+
+# CHECK: BOLT-ERROR: cannot relax ADR in non-simple function _start
+
+## The function contains unknown control flow and llvm-bolt fails to recover
+## CFG. As BOLT has to preserve the function layout, the ADR instruction cannot
+## be relaxed into ADRP+ADD.
+  .text
+  .globl _start
+  .type _start, %function
+_start:
+  .cfi_startproc
+  adr x1, foo
+  br x0
+  ret  x0
+  .cfi_endproc
+  .size _start, .-_start
+
+  .globl foo
+  .type foo, %function
+foo:
+  .cfi_startproc
+  cmp  x1, x11
+  b.hi  .L2
+  mov  x0, #0x0
+.L2:
+  adr x1, foo
+  ret  x30
+  .cfi_endproc
+  .size foo, .-foo
diff --git a/bolt/test/AArch64/r_aarch64_prelxx.s b/bolt/test/AArch64/r_aarch64_prelxx.s
index 73bf8387d3634..708b66cb5ffd0 100644
--- a/bolt/test/AArch64/r_aarch64_prelxx.s
+++ b/bolt/test/AArch64/r_aarch64_prelxx.s
@@ -12,7 +12,7 @@
 // CHECKPREL-NEXT:  R_AARCH64_PREL32      {{.*}} _start + 4
 // CHECKPREL-NEXT:  R_AARCH64_PREL64      {{.*}} _start + 8
 
-// RUN: llvm-bolt %t.exe -o %t.bolt --strict
+// RUN: llvm-bolt %t.exe -o %t.bolt
 // RUN: llvm-objdump -D %t.bolt | FileCheck %s --check-prefix=CHECKPREL32
 
 // CHECKPREL32: [[#%x,DATATABLEADDR:]] <datatable>:
@@ -21,7 +21,7 @@
 
 // 4 is offset in datatable
 // 8 is addend
-// CHECKPREL32: [[#DATATABLEADDR + 4 - 8 + VALUE]] <_start>:
+// CHECKPREL32: [[#DATATABLEADDR + VALUE]] <_start>:
 
 // RUN: llvm-objdump -D %t.bolt | FileCheck %s --check-prefix=CHECKPREL64
 // CHECKPREL64: [[#%x,DATATABLEADDR:]] <datatable>:
@@ -32,14 +32,15 @@
 
 // 8 is offset in datatable
 // 12 is addend
-// CHECKPREL64: [[#DATATABLEADDR + 8 - 12 + VALUE]] <_start>:
+// CHECKPREL64: [[#DATATABLEADDR + VALUE]] <_start>:
 
   .section .text
   .align 4
   .globl _start
   .type _start, %function
 _start:
-  adr x0, datatable
+  adrp x0, datatable
+  add x0, x0, :lo12:datable
   mov x0, #0
   ret 
 
diff --git a/bolt/test/AArch64/test-indirect-branch.s b/bolt/test/AArch64/test-indirect-branch.s
index b99737ee97acc..a9fa7737d3d2d 100644
--- a/bolt/test/AArch64/test-indirect-branch.s
+++ b/bolt/test/AArch64/test-indirect-branch.s
@@ -7,8 +7,8 @@
 
 // RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown -mattr=+pauth %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 --strict --debug-only=mcplus \
-// RUN:  -v=1 2>&1 | FileCheck %s
+// RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg --debug-only=mcplus -v=1 2>&1 \
+// RUN:   | FileCheck %s
 
 // Pattern 1: there is no shift amount after the 'add' instruction.
 //
@@ -45,6 +45,7 @@ _start:
   .type  test1, %function
 test1:
   mov     x1, #0
+  nop
   adr     x3, datatable
   add     x3, x3, x1, lsl #2
   ldr     w2, [x3]
@@ -56,6 +57,11 @@ test1_1:
    ret
 test1_2:
    ret
+   .size test1, .-test1
+
+// Temporary workaround for PC-relative relocations from datatable leaking into
+// test2 function and creating phantom entry points.
+.skip 0x100
 
 // Pattern 2
 // CHECK: BOLT-DEBUG: failed to match indirect branch: nop/adr instead of adrp/add
@@ -65,6 +71,7 @@ test2:
   nop
   adr     x3, jump_table
   ldrh    w3, [x3, x1, lsl #1]
+  nop
   adr     x1, test2_0
   add     x3, x1, w3, sxth #2
   br      x3

maksfb · 2025-06-03T06:03:12Z

bolt/test/AArch64/r_aarch64_prelxx.s

@@ -21,7 +21,7 @@

 // 4 is offset in datatable
 // 8 is addend
-// CHECKPREL32: [[#DATATABLEADDR + 4 - 8 + VALUE]] <_start>:
+// CHECKPREL32: [[#DATATABLEADDR + VALUE]] <_start>:


@yavtuk, I had to modify this test case as I believe it was broken. The relocations that are generated for datatable are pointing to phantom entry points in _start. As ADR in _start was previously relaxed into ADRP+ADD, the entry points were shifted by 4 bytes.

maksfb · 2025-06-03T06:06:10Z

bolt/test/AArch64/test-indirect-branch.s

@@ -56,6 +57,11 @@ test1_1:
   ret
 test1_2:
   ret
+   .size test1, .-test1
+
+// Temporary workaround for PC-relative relocations from datatable leaking into


@yavtuk, here I ran into a similar problem. The way PC-relative data-to-code relocations are handled on AArch64 looks broken to me. Do you see such relocations in your internal binaries? LLVM compiler may generate those for relative vtables. Are there any other cases?

paschalis-mpeis

Hey Maksim, thanks for the patch.

With this PR we'll also have to adapt: runtime/AArch64/adrrelaxationpass.s

bolt/test/AArch64/adr-relaxation-fail.s

…tion

… relaxation

maksfb · 2025-06-03T08:30:13Z

With this PR we'll also have to adapt: runtime/AArch64/adrrelaxationpass.s

Thanks for spotting. I've deleted this test and moved useful bits into adr-relaxation.s.

paschalis-mpeis

Hey Maksim,

Great, thanks for the updates, looks good!

yota9

LG thanks

maksfb requested review from aaupov, rafaelauler, ayermolo, yota9 and paschalis-mpeis as code owners June 3, 2025 05:57

maksfb added the BOLT label Jun 3, 2025

maksfb commented Jun 3, 2025

View reviewed changes

fixup! [BOLT][AArch64] Fix error message for failed ADR relaxation

b9600c1

paschalis-mpeis reviewed Jun 3, 2025

View reviewed changes

bolt/test/AArch64/adr-relaxation-fail.s Outdated Show resolved Hide resolved

maksfb added 2 commits June 3, 2025 01:26

fixup! fixup! [BOLT][AArch64] Fix error message for failed ADR relaxa…

3692f52

…tion

fixup! fixup! fixup! [BOLT][AArch64] Fix error message for failed ADR…

f954a9f

… relaxation

paschalis-mpeis approved these changes Jun 3, 2025

View reviewed changes

yota9 approved these changes Jun 3, 2025

View reviewed changes

maksfb merged commit 8d86963 into llvm:main Jun 3, 2025
10 checks passed

paschalis-mpeis mentioned this pull request Jun 6, 2025

[BOLT][AArch64] Fix adr-relaxation.s test #143151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BOLT][AArch64] Fix error message for failed ADR relaxation #142533

[BOLT][AArch64] Fix error message for failed ADR relaxation #142533

maksfb commented Jun 3, 2025

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

maksfb Jun 3, 2025

Uh oh!

maksfb Jun 3, 2025

Uh oh!

paschalis-mpeis left a comment

Uh oh!

Uh oh!

maksfb commented Jun 3, 2025

Uh oh!

paschalis-mpeis left a comment

Uh oh!

yota9 left a comment

Uh oh!

Uh oh!

Uh oh!

[BOLT][AArch64] Fix error message for failed ADR relaxation #142533

[BOLT][AArch64] Fix error message for failed ADR relaxation #142533

Conversation

maksfb commented Jun 3, 2025

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

maksfb Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

maksfb Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

paschalis-mpeis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maksfb commented Jun 3, 2025

Uh oh!

paschalis-mpeis left a comment

Choose a reason for hiding this comment

Uh oh!

yota9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!