[MachineBasicBlock] Fix use after free in SplitCriticalEdge #66188

perlfu · 2023-09-13T09:17:23Z

Remove use after free when attempting to update SlotIndexes in MachineBasicBlock::SplitCriticalEdge.
Add support to targets for updating SlotIndexes when inserting or removing branches.

Remove use after free when attempting to update SlotIndexes in MachineBasicBlock::SplitCriticalEdge. Add support to targets for updating SlotIndexes when inserting or removing branches.

llvmbot · 2023-09-13T09:18:31Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-backend-msp430

Changes

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 &amp;amp;DL,

                           int *BytesAdded = nullptr) const {

                           const DebugLoc &amp;amp;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-&amp;gt;removeBranch(*this);
```

```
   TII-&amp;gt;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-&amp;gt;insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL);

   TII-&amp;gt;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-&amp;gt;removeBranch(*this);
```

 TII-&amp;gt;insertBranch(*this, FBB, nullptr, Cond, DL);

 TII-&amp;gt;removeBranch(*this, nullptr, Indexes);

 TII-&amp;gt;insertBranch(*this, FBB, nullptr, Cond, DL, nullptr, Indexes);

} else if (isLayoutSuccessor(FBB)) {

```
 TII-&amp;gt;removeBranch(*this);
```

 TII-&amp;gt;insertBranch(*this, TBB, nullptr, Cond, DL);

 TII-&amp;gt;removeBranch(*this, nullptr, Indexes);

```
 TII-&amp;gt;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-&amp;gt;insertBranch(*this, TBB, nullptr, Cond, DL);

```
 TII-&amp;gt;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-&amp;gt;insertBranch(*this, PreviousLayoutSuccessor, nullptr, Cond, DL);

 TII-&amp;gt;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(&amp;amp;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(&amp;amp;MI);

for (MachineInstr *Terminator : Terminators) {

 if (!is_contained(NewTerminators, Terminator))

   Indexes-&amp;gt;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 &amp;amp;MI : NMBB-&amp;gt;instrs()) {

   // Some instructions may have been moved to NMBB by updateTerminator(),

   // so we first remove any instruction that already has an index.

```
   if (Indexes-&amp;gt;hasIndex(MI))
```

     Indexes-&amp;gt;removeMachineInstrFromMaps(MI);

   Indexes-&amp;gt;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 &amp;amp;DL,

                                        MachineBasicBlock *TBB,

                                        ArrayRef&amp;lt;MachineOperand&amp;gt; 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-&amp;gt;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-&amp;gt;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&amp;lt;MachineOperand&amp;gt; Cond,

                                   const DebugLoc &amp;amp;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(&amp;amp;MBB, DL, get(AArch64::B)).addMBB(TBB);

else

 instantiateCondBranch(MBB, DL, TBB, Cond);

if (Cond.empty()) { // Unconditional branch?

 MachineInstr *MI = BuildMI(&amp;amp;MBB, DL, get(AArch64::B)).addMBB(TBB);

```
 if (Indexes)
```

   Indexes-&amp;gt;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 &amp;amp;DL,

                   int *BytesAdded = nullptr) const override;

                   const DebugLoc &amp;amp;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&amp;lt;MachineOperand&amp;gt; Cond) const;

                        ArrayRef&amp;lt;MachineOperand&amp;gt; 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,
-...

perlfu · 2023-09-13T09:28:46Z

This touches all backends, but the code changes are very fairly straightforward and similar between most.
There is some addition churn from clang-format on code that has not been touched in a long time.

llvm/include/llvm/CodeGen/MachineBasicBlock.h

jayfoad · 2023-09-13T09:54:08Z

llvm/lib/CodeGen/MachineBasicBlock.cpp

@@ -693,15 +693,15 @@ 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);


This change is NFC, right?

This is NFC, but I felt it was appropriate to add an explicit configuration of AllowModify=false here as SlotIndexes will be invalidated if analyzeBranch modifies the instructions.

llvm/include/llvm/CodeGen/TargetInstrInfo.h

jayfoad · 2023-09-13T09:57:16Z

LGTM overall. I would suggest that approval from a small handful of target maintainers is enough to merge this.

qcolombet · 2023-09-13T09:58:39Z

Could we fix SplitCriticalEdges instead of pushing the burden of the SlotIndex update to all backends?

llvm/lib/CodeGen/MachineBasicBlock.cpp

llvm/include/llvm/CodeGen/TargetInstrInfo.h

perlfu · 2023-09-13T13:49:19Z

Could we fix SplitCriticalEdges instead of pushing the burden of the SlotIndex update to all backends?

I am open to alternatives.
From my perspective it is this, or add some kind of listener pattern to MachineBasicBlock so SplitCriticalEdges can intercept instructions added or removed by target specific code.

- Reverse order of Indexes and BytesAdded/BytesRemoved - Describe new parameters in comments

qcolombet · 2023-09-18T15:09:39Z

Could we fix SplitCriticalEdges instead of pushing the burden of the SlotIndex update to all backends?

I am open to alternatives. From my perspective it is this, or add some kind of listener pattern to MachineBasicBlock so SplitCriticalEdges can intercept instructions added or removed by target specific code.

The listener alternative sounds better to me.
This is how we handle this kind of things in MIR in general (look for Delegate in MachineFunction, MachineRegisterInfo, etc.)

perlfu · 2023-10-11T10:26:01Z

Close this in favour of observer pattern in #68786.
(Created a fresh PR to avoid noise for all review groups added by files touched in this PR.)

[MachineBasicBlock] Fix use after free in SplitCriticalEdge

c7b575f

Remove use after free when attempting to update SlotIndexes in MachineBasicBlock::SplitCriticalEdge. Add support to targets for updating SlotIndexes when inserting or removing branches.

perlfu requested review from a team as code owners September 13, 2023 09:17

llvmbot added backend:ARM backend:AArch64 backend:AMDGPU backend:MSP430 backend:RISC-V backend:Sparc backend:WebAssembly backend:X86 backend:m68k backend:loongarch labels Sep 13, 2023

bzEq added the backend:PowerPC label Sep 13, 2023

jayfoad mentioned this pull request Sep 13, 2023

Improve the pull-request subcription notification format by adding the description and files statistics #65828

Merged

jayfoad reviewed Sep 13, 2023

View reviewed changes

llvm/include/llvm/CodeGen/MachineBasicBlock.h Show resolved Hide resolved

jayfoad reviewed Sep 13, 2023

View reviewed changes

llvm/include/llvm/CodeGen/TargetInstrInfo.h Outdated Show resolved Hide resolved

qcolombet reviewed Sep 13, 2023

View reviewed changes

llvm/lib/CodeGen/MachineBasicBlock.cpp Outdated Show resolved Hide resolved

llvm/lib/CodeGen/MachineBasicBlock.cpp Outdated Show resolved Hide resolved

llvm/include/llvm/CodeGen/TargetInstrInfo.h Outdated Show resolved Hide resolved

Address reviewer feedback.

bec43c3

- Reverse order of Indexes and BytesAdded/BytesRemoved - Describe new parameters in comments

perlfu closed this Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MachineBasicBlock] Fix use after free in SplitCriticalEdge #66188

[MachineBasicBlock] Fix use after free in SplitCriticalEdge #66188

perlfu commented Sep 13, 2023

llvmbot commented Sep 13, 2023 •

edited

perlfu commented Sep 13, 2023

jayfoad Sep 13, 2023

perlfu Sep 14, 2023

jayfoad commented Sep 13, 2023

qcolombet commented Sep 13, 2023

perlfu commented Sep 13, 2023

qcolombet commented Sep 18, 2023

perlfu commented Oct 11, 2023

[MachineBasicBlock] Fix use after free in SplitCriticalEdge #66188

[MachineBasicBlock] Fix use after free in SplitCriticalEdge #66188

Conversation

perlfu commented Sep 13, 2023

llvmbot commented Sep 13, 2023 • edited

perlfu commented Sep 13, 2023

jayfoad Sep 13, 2023

Choose a reason for hiding this comment

perlfu Sep 14, 2023

Choose a reason for hiding this comment

jayfoad commented Sep 13, 2023

qcolombet commented Sep 13, 2023

perlfu commented Sep 13, 2023

qcolombet commented Sep 18, 2023

perlfu commented Oct 11, 2023

llvmbot commented Sep 13, 2023 •

edited