-
Notifications
You must be signed in to change notification settings - Fork 729
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
wasm-interp: Handle refs_ correctly #2484
base: main
Are you sure you want to change the base?
Conversation
@sbc100 (rebased, ptal) |
oh yeah, we should probably bring it up: an alternative to the funny stuff with locals and |
we don't know how complete the interpreter GC (not to be confused with the GC proposal and its implementation) is (how do you even test something like that), but this "should" mark refs correctly now. we think. we're pretty sure, but we wouldn't mind a second opinion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any tests we can now pass after this change?
as we were saying, we're not sure if there's a (good) way to test this? as far as we can tell, |
So are you proposing landing this without tests? Or is this still a draft PR with more changes coming maybe? |
we suppose we can try making the GC run regularly and see if it blows up... (but actually enabling the GC to run on its own should be a separate change) |
running GC every instruction does indeed blow up. |
we can't currently add tests (actually this doesn't need new tests as much as it needs a way to run the existing tests with actual collection enabled), blocked on fixing a nasty issue with spectest-interp, but these changes do fix a bunch of assertion failures when running |
This fills in some TODOs, and cleans up some other issues, relevant for GC later on.
We tried our best to make this efficient, hence the custom wabt opcodes.