Stream: git-wasmtime

Topic: wasmtime / PR #7573 Winch: fix bug by spilling when emitt...

Wasmtime GitHub notifications bot (Nov 22 2023 at 19:52):

jeffcharles requested wasmtime-compiler-reviewers for a review on PR #7573.

Wasmtime GitHub notifications bot (Nov 22 2023 at 19:52):

jeffcharles requested abrown for a review on PR #7573.

Wasmtime GitHub notifications bot (Nov 22 2023 at 19:52):

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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
The differential fuzzer triggered a mem.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 placing Memory entries after Local and Reg 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 funcrefs. I also needed to add a spill at the start of visiting a call_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.

Wasmtime GitHub notifications bot (Nov 22 2023 at 19:55):

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 if v is a register and running the test suite and filetests and both passed.

Wasmtime GitHub notifications bot (Nov 22 2023 at 19:55):

jeffcharles submitted PR review.

Wasmtime GitHub notifications bot (Nov 22 2023 at 20:10):

jeffcharles updated PR #7573.

Wasmtime GitHub notifications bot (Nov 22 2023 at 20:47):

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 for FnCall::emit both state that the implementation only saves live registers, which is no longer true.

Wasmtime GitHub notifications bot (Nov 22 2023 at 20:47):

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.

Wasmtime GitHub notifications bot (Nov 22 2023 at 20:47):

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 for FnCall::emit both state that the implementation only saves live registers, which is no longer true.

Wasmtime GitHub notifications bot (Nov 22 2023 at 20:47):

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.

Wasmtime GitHub notifications bot (Nov 22 2023 at 20:49):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 22 2023 at 20:49):

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.

Wasmtime GitHub notifications bot (Nov 22 2023 at 22:06):

jeffcharles updated PR #7573.

Wasmtime GitHub notifications bot (Nov 22 2023 at 23:01):

jeffcharles requested saulecabrera for a review on PR #7573.

Wasmtime GitHub notifications bot (Nov 23 2023 at 11:59):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2023 at 12:43):

saulecabrera merged PR #7573.

Last updated: Nov 22 2024 at 16:03 UTC