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

Update to new GC cast instruction encoding #1127

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

bvisness
Copy link
Contributor

@bvisness bvisness commented Jul 17, 2023

Per WebAssembly/gc#359 (comment), the encoding for GC cast instructions has changed to use two separate opcodes for br_on_cast and br_on_cast_fail, rather than being disambiguated by a flag bit.

I looked around for other places to update, but it seems like parsing isn't implemented for GC instructions yet? Roundtrip tests are also crashing on my local computer on the latest main, so I'm unable to properly test this. Would appreciate any guidance.

@alexcrichton
Copy link
Member

Ah yeah support for GC instructions isn't implemented in the rest of the tooling yet, and we don't otherwise run many other GC tests at this time due to the limited support in this repository.

Could you paste the errors you're seeing with the roundtrip suite? Are they related to this PR or probabaly unrelated? (given the green CI run here happy to merge, but wanted to confirm too)

@bvisness
Copy link
Contributor Author

bvisness commented Jul 17, 2023

Here's what I get when I run cargo test on my Mac:

     Running tests/roundtrip.rs (target/debug/deps/roundtrip-8e565f4fdc7362cd)
running 381 test files


thread '<unknown>' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass `--test roundtrip`

Caused by:
  process didn't exit successfully: `/Users/mozilla/Developer/bytecodealliance/wasm-tools/target/debug/deps/roundtrip-8e565f4fdc7362cd` (signal: 6, SIGABRT: process abort signal)

I see this test runs fine in CI, so I have no idea what the issue might be. And this happens on main, so unrelated to this PR.

@alexcrichton
Copy link
Member

Oh dear! If you're up for a bit of debugging could you try narrowing down to which test is failing? Or capture a stack trace to see what's eating so much stack?

Otherwise though this looks good to me so I'm gonna go ahead and merge.

@alexcrichton alexcrichton enabled auto-merge (squash) July 17, 2023 14:19
@alexcrichton alexcrichton merged commit 07229fe into bytecodealliance:main Jul 17, 2023
15 checks passed
@bvisness bvisness deleted the gc-cast-encoding branch July 17, 2023 14:31
@bvisness
Copy link
Contributor Author

Thanks! Would it be possible to get a release with this update as well?

@alexcrichton
Copy link
Member

Sure yeah, I'll post that at #1128

dhil pushed a commit to dhil/wasm-tools that referenced this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants