jeffcharles requested wasmtime-compiler-reviewers for a review on PR #7573.
jeffcharles requested abrown for a review on PR #7573.
jeffcharles opened PR #7573 from jeffcharles:winch-spill-when-calling
to bytecodealliance:main
:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
The differential fuzzer triggered amem.slot.offset.as_u32() == masm.sp_offset().as_u32()
assertion while compiling the following module:(module (func (;0;) (param i32) (result i32) local.get 0 i32.const 1 call 0 i32.const 1 call 0 br_if 0 (;@0;) unreachable ) (export "" (func 0)) )
It looks like cause of the issue is that using
save_live_registers_and_calculate_sizeof
can violate a value stack invariant by placingMemory
entries afterLocal
andReg
entries.To remediate this, we can trigger a spill instead of only saving live registers when emitting the machine code for a function call and we can calculate the amount of stack space consumed by the call in the
cleanup
method by summing the size of each memory entry used as an argument for the call.As a result of spilling, I needed to add specify which scratch register to use for
funcref
s. I also needed to add a spill at the start of visiting acall_indirect
operator. This is because the machine code associated with a spill that occurs when calling a function to initialize the table element may be jumped over, resulting in the codegen context state being out of sync with the machine state, which potentially causes the machine stack to become unbalanced.
jeffcharles created PR review comment:
My understanding is
v
should never be a register when we reach this code because we spilled the value stack earlier. I've tried to confirm that by adding a panic ifv
is a register and running the test suite and filetests and both passed.
jeffcharles submitted PR review.
jeffcharles updated PR #7573.
saulecabrera submitted PR review:
Thanks for digging into this. Left a few comments in the code, and also: can we update the comments in
call.rs
to reflect the new changes? The module documentation and the documentation forFnCall::emit
both state that the implementation only saves live registers, which is no longer true.
saulecabrera created PR review comment:
Your understanding is correct, aside from matching against
Val::Memory
, I think it'd be useful to assert for such invariant in the code (e.g.debug_assert!(v.is_mem() || v.is_const())
, that way we'd at least have a way of identifying any issues early on if this invariant ever breaks.NB:
is_const()
currently doesn't exist, so if we go with this approach, it needs to be added.
saulecabrera submitted PR review:
Thanks for digging into this. Left a few comments in the code, and also: can we update the comments in
call.rs
to reflect the new changes? The module documentation and the documentation forFnCall::emit
both state that the implementation only saves live registers, which is no longer true.
saulecabrera created PR review comment:
Only
WasmHeapType::Func
is supported at the moment. Can we update this snippet to reflect that? We'll be able to lift such restriction once externrefs and typed funcs are supported.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Can you add a bit more context here on when these instructions get jumped over? e.g. if the funcref was previously initialized.
jeffcharles updated PR #7573.
jeffcharles requested saulecabrera for a review on PR #7573.
saulecabrera submitted PR review.
saulecabrera merged PR #7573.
Last updated: Dec 23 2024 at 12:05 UTC