Skip to content

[BOLT] Fix ValidateMemRefs pass #94406

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
Jun 4, 2024
Merged

Conversation

maksfb
Copy link
Contributor

@maksfb maksfb commented Jun 4, 2024

In ValidateMemRefs pass, when we validate references in the form of Symbol + Addend, we should check Symbol not Symbol + Addend against aliasing a jump table.

Recommitting with a modified test case: #88838

In ValidateMemRefs pass, when we validate references in the form of
`Symbol + Addend`, we should check `Symbol` not `Symbol + Addend`
against aliasing a jump table.
@llvmbot
Copy link
Member

llvmbot commented Jun 4, 2024

@llvm/pr-subscribers-bolt

Author: Maksim Panchenko (maksfb)

Changes

In ValidateMemRefs pass, when we validate references in the form of Symbol + Addend, we should check Symbol not Symbol + Addend against aliasing a jump table.

Recommitting with a modified test case: #88838


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

2 Files Affected:

  • (modified) bolt/lib/Passes/ValidateMemRefs.cpp (+4-4)
  • (added) bolt/test/X86/jt-symbol-disambiguation-4.s (+63)
diff --git a/bolt/lib/Passes/ValidateMemRefs.cpp b/bolt/lib/Passes/ValidateMemRefs.cpp
index f29a97c43f497..ca58493b279c9 100644
--- a/bolt/lib/Passes/ValidateMemRefs.cpp
+++ b/bolt/lib/Passes/ValidateMemRefs.cpp
@@ -29,8 +29,7 @@ bool ValidateMemRefs::checkAndFixJTReference(BinaryFunction &BF, MCInst &Inst,
   if (!BD)
     return false;
 
-  const uint64_t TargetAddress = BD->getAddress() + Offset;
-  JumpTable *JT = BC.getJumpTableContainingAddress(TargetAddress);
+  JumpTable *JT = BC.getJumpTableContainingAddress(BD->getAddress());
   if (!JT)
     return false;
 
@@ -43,8 +42,9 @@ bool ValidateMemRefs::checkAndFixJTReference(BinaryFunction &BF, MCInst &Inst,
   // the jump table label with a regular rodata reference. Get a
   // non-JT reference by fetching the symbol 1 byte before the JT
   // label.
-  MCSymbol *NewSym = BC.getOrCreateGlobalSymbol(TargetAddress - 1, "DATAat");
-  BC.MIB->setOperandToSymbolRef(Inst, OperandNum, NewSym, 1, &*BC.Ctx, 0);
+  MCSymbol *NewSym = BC.getOrCreateGlobalSymbol(BD->getAddress() - 1, "DATAat");
+  BC.MIB->setOperandToSymbolRef(Inst, OperandNum, NewSym, Offset + 1, &*BC.Ctx,
+                                0);
   LLVM_DEBUG(dbgs() << "BOLT-DEBUG: replaced reference @" << BF.getPrintName()
                     << " from " << BD->getName() << " to " << NewSym->getName()
                     << " + 1\n");
diff --git a/bolt/test/X86/jt-symbol-disambiguation-4.s b/bolt/test/X86/jt-symbol-disambiguation-4.s
new file mode 100644
index 0000000000000..d3d3dcd807054
--- /dev/null
+++ b/bolt/test/X86/jt-symbol-disambiguation-4.s
@@ -0,0 +1,63 @@
+## If the operand references a symbol that differs from the jump table label,
+## no reference updating is required even if its target address resides within
+## the jump table's range.
+## In this test case, consider the second instruction within the main function,
+## where the address resulting from 'c + 17' corresponds to one byte beyond the
+## address of the .LJTI2_0 jump table label. However, this operand represents
+## an offset calculation related to the global variable 'c' and should remain
+## unaffected by the jump table.
+
+# REQUIRES: system-linux
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
+# RUN: %clang -no-pie %t.o -o %t.exe -Wl,-q
+# RUN: llvm-bolt --funcs=main,foo/1 %t.exe -o %t.exe.bolt --print-normalized \
+# RUN:   2>&1 | FileCheck %s
+
+	.text
+	.globl	main
+	.type	main,@function
+main:
+# CHECK: Binary Function "main"
+	pushq   %rbp
+	movq	%rsp, %rbp
+	movq	$-16, %rax
+	movl	c+17(%rax), %edx
+# CHECK: movl	c+17(%rax), %edx
+	cmpl	$255, %edx
+	je	.LCorrect
+	movl	$1, %eax
+	popq	%rbp
+	ret
+.LCorrect:
+	movl	$0, %eax
+	popq	%rbp
+	ret
+
+	.p2align	4, 0x90
+	.type	foo,@function
+foo:
+# CHECK: Binary Function "foo
+	movq	$0, %rax
+	jmpq	*.LJTI2_0(,%rax,8)
+# CHECK: jmpq	*{{.*}} # JUMPTABLE
+	addl	$-36, %eax
+.LBB2_2:
+	addl	$-16, %eax
+	retq
+	.section	.rodata,"a",@progbits
+	.type	c,@object
+	.data
+	.globl	c
+	.p2align	4, 0x0
+c:
+	.byte 1
+  .byte 0xff
+	.zero	14
+	.size	c, 16
+.LJTI2_0:
+	.quad	.LBB2_2
+	.quad	.LBB2_2
+	.quad	.LBB2_2
+	.quad	.LBB2_2
+

@maksfb maksfb merged commit c923d39 into llvm:main Jun 4, 2024
6 of 7 checks passed
@maksfb maksfb deleted the gh-fix-validatememrefs branch March 6, 2025 02:47
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.

5 participants