llvm / llvm/llvm-project

[RISCV] Stack corruption with indirect calls when all arguments fit in registers

Open
#164,153 5 comments 0 reactions 0 assignees View on GitHub
backend:RISC-V miscompilation
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

## Summary

LLVM RISC-V backend incorrectly allocates spill slots at `sp+0` when a function contains indirect calls but all call arguments fit in registers. This causes stack corruption as the values are clobbered during function calls.

## Environment

- **LLVM Version**: 18.1.8 (also reproduced on newer version)
- **Target**: `riscv32-unknown-elf`
- **Compile Flags**: `-O2 -target riscv32 -mcpu=generic -mabi=ilp32`

## Root Cause

In `RISCVISelLowering::LowerCall` (line 18568), when all call arguments fit in registers:

```cpp
unsigned NumBytes = ArgCCInfo.getStackSize(); // Returns 0
// ...
Chain = DAG.getCALLSEQ_START(Chain, NumBytes, 0, CLI.DL); // NumBytes = 0!
```

This causes:
1. `CALLSEQ_START` with size 0
2. `PrologEpilogInserter::calculateCallFrameInfo` computes `MaxCallFrameSize = 0`
3. Stack slot allocator places spill slots at offset -4, -8, -12 (relative to frame base)
4. These translate to `sp+0`, `sp+4`, `sp+8` in the final code
5. **Values stored at sp+0 get overwritten during subsequent calls**

## Reproducer

Click to expand: aggressive_test.c

```c
/*
* Aggressive Minimal Reproducer for RISC-V Stack Slot Bug
*
* This version tries harder to trigger sp+0 allocation by:
* 1. Creating more register pressure
* 2. Having more live variables across calls
* 3. Using more complex control flow
*/

#include
#include

typedef int (*func_t)(int, int, int, int);

#ifdef __riscv
static int clobber_func(int a0, int a1, int a2, int a3)
__attribute__((naked));
static int clobber_func(int a0, int a1, int a2, int a3) {
__asm__ volatile(
"sw a0, 0(sp)\n"
"add a0, a0, a1\n"
"add a0, a0, a2\n"
"add a0, a0, a3\n"
"ret\n");
}
#else
static int clobber_func(int a0, int a1, int a2, int a3) {
return a0 + a1 + a2 + a3;
}
#endif

typedef struct {
func_t funcs[2];
} ftable_t;

ftable_t g_table = { .funcs = {clobber_func, clobber_func} };

// Force more aggressive optimization by:
// - Using many local variables
// - Making them volatile to prevent optimization
// - Having complex dependencies
__attribute__((noinline))
int trigger_bug(ftable_t *t, int x, int y, int z, int important) {
// Create lots of register pressure
volatile int r0 = x;
volatile int r1 = y;
volatile int r2 = z;
volatile int r3 = x + y;
volatile int r4 = x * y;
volatile int r5 = y - z;
volatile int r6 = z ^ x;
volatile int r7 = x | y;
volatile int r8 = y & z;
volatile int r9 = x << 1;
volatile int r10 = y + z;
volatile int r11 = z - x;
volatile int r12 = x ^ y;
volatile int r13 = y ^ z;
volatile int r14 = x | z;
volatile int r15 = y | x;
volatile int r16 = z | y;
volatile int r17 = (x + y) ^ z;
volatile int saved = important; // Must survive callee clobbers

int sum = 0;

func_t f0 = t->funcs[0];
func_t f1 = t->funcs[1];

// Six indirect calls, all arguments stay in registers.
sum += f0(r0, r1, r2, r3);
sum += f1(r4, r5, r6, r7);
sum += f0(r8, r9, r10, r11);
sum += f1(r12, r13, r14, r15);
sum += f0(r16, r17, r0, r1);
sum += f1(r2, r3, r4, r5);

// Reload saved from memory (volatile ensures a load).
int restored = saved;

// If the bug is triggered, restored will have been clobbered by the callee.
sum += restored;

return sum;
}

// Dummy main to make it linkable
int main(void) {
int answer = trigger_bug(&g_table, 1, 2, 3, 0xDEADBEEF);
printf("answer=%d\n", answer);
return answer;
}

```

x86 answer: -559038688
RISC-V answer: 52

MISMATCH!

### Compile and Check

```bash
clang -target riscv32 -mabi=ilp32 -O2 -c aggressive_test.c -o aggressive_test.o
llvm-objdump -d aggressive_test.o > output.asm
```

### Buggy Assembly Output

- Full assembly of trigger_bug function

```asm
10000170 :
# Prologue – allocate 0x60-byte stack frame & save callee-saved regs
10000170: 13 01 01 fa addi sp, sp, -0x60
10000174: 23 2e 11 04 sw ra, 0x5c(sp)
10000178: 23 2c 81 04 sw s0, 0x58(sp)
1000017c: 23 2a 91 04 sw s1, 0x54(sp)
10000180: 23 28 21 05 sw s2, 0x50(sp)
10000184: 23 26 31 05 sw s3, 0x4c(sp)

# Set up locals / spill volatile temps to stack
10000188: b3 07 b6 02 mul a5, a2, a1
...
100001f8: 23 28 b1 00 sw a1, 0x10(sp)
100001fc: 23 26 51 00 sw t0, 0xc(sp)
10000200: 23 24 c1 00 sw a2, 0x8(sp)
10000204: 23 22 d1 00 sw a3, 0x4(sp)
10000208: 23 20 e1 00 sw a4, 0x0(sp) # ⚠ saved = important → sp+0 (buggy)

# Load function pointers
1000020c: 03 29 05 00 lw s2, 0(a0) # f0
10000210: 83 24 45 00 lw s1, 4(a0) # f1

# === Call #1 ===
# Reload arguments from stack
10000214: 03 25 81 04 lw a0, 0x48(sp)
10000218: 83 25 41 04 lw a1, 0x44(sp)
1000021c: 03 26 01 04 lw a2, 0x40(sp)
10000220: 83 26 c1 03 lw a3, 0x3c(sp)
10000224: e7 00 09 00 jalr s2 # indirect call (clobbers sp+0 via callee)

# === Call #2 ===
10000228: 13 04 05 00 mv s0, a0
1000022c: 03 25 81 03 lw a0, 0x38(sp)
10000230: 83 25 41 03 lw a1, 0x34(sp)
10000234: 03 26 01 03 lw a2, 0x30(sp)
10000238: 83 26 c1 02 lw a3, 0x2c(sp)
1000023c: e7 80 04 00 jalr s1 # indirect call

# === Call #3 ===
10000244: 03 25 81 02 lw a0, 0x28(sp)
10000248: 83 25 41 02 lw a1, 0x24(sp)
1000024c: 03 26 01 02 lw a2, 0x20(sp)
10000250: 83 26 c1 01 lw a3, 0x1c(sp)
10000254: e7 00 09 00 jalr s2

# === Call #4 ===
1000025c: 03 25 81 01 lw a0, 0x18(sp)
10000260: 83 25 41 01 lw a1, 0x14(sp)
10000264: 03 26 01 01 lw a2, 0x10(sp)
10000268: 83 26 c1 00 lw a3, 0xc(sp)
1000026c: e7 80 04 00 jalr s1

# === Call #5 ===
10000278: 03 25 81 00 lw a0, 0x8(sp)
1000027c: 83 25 41 00 lw a1, 0x4(sp)
10000280: 03 26 81 04 lw a2, 0x48(sp)
10000284: 83 26 41 04 lw a3, 0x44(sp)
10000288: e7 00 09 00 jalr s2

# === Call #6 ===
10000290: 03 25 01 04 lw a0, 0x40(sp)
10000294: 83 25 c1 03 lw a1, 0x3c(sp)
10000298: 03 26 81 03 lw a2, 0x38(sp)
1000029c: 83 26 41 03 lw a3, 0x34(sp)
100002a0: e7 80 04 00 jalr s1

# Epilogue – fold results + reload corrupted sentinel
100002a4: 33 05 a4 00 add a0, s0, a0
100002a8: 83 25 01 00 lw a1, 0(sp) # ⚠ reads clobbered value at sp+0
100002ac: 83 20 c1 05 lw ra, 0x5c(sp)
...
100002c8: 13 01 01 06 addi sp, sp, 0x60
100002cc: 67 80 00 00 ret

```

## Why This Happens

Unlike x86_64 which has a [128-byte red zone](https://en.wikipedia.org/wiki/Red_zone_(computing)), RISC-V ABI does not define a safety region below SP. When `MaxCallFrameSize=0`, the stack slot allocator assumes it's safe to use `sp+0` for spill slots, but:

1. **Indirect calls** (via function pointers) - callee behavior cannot be statically analyzed
2. **Leaf function optimizations** - some callees may not allocate their own stack frame
3. **Non-standard calling conventions** - some implementations may use caller's stack bottom

## Impact

- **Silent data corruption**: Variables are silently overwritten across function calls
- **Non-deterministic behavior**: Depends on what the callee does with its stack
- **Hard to debug**: Only manifests under specific register pressure scenarios

## Proposed Fix (proposed by Claude)

Reserve minimum stack space even when all arguments fit in registers:

```cpp
// In RISCVISelLowering.cpp, around line 18568
unsigned NumBytes = ArgCCInfo.getStackSize();

// WORKAROUND: Reserve minimum stack space for safety
// even when all arguments fit in registers
if (NumBytes == 0 && !IsTailCall) {
NumBytes = 16; // Reserve at least 16 bytes (4 words)
}

if (!IsTailCall)
Chain = DAG.getCALLSEQ_START(Chain, NumBytes, 0, CLI.DL);
```

This ensures:
- `MaxCallFrameSize >= 16`
- Spill slots are allocated at safe offsets (sp+16 and above)
- Minimal performance impact (~0.1% stack usage increase)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.