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

[GC] Illegal instruction #10181

Open
vouillon opened this issue Feb 4, 2025 · 8 comments
Open

[GC] Illegal instruction #10181

vouillon opened this issue Feb 4, 2025 · 8 comments
Assignees
Labels
bug Incorrect behavior in the current implementation that needs fixing wasm-proposal:gc Issues with the implementation of the gc wasm proposal

Comments

@vouillon
Copy link

vouillon commented Feb 4, 2025

Test Case

constprop.zip

Steps to Reproduce

Run the following command:

./target/debug/wasmtime  -W=all-proposals=y constprop.wasm

Expected Results

This works fine if one add the -C collector=null option:

$ ./target/debug/wasmtime -C collector=null  -W=all-proposals=y constprop.wasm
booleans: passed
integers: passed
floats: passed
32-bit integers: passed
native integers: passed
64-bit integers: passed
integer conversions: passed
32-bit integer conversions: passed
native integer conversions: passed
64-bit integer conversions: passed

Actual Results

This crashes with an Illegal instruction error.

Versions and Environment

Wasmtime commit: 0e05600

Operating system: Linux

Architecture: x64

@vouillon vouillon added the bug Incorrect behavior in the current implementation that needs fixing label Feb 4, 2025
@alexcrichton alexcrichton assigned afonso360 and fitzgen and unassigned afonso360 Feb 4, 2025
@fitzgen
Copy link
Member

fitzgen commented Feb 4, 2025

Thanks for the bug report!

If you have time, minimizing the test case wat with creduce would be very appreciated and make diagnosing and fixing this easier. If not, I'll get around to that eventually.

@fitzgen fitzgen added the wasm-proposal:gc Issues with the implementation of the gc wasm proposal label Feb 4, 2025
@just-an-engineer
Copy link

I'll take this.
After some initial looking, it has to do with the async stuff, I'm guessing it pops the wrong address somehow.

@just-an-engineer
Copy link

Where is this wasm file from, and how did you compile it?
I suspect it may be from an invalid wasm file, which the code should check for currently, but doesn't seem to. Running wasm-validate on it gives me an error of invalid type a few bytes in, but I imagine that tools stops after the first error, so no idea how many more.
Anyways, it could be that there's some invalid stuff here that operates outside of the specification, that leds to an incorrect address being jumped to during the async execution stuffs.

@vouillon
Copy link
Author

vouillon commented Feb 4, 2025

This code relies on the Wasm GC proposal. This proposal is not supported by wasm-validate. That's why you get errors.
The code is generated using wasm_of_ocaml, which compiles OCaml code to Wam.
The source code comes from the OCaml test suite, so it is a bit weird.
wasm-opt from the binaryen toolchain is used to optimize the code. And the code runs fine in node. So I'm quite confident regarding its validity.

@fitzgen
Copy link
Member

fitzgen commented Feb 4, 2025

@just-an-engineer

I'll take this.

You're definitely welcome to investigate this bug, but this isn't likely the best good-first-issue kind of bug if you aren't already somewhat familiar with Wasmtime and Cranelift.

What would be super helpful whether you continue to investigate this bug or leave it for someone else would be to run creduce on the .wat disassembly of the test case and reduce it to a minimal example that triggers the illegal instruction fault. This would involve something like:

  • wasm-tools wasm2wat test.wasm -o test.wat
  • Writing a predicate.sh that does something like
    #!/usr/bin/env bash
    set -eu
    cargo run --manifest-path ~/path/to/wasmtime/Cargo.toml -- \
        -C collector=null -W=all-proposals=y \
        test.wat \
        2>&1 | grep -q 'Illegal instruction'
    (Note: the above is untested, you would want to make sure that it exits 1 if test.wasm doesn't trigger the illegal instruction and 0 if it does trigger the illegal instruction.)
  • And finally run creduce something like
    creduce --not-c predicate.sh test.wat
    

And when creduce is finished, post the reduced test case here.

I suspect it may be from an invalid wasm file

Wasmtime always validates the Wasm as part of its process of translating it to Cranelift's IR (called CLIF) so, barring validator bugs, we should never actually execute any invalid Wasm file.

@vouillon
Copy link
Author

vouillon commented Feb 7, 2025

I have used wasm-reduce (from the Binaryen toolchain) and creduce, plus some manual rewriting, to reduce the size of the code. It remains a bit large, probably because one needs a specific allocation pattern to trigger the bug. In particular, there is a global array which is not used anywhere, and some values that are dropped right after being allocated.
A circular datastructure is built using struct.set. Maybe this confuses the GC? I don't see anything else suspicious.
constprop.wat.txt

@tanishiking
Copy link

tanishiking commented Feb 18, 2025

We encountered an "Illegal Instruction" error in our project (scala-wasm fork of Scala.js to support wasm component model).

The error occurs at least when we try to set an i31 value (converted from i32 with the MSB has 1) to an anyref struct field.

(module
  (type $c.scala.Tuple1 (sub (struct
    (field $f.scala.Tuple1._1 (mut anyref)))))

  (func (export "test")
    (local $instance (ref $c.scala.Tuple1))
    i32.const -1
    ref.i31
    struct.new $c.scala.Tuple1
    drop
  )
)
$ wasm-tools parse test.wat -o test.wasm
$ wasmtime run -W function-references,gc --invoke test test.wasm
make: *** [run] Illegal instruction: 4

see: https://github.com/tanishiking/kitchensink/tree/main/wasmtime-illegal-instruction

  • As @vouillon mentioned, the problem doesn't happen when using the -C collector=null option.
  • I guess there're other reproductions as well, because I find there's only (ref.i31 (i32.const 0)) in @vouillon 's reproduction case.
$ wasmtime --version
wasmtime 29.0.1 (58282df89 2025-01-21)

@tanishiking
Copy link

tanishiking commented Feb 21, 2025

This particular example #10181 (comment) has fixed in local-built wasmtime (287e8fb) 🎉 not sure what was the fix though.

We still hit the Illegal instruction: 4 error in larger binary, I'll share it when I could make a bit smaller reproducible example.

tanishiking added a commit to scala-wasm/scala-wasm that referenced this issue Feb 21, 2025
Enabling GC causes an illegal instruction error,
bytecodealliance/wasmtime#10181
and in rare cases, the content of array/struct changes unexpectedly
during execution (though I haven't been able to create a
reproducible test case for this).
For now, continue development with `-C collector=null`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing wasm-proposal:gc Issues with the implementation of the gc wasm proposal
Projects
None yet
Development

No branches or pull requests

5 participants