https://github.com/aaupov updated https://github.com/llvm/llvm-project/pull/91667
>From dd4d0de42048c063d5e5095a0c2594c7cc578df5 Mon Sep 17 00:00:00 2001 From: Amir Ayupov <aau...@fb.com> Date: Thu, 9 May 2024 19:35:26 -0700 Subject: [PATCH 1/6] Fix RISCVMCPlusBuilder Created using spr 1.3.4 --- bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp index 74f2f0aae91e6..020e62463ee2f 100644 --- a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp +++ b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp @@ -177,13 +177,14 @@ class RISCVMCPlusBuilder : public MCPlusBuilder { MCInst &Instruction, InstructionIterator Begin, InstructionIterator End, const unsigned PtrSize, MCInst *&MemLocInstr, unsigned &BaseRegNum, unsigned &IndexRegNum, int64_t &DispValue, const MCExpr *&DispExpr, - MCInst *&PCRelBaseOut) const override { + MCInst *&PCRelBaseOut, MCInst *&FixedEntryLoadInst) const override { MemLocInstr = nullptr; BaseRegNum = 0; IndexRegNum = 0; DispValue = 0; DispExpr = nullptr; PCRelBaseOut = nullptr; + FixedEntryLoadInst = nullptr; // Check for the following long tail call sequence: // 1: auipc xi, %pcrel_hi(sym) >From 62391bb5aa01f2b77d4315d1e72a9924eec9ecc0 Mon Sep 17 00:00:00 2001 From: Amir Ayupov <aau...@fb.com> Date: Fri, 5 Jul 2024 14:54:51 -0700 Subject: [PATCH 2/6] Drop deregisterJumpTable Created using spr 1.3.4 --- bolt/lib/Core/BinaryFunction.cpp | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index 09a6ca1d68730..f587d5a2cadd4 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -899,17 +899,9 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size, 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); + JumpTable *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); >From 5336879ab68aedb1217e2c6c139d171f31e89e03 Mon Sep 17 00:00:00 2001 From: Amir Ayupov <aau...@fb.com> Date: Sat, 6 Jul 2024 22:26:14 -0700 Subject: [PATCH 3/6] Surgically drop spurious jump table Created using spr 1.3.4 --- bolt/include/bolt/Core/BinaryContext.h | 5 +++++ bolt/lib/Core/BinaryFunction.cpp | 12 ++++++++++-- bolt/test/X86/jump-table-fixed-ref-pic.test | 11 ++++------- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h index 73932c4ca2fb3..c5e2c6cd02179 100644 --- a/bolt/include/bolt/Core/BinaryContext.h +++ b/bolt/include/bolt/Core/BinaryContext.h @@ -431,6 +431,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/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index f587d5a2cadd4..2ecca32a5985c 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -899,9 +899,17 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size, 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. - JumpTable *JT = BC.getJumpTableContainingAddress(ArrayStart); + 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); @@ -1158,10 +1166,10 @@ void BinaryFunction::handleIndirectBranch(MCInst &Instruction, uint64_t Size, } case IndirectBranchType::POSSIBLE_JUMP_TABLE: case IndirectBranchType::POSSIBLE_PIC_JUMP_TABLE: + case IndirectBranchType::POSSIBLE_PIC_FIXED_BRANCH: 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); diff --git a/bolt/test/X86/jump-table-fixed-ref-pic.test b/bolt/test/X86/jump-table-fixed-ref-pic.test index d43c9583f0d07..d215c565b31e5 100644 --- a/bolt/test/X86/jump-table-fixed-ref-pic.test +++ b/bolt/test/X86/jump-table-fixed-ref-pic.test @@ -7,10 +7,7 @@ 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: .Ltmp1 (2 instructions, align : 1) -CHECK-NEXT: Secondary Entry Point: __ENTRY_main@0x[[#TGT]] +CHECK: movslq ".rodata/1"+8(%rip), %rax +CHECK-NEXT: leaq ".rodata/1"(%rip), %rdx +CHECK-NEXT: addq %rdx, %rax +CHECK-NEXT: jmpq *%rax # UNKNOWN CONTROL FLOW >From e80b4ddfdb45f40a6d286b959f89f8fff818b5d7 Mon Sep 17 00:00:00 2001 From: Amir Ayupov <aau...@fb.com> Date: Sun, 14 Jul 2024 14:50:22 -0700 Subject: [PATCH 4/6] Address comments Created using spr 1.3.4 --- bolt/include/bolt/Core/BinaryContext.h | 6 +- bolt/lib/Core/BinaryContext.cpp | 10 +++ bolt/lib/Core/BinaryFunction.cpp | 15 ++-- bolt/lib/Target/X86/X86MCPlusBuilder.cpp | 98 +++++++++++------------- 4 files changed, 64 insertions(+), 65 deletions(-) diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h index c5e2c6cd02179..befdc9974c683 100644 --- a/bolt/include/bolt/Core/BinaryContext.h +++ b/bolt/include/bolt/Core/BinaryContext.h @@ -431,10 +431,8 @@ class BinaryContext { return nullptr; } - /// Deregister JumpTable registered at a given \p Address. - bool deregisterJumpTable(uint64_t Address) { - return JumpTables.erase(Address); - } + /// Deregister JumpTable registered at a given \p Address and delete it. + void deleteJumpTable(uint64_t Address); unsigned getDWARFEncodingSize(unsigned Encoding) { if (Encoding == dwarf::DW_EH_PE_omit) diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp index 0a1f1bb9e0d20..23b8eb266612e 100644 --- a/bolt/lib/Core/BinaryContext.cpp +++ b/bolt/lib/Core/BinaryContext.cpp @@ -2523,6 +2523,16 @@ BinaryFunction *BinaryContext::getBinaryFunctionAtAddress(uint64_t Address) { return nullptr; } +/// Deregister JumpTable registered at a given \p Address and delete it. +void BinaryContext::deleteJumpTable(uint64_t Address) { + JumpTable *JT = getJumpTableContainingAddress(Address); + assert(JT && "Must have a jump table at address"); + for (BinaryFunction *Parent : JT->Parents) + Parent->JumpTables.erase(Address); + JumpTables.erase(Address); + delete JT; +} + DebugAddressRangesVector BinaryContext::translateModuleAddressRanges( const DWARFAddressRangesVector &InputRanges) const { DebugAddressRangesVector OutputRanges; diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index 2ecca32a5985c..c0d570d6d909c 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -883,8 +883,11 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size, if (FixedEntryLoadInstr) { assert(BranchType == IndirectBranchType::POSSIBLE_PIC_FIXED_BRANCH && "Invalid IndirectBranch type"); - const MCExpr *FixedEntryDispExpr = - BC.MIB->getMemOperandDisp(*FixedEntryLoadInstr)->getExpr(); + MCInst::iterator FixedEntryDispOperand = + BC.MIB->getMemOperandDisp(*FixedEntryLoadInstr); + assert(FixedEntryDispOperand != FixedEntryLoadInstr->end() && + "Invalid memory instruction"); + const MCExpr *FixedEntryDispExpr = FixedEntryDispOperand->getExpr(); const uint64_t EntryAddress = getExprValue(FixedEntryDispExpr); uint64_t EntrySize = BC.getJumpTableEntrySize(JumpTable::JTT_PIC); ErrorOr<int64_t> Value = BC.getSignedValueAtAddress(EntryAddress, EntrySize); @@ -901,15 +904,11 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size, // 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; + BC.deleteJumpTable(EntryAddress); // Replace FixedEntryDispExpr used in target address calculation with outer // jump table reference. - JT = BC.getJumpTableContainingAddress(ArrayStart); + JumpTable *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); diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp index 527ef5932c550..604ed7f3b00b7 100644 --- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp +++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp @@ -1923,14 +1923,14 @@ class X86MCPlusBuilder : public MCPlusBuilder { MO.SegRegNum == X86::NoRegister; }; LLVM_DEBUG(dbgs() << "Checking for PIC jump table\n"); - MCInst *MemLocInstr = nullptr; - MCInst *MovInstr = nullptr; - bool IsFixedBranch = false; - // Allow renaming R1/R2 once. - bool SwappedRegs = false; + MCInst *FirstInstr = nullptr; + MCInst *SecondInstr = nullptr; + enum { + NOMATCH = 0, + MATCH_JUMP_TABLE, + MATCH_FIXED_BRANCH, + } MatchingState = NOMATCH; while (++II != IE) { - if (MovInstr && MemLocInstr) - break; MCInst &Instr = *II; const MCInstrDesc &InstrDesc = Info->get(Instr.getOpcode()); if (!InstrDesc.hasDefOfPhysReg(Instr, R1, *RegInfo) && @@ -1938,75 +1938,67 @@ class X86MCPlusBuilder : public MCPlusBuilder { // Ignore instructions that don't affect R1, R2 registers. continue; } - 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 (!SwappedRegs && MovDestReg != R2) { + const bool IsMOVSXInstr = isMOVSX64rm32(Instr); + const bool IsLEAInstr = isLEA64r(Instr); + if (MatchingState == NOMATCH) { + if (IsMOVSXInstr) + MatchingState = MATCH_JUMP_TABLE; + else if (IsLEAInstr) + MatchingState = MATCH_FIXED_BRANCH; + else + break; + + // Check if the first instruction is setting %r1 or %r2. In canonical + // form lea sets %r1 and mov sets %r2. If it's the opposite - rename so + // we have to only check a single form. + unsigned DestReg = Instr.getOperand(0).getReg(); + MCPhysReg &ExpectReg = MatchingState == MATCH_JUMP_TABLE ? R2 : R1; + if (DestReg != ExpectReg) std::swap(R1, R2); - SwappedRegs = true; - } - if (MovDestReg != R2) { - LLVM_DEBUG(dbgs() << "MOV instruction expected to set %r2\n"); + if (DestReg != ExpectReg) break; - } - // Verify operands for MOV. + // Verify operands std::optional<X86MemOperand> MO = evaluateX86MemoryOperand(Instr); if (!MO) break; - if (isRIPRel(*MO)) - IsFixedBranch = true; - else if (isIndexed(*MO, R1)) - ; // POSSIBLE_PIC_JUMP_TABLE + if ((MatchingState == MATCH_JUMP_TABLE && isIndexed(*MO, R1)) || + (MatchingState == MATCH_FIXED_BRANCH && isRIPRel(*MO))) + FirstInstr = &Instr; else break; - MovInstr = &Instr; - 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"); + } else { + unsigned ExpectReg = MatchingState == MATCH_JUMP_TABLE ? R1 : R2; + if (!InstrDesc.hasDefOfPhysReg(Instr, ExpectReg, *RegInfo)) + continue; + if ((MatchingState == MATCH_JUMP_TABLE && !IsLEAInstr) || + (MatchingState == MATCH_FIXED_BRANCH && !IsMOVSXInstr)) + break; + if (Instr.getOperand(0).getReg() != ExpectReg) break; - } - // Verify operands for LEA. + // Verify operands. std::optional<X86MemOperand> MO = evaluateX86MemoryOperand(Instr); if (!MO) break; if (!isRIPRel(*MO)) break; - MemLocInstr = &Instr; - continue; + SecondInstr = &Instr; + break; } } - if (!MemLocInstr) + if (!SecondInstr) return std::make_tuple(IndirectBranchType::UNKNOWN, nullptr, nullptr); - LLVM_DEBUG(dbgs() << "checking potential fixed indirect branch\n"); - if (IsFixedBranch) + if (MatchingState == MATCH_FIXED_BRANCH) { + LLVM_DEBUG(dbgs() << "checking potential fixed indirect branch\n"); return std::make_tuple(IndirectBranchType::POSSIBLE_PIC_FIXED_BRANCH, - MemLocInstr, MovInstr); - + FirstInstr, SecondInstr); + } LLVM_DEBUG(dbgs() << "checking potential PIC jump table\n"); return std::make_tuple(IndirectBranchType::POSSIBLE_PIC_JUMP_TABLE, - MemLocInstr, nullptr); + SecondInstr, nullptr); } IndirectBranchType >From 677cb3248aaae5c441cceff074e3569f84556cdb Mon Sep 17 00:00:00 2001 From: Amir Ayupov <aau...@fb.com> Date: Thu, 18 Jul 2024 18:02:00 -0700 Subject: [PATCH 5/6] Address comments Created using spr 1.3.4 --- bolt/lib/Core/BinaryContext.cpp | 4 ++-- bolt/lib/Target/X86/X86MCPlusBuilder.cpp | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp index 23b8eb266612e..035f68e39751b 100644 --- a/bolt/lib/Core/BinaryContext.cpp +++ b/bolt/lib/Core/BinaryContext.cpp @@ -2525,8 +2525,8 @@ BinaryFunction *BinaryContext::getBinaryFunctionAtAddress(uint64_t Address) { /// Deregister JumpTable registered at a given \p Address and delete it. void BinaryContext::deleteJumpTable(uint64_t Address) { - JumpTable *JT = getJumpTableContainingAddress(Address); - assert(JT && "Must have a jump table at address"); + assert(JumpTables.count(Address) && "Must have a jump table at address"); + JumpTable *JT = JumpTables.at(Address); for (BinaryFunction *Parent : JT->Parents) Parent->JumpTables.erase(Address); JumpTables.erase(Address); diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp index 604ed7f3b00b7..9f6c552977b34 100644 --- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp +++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp @@ -1866,6 +1866,9 @@ class X86MCPlusBuilder : public MCPlusBuilder { return true; } + /// Analyzes PIC-style jump table code template and return identified + /// IndirectBranchType, MemLocInstr (all cases) and FixedEntryLoadInstr + /// (POSSIBLE_PIC_FIXED_BRANCH case). template <typename Itr> std::tuple<IndirectBranchType, MCInst *, MCInst *> analyzePICJumpTable(Itr II, Itr IE, MCPhysReg R1, MCPhysReg R2) const { @@ -1878,7 +1881,7 @@ class X86MCPlusBuilder : public MCPlusBuilder { // // or a fixed indirect jump template: // - // movslq En(%rip), {%r2|%r1} + // movslq En(%rip), {%r2|%r1} <- FixedEntryLoadInstr // lea PIC_JUMP_TABLE(%rip), {%r1|%r2} <- MemLocInstr // add %r2, %r1 // jmp *%r1 >From 24fc1a0caca7aa4fe9c6451300ddd1b6c64d126f Mon Sep 17 00:00:00 2001 From: Amir Ayupov <aau...@fb.com> Date: Thu, 18 Jul 2024 18:29:02 -0700 Subject: [PATCH 6/6] clang-format Created using spr 1.3.4 --- bolt/include/bolt/Core/MCPlusBuilder.h | 12 +++++------- bolt/lib/Core/BinaryFunction.cpp | 3 ++- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index 577db83a11fa4..e543513b0f3d3 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -1476,13 +1476,11 @@ class MCPlusBuilder { /// will be set to the different components of the branch. \p MemLocInstr /// is the instruction that loads up the indirect function pointer. It may /// or may not be same as \p Instruction. - virtual IndirectBranchType - analyzeIndirectBranch(MCInst &Instruction, InstructionIterator Begin, - InstructionIterator End, const unsigned PtrSize, - MCInst *&MemLocInstr, unsigned &BaseRegNum, - unsigned &IndexRegNum, int64_t &DispValue, - const MCExpr *&DispExpr, MCInst *&PCRelBaseOut, - MCInst *&FixedEntryLoadInst) const { + virtual IndirectBranchType analyzeIndirectBranch( + MCInst &Instruction, InstructionIterator Begin, InstructionIterator End, + const unsigned PtrSize, MCInst *&MemLocInstr, unsigned &BaseRegNum, + unsigned &IndexRegNum, int64_t &DispValue, 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 c0d570d6d909c..0db1e0d994448 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -890,7 +890,8 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size, const MCExpr *FixedEntryDispExpr = FixedEntryDispOperand->getExpr(); const uint64_t EntryAddress = getExprValue(FixedEntryDispExpr); uint64_t EntrySize = BC.getJumpTableEntrySize(JumpTable::JTT_PIC); - ErrorOr<int64_t> Value = BC.getSignedValueAtAddress(EntryAddress, EntrySize); + ErrorOr<int64_t> Value = + BC.getSignedValueAtAddress(EntryAddress, EntrySize); if (!Value) return IndirectBranchType::UNKNOWN; _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits