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: Different results than Cranelift with floating-point f64.ne instruction #8632

Closed
alexcrichton opened this issue May 15, 2024 · 5 comments · Fixed by #8685
Closed

Winch: Different results than Cranelift with floating-point f64.ne instruction #8632

alexcrichton opened this issue May 15, 2024 · 5 comments · Fixed by #8685
Assignees
Labels
fuzz-bug Bugs found by a fuzzer winch Winch issues or pull requests

Comments

@alexcrichton
Copy link
Member

Given this input:

(module
  (func (result i32 i32 i32)
    i32.const 1
    i32.eqz
    f64.const 0
    f64.const 1
    f64.ne
    i32.const 1111
  )
  (export "d" (func 0))
)

I locally get:

$ ./target/release/wasmtime run -C compiler=cranelift --invoke d out.wat
warning: using `--invoke` with a function that returns values is experimental and may break in the future
0
1
1111
$ ./target/release/wasmtime run -C compiler=winch --invoke d out.wat
warning: using `--invoke` with a function that returns values is experimental and may break in the future
0
0
1111

Notably the second return value here, the f64.ne branch, differs in Winch and Cranelift. I believe that Cranelift is correct in this case, hence the bug report for winch.

cc @saulecabrera

@alexcrichton alexcrichton added the winch Winch issues or pull requests label May 15, 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.

@alexcrichton alexcrichton added the fuzz-bug Bugs found by a fuzzer label May 15, 2024
@alexcrichton
Copy link
Member Author

For reference the compiler outputs for this function are:

cranelift
0000000000000000 <wasm[0]::function[0]>:
       0:       55                      push   %rbp
       1:       48 89 e5                mov    %rsp,%rbp
       4:       31 c0                   xor    %eax,%eax
       6:       c5 c1 57 cf             vxorpd %xmm7,%xmm7,%xmm1
       a:       c5 f9 2e 0d 1e 00 00    vucomisd 0x1e(%rip),%xmm1        # 30 <wasm[0]::function[0]+0x30>
      11:       00
      12:       40 0f 9a c6             setp   %sil
      16:       40 0f 95 c7             setne  %dil
      1a:       09 fe                   or     %edi,%esi
      1c:       40 0f b6 ce             movzbl %sil,%ecx
      20:       ba 57 04 00 00          mov    $0x457,%edx
      25:       48 89 ec                mov    %rbp,%rsp
      28:       5d                      pop    %rbp
      29:       c3                      ret
        ...
      36:       f0 3f                   lock (bad)
        ...
winch
0000000000000000 <wasm[0]::function[0]>:
       0:       55                      push   %rbp
       1:       48 89 e5                mov    %rsp,%rbp
       4:       4c 8b 5f 08             mov    0x8(%rdi),%r11
       8:       4d 8b 1b                mov    (%r11),%r11
       b:       49 81 c3 24 00 00 00    add    $0x24,%r11
      12:       49 39 e3                cmp    %rsp,%r11
      15:       0f 87 83 00 00 00       ja     9e <wasm[0]::function[0]+0x9e>
      1b:       49 89 fe                mov    %rdi,%r14
      1e:       48 83 ec 18             sub    $0x18,%rsp
      22:       48 89 7c 24 10          mov    %rdi,0x10(%rsp)
      27:       48 89 74 24 08          mov    %rsi,0x8(%rsp)
      2c:       48 89 14 24             mov    %rdx,(%rsp)
      30:       b8 01 00 00 00          mov    $0x1,%eax
      35:       83 f8 00                cmp    $0x0,%eax
      38:       b8 00 00 00 00          mov    $0x0,%eax
      3d:       40 0f 94 c0             rex sete %al
      41:       f2 0f 10 05 57 00 00    movsd  0x57(%rip),%xmm0        # a0 <wasm[0]::function[0]+0xa0>
      48:       00
      49:       f2 0f 10 0d 57 00 00    movsd  0x57(%rip),%xmm1        # a8 <wasm[0]::function[0]+0xa8>
      50:       00
      51:       66 0f 2e c8             ucomisd %xmm0,%xmm1
      55:       b9 00 00 00 00          mov    $0x0,%ecx
      5a:       40 0f 95 c1             rex setne %cl
      5e:       41 bb 00 00 00 00       mov    $0x0,%r11d
      64:       41 0f 9a c3             setp   %r11b
      68:       4c 09 d9                or     %r11,%rcx
      6b:       48 83 ec 04             sub    $0x4,%rsp
      6f:       89 04 24                mov    %eax,(%rsp)
      72:       51                      push   %rcx
      73:       b8 57 04 00 00          mov    $0x457,%eax
      78:       48 83 c4 04             add    $0x4,%rsp
      7c:       48 8b 4c 24 08          mov    0x8(%rsp),%rcx
      81:       44 8b 1c 24             mov    (%rsp),%r11d
      85:       48 83 c4 04             add    $0x4,%rsp
      89:       44 89 19                mov    %r11d,(%rcx)
      8c:       44 8b 1c 24             mov    (%rsp),%r11d
      90:       48 83 c4 04             add    $0x4,%rsp
      94:       44 89 59 04             mov    %r11d,0x4(%rcx)
      98:       48 83 c4 18             add    $0x18,%rsp
      9c:       5d                      pop    %rbp
      9d:       c3                      ret
      9e:       0f 0b                   ud2
      a0:       00 00                   add    %al,(%rax)
      a2:       00 00                   add    %al,(%rax)
      a4:       00 00                   add    %al,(%rax)
      a6:       f0 3f                   lock (bad)
        ...

@saulecabrera saulecabrera self-assigned this May 15, 2024
@saulecabrera
Copy link
Member

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

@saulecabrera
Copy link
Member

A quick update: I've identified the root cause and I'm working on fix. There's a bug in the stack shuffling algorithm which is causing truncation of values when handling multi-value returns.

saulecabrera added a commit to saulecabrera/wasmtime that referenced this issue May 23, 2024
Fixes: bytecodealliance#8632

This commit fixes the handling of f64 comparison operations in Winch.
f64 comparison operations are defined as

	[f64 f64] -> [i32]

The previous implemementation was widening the result to `[i64]`  which
caused issues which stack shuffling in multi-value returns.
@saulecabrera
Copy link
Member

The stack shuffling algorithm was a bit of a red herring. Here's a fix: https://github.com/bytecodealliance/wasmtime/pull/8685/files

github-merge-queue bot pushed a commit that referenced this issue May 23, 2024
Fixes: #8632

This commit fixes the handling of f64 comparison operations in Winch.
f64 comparison operations are defined as

	[f64 f64] -> [i32]

The previous implemementation was widening the result to `[i64]`  which
caused issues which stack shuffling in multi-value returns.
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