Stream: git-wasmtime

Topic: wasmtime / issue #5047 Cranelift: `uextend` + `iconst` fa...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 12 2022 at 13:34):

afonso360 labeled issue #5047:

:wave: Hey,

The fuzzer crashed on this today when I was testing the new egraph work.

This issue is related to us allowing a 64 bit value on iconst.i32, it fails both on x86 and aarch64.

I'm writing this as an issue because I think the a better solution might be to restrict the values that we allow on iconst in the frontend instead of trying to mask it in the optimizations done by egraphs.

However, I'm not sure if doing that impacts something else, or if there is a better solution.

.clif Test Case

test interpret
test run
set use_egraphs=true
target aarch64

function %a() -> i64 {
block0:
    v1 = iconst.i32 0xffff_ffff_f879_ff08
    v2 = uextend.i64 v1
    return v2
}

; run: %a() == 4168744712

Steps to Reproduce

clif-util ./the-above.clif

Expected Results

The test to pass

Actual Results

afonso@DESKTOP-VSTS4BC:~/git/wasmtime/cranelift$ cargo run --target aarch64-unknown-linux-gnu -- test ./lmao.clif
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `qemu-aarch64 -L /usr/aarch64-linux-gnu -E LD_LIBRARY_PATH=/usr/aarch64-linux-gnu/lib /home/afonso/git/wasmtime/target/aarch64-unknown-linux-gnu/debug/clif-util test ./lmao.clif`
 ERROR cranelift_filetests::concurrent > FAIL: run
FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a() == 4168744712, actual: -126222584
1 tests
Error: 1 failure

Versions and Environment

Cranelift version or commit: main
Operating system: linux
Architecture: aarch64 and x86

cc: @cfallin

view this post on Zulip Wasmtime GitHub notifications bot (Oct 12 2022 at 13:34):

afonso360 labeled issue #5047:

:wave: Hey,

The fuzzer crashed on this today when I was testing the new egraph work.

This issue is related to us allowing a 64 bit value on iconst.i32, it fails both on x86 and aarch64.

I'm writing this as an issue because I think the a better solution might be to restrict the values that we allow on iconst in the frontend instead of trying to mask it in the optimizations done by egraphs.

However, I'm not sure if doing that impacts something else, or if there is a better solution.

.clif Test Case

test interpret
test run
set use_egraphs=true
target aarch64

function %a() -> i64 {
block0:
    v1 = iconst.i32 0xffff_ffff_f879_ff08
    v2 = uextend.i64 v1
    return v2
}

; run: %a() == 4168744712

Steps to Reproduce

clif-util ./the-above.clif

Expected Results

The test to pass

Actual Results

afonso@DESKTOP-VSTS4BC:~/git/wasmtime/cranelift$ cargo run --target aarch64-unknown-linux-gnu -- test ./lmao.clif
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `qemu-aarch64 -L /usr/aarch64-linux-gnu -E LD_LIBRARY_PATH=/usr/aarch64-linux-gnu/lib /home/afonso/git/wasmtime/target/aarch64-unknown-linux-gnu/debug/clif-util test ./lmao.clif`
 ERROR cranelift_filetests::concurrent > FAIL: run
FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a() == 4168744712, actual: -126222584
1 tests
Error: 1 failure

Versions and Environment

Cranelift version or commit: main
Operating system: linux
Architecture: aarch64 and x86

cc: @cfallin

view this post on Zulip Wasmtime GitHub notifications bot (Oct 12 2022 at 13:48):

afonso360 edited issue #5047:

:wave: Hey,

The fuzzer crashed on this today when I was testing the new egraph work.

This issue is related to us allowing a 64 bit value on iconst.i32, it fails both on x86 and aarch64.

I'm writing this as an issue because I think the a better solution might be to restrict the values that we allow on iconst in the frontend instead of trying to mask it in the optimizations done by egraphs.

However, I'm not sure if doing that impacts something else, or if there is a better solution.

Edit: Maybe we can solve this with https://github.com/bytecodealliance/wasmtime/issues/3059 ?

.clif Test Case

test interpret
test run
set use_egraphs=true
target aarch64

function %a() -> i64 {
block0:
    v1 = iconst.i32 0xffff_ffff_f879_ff08
    v2 = uextend.i64 v1
    return v2
}

; run: %a() == 4168744712

Steps to Reproduce

clif-util ./the-above.clif

Expected Results

The test to pass

Actual Results

afonso@DESKTOP-VSTS4BC:~/git/wasmtime/cranelift$ cargo run --target aarch64-unknown-linux-gnu -- test ./lmao.clif
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `qemu-aarch64 -L /usr/aarch64-linux-gnu -E LD_LIBRARY_PATH=/usr/aarch64-linux-gnu/lib /home/afonso/git/wasmtime/target/aarch64-unknown-linux-gnu/debug/clif-util test ./lmao.clif`
 ERROR cranelift_filetests::concurrent > FAIL: run
FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a() == 4168744712, actual: -126222584
1 tests
Error: 1 failure

Versions and Environment

Cranelift version or commit: main
Operating system: linux
Architecture: aarch64 and x86

cc: @cfallin

view this post on Zulip Wasmtime GitHub notifications bot (Oct 12 2022 at 18:19):

cfallin commented on issue #5047:

Thanks! This is the result of this rewrite rule:

(rule (simplify (uextend $I64 (iconst $I32 imm)))
      (iconst $I64 imm))

coupled with lack of masking on the bits. As we talked about in the meeting today I think it makes sense to mask the 64-bit payload of iconst to the actual type's width, with zero bits above. Probably we should ensure this in the parser, and we should check it in the validator (or maybe even a dedicated assert somewhere).

view this post on Zulip Wasmtime GitHub notifications bot (Oct 17 2022 at 19:45):

cfallin closed issue #5047:

:wave: Hey,

The fuzzer crashed on this today when I was testing the new egraph work.

This issue is related to us allowing a 64 bit value on iconst.i32, it fails both on x86 and aarch64.

I'm writing this as an issue because I think the a better solution might be to restrict the values that we allow on iconst in the frontend instead of trying to mask it in the optimizations done by egraphs.

However, I'm not sure if doing that impacts something else, or if there is a better solution.

Edit: Maybe we can solve this with https://github.com/bytecodealliance/wasmtime/issues/3059 ?

.clif Test Case

test interpret
test run
set use_egraphs=true
target aarch64

function %a() -> i64 {
block0:
    v1 = iconst.i32 0xffff_ffff_f879_ff08
    v2 = uextend.i64 v1
    return v2
}

; run: %a() == 4168744712

Steps to Reproduce

clif-util ./the-above.clif

Expected Results

The test to pass

Actual Results

afonso@DESKTOP-VSTS4BC:~/git/wasmtime/cranelift$ cargo run --target aarch64-unknown-linux-gnu -- test ./lmao.clif
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `qemu-aarch64 -L /usr/aarch64-linux-gnu -E LD_LIBRARY_PATH=/usr/aarch64-linux-gnu/lib /home/afonso/git/wasmtime/target/aarch64-unknown-linux-gnu/debug/clif-util test ./lmao.clif`
 ERROR cranelift_filetests::concurrent > FAIL: run
FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a() == 4168744712, actual: -126222584
1 tests
Error: 1 failure

Versions and Environment

Cranelift version or commit: main
Operating system: linux
Architecture: aarch64 and x86

cc: @cfallin

Last updated: Nov 22 2024 at 16:03 UTC