[llvm] [MachineBasicBlock] Fix use after free in SplitCriticalEdge (PR #66188)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 13 02:18:36 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-backend-msp430
<details>
<summary>Changes</summary>
Remove use after free when attempting to update SlotIndexes in MachineBasicBlock::SplitCriticalEdge.
Add support to targets for updating SlotIndexes when inserting or removing branches.
--
Patch is 127.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66188.diff
51 Files Affected:
- (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+2-1)
- (modified) llvm/include/llvm/CodeGen/TargetInstrInfo.h (+8-5)
- (modified) llvm/lib/CodeGen/MachineBasicBlock.cpp (+21-48)
- (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.cpp (+37-15)
- (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.h (+6-5)
- (modified) llvm/lib/Target/AMDGPU/R600InstrInfo.cpp (+29-12)
- (modified) llvm/lib/Target/AMDGPU/R600InstrInfo.h (+4-4)
- (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+26-16)
- (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/ARC/ARCInstrInfo.cpp (+17-5)
- (modified) llvm/lib/Target/ARC/ARCInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp (+33-17)
- (modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/AVR/AVRInstrInfo.cpp (+13-3)
- (modified) llvm/lib/Target/AVR/AVRInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/BPF/BPFInstrInfo.cpp (+10-5)
- (modified) llvm/lib/Target/BPF/BPFInstrInfo.h (+5-4)
- (modified) llvm/lib/Target/CSKY/CSKYInstrInfo.cpp (+19-5)
- (modified) llvm/lib/Target/CSKY/CSKYInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp (+41-16)
- (modified) llvm/lib/Target/Hexagon/HexagonInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/Lanai/LanaiInstrInfo.cpp (+18-7)
- (modified) llvm/lib/Target/Lanai/LanaiInstrInfo.h (+5-5)
- (modified) llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp (+19-4)
- (modified) llvm/lib/Target/LoongArch/LoongArchInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/M68k/M68kInstrInfo.cpp (+20-8)
- (modified) llvm/lib/Target/M68k/M68kInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/MSP430/MSP430InstrInfo.cpp (+17-6)
- (modified) llvm/lib/Target/MSP430/MSP430InstrInfo.h (+4-4)
- (modified) llvm/lib/Target/Mips/MipsInstrInfo.cpp (+21-11)
- (modified) llvm/lib/Target/Mips/MipsInstrInfo.h (+6-5)
- (modified) llvm/lib/Target/NVPTX/NVPTXInstrInfo.cpp (+21-8)
- (modified) llvm/lib/Target/NVPTX/NVPTXInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/PowerPC/PPCInstrInfo.cpp (+38-24)
- (modified) llvm/lib/Target/PowerPC/PPCInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+18-5)
- (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp (+8-5)
- (modified) llvm/lib/Target/SPIRV/SPIRVInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/Sparc/SparcInstrInfo.cpp (+19-8)
- (modified) llvm/lib/Target/Sparc/SparcInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp (+18-7)
- (modified) llvm/lib/Target/SystemZ/SystemZInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/VE/VEInstrInfo.cpp (+27-17)
- (modified) llvm/lib/Target/VE/VEInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp (+27-10)
- (modified) llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.h (+4-4)
- (modified) llvm/lib/Target/X86/X86InstrInfo.cpp (+25-19)
- (modified) llvm/lib/Target/X86/X86InstrInfo.h (+4-4)
- (modified) llvm/lib/Target/XCore/XCoreInstrInfo.cpp (+21-10)
- (modified) llvm/lib/Target/XCore/XCoreInstrInfo.h (+4-4)
<pre>
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index ed9fc8f7ec3d75e..74e7b98ca8a7526 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -693,7 +693,8 @@ class MachineBasicBlock
/// layout was modified. If the block previously fell through to that block,
/// it may now need a branch. If it previously branched to another block, it
/// may now be able to fallthrough to the current layout successor.
- void updateTerminator(MachineBasicBlock *PreviousLayoutSuccessor);
+ void updateTerminator(MachineBasicBlock *PreviousLayoutSuccessor,
+ SlotIndexes *Indexes = nullptr);
// Machine-CFG mutators
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 1c2ca8678346472..8d40b8de8535f98 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -53,6 +53,7 @@ class ScheduleDAGMI;
class ScheduleHazardRecognizer;
class SDNode;
class SelectionDAG;
+class SlotIndexes;
class SMSchedule;
class SwingSchedulerDAG;
class RegScavenger;
@@ -682,7 +683,8 @@ class TargetInstrInfo : public MCInstrInfo {
/// If \p BytesRemoved is non-null, report the change in code size from the
/// removed instructions.
virtual unsigned removeBranch(MachineBasicBlock &MBB,
- int *BytesRemoved = nullptr) const {
+ int *BytesRemoved = nullptr,
+ SlotIndexes *Indexes = nullptr) const {
llvm_unreachable("Target didn't implement TargetInstrInfo::removeBranch!");
}
@@ -702,17 +704,18 @@ class TargetInstrInfo : public MCInstrInfo {
virtual unsigned insertBranch(MachineBasicBlock &MBB, MachineBasicBlock *TBB,
MachineBasicBlock *FBB,
ArrayRef<MachineOperand> Cond,
- const DebugLoc &DL,
- int *BytesAdded = nullptr) const {
+ const DebugLoc &DL, int *BytesAdded = nullptr,
+ SlotIndexes *Indexes = nullptr) const {
llvm_unreachable("Target didn't implement TargetInstrInfo::insertBranch!");
}
unsigned insertUnconditionalBranch(MachineBasicBlock &MBB,
MachineBasicBlock *DestBB,
const DebugLoc &DL,
- int *BytesAdded = nullptr) const {
+ int *BytesAdded = nullptr,
+ SlotIndexes *Indexes = nullptr) const {
return insertBranch(MBB, DestBB, nullptr, ArrayRef<MachineOperand>(), DL,
- BytesAdded);
+ BytesAdded, Indexes);
}
/// Object returned by analyzeLoopForPipelining. Allows software pipelining
diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp
index 280ced65db7d8c0..09df32afb37c00c 100644
--- a/llvm/lib/CodeGen/MachineBasicBlock.cpp
+++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp
@@ -681,7 +681,7 @@ static int findJumpTableIndex(const MachineBasicBlock &MBB) {
}
void MachineBasicBlock::updateTerminator(
- MachineBasicBlock *PreviousLayoutSuccessor) {
+ MachineBasicBlock *PreviousLayoutSuccessor, SlotIndexes *Indexes) {
LLVM_DEBUG(dbgs() << "Updating terminators on " << printMBBReference(*this)
<< "\n");
@@ -693,7 +693,7 @@ void MachineBasicBlock::updateTerminator(
MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
SmallVector<MachineOperand, 4> Cond;
DebugLoc DL = findBranchDebugLoc();
- bool B = TII->analyzeBranch(*this, TBB, FBB, Cond);
+ bool B = TII->analyzeBranch(*this, TBB, FBB, Cond, /*AllowModify=*/false);
(void) B;
assert(!B && "UpdateTerminators requires analyzable predecessors!");
if (Cond.empty()) {
@@ -701,7 +701,7 @@ void MachineBasicBlock::updateTerminator(
// The block has an unconditional branch. If its successor is now its
// layout successor, delete the branch.
if (isLayoutSuccessor(TBB))
- TII->removeBranch(*this);
+ TII->removeBranch(*this, nullptr, Indexes);
} else {
// The block has an unconditional fallthrough, or the end of the block is
// unreachable.
@@ -717,7 +717,8 @@ void MachineBasicBlock::updateTerminator(
// If the unconditional successor block is not the current layout
// successor, insert a branch to jump to it.
if (!isLayoutSuccessor(PreviousLayoutSuccessor))
- TII->insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL);
+ TII->insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL,
+ nullptr, Indexes);
}
return;
}
@@ -729,11 +730,11 @@ void MachineBasicBlock::updateTerminator(
if (isLayoutSuccessor(TBB)) {
if (TII->reverseBranchCondition(Cond))
return;
- TII->removeBranch(*this);
- TII->insertBranch(*this, FBB, nullptr, Cond, DL);
+ TII->removeBranch(*this, nullptr, Indexes);
+ TII->insertBranch(*this, FBB, nullptr, Cond, DL, nullptr, Indexes);
} else if (isLayoutSuccessor(FBB)) {
- TII->removeBranch(*this);
- TII->insertBranch(*this, TBB, nullptr, Cond, DL);
+ TII->removeBranch(*this, nullptr, Indexes);
+ TII->insertBranch(*this, TBB, nullptr, Cond, DL, nullptr, Indexes);
}
return;
}
@@ -747,10 +748,10 @@ void MachineBasicBlock::updateTerminator(
// We had a fallthrough to the same basic block as the conditional jump
// targets. Remove the conditional jump, leaving an unconditional
// fallthrough or an unconditional jump.
- TII->removeBranch(*this);
+ TII->removeBranch(*this, nullptr, Indexes);
if (!isLayoutSuccessor(TBB)) {
Cond.clear();
- TII->insertBranch(*this, TBB, nullptr, Cond, DL);
+ TII->insertBranch(*this, TBB, nullptr, Cond, DL, nullptr, Indexes);
}
return;
}
@@ -760,14 +761,17 @@ void MachineBasicBlock::updateTerminator(
if (TII->reverseBranchCondition(Cond)) {
// We can't reverse the condition, add an unconditional branch.
Cond.clear();
- TII->insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL);
+ TII->insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL,
+ nullptr, Indexes);
return;
}
- TII->removeBranch(*this);
- TII->insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL);
+ TII->removeBranch(*this, nullptr, Indexes);
+ TII->insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL,
+ nullptr, Indexes);
} else if (!isLayoutSuccessor(PreviousLayoutSuccessor)) {
- TII->removeBranch(*this);
- TII->insertBranch(*this, TBB, PreviousLayoutSuccessor, Cond, DL);
+ TII->removeBranch(*this, nullptr, Indexes);
+ TII->insertBranch(*this, TBB, PreviousLayoutSuccessor, Cond, DL, nullptr,
+ Indexes);
}
}
@@ -1166,51 +1170,20 @@ MachineBasicBlock *MachineBasicBlock::SplitCriticalEdge(
ReplaceUsesOfBlockWith(Succ, NMBB);
- // If updateTerminator() removes instructions, we need to remove them from
- // SlotIndexes.
- SmallVector<MachineInstr*, 4> Terminators;
- if (Indexes) {
- for (MachineInstr &MI :
- llvm::make_range(getFirstInstrTerminator(), instr_end()))
- Terminators.push_back(&MI);
- }
-
// Since we replaced all uses of Succ with NMBB, that should also be treated
// as the fallthrough successor
if (Succ == PrevFallthrough)
PrevFallthrough = NMBB;
if (!ChangedIndirectJump)
- updateTerminator(PrevFallthrough);
-
- if (Indexes) {
- SmallVector<MachineInstr*, 4> NewTerminators;
- for (MachineInstr &MI :
- llvm::make_range(getFirstInstrTerminator(), instr_end()))
- NewTerminators.push_back(&MI);
-
- for (MachineInstr *Terminator : Terminators) {
- if (!is_contained(NewTerminators, Terminator))
- Indexes->removeMachineInstrFromMaps(*Terminator);
- }
- }
+ updateTerminator(PrevFallthrough, Indexes);
// Insert unconditional "jump Succ" instruction in NMBB if necessary.
NMBB->addSuccessor(Succ);
if (!NMBB->isLayoutSuccessor(Succ)) {
SmallVector<MachineOperand, 4> Cond;
const TargetInstrInfo *TII = getParent()->getSubtarget().getInstrInfo();
- TII->insertBranch(*NMBB, Succ, nullptr, Cond, DL);
-
- if (Indexes) {
- for (MachineInstr &MI : NMBB->instrs()) {
- // Some instructions may have been moved to NMBB by updateTerminator(),
- // so we first remove any instruction that already has an index.
- if (Indexes->hasIndex(MI))
- Indexes->removeMachineInstrFromMaps(MI);
- Indexes->insertMachineInstrInMaps(MI);
- }
- }
+ TII->insertBranch(*NMBB, Succ, nullptr, Cond, DL, nullptr, Indexes);
}
// Fix PHI nodes in Succ so they refer to NMBB instead of this.
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index a41ac0e44a7700b..012f56f306f0527 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -10,8 +10,8 @@
//
//===----------------------------------------------------------------------===//
-#include "AArch64ExpandImm.h"
#include "AArch64InstrInfo.h"
+#include "AArch64ExpandImm.h"
#include "AArch64FrameLowering.h"
#include "AArch64MachineFunctionInfo.h"
#include "AArch64Subtarget.h"
@@ -31,6 +31,7 @@
#include "llvm/CodeGen/MachineOperand.h"
#include "llvm/CodeGen/MachineRegisterInfo.h"
#include "llvm/CodeGen/RegisterScavenging.h"
+#include "llvm/CodeGen/SlotIndexes.h"
#include "llvm/CodeGen/StackMaps.h"
#include "llvm/CodeGen/TargetRegisterInfo.h"
#include "llvm/CodeGen/TargetSubtargetInfo.h"
@@ -534,7 +535,8 @@ bool AArch64InstrInfo::reverseBranchCondition(
}
unsigned AArch64InstrInfo::removeBranch(MachineBasicBlock &MBB,
- int *BytesRemoved) const {
+ int *BytesRemoved,
+ SlotIndexes *Indexes) const {
MachineBasicBlock::iterator I = MBB.getLastNonDebugInstr();
if (I == MBB.end())
return 0;
@@ -544,6 +546,8 @@ unsigned AArch64InstrInfo::removeBranch(MachineBasicBlock &MBB,
return 0;
// Remove the branch.
+ if (Indexes)
+ Indexes->removeMachineInstrFromMaps(*I);
I->eraseFromParent();
I = MBB.end();
@@ -561,6 +565,8 @@ unsigned AArch64InstrInfo::removeBranch(MachineBasicBlock &MBB,
}
// Remove the branch.
+ if (Indexes)
+ Indexes->removeMachineInstrFromMaps(*I);
I->eraseFromParent();
if (BytesRemoved)
*BytesRemoved = 8;
@@ -568,12 +574,18 @@ unsigned AArch64InstrInfo::removeBranch(MachineBasicBlock &MBB,
return 2;
}
-void AArch64InstrInfo::instantiateCondBranch(
- MachineBasicBlock &MBB, const DebugLoc &DL, MachineBasicBlock *TBB,
- ArrayRef<MachineOperand> Cond) const {
+void AArch64InstrInfo::instantiateCondBranch(MachineBasicBlock &MBB,
+ const DebugLoc &DL,
+ MachineBasicBlock *TBB,
+ ArrayRef<MachineOperand> Cond,
+ SlotIndexes *Indexes) const {
if (Cond[0].getImm() != -1) {
// Regular Bcc
- BuildMI(&MBB, DL, get(AArch64::Bcc)).addImm(Cond[0].getImm()).addMBB(TBB);
+ MachineInstr *MI = BuildMI(&MBB, DL, get(AArch64::Bcc))
+ .addImm(Cond[0].getImm())
+ .addMBB(TBB);
+ if (Indexes)
+ Indexes->insertMachineInstrInMaps(*MI);
} else {
// Folded compare-and-branch
// Note that we use addOperand instead of addReg to keep the flags.
@@ -582,20 +594,27 @@ void AArch64InstrInfo::instantiateCondBranch(
if (Cond.size() > 3)
MIB.addImm(Cond[3].getImm());
MIB.addMBB(TBB);
+ if (Indexes)
+ Indexes->insertMachineInstrInMaps(*MIB);
}
}
-unsigned AArch64InstrInfo::insertBranch(
- MachineBasicBlock &MBB, MachineBasicBlock *TBB, MachineBasicBlock *FBB,
- ArrayRef<MachineOperand> Cond, const DebugLoc &DL, int *BytesAdded) const {
+unsigned AArch64InstrInfo::insertBranch(MachineBasicBlock &MBB,
+ MachineBasicBlock *TBB,
+ MachineBasicBlock *FBB,
+ ArrayRef<MachineOperand> Cond,
+ const DebugLoc &DL, int *BytesAdded,
+ SlotIndexes *Indexes) const {
// Shouldn't be a fall through.
assert(TBB && "insertBranch must not be told to insert a fallthrough");
if (!FBB) {
- if (Cond.empty()) // Unconditional branch?
- BuildMI(&MBB, DL, get(AArch64::B)).addMBB(TBB);
- else
- instantiateCondBranch(MBB, DL, TBB, Cond);
+ if (Cond.empty()) { // Unconditional branch?
+ MachineInstr *MI = BuildMI(&MBB, DL, get(AArch64::B)).addMBB(TBB);
+ if (Indexes)
+ Indexes->insertMachineInstrInMaps(*MI);
+ } else
+ instantiateCondBranch(MBB, DL, TBB, Cond, Indexes);
if (BytesAdded)
*BytesAdded = 4;
@@ -604,8 +623,11 @@ unsigned AArch64InstrInfo::insertBranch(
}
// Two-way conditional branch.
- instantiateCondBranch(MBB, DL, TBB, Cond);
- BuildMI(&MBB, DL, get(AArch64::B)).addMBB(FBB);
+ instantiateCondBranch(MBB, DL, TBB, Cond, Indexes);
+ MachineInstr *MI = BuildMI(&MBB, DL, get(AArch64::B)).addMBB(FBB);
+
+ if (Indexes)
+ Indexes->insertMachineInstrInMaps(*MI);
if (BytesAdded)
*BytesAdded = 8;
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index 24ff676218cbe90..cd6fcd32ee8ac11 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -225,12 +225,12 @@ class AArch64InstrInfo final : public AArch64GenInstrInfo {
bool analyzeBranchPredicate(MachineBasicBlock &MBB,
MachineBranchPredicate &MBP,
bool AllowModify) const override;
- unsigned removeBranch(MachineBasicBlock &MBB,
- int *BytesRemoved = nullptr) const override;
+ unsigned removeBranch(MachineBasicBlock &MBB, int *BytesRemoved = nullptr,
+ SlotIndexes *Indexes = nullptr) const override;
unsigned insertBranch(MachineBasicBlock &MBB, MachineBasicBlock *TBB,
MachineBasicBlock *FBB, ArrayRef<MachineOperand> Cond,
- const DebugLoc &DL,
- int *BytesAdded = nullptr) const override;
+ const DebugLoc &DL, int *BytesAdded = nullptr,
+ SlotIndexes *Indexes = nullptr) const override;
bool
reverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const override;
bool canInsertSelect(const MachineBasicBlock &, ArrayRef<MachineOperand> Cond,
@@ -378,7 +378,8 @@ class AArch64InstrInfo final : public AArch64GenInstrInfo {
void instantiateCondBranch(MachineBasicBlock &MBB, const DebugLoc &DL,
MachineBasicBlock *TBB,
- ArrayRef<MachineOperand> Cond) const;
+ ArrayRef<MachineOperand> Cond,
+ SlotIndexes *Indexes) const;
bool substituteCmpToZero(MachineInstr &CmpInstr, unsigned SrcReg,
const MachineRegisterInfo &MRI) const;
bool removeCmpToZeroOrOne(MachineInstr &CmpInstr, unsigned SrcReg,
diff --git a/llvm/lib/Target/AMDGPU/R600InstrInfo.cpp b/llvm/lib/Target/AMDGPU/R600InstrInfo.cpp
index 7f874b245b8f4f4..7d495553e945a7a 100644
--- a/llvm/lib/Target/AMDGPU/R600InstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/R600InstrInfo.cpp
@@ -19,6 +19,7 @@
#include "R600Subtarget.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
+#include "llvm/CodeGen/SlotIndexes.h"
using namespace llvm;
@@ -730,14 +731,16 @@ unsigned R600InstrInfo::insertBranch(MachineBasicBlock &MBB,
MachineBasicBlock *TBB,
MachineBasicBlock *FBB,
ArrayRef<MachineOperand> Cond,
-...
<truncated>
</pre>
</details>
https://github.com/llvm/llvm-project/pull/66188
More information about the llvm-commits
mailing list