Stream: git-wasmtime

Topic: wasmtime / PR #6685 winch(x64): Fix a couple of issues wi...

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:09):

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:

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

2. 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 and else 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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:09):

saulecabrera requested cfallin for a review on PR #6685.

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:09):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6685.

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:09):

saulecabrera requested alexcrichton for a review on PR #6685.

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:09):

saulecabrera requested wasmtime-core-reviewers for a review on PR #6685.

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:09):

saulecabrera requested fitzgen for a review on PR #6685.

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:09):

saulecabrera requested wasmtime-core-reviewers for a review on PR #6685.

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:12):

saulecabrera updated PR #6685.

Wasmtime GitHub notifications bot (Jul 04 2023 at 21:21):

saulecabrera updated PR #6685.

Wasmtime GitHub notifications bot (Jul 05 2023 at 16:19):

cfallin submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Jul 05 2023 at 16:19):

cfallin submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Jul 05 2023 at 16:19):

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?

Wasmtime GitHub notifications bot (Jul 05 2023 at 18:10):

saulecabrera updated PR #6685.

Wasmtime GitHub notifications bot (Jul 05 2023 at 18:12):

saulecabrera created PR review comment:

Good catch. Done, thanks!

Wasmtime GitHub notifications bot (Jul 05 2023 at 18:13):

saulecabrera has enabled auto merge for PR #6685.

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:03):

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 reaching br 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.

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:04):

saulecabrera updated PR #6685.

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:07):

saulecabrera has disabled auto merge for PR #6685.

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:14):

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?

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:56):

saulecabrera updated PR #6685.

Wasmtime GitHub notifications bot (Jul 05 2023 at 21:03):

saulecabrera updated PR #6685.

Wasmtime GitHub notifications bot (Jul 05 2023 at 21:25):

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!

Wasmtime GitHub notifications bot (Jul 05 2023 at 21:28):

cfallin created PR review comment:

Sounds good!

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:15):

saulecabrera merged PR #6685.

Last updated: Nov 22 2024 at 16:03 UTC