Stream: git-wasmtime

Topic: wasmtime / issue #5163 cranelift-interpreter: Clean up ad...

Wasmtime GitHub notifications bot (Oct 31 2022 at 22:43):

jameysharp labeled issue #5163:

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.

Wasmtime GitHub notifications bot (Oct 31 2022 at 22:43):

jameysharp opened issue #5163:

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.

Wasmtime GitHub notifications bot (Mar 29 2023 at 17:31):

jameysharp commented on issue #5163:

After #5386 removed heaps from Cranelift, all of the heap-address helpers are now gone and validate_address is not called anywhere. So some of this issue has become irrelevant but now there's more to clean up.

Some of this might also become irrelevant with #5793.

Last updated: Nov 22 2024 at 17:03 UTC