bytecodealliance / bytecodealliance/wasmtime

Cranelift: missing zero extension of i8,i16 causes undetermined behavior on x64

Open
#5,645 6 comments 0 reactions 0 assignees View on GitHub
bug cranelift
Dominant language
Rust
Stars
18.6k
Forks
1.8k
Avg merge
1d 18h
Merged PRs (30d)
126

Description

I have stumbled upon a rather nasty 'bug' this week.
When compiling IR like this:
```
function u0:0(i64) -> i64 system_v {
sig2 = (i64, i8) -> i64 system_v
fn2 = u0:112 sig2

block0(v0: i64):
v1 = icmp_imm ne v0, 0
v2 = call fn2(v0, v1)
return v2
}
```

you will get assembly that looks like this:
```asm
pushq %rbp
unwind PushFrameRegs { offset_upward_to_caller_sp: 16 }
movq %rsp, %rbp
unwind DefineNewFrame { offset_upward_to_caller_sp: 16, offset_downward_to_clobbers: 0 }
block0:
cmpq $0, %rdi
setnz %sil
load_ext_name userextname0+0, %r8
call *%r8
movq %rbp, %rsp
popq %rbp
ret
```

Note that the `icmp_imm` gets compiled to a `cmp` followed by a `setcc` instruction using `sil` as a parameter.
This means that the upper bytes of `rsi` are undefined. Now when `fn2` is a C function that looks like this:
```c
void fun(uint64_t x, bool a) {
do_smth(a ? x : 0);
}
```
and you compile this with clang the assembly looks like this:
```asm
xor eax, eax
test esi, esi
cmove rdi, rax
jmp do_smth(unsigned long)@PLT
```
while with gcc you get
```asm
xor eax, eax
test sil, sil
cmove rdi, rax
jmp do_smth(unsigned long)
```

Note that gcc emits a `test sil, sil` instruction while clang emits `test esi, esi` meaning that it relies on the 8bit argument getting zero-extended to at least 32 bit and therefore you will get random behavior depending on whether the upper bytes of `rsi` were zero or not. ([Compiler Explorer link](https://godbolt.org/z/Mn5qzjP8j))
[According to the people at LLVM](https://bugs.llvm.org/show_bug.cgi?id=44228), whether arguments should be zero-extended or not is unclear in the ABI and therefore they do not consider it a bug until this issue has been resolved on the ABI level since the major compilers do zero-extend the arguments.

Now my question is, do you consider this a bug in cranelift or at least an issue that should be addressed on the codegen level inside cranelift? (I have fixed this by manually checking if an argument is I8/I16 when emitting a function call and then passed that through a `uextend`/`inarrow` combo)
Maybe this could be done when the `enable_llvm_abi_extensions` flag is set?
And then the question would be where to fix it: When passing a variable to an external function or making sure that when instructions like `setcc` are emitted the upper parts of the destination register are cleared beforehand?

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.