[BOLT] Support POSSIBLE_PIC_FIXED_BRANCH #91667
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
aaupov
requested review from
maksfb,
rafaelauler,
ayermolo and
dcci
as code owners
|
@llvm/pr-subscribers-bolt Author: Amir Ayupov (aaupov) ChangesDetect and support fixed PIC indirect jumps of the following form: with PIC_JUMP_TABLE that looks like following: The code could be produced by compilers, see Test Plan: updated jump-table-fixed-ref-pic.test Full diff: https://github.com/llvm/llvm-project/pull/91667.diff 8 Files Affected:
diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h
index 4a59a581dfedb..f8bf29c674b54 100644
--- a/bolt/include/bolt/Core/BinaryContext.h
+++ b/bolt/include/bolt/Core/BinaryContext.h
@@ -430,6 +430,11 @@ class BinaryContext {
return nullptr;
}
+ /// Deregister JumpTable registered at a given \p Address.
+ bool deregisterJumpTable(uint64_t Address) {
+ return JumpTables.erase(Address);
+ }
+
unsigned getDWARFEncodingSize(unsigned Encoding) {
if (Encoding == dwarf::DW_EH_PE_omit)
return 0;
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index f7614cf9ac977..42ec006fba9f1 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -58,6 +58,8 @@ enum class IndirectBranchType : char {
POSSIBLE_PIC_JUMP_TABLE, /// Possibly a jump table for PIC.
POSSIBLE_GOTO, /// Possibly a gcc's computed goto.
POSSIBLE_FIXED_BRANCH, /// Possibly an indirect branch to a fixed location.
+ POSSIBLE_PIC_FIXED_BRANCH, /// Possibly an indirect jump to a fixed entry in a
+ /// PIC jump table.
};
class MCPlusBuilder {
@@ -1472,7 +1474,8 @@ class MCPlusBuilder {
InstructionIterator End, const unsigned PtrSize,
MCInst *&MemLocInstr, unsigned &BaseRegNum,
unsigned &IndexRegNum, int64_t &DispValue,
- const MCExpr *&DispExpr, MCInst *&PCRelBaseOut) const {
+ const MCExpr *&DispExpr, MCInst *&PCRelBaseOut,
+ MCInst *&FixedEntryLoadInst) const {
llvm_unreachable("not implemented");
return IndirectBranchType::UNKNOWN;
}
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index f74ecea8ac0a1..799065a6f194e 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -779,6 +779,9 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size,
// setting the value of the register used by the branch.
MCInst *MemLocInstr;
+ // The instruction loading the fixed PIC jump table entry value.
+ MCInst *FixedEntryLoadInstr;
+
// Address of the table referenced by MemLocInstr. Could be either an
// array of function pointers, or a jump table.
uint64_t ArrayStart = 0;
@@ -810,7 +813,7 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size,
IndirectBranchType BranchType = BC.MIB->analyzeIndirectBranch(
Instruction, Begin, Instructions.end(), PtrSize, MemLocInstr, BaseRegNum,
- IndexRegNum, DispValue, DispExpr, PCRelBaseInstr);
+ IndexRegNum, DispValue, DispExpr, PCRelBaseInstr, FixedEntryLoadInstr);
if (BranchType == IndirectBranchType::UNKNOWN && !MemLocInstr)
return BranchType;
@@ -876,6 +879,43 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size,
if (BaseRegNum == BC.MRI->getProgramCounter())
ArrayStart += getAddress() + Offset + Size;
+ if (FixedEntryLoadInstr) {
+ assert(BranchType == IndirectBranchType::POSSIBLE_PIC_FIXED_BRANCH &&
+ "Invalid IndirectBranch type");
+ const MCExpr *FixedEntryDispExpr =
+ BC.MIB->getMemOperandDisp(*FixedEntryLoadInstr)->getExpr();
+ const uint64_t EntryAddress = getExprValue(FixedEntryDispExpr);
+ uint64_t EntrySize = BC.getJumpTableEntrySize(JumpTable::JTT_PIC);
+ ErrorOr<int64_t> Value = BC.getSignedValueAtAddress(EntryAddress, EntrySize);
+ if (!Value)
+ return IndirectBranchType::UNKNOWN;
+
+ BC.outs() << "BOLT-INFO: fixed PIC indirect branch detected in " << *this
+ << " at 0x" << Twine::utohexstr(getAddress() + Offset)
+ << " referencing data at 0x" << Twine::utohexstr(EntryAddress)
+ << " the destination value is 0x"
+ << Twine::utohexstr(ArrayStart + *Value) << '\n';
+
+ TargetAddress = ArrayStart + *Value;
+
+ // Remove spurious JumpTable at EntryAddress caused by PIC reference from
+ // the load instruction.
+ JumpTable *JT = BC.getJumpTableContainingAddress(EntryAddress);
+ assert(JT && "Must have a jump table at fixed entry address");
+ BC.deregisterJumpTable(EntryAddress);
+ JumpTables.erase(EntryAddress);
+ delete JT;
+
+ // Replace FixedEntryDispExpr used in target address calculation with outer
+ // jump table reference.
+ JT = BC.getJumpTableContainingAddress(ArrayStart);
+ assert(JT && "Must have a containing jump table for PIC fixed branch");
+ BC.MIB->replaceMemOperandDisp(*FixedEntryLoadInstr, JT->getFirstLabel(),
+ EntryAddress - ArrayStart, &*BC.Ctx);
+
+ return BranchType;
+ }
+
LLVM_DEBUG(dbgs() << "BOLT-DEBUG: addressed memory is 0x"
<< Twine::utohexstr(ArrayStart) << '\n');
@@ -1128,6 +1168,7 @@ void BinaryFunction::handleIndirectBranch(MCInst &Instruction, uint64_t Size,
if (opts::JumpTables == JTS_NONE)
IsSimple = false;
break;
+ case IndirectBranchType::POSSIBLE_PIC_FIXED_BRANCH:
case IndirectBranchType::POSSIBLE_FIXED_BRANCH: {
if (containsAddress(IndirectTarget)) {
const MCSymbol *TargetSymbol = getOrCreateLocalLabel(IndirectTarget);
@@ -1894,9 +1935,11 @@ bool BinaryFunction::postProcessIndirectBranches(
int64_t DispValue;
const MCExpr *DispExpr;
MCInst *PCRelBaseInstr;
+ MCInst *FixedEntryLoadInstr;
IndirectBranchType Type = BC.MIB->analyzeIndirectBranch(
Instr, BB.begin(), II, PtrSize, MemLocInstr, BaseRegNum,
- IndexRegNum, DispValue, DispExpr, PCRelBaseInstr);
+ IndexRegNum, DispValue, DispExpr, PCRelBaseInstr,
+ FixedEntryLoadInstr);
if (Type != IndirectBranchType::UNKNOWN || MemLocInstr != nullptr)
continue;
diff --git a/bolt/lib/Passes/IndirectCallPromotion.cpp b/bolt/lib/Passes/IndirectCallPromotion.cpp
index 55eede641fd2f..9483f7c86e366 100644
--- a/bolt/lib/Passes/IndirectCallPromotion.cpp
+++ b/bolt/lib/Passes/IndirectCallPromotion.cpp
@@ -386,13 +386,14 @@ IndirectCallPromotion::maybeGetHotJumpTableTargets(BinaryBasicBlock &BB,
JumpTableInfoType HotTargets;
MCInst *MemLocInstr;
MCInst *PCRelBaseOut;
+ MCInst *FixedEntryLoadInstr;
unsigned BaseReg, IndexReg;
int64_t DispValue;
const MCExpr *DispExpr;
MutableArrayRef<MCInst> Insts(&BB.front(), &CallInst);
const IndirectBranchType Type = BC.MIB->analyzeIndirectBranch(
CallInst, Insts.begin(), Insts.end(), BC.AsmInfo->getCodePointerSize(),
- MemLocInstr, BaseReg, IndexReg, DispValue, DispExpr, PCRelBaseOut);
+ MemLocInstr, BaseReg, IndexReg, DispValue, DispExpr, FixedEntryLoadInstr);
assert(MemLocInstr && "There should always be a load for jump tables");
if (!MemLocInstr)
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 0ae9d3668b93b..8fa004b817579 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -832,16 +832,20 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return Uses;
}
- IndirectBranchType analyzeIndirectBranch(
- MCInst &Instruction, InstructionIterator Begin, InstructionIterator End,
- const unsigned PtrSize, MCInst *&MemLocInstrOut, unsigned &BaseRegNumOut,
- unsigned &IndexRegNumOut, int64_t &DispValueOut,
- const MCExpr *&DispExprOut, MCInst *&PCRelBaseOut) const override {
+ IndirectBranchType
+ analyzeIndirectBranch(MCInst &Instruction, InstructionIterator Begin,
+ InstructionIterator End, const unsigned PtrSize,
+ MCInst *&MemLocInstrOut, unsigned &BaseRegNumOut,
+ unsigned &IndexRegNumOut, int64_t &DispValueOut,
+ const MCExpr *&DispExprOut, MCInst *&PCRelBaseOut,
+ const MCExpr *&JTBaseDispExprOut, MCInst *&PCRelBaseOut,
+ MCInst *&FixedEntryLoadInstr) const override {
MemLocInstrOut = nullptr;
BaseRegNumOut = AArch64::NoRegister;
IndexRegNumOut = AArch64::NoRegister;
DispValueOut = 0;
DispExprOut = nullptr;
+ FixedEntryLoadInstr = nullptr;
// An instruction referencing memory used by jump instruction (directly or
// via register). This location could be an array of function pointers
diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
index 86e7d4dfaed8d..f7c173aa23b35 100644
--- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -1897,7 +1897,7 @@ class X86MCPlusBuilder : public MCPlusBuilder {
}
template <typename Itr>
- std::pair<IndirectBranchType, MCInst *>
+ std::tuple<IndirectBranchType, MCInst *, MCInst *>
analyzePICJumpTable(Itr II, Itr IE, MCPhysReg R1, MCPhysReg R2) const {
// Analyze PIC-style jump table code template:
//
@@ -1906,6 +1906,13 @@ class X86MCPlusBuilder : public MCPlusBuilder {
// add %r2, %r1
// jmp *%r1
//
+ // or a fixed indirect jump template:
+ //
+ // movslq En(%rip), {%r2|%r1}
+ // lea PIC_JUMP_TABLE(%rip), {%r1|%r2} <- MemLocInstr
+ // add %r2, %r1
+ // jmp *%r1
+ //
// (with any irrelevant instructions in-between)
//
// When we call this helper we've already determined %r1 and %r2, and
@@ -1947,8 +1954,13 @@ class X86MCPlusBuilder : public MCPlusBuilder {
};
LLVM_DEBUG(dbgs() << "Checking for PIC jump table\n");
MCInst *MemLocInstr = nullptr;
- const MCInst *MovInstr = nullptr;
+ MCInst *MovInstr = nullptr;
+ bool IsFixedBranch = false;
+ // Allow renaming R1/R2 once.
+ bool SwappedRegs = false;
while (++II != IE) {
+ if (MovInstr && MemLocInstr)
+ break;
MCInst &Instr = *II;
const MCInstrDesc &InstrDesc = Info->get(Instr.getOpcode());
if (!InstrDesc.hasDefOfPhysReg(Instr, R1, *RegInfo) &&
@@ -1956,19 +1968,18 @@ class X86MCPlusBuilder : public MCPlusBuilder {
// Ignore instructions that don't affect R1, R2 registers.
continue;
}
- if (!MovInstr) {
- // Expect to see MOV instruction.
- if (!isMOVSX64rm32(Instr)) {
- LLVM_DEBUG(dbgs() << "MOV instruction expected.\n");
- break;
- }
-
+ if (isMOVSX64rm32(Instr)) {
+ // Potential redefinition of MovInstr - bail.
+ if (MovInstr)
+ return std::make_tuple(IndirectBranchType::UNKNOWN, nullptr, nullptr);
// Check if it's setting %r1 or %r2. In canonical form it sets %r2.
// If it sets %r1 - rename the registers so we have to only check
// a single form.
unsigned MovDestReg = Instr.getOperand(0).getReg();
- if (MovDestReg != R2)
+ if (!SwappedRegs && MovDestReg != R2) {
std::swap(R1, R2);
+ SwappedRegs = true;
+ }
if (MovDestReg != R2) {
LLVM_DEBUG(dbgs() << "MOV instruction expected to set %r2\n");
break;
@@ -1978,16 +1989,26 @@ class X86MCPlusBuilder : public MCPlusBuilder {
std::optional<X86MemOperand> MO = evaluateX86MemoryOperand(Instr);
if (!MO)
break;
- if (!isIndexed(*MO, R1))
- // POSSIBLE_PIC_JUMP_TABLE
+ if (isRIPRel(*MO))
+ IsFixedBranch = true;
+ else if (isIndexed(*MO, R1))
+ ; // POSSIBLE_PIC_JUMP_TABLE
+ else
break;
MovInstr = &Instr;
- } else {
- if (!InstrDesc.hasDefOfPhysReg(Instr, R1, *RegInfo))
- continue;
- if (!isLEA64r(Instr)) {
- LLVM_DEBUG(dbgs() << "LEA instruction expected\n");
- break;
+ continue;
+ }
+ if (isLEA64r(Instr)) {
+ // Potential redefinition of MemLocInstr - bail.
+ if (MemLocInstr)
+ return std::make_tuple(IndirectBranchType::UNKNOWN, nullptr, nullptr);
+ // Check if it's setting %r1 or %r2. In canonical form it sets %r1.
+ // If it sets %r2 - rename the registers so we have to only check
+ // a single form.
+ unsigned LeaDestReg = Instr.getOperand(0).getReg();
+ if (!SwappedRegs && LeaDestReg != R1) {
+ std::swap(R1, R2);
+ SwappedRegs = true;
}
if (Instr.getOperand(0).getReg() != R1) {
LLVM_DEBUG(dbgs() << "LEA instruction expected to set %r1\n");
@@ -2001,23 +2022,30 @@ class X86MCPlusBuilder : public MCPlusBuilder {
if (!isRIPRel(*MO))
break;
MemLocInstr = &Instr;
- break;
+ continue;
}
}
if (!MemLocInstr)
- return std::make_pair(IndirectBranchType::UNKNOWN, nullptr);
+ return std::make_tuple(IndirectBranchType::UNKNOWN, nullptr, nullptr);
+
+ LLVM_DEBUG(dbgs() << "checking potential fixed indirect branch\n");
+ if (IsFixedBranch)
+ return std::make_tuple(IndirectBranchType::POSSIBLE_PIC_FIXED_BRANCH,
+ MemLocInstr, MovInstr);
LLVM_DEBUG(dbgs() << "checking potential PIC jump table\n");
- return std::make_pair(IndirectBranchType::POSSIBLE_PIC_JUMP_TABLE,
- MemLocInstr);
+ return std::make_tuple(IndirectBranchType::POSSIBLE_PIC_JUMP_TABLE,
+ MemLocInstr, nullptr);
}
- IndirectBranchType analyzeIndirectBranch(
- MCInst &Instruction, InstructionIterator Begin, InstructionIterator End,
- const unsigned PtrSize, MCInst *&MemLocInstrOut, unsigned &BaseRegNumOut,
- unsigned &IndexRegNumOut, int64_t &DispValueOut,
- const MCExpr *&DispExprOut, MCInst *&PCRelBaseOut) const override {
+ IndirectBranchType
+ analyzeIndirectBranch(MCInst &Instruction, InstructionIterator Begin,
+ InstructionIterator End, const unsigned PtrSize,
+ MCInst *&MemLocInstrOut, unsigned &BaseRegNumOut,
+ unsigned &IndexRegNumOut, int64_t &DispValueOut,
+ const MCExpr *&DispExprOut, MCInst *&PCRelBaseOut,
+ MCInst *&FixedEntryLoadInst) const override {
// Try to find a (base) memory location from where the address for
// the indirect branch is loaded. For X86-64 the memory will be specified
// in the following format:
@@ -2044,6 +2072,7 @@ class X86MCPlusBuilder : public MCPlusBuilder {
IndexRegNumOut = X86::NoRegister;
DispValueOut = 0;
DispExprOut = nullptr;
+ FixedEntryLoadInst = nullptr;
std::reverse_iterator<InstructionIterator> II(End);
std::reverse_iterator<InstructionIterator> IE(Begin);
@@ -2076,7 +2105,8 @@ class X86MCPlusBuilder : public MCPlusBuilder {
unsigned R2 = PrevInstr.getOperand(2).getReg();
if (R1 == R2)
return IndirectBranchType::UNKNOWN;
- std::tie(Type, MemLocInstr) = analyzePICJumpTable(PrevII, IE, R1, R2);
+ std::tie(Type, MemLocInstr, FixedEntryLoadInst) =
+ analyzePICJumpTable(PrevII, IE, R1, R2);
break;
}
return IndirectBranchType::UNKNOWN;
@@ -2120,6 +2150,8 @@ class X86MCPlusBuilder : public MCPlusBuilder {
if (MO->ScaleImm != 1 || MO->BaseRegNum != RIPRegister)
return IndirectBranchType::UNKNOWN;
break;
+ case IndirectBranchType::POSSIBLE_PIC_FIXED_BRANCH:
+ break;
default:
if (MO->ScaleImm != PtrSize)
return IndirectBranchType::UNKNOWN;
diff --git a/bolt/test/X86/Inputs/jump-table-fixed-ref-pic.s b/bolt/test/X86/Inputs/jump-table-fixed-ref-pic.s
index 66629a4880e64..6407964593e2d 100644
--- a/bolt/test/X86/Inputs/jump-table-fixed-ref-pic.s
+++ b/bolt/test/X86/Inputs/jump-table-fixed-ref-pic.s
@@ -6,7 +6,7 @@ main:
jae .L4
cmpq $0x1, %rdi
jne .L4
- mov .Ljt_pic+8(%rip), %rax
+ movslq .Ljt_pic+8(%rip), %rax
lea .Ljt_pic(%rip), %rdx
add %rdx, %rax
jmpq *%rax
diff --git a/bolt/test/X86/jump-table-fixed-ref-pic.test b/bolt/test/X86/jump-table-fixed-ref-pic.test
index 4195b97aac501..d2e90eb1d4099 100644
--- a/bolt/test/X86/jump-table-fixed-ref-pic.test
+++ b/bolt/test/X86/jump-table-fixed-ref-pic.test
@@ -1,9 +1,16 @@
# Verify that BOLT detects fixed destination of indirect jump for PIC
# case.
-XFAIL: *
-
RUN: %clang %cflags -no-pie %S/Inputs/jump-table-fixed-ref-pic.s -Wl,-q -o %t
-RUN: llvm-bolt %t --relocs -o %t.null 2>&1 | FileCheck %s
+RUN: llvm-bolt %t --relocs -o %t.null -print-cfg 2>&1 | FileCheck %s
+
+CHECK: BOLT-INFO: fixed PIC indirect branch detected in main {{.*}} the destination value is 0x[[#TGT:]]
+CHECK: Binary Function "main" after building cfg
+
+CHECK: movslq ".rodata/1"+8(%rip), %rax
+CHECK-NEXT: leaq ".rodata/1"(%rip), %rdx
+CHECK-NEXT: addq %rdx, %rax
+CHECK-NEXT: jmp .Ltmp1
-CHECK: BOLT-INFO: fixed indirect branch detected in main
+CHECK: .Ltmp1 (2 instructions, align : 1)
+CHECK-NEXT: Secondary Entry Point: __ENTRY_main@0x[[#TGT]]
|
This was referenced May 9, 2024
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
aaupov
added a commit
that referenced
this pull request
May 24, 2024
Move out common X86MemOperand checks into helper lambdas. To be reused in #91667. Test Plan: NFC
aaupov
added a commit
that referenced
this pull request
May 24, 2024
Simplify mutually exclusive sanity checks in analyzeIndirectBranch, where an UNKNOWN IndirectBranchType is to be returned. Reduces confusion and code duplication when adding a new IndirectBranchType (to be added in #91667). Test Plan: NFC
aaupov
added a commit
that referenced
this pull request
May 24, 2024
Move out common code extracting the address of a MCExpr. To be reused in #91667. Test Plan: NFC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Detect and support fixed PIC indirect jumps of the following form:
with PIC_JUMP_TABLE that looks like following:
The code could be produced by compilers, see
#91648.
Test Plan: updated jump-table-fixed-ref-pic.test