Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Winch: Assertion failure with saturating conversion instructions #8848

Closed
alexcrichton opened this issue Jun 20, 2024 · 2 comments · Fixed by #8886
Closed

Winch: Assertion failure with saturating conversion instructions #8848

alexcrichton opened this issue Jun 20, 2024 · 2 comments · Fixed by #8886
Labels
fuzz-bug Bugs found by a fuzzer winch Winch issues or pull requests

Comments

@alexcrichton
Copy link
Member

Given this input:

(module
  (func (param f64 f64 f64 f64) (result f32 f64)
    f64.const 0
    local.get 0
    i64.const 0
    f64.const 0
    i64.const 0
    local.get 0
    f64.const 0
    i64.const 1
    i32.const 1
    i64.const 1
    f32.const 0
    local.get 0

    i32.const 0
    br_if 0

    drop
    drop
    drop
    drop
    drop
    drop
    i64.reinterpret_f64
    i64.const 0
    i64.xor
    drop
    drop
    drop
    drop
    drop
    drop
    f32.const 0
    f64.const 0
  )
)

since the implementation of saturating conversion instructions in #7909 this has panicked

$ cargo run --features winch --no-default-features --features compile compile -C compiler=winch ./testcase1.wasm -o /dev/null
...
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/wasmtime compile -C compiler=winch -o /dev/null ./foo.wat`
thread 'main' panicked at winch/codegen/src/codegen/context.rs:165:13:
assertion `left == right` failed
  left: 64
 right: 48
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @saulecabrera

@alexcrichton alexcrichton added fuzz-bug Bugs found by a fuzzer winch Winch issues or pull requests labels Jun 20, 2024
Copy link

Subscribe to Label Action

cc @saulecabrera

This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@saulecabrera
Copy link
Member

Thanks for catching this one Alex. I'll take a look.

saulecabrera added a commit to saulecabrera/wasmtime that referenced this issue Jun 28, 2024
Fixes: bytecodealliance#8848

Similar to all the control instructions, any state must be explicitly
saved before emitting the code for `br_if`.

This commit ensures that live locals and registers are explicilty saved
before emitting the code for `br_if`. Prior to this commit, live
locals and registers were not saved every time causing incorrect
behavior in cases where the calculation of the conditional argument
didn't trigger a spill.

This change introduces the explicit spill after calculating the branch
condition argument to minimize memory traffic in case the conditional is
already in a register.
github-merge-queue bot pushed a commit that referenced this issue Jun 28, 2024
Fixes: #8848

Similar to all the control instructions, any state must be explicitly
saved before emitting the code for `br_if`.

This commit ensures that live locals and registers are explicilty saved
before emitting the code for `br_if`. Prior to this commit, live
locals and registers were not saved every time causing incorrect
behavior in cases where the calculation of the conditional argument
didn't trigger a spill.

This change introduces the explicit spill after calculating the branch
condition argument to minimize memory traffic in case the conditional is
already in a register.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzz-bug Bugs found by a fuzzer winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants