wabt icon indicating copy to clipboard operation
wabt copied to clipboard

wasm-interp: Handle Refs

Open SoniEx2 opened this issue 1 year ago • 2 comments

This fills in some TODOs, may or may not be relevant for EHv4 but will definitely be relevant for GC later on.

We tried our best to make this efficient, hence the interpreter-only opcodes.

SoniEx2 avatar Oct 05 '24 13:10 SoniEx2

@sbc100 (rebased, ptal)

SoniEx2 avatar Oct 07 '24 23:10 SoniEx2

oh yeah, we should probably bring it up: an alternative to the funny stuff with locals and refs_ would be to mark ref locals at function start, which would be decidedly faster if slightly more annoying to set up. but then again, we weren't really going for maximum performance with this.

SoniEx2 avatar Oct 07 '24 23:10 SoniEx2

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.

SoniEx2 avatar Mar 02 '25 01:03 SoniEx2

as we were saying, we're not sure if there's a (good) way to test this?

as far as we can tell, Store::Collect is never called while the interpreter is running, so it kinda just... leaks?

SoniEx2 avatar Mar 04 '25 18:03 SoniEx2

So are you proposing landing this without tests? Or is this still a draft PR with more changes coming maybe?

sbc100 avatar Mar 04 '25 18:03 sbc100

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)

SoniEx2 avatar Mar 04 '25 18:03 SoniEx2

running GC every instruction does indeed blow up.

SoniEx2 avatar Mar 04 '25 19:03 SoniEx2

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 store_.Collect() for every instruction. we would indeed like to land this without tests (for now).

SoniEx2 avatar Mar 04 '25 22:03 SoniEx2

any chance of getting this merged? @sbc100

SoniEx2 avatar Mar 09 '25 14:03 SoniEx2

I suppose I'm ok with landing this as an internal refactor. Hopefully any followups can have tests?

sbc100 avatar Mar 10 '25 18:03 sbc100