| Issue |
174251
|
| Summary |
RegScavenger Crash in replaceFrameIndicesBackward Due to Invalid MBBI After Frame Instruction Elimination
|
| Labels |
new issue
|
| Assignees |
|
| Reporter |
Sightem
|
There is a crash in `PrologEpilogInserter::replaceFrameIndicesBackward` when a basic block's last instruction is a call frame pseudo instruction. The `RegScavenger::MBBI` iterator becomes invalid (dangling) after `eliminateCallFramePseudoInstr` erases the instruction, causing a segmentation fault on the next `backward()` call.
## Environment
- **LLVM Version**: 17.0.6 ([CE-Programming/llvm-project](https://github.com/CE-Programming/llvm-project) fork with Z80 experimental target)
- **Target**: Z80/eZ80
- **OS**: Linux x86_64
- **Bug Location**: Target-independent code in `PrologEpilogInserter.cpp`
We have not verified whether this bug is present in LLVM trunk/21. The analysis is based on LLVM 17.0.6.
## Minimal reproducer
```c
void callee(const char *);
__attribute__((noreturn)) void crash_repro(void) {
callee("message");
__builtin_unreachable();
}
```
Compile with:
```bash
clang --target=ez80-none-elf -Oz -S repro.c
```
Crash log is attached at the end of the issue.
## Our Attempt at Root Cause Analysis:
In `replaceFrameIndicesBackward` (in `PrologEpilogInserter.cpp`) , the original code flow was:
```cpp
for (MachineInstr &MI : make_early_inc_range(reverse(*BB))) {
if (TII.isFrameInstr(MI)) {
TFI.eliminateCallFramePseudoInstr(MF, *BB, &MI); // erases MI
continue;
}
// Step backwards to get the liveness state at (immediately after) MI.
if (LocalRS)
LocalRS->backward(MI); // called AFTER frame instruction check
// ... rest of frame index elimination ...
}
```
The issue appears to be that `enterBasicBlockEnd` initializes MBBI to the last instruction in the BB:
```cpp
void RegScavenger::enterBasicBlockEnd(MachineBasicBlock &MBB) {
// ...
if (!MBB.empty()) {
MBBI = std::prev(MBB.end()); // points to last instruction
Tracking = true;
}
}
```
### Apparent Crash Scenario
>From what we can tell , the following needs to happen for the crash to reproduce:
1. `enterBasicBlockEnd` sets `MBBI` to the last instruction in the BB
2. the loop's first iteration processes this last instruction
3. if it's a frame pseudo instruction (e.g., `ADJCALLSTACKDOWN`/`ADJCALLSTACKUP`), it gets erased by `eliminateCallFramePseudoInstr`
4. `LocalRS->backward(MI)` is NOT called for frame instructions (it was after the check)
5. `MBBI` now points to erased memory (Parent=NULL, Prev=NULL)
6. on the next iteration, `backward(MI)` is called, which internally does:
```cpp
void backward(MachineBasicBlock::iterator I) {
while (MBBI != I)
backward(); // dereferences invalid MBBI
}
```
The following conditions are required for the crash:
1. Target uses backward scavenging (`supportsBackwardScavenger() == true`)
2. Target generates call frame pseudos: `hasReservedCallFrame() == false`
3. A call frame pseudo is the last instruction in a BB
Z80 always generates `ADJCALLSTACKDOWN`/`ADJCALLSTACKUP` pseudo instructions around calls.
Mips on the other hand *usually* reserves the call frame (returns `true`) when the call frame fits in 16 bit offset and there are no variable sized objects, so it typically doesnt generate these pseudo instructions.
This bug appears to be latent in mainline LLVM and could affect Mips when when compiling functions with variable-sized objects, or other backward scavenging targets if they do not reserve call frames, though we were unable to reproduce this.
## Fix
Our fix involves two changes:
1. Move `backward(MI)` Before Frame Instruction Check
This makes it so backward is called for all instructions, including frame instructions, before any modifications. This ensures the scavenger's iterator is valid and not pointing to an instruction that might be erased.
2. Step Past Frame Instructions Before Erasing
`backward(MI)` only walks MBBI until `MBBI == MI`, it doesnt step past MI. For frame instructions that will be erased, we must step past MI first so that MBBI does not point to the instruction being erased.
```diff
--- a/llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -1470,14 +1470,17 @@ void PEI::replaceFrameIndicesBackward(MachineBasicBlock *BB,
for (MachineInstr &MI : make_early_inc_range(reverse(*BB))) {
- if (TII.isFrameInstr(MI)) {
- TFI.eliminateCallFramePseudoInstr(MF, *BB, &MI);
- continue;
- }
-
if (LocalRS)
LocalRS->backward(MI);
+ if (TII.isFrameInstr(MI)) {
+ if (LocalRS)
+ LocalRS->backward();
+ TFI.eliminateCallFramePseudoInstr(MF, *BB, &MI);
+ continue;
+ }
+
for (unsigned i = 0; i != MI.getNumOperands(); ++i) {
```
After this fix, the minimal reproducer compiles, as well as real world (e)z80 projects.
## Questions
1. Is this fix correct? we are calling backward before erasing frame instructions. Is this the right approach, or should `eliminateCallFramePseudoInstr` handle scavenger state?
2. Should backward step past I also? the semantics of `backward(I)` is to walk MBBI until it equals I, but not step past I. Should there be a variant that also steps past the target instruction?
3. Currently, if MBBI points to an erased instruction (Parent=NULL), the crash happens deep in the ilist iteration. Should `backward()` check for invalid MBBI state?
4. Is this issue present in trunk/21? We have only been able to test on 17.0.6 as we are somewhat behind upstream.
Crash log can be found here: https://pastebin.com/gbz8Y7kZ
![]()
_______________________________________________
llvm-bugs mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
