bytecodealliance / bytecodealliance/wasmtime
cranelift-interpreter: Clean up address computation
- Dominant language
- Rust
- Stars
- 18.6k
- Forks
- 1.8k
- Avg merge
- 1d 18h
- Merged PRs (30d)
- 126
Description
The `sum` and `calculate_addr` functions in `cranelift/interpreter/src/step.rs`, and the `get_heap_address` and `heap_address` and `validate_address` functions on `InterpreterState`, are too general in some ways and not general enough in others.
In #5155 @afonso360 discovered that `calculate_address` was being called incorrectly in the interpreter's implementation of `heap_addr`. It was passed the `imm()`, which for that instruction is the size to validate, along with the dynamic offset. So the returned address started immediately _after_ the memory region it was supposed to return. Since that function exists solely to 1) convert values to a specified type and 2) sum them, an instruction where there's only one operand doesn't really need that.
The only two other uses of `calculate_address` pass a constant type (`I64`), so the value conversion is more general than necessary. And they always pass an array of exactly one value, so the iterators and generalized sums are also more general than necessary.
Meanwhile, the `sum` helper used in `calculate_address` is also used in `stack_load`, `stack_store`, and `stack_addr`, but nowhere else. It seems like any `calculate_address`-style helper that's useful for calculating heap offsets should work for stack instructions too. So in that sense apparently `calculate_address` is _less_ general than necessary. Regardless, I'd like to see `sum` deleted since there are always exactly two operands.
There's very little difference between `get_heap_address` and `heap_address`, except the latter is in the `State` trait and the former is an inherent method on `InterpreterState` specifically. Perhaps we could merge those? Since `heap_address` is only called from the implementation of the `heap_addr` instruction and `get_heap_address` is only called from tests, I suspect changing their signatures shouldn't be hard.
Calling `heap_address` twice, once to check the upper bound and a second time to get the actual address, isn't really necessary. We can and should check whether any of the address computation additions overflow; that definitely indicates invalid addresses. But only the upper bound needs to get checked against the heap size; if that check passes without overflow, the start address is definitely safe. So I think `heap_address` should be passed the width, so it can ask `validate_address` to check the start address plus width.
The interpreter currently does not check for overflow, as far as I can tell, since `Value::add` is implemented using `wrapping_add`.
`validate_address` has cases for both heap and stack addresses, but is only ever called with heap addresses. (From `get_heap_address` and `heap_address`, specifically.) It might be useful to actually use it with stack addresses.
None of this is urgent; it'd just be nice for future maintenance.
Contributor guide
Assessment
This issue has not been assessed yet.