Skip to content

[BOLT][AArch64] Fix crash for conditional tail calls #140669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 20, 2025

Conversation

maksfb
Copy link
Contributor

@maksfb maksfb commented May 20, 2025

When conditional tail call is located in old code while BOLT is operating in lite mode, the call will require optional pending relocation with a type that is currently not supported resulting in a build-time crash.

Before a proper fix is implemented, ignore conditional tail calls for relocation purposes and mark their target functions to be patched, i.e. to be served as veneers/thunks.

When conditional tail call is located in old code while BOLT is
operating in lite mode, the call will require optional pending
relocation with a type that is currently not supported resulting in a
build-time crash.

Before a proper fix is implemented, ignore conditional tail calls for
relocation purposes and mark their target functions to be patched, i.e.
to be served as veneers/thunks.
@llvmbot
Copy link
Member

llvmbot commented May 20, 2025

@llvm/pr-subscribers-bolt

Author: Maksim Panchenko (maksfb)

Changes

When conditional tail call is located in old code while BOLT is operating in lite mode, the call will require optional pending relocation with a type that is currently not supported resulting in a build-time crash.

Before a proper fix is implemented, ignore conditional tail calls for relocation purposes and mark their target functions to be patched, i.e. to be served as veneers/thunks.


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

2 Files Affected:

  • (modified) bolt/lib/Core/BinaryFunction.cpp (+16-4)
  • (modified) bolt/test/AArch64/lite-mode.s (+9)
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 851fa36a6b4b7..1f9f023a10811 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1783,10 +1783,22 @@ bool BinaryFunction::scanExternalRefs() {
     // On AArch64, we use instruction patches for fixing references. We make an
     // exception for branch instructions since they require optional
     // relocations.
-    if (BC.isAArch64() && !BranchTargetSymbol) {
-      LLVM_DEBUG(BC.printInstruction(dbgs(), Instruction, AbsoluteInstrAddr));
-      InstructionPatches.push_back({AbsoluteInstrAddr, Instruction});
-      continue;
+    if (BC.isAArch64()) {
+      if (!BranchTargetSymbol) {
+        LLVM_DEBUG(BC.printInstruction(dbgs(), Instruction, AbsoluteInstrAddr));
+        InstructionPatches.push_back({AbsoluteInstrAddr, Instruction});
+        continue;
+      }
+
+      // Conditional tail calls require new relocation types that are currently
+      // not supported. https://github.com/llvm/llvm-project/issues/138264
+      if (BC.MIB->isConditionalBranch(Instruction)) {
+        if (BinaryFunction *TargetBF =
+                BC.getFunctionForSymbol(BranchTargetSymbol)) {
+          TargetBF->setNeedsPatch(true);
+          continue;
+        }
+      }
     }
 
     // Emit the instruction using temp emitter and generate relocations.
diff --git a/bolt/test/AArch64/lite-mode.s b/bolt/test/AArch64/lite-mode.s
index d1e35ef75de46..f2d06219f7a2d 100644
--- a/bolt/test/AArch64/lite-mode.s
+++ b/bolt/test/AArch64/lite-mode.s
@@ -129,6 +129,15 @@ cold_function:
 # CHECK-INPUT-NEXT: b {{.*}} <_start>
 # CHECK-NEXT:       b {{.*}} <_start.org.0>
 
+## Quick test for conditional tail calls. A proper test is being added in:
+## https://github.com/llvm/llvm-project/pull/139565
+## For now check that llvm-bolt doesn't choke on CTCs.
+.ifndef COMPACT
+  b.eq _start
+  cbz x0, _start
+  tbz x0, 42, _start
+.endif
+
   .cfi_endproc
   .size cold_function, .-cold_function
 

Copy link
Member

@paschalis-mpeis paschalis-mpeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@maksfb maksfb merged commit 51e222e into llvm:main May 20, 2025
12 checks passed
kostasalv pushed a commit to kostasalv/llvm-project that referenced this pull request May 21, 2025
When conditional tail call is located in old code while BOLT is
operating in lite mode, the call will require optional pending
relocation with a type that is currently not supported resulting in a
build-time crash.

Before a proper fix is implemented, ignore conditional tail calls for
relocation purposes and mark their target functions to be patched, i.e.
to be served as veneers/thunks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants