llvm / llvm/llvm-project

[SPIRV] `llc` crashes in `addOpAccessChainReqs` on an `OpAccessChain` with no index operands

Open
#201,662 1 comment 0 reactions 0 assignees View on GitHub
backend:SPIR-V confirmed crash crash-on-valid
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

The SPIR-V backend's module-analysis pass crashes when it encounters an
`OpAccessChain` that has **no index operands**. `addOpAccessChainReqs()`
(`llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp`) unconditionally reads
`Instr.getOperand(3)` — the *first index* of the access chain — to decide whether
the access uses dynamic array indexing. But an `OpAccessChain` is only required
to carry `{result-id, result-type, base-pointer}`; the index operands are
optional. A byte `getelementptr` on a pointer whose pointee type is **not** `i8`
lowers to exactly such an index-less access chain, so operand 3 does not exist and
reading it runs off the end of the instruction's operand list.

* **Release builds (assertions off):** the out-of-bounds read returns a garbage
`MachineOperand`; the resulting garbage register is fed to
`MachineRegisterInfo::getVRegDef()`, which dereferences it → **SIGSEGV**.
* **Assertions-enabled builds:** the out-of-bounds read trips the bounds assert in
`ArrayRef::operator[]` (`Index < Length && "Invalid index!"`)
inside `getOperand(3)`, aborting before `getVRegDef` is reached.

Both are the same root cause and are fixed by the same one-line guard.

## Affected versions

The bug was reproduced in `52a43992cd92b98f5059851fe43a33ba94527bd4` and in (my CachyOS) 22.1.6 llc distro package. So the bug is live on the `release/22.x` line and on `main` / `23.0.0git`.

## Steps to reproduce

`access-chain-no-index.ll`:

```llvm
; A byte getelementptr on a pointer whose pointee is not i8 lowers to an
; OpAccessChain with no explicit index operands.
define ptr addrspace(2) @access_chain_no_index(ptr addrspace(2) %p) {
%gep = getelementptr i8, ptr addrspace(2) %p, i64 8
ret ptr addrspace(2) %gep
}
```

```console
$ llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-vulkan1.3 access-chain-no-index.ll -o -
```

## Actual behavior

### Release build (22.1.6, assertions off) — SIGSEGV

```
Stack dump:
1. Running pass 'SPIRV module analysis' on module 'access-chain-no-index.ll'.
#4 ... defusechain_instr_iterator MachineRegisterInfo.h:1171
#5 ... def_instr_begin MachineRegisterInfo.h:402
#6 ... getVRegDef MachineRegisterInfo.cpp:406
#7 ... addOpAccessChainReqs SPIRVModuleAnalysis.cpp
#8 ... addInstrRequirements SPIRVModuleAnalysis.cpp
...
exit code 139 (SIGSEGV)
```

### Assertions-enabled `main` build — assertion failure

```
llc: llvm/include/llvm/ADT/ArrayRef.h:247:
const T &llvm::ArrayRef::operator[](size_t) const:
Assertion `Index < Length && "Invalid index!"' failed.
Stack dump:
1. Running pass 'SPIRV module analysis' on module 'access-chain-no-index.ll'.
#15 addOpAccessChainReqs(...) SPIRVModuleAnalysis.cpp
```

## Expected behavior

`llc` compiles the module without crashing and emits the access chain. An access
chain with no index has no dynamic array indexing, so no
`*ArrayDynamicIndexing` capability is required.

## Root cause

In `addOpAccessChainReqs()` the offending code is (on `main`, ~line 1375):

```cpp
auto FirstIndexReg = Instr.getOperand(3).getReg();
bool FirstIndexIsConstant =
Subtarget.getInstrInfo()->isConstantInstr(*MRI.getVRegDef(FirstIndexReg));
```

As a `MachineInstr`, an `OpAccessChain` is laid out as
`{op0: result-id (def), op1: result-type, op2: base-pointer, op3..: indices}`.
For the repro above, the MIR immediately before the SPIR-V module-analysis pass is:

```
%7:id = OpAccessChain %2, %0 ; def=%7, result-type=%2, base=%0 — and NO index
```

i.e. `getNumOperands() == 3`, so `getOperand(3)` is out of range. `FirstIndexReg`
is therefore garbage, and `MRI.getVRegDef(FirstIndexReg)` crashes (or the read
asserts first, in an assertions build).

A byte `getelementptr` with a non-zero offset on a pointer whose pointee type is
not `i8` is what produces this no-index access chain. (This pattern shows up in
practice when reinterpreting buffers, e.g. AIR→SPIR-V translation reading a
`device float3*` as `float4` via byte GEPs.)

## Proposed fix

Guard the first-index inspection with an operand-count check; when the access
chain has no index, treat the index as constant (no dynamic array indexing
capability required):

```cpp
// An OpAccessChain may have no index operands at all (for example, a byte
// getelementptr on a pointer whose pointee is not i8 lowers to an access
// chain with no explicit indices). In that case there is no array indexing
// and operand 3 does not exist; reading it would index past the end of the
// instruction's operand list and crash. Only inspect the first index when it
// is present.
bool FirstIndexIsConstant = true;
if (Instr.getNumOperands() > 3) {
Register FirstIndexReg = Instr.getOperand(3).getReg();
FirstIndexIsConstant = Subtarget.getInstrInfo()->isConstantInstr(
*MRI.getVRegDef(FirstIndexReg));
}
```

`FirstIndexIsConstant` is only ever consumed in `else if (!FirstIndexIsConstant)`
branches that add `*ArrayDynamicIndexing` capabilities, so defaulting it to `true`
for an index-less chain is correct: it simply skips the dynamic-indexing
requirement.

## Verification

Performed against `main` @ `52a43992cd92b98f5059851fe43a33ba94527bd4`
(`-DLLVM_TARGETS_TO_BUILD=SPIRV -DLLVM_ENABLE_ASSERTIONS=ON`, Release):

1. **Reproduces without the patch** — both the release 22.1.6 package (SIGSEGV) and
a fresh assertions-on `main` build (bounds assert) crash on the repro above.
2. **Fixed with the patch** — patched `llc` compiles the module and emits the
expected index-less access chain:
```
%6 = OpAccessChain %2 %4
OpReturnValue %6
```
3. **Regression suite** — `ninja check-llvm-codegen-spirv` on the patched tree:
```
Total Discovered Tests: 1024
Unsupported : 3
Passed : 1020 (includes the new test below)
Expectedly Failed : 1
Failed : 0
```
Zero unexpected failures / zero regressions.

## Suggested regression test

`llvm/test/CodeGen/SPIRV/pointers/access-chain-no-index-crash.ll`:

```llvm
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-vulkan1.3 %s -o - | FileCheck %s

; A byte getelementptr on a pointer whose pointee is not i8 lowers to an
; OpAccessChain with no explicit index operands. The SPIR-V requirement
; analysis used to unconditionally read the (non-existent) first-index operand
; of every OpAccessChain, which indexed past the end of the instruction's
; operand list and crashed. Check that such an access chain now compiles
; without crashing and still emits the OpAccessChain.

; CHECK: OpFunction
; CHECK: %[[#PTR:]] = OpFunctionParameter
; CHECK: %[[#]] = OpAccessChain %[[#]] %[[#PTR]]
; CHECK: OpReturnValue
; CHECK: OpFunctionEnd

define ptr addrspace(2) @access_chain_no_index(ptr addrspace(2) %p) {
%gep = getelementptr i8, ptr addrspace(2) %p, i64 8
ret ptr addrspace(2) %gep
}
```

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.