saulecabrera opened PR #6685 from saulecabrera:winch-control-flow-fixes
to bytecodealliance:main
:
This change fixes two issues with the control flow implementation found when working on https://github.com/bytecodealliance/wasmtime/pull/6610
The two fixes included in this change are:
Correctly handle the stack pointer at jump sites: when emitting an unconditional jump, the stack pointer might be left unbalanced due to register spilling, this could cause invalid memory accesses.
Explicitly track the exit label of the if block: previously the continuation label of the if block was implicitly treated as the exit label, which would cause undefined behaviour in programs that use unconditional branches in the
if
andelse
branches. This slight disadvantage of the approach in this change is that it involves special casing the emission of the end of an if block (without else) to account for the continuation label, since the continuation label is only naturally bound when there's an else branch.<!--
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
-->
saulecabrera requested cfallin for a review on PR #6685.
saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6685.
saulecabrera requested alexcrichton for a review on PR #6685.
saulecabrera requested wasmtime-core-reviewers for a review on PR #6685.
saulecabrera requested fitzgen for a review on PR #6685.
saulecabrera requested wasmtime-core-reviewers for a review on PR #6685.
saulecabrera updated PR #6685.
saulecabrera updated PR #6685.
cfallin submitted PR review:
Thanks!
cfallin submitted PR review:
Thanks!
cfallin created PR review comment:
Can we add an assert here that
current_sp_offset >= original_sp_offset
? The invariant we want in the end is that the offsets are equal, and we handle the greater-than case above; I think we should never see the less-than case because we're going "up" the control-stack, is that right?
saulecabrera updated PR #6685.
saulecabrera created PR review comment:
Good catch. Done, thanks!
saulecabrera has enabled auto merge for PR #6685.
saulecabrera created PR review comment:
CI disagrees, and after digging into it a bit I think that the invariant in question not always holds, legitimately:
(module (func (export "cont-inner") (result i32) (local i32) (local.set 0 (i32.const 0)) (local.set 0 (i32.add (local.get 0) (loop (result i32) (loop (result i32) (br 1))))) ;; <==== (local.set 0 (i32.add (local.get 0) (loop (result i32) (i32.ctz (br 0))))) (local.set 0 (i32.add (local.get 0) (loop (result i32) (i32.ctz (loop (result i32) (br 1)))))) (local.get 0) ) )
When entering the second loop (in the second
local.set
), there are no values to spill so the current stack pointer offset is 24, same as the sp offset at the outer loop which spilled one local, then when reachingbr 1
, since the value stack only contains spilled values we need to pop that value to account for the return of the loop, which will leave the current sp at 16, effectively less than the original.This makes sense to me, but do let me know if you see anything different.
saulecabrera updated PR #6685.
saulecabrera has disabled auto merge for PR #6685.
cfallin created PR review comment:
Ah, hmm, OK. Can we leave a comment as to how this works then? It's not obvious to me at least how this local bit of code works -- why is it OK that we can branch to a place if we have a mismatched stack -- unless the needed values are pushed somewhere else?
saulecabrera updated PR #6685.
saulecabrera updated PR #6685.
saulecabrera created PR review comment:
Added a comment and put back an assert that goes along with the comment: in general the stack pointer can only be
word_bytes
less than stack pointer offset of destination frame, which accounts for the handling of the return values at the jump site. I hope this clarifies things!
cfallin created PR review comment:
Sounds good!
saulecabrera merged PR #6685.
Last updated: Dec 23 2024 at 12:05 UTC