Skip to content

Commit

Permalink
[BOLT] Ignore functions accessing false positive jump tables
Browse files Browse the repository at this point in the history
Disassembly and branch target analysis are not decoupled, so any
analysis that depends on disassembly may not operate properly.

In specific, analyzeJumpTable uses instruction bounds check property.
A jump table was analyzed twice: (a) during disassembly, and (b) after
disassembly, so there are potentially some mismatched results.

In this update, functions that access JTs which fail the second check
will be marked as ignored.

Test Plan:
```
ninja check-bolt
```

Reviewed By: Amir

Differential Revision: https://reviews.llvm.org/D130431
  • Loading branch information
nhuhuan authored and aaupov committed Jul 29, 2022
1 parent ccabbff commit 52cd00c
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 1 deletion.
23 changes: 22 additions & 1 deletion bolt/lib/Core/BinaryContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,10 @@ bool BinaryContext::analyzeJumpTable(
void BinaryContext::populateJumpTables() {
LLVM_DEBUG(dbgs() << "DataPCRelocations: " << DataPCRelocations.size()
<< '\n');

// Collect jump tables that pass the analyzeJumpTable's first check,
// but fail the analyzeJumpTable's second check
SmallVector<JumpTable *, 1> AbortedJTs;
for (auto JTI = JumpTables.begin(), JTE = JumpTables.end(); JTI != JTE;
++JTI) {
JumpTable *JT = JTI->second;
Expand All @@ -640,6 +644,13 @@ void BinaryContext::populateJumpTables() {
const bool Success =
analyzeJumpTable(JT->getAddress(), JT->Type, *(JT->Parents[0]),
NextJTAddress, &JT->EntriesAsAddress);
// !Success means a false positive from earlier analysis run due to
// different context. A possible culprit is instruction bounds check.
// Previous run happens during disassembly. If the target function
// is not disassembled, the check will be skipped, leading to a false
// positive
//
// Solution: Ignore fragments accessing JT that fails the check
if (!Success) {
LLVM_DEBUG(ListSeparator LS;
dbgs() << "failed to analyze jump table in function ";
Expand All @@ -659,7 +670,8 @@ void BinaryContext::populateJumpTables() {
dbgs() << "\n";);
NextJTI->second->print(dbgs());
}
llvm_unreachable("jump table heuristic failure");
AbortedJTs.push_back(JT);
continue;
}
for (BinaryFunction *Frag : JT->Parents) {
for (uint64_t EntryAddress : JT->EntriesAsAddress)
Expand Down Expand Up @@ -689,6 +701,15 @@ void BinaryContext::populateJumpTables() {
addFragmentsToSkip(Frag);
}

// Ignore fragments accessing JT that fails analyzeJumpTable check
for (JumpTable *JT : AbortedJTs) {
for (BinaryFunction *Frag : JT->Parents) {
Frag->setIgnored();
Frag->JumpTables.erase(Frag->JumpTables.find(JT->getAddress()));
}
JumpTables.erase(JumpTables.find(JT->getAddress()));
}

if (opts::StrictMode && DataPCRelocations.size()) {
LLVM_DEBUG({
dbgs() << DataPCRelocations.size()
Expand Down
49 changes: 49 additions & 0 deletions bolt/test/X86/fake_jump_table.s
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Currently disassembly is not decoupled from branch target analysis.
# This causes a few checks related to availability of target insn to
# fail for stripped binaries:
# (a) analyzeJumpTable
# (b) postProcessEntryPoints
# This test checks if BOLT can safely support instruction bounds check
# for cross-function targets.

# REQUIRES: system-linux

# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q
# RUN: llvm-bolt %t.exe -o %t.out -v=1 -print-cfg

.text
.globl main
.type main, %function
.p2align 2
main:
LBB0:
.cfi_startproc
andl $0xf, %ecx
cmpb $0x4, %cl
ja .main.cold.1
LBB1:
leaq FAKE_JUMP_TABLE(%rip), %r8
cmpq %r8, %r9
LBB2:
xorq %rax, %rax
ret
.cfi_endproc
.size main, .-main

.globl main.cold.1
.type main.cold.1, %function
.p2align 2
main.cold.1:
.cfi_startproc
nop
LBB3:
callq abort
.cfi_endproc
.size main.cold.1, .-main.cold.1

.rodata
.globl FAKE_JUMP_TABLE
FAKE_JUMP_TABLE:
.long LBB2-FAKE_JUMP_TABLE
.long LBB3-FAKE_JUMP_TABLE+0x1

0 comments on commit 52cd00c

Please sign in to comment.