Skip to content

[BOLT] Avoid reference updates for non-JT symbol operands #88838

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.

Sign up for GitHub

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
Apr 30, 2024
Merged

[BOLT] Avoid reference updates for non-JT symbol operands #88838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions bolt/lib/Passes/ValidateMemRefs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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");
Expand Down
60 changes: 60 additions & 0 deletions bolt/test/X86/jt-symbol-disambiguation-4.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# 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: %clang -no-pie %s -o %t.exe -Wl,-q
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test fails on my AArch64-linux machine.
It seems that something is missing on this clang command line to tell it explicitly to target x86? In some of the other tests in this directory, it seems that might be done indirectly by adding %cflags to the clang command line?

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://lab.llvm.org/buildbot/#/builders/221/builds/22130

Copy link
Contributor

Choose a reason for hiding this comment

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

On it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use llvm-mc + lld for assembly tests (check bolt/test/X86 for examples).


# RUN: %t.exe
Copy link
Contributor

@aaupov aaupov Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Runnable tests need to be under bolt/test/runtime. But in this case, there's no need to run the binary to verify the behavior. Please remove these lines with running the binary.

# RUN: llvm-bolt -funcs=main,foo/1 %t.exe -o %t.exe.bolt -jump-tables=move
# RUN: %t.exe.bolt

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a CHECK line verifying the intended behavior of the pass: that the output binary contains the correct reference.

.text
.globl main
.type main,@function
main:
pushq %rbp
movq %rsp, %rbp
movq $-16, %rax
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:
movq $0, %rax
jmpq *.LJTI2_0(,%rax,8)
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