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.

Sign up for GitHub

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

Jump to bottom

WasmFX reference interpreter and testsuite #47

Merged
merged 147 commits into from
Apr 22, 2024
Merged

WasmFX reference interpreter and testsuite #47

merged 147 commits into from
Apr 22, 2024

Conversation

dhil
Copy link
Member

@dhil dhil commented Apr 12, 2024

This patch adds the implementation of the WebAssembly reference interpreter extended with support for the WasmFX instruction set. It also adds the WasmFX testsuite.

Please merge this after #46.

dhil and others added 30 commits February 15, 2021 15:56
@dhil
Merge pull request WebAssembly#2 from effect-handlers/merge-funcref
9ef4a9b 
Merge typed-func-refs proposal
@dhil
Minimal exception handling extension
41d0c6e 
This patch extends the interpreter with a minimal exception handling
facility. It is minimal in the sense that it supports only a single
exception which can be thrown and caught.
@dhil @rossberg
Update interpreter/syntax/free.ml
f81d04a 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil @rossberg
Update interpreter/text/arrange.ml
1572421 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil @rossberg
Update interpreter/text/parser.mly
7fe3e89 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil @rossberg
Update interpreter/exec/eval.ml
f293c24 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil @rossberg
Update interpreter/exec/eval.ml
ad622f3 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil @rossberg
Update interpreter/binary/encode.ml
449e176 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil @rossberg
Update interpreter/exec/eval.ml
8e0a378 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil
Declare DO as a token.
147024b
@dhil
Add lexical rules for try/catch/do/throw.
bd8a611
@dhil
Add a few test cases.
b0fcf75
@dhil
Implement binary encoding and decoding.
62fc2d9 
The opcodes for try/catch/throw are taken from the provisional
exception-handling proposal.
@dhil @rossberg
Update interpreter/script/run.ml
4d2b207 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil @rossberg
Update interpreter/script/run.ml
bc83c41 
Co-authored-by: Andreas Rossberg <[email protected]>
@dhil
Address @rossberg's comments.
5d4602a
@dhil
Adds missing success reduction for catch.
1b59e24 
Extends test suite too.
@dhil @rossberg
Update interpreter/script/js.ml
68e69ec 
Co-authored-by: Andreas Rossberg <[email protected]>
@rossberg
Implement events
ce612e8
@rossberg
Add cont type and instructions; validation, en/decoding
ac039db
@dhil
Merge pull request WebAssembly#1 from effect-handlers/tiny-exn
2d7252d 
Minimal exception handling extension
@dhil
Merge pull request WebAssembly#4 from effect-handlers/mini-exn
3eebbfe 
Implement events
@dhil
Merge branch 'master' of github.com:effect-handlers/wasm-spec into ti…
c1e7772 
…ny-exn
@dhil
Merge pull request WebAssembly#5 from effect-handlers/cont
a6ab02b 
Add cont type and instructions
@dhil
Merge branch 'mini-exn' of github.com:effect-handlers/wasm-spec into …
fdb69cc 
…tiny-exn
@rossberg
Typos
fb297a0
@rossberg
Merge branch 'master' into cont
7704c23
@rossberg
First semantics
c9f369b
@rossberg
Simplify
7f092d0
@rossberg
Text, simple test
5e510ee
@fgmccabe
Copy link
Collaborator

fgmccabe commented Apr 12, 2024 via email

@tlively
Copy link
Member

tlively commented Apr 15, 2024

What do we gain by not merging this?

@fgmccabe
Copy link
Collaborator

I am currently using the 'main' branch of the repo for my own experiments. It would be seriously conflicting to merge the wasfx interpreter at this point. (Note: I will also not be merging my changes into the main branch).

@fgmccabe fgmccabe closed this Apr 15, 2024
@dhil
Copy link
Member Author

dhil commented Apr 15, 2024

I am currently using the 'main' branch of the repo for my own experiments. It would be seriously conflicting to merge the wasfx interpreter at this point. (Note: I will also not be merging my changes into the main branch).

I think your basis for rejection is weak. If you are worried about merge conflicts then I will gladly do the merge for you. Regardless, I think your current thinking is wrong: we absolutely should have both stack switching proposals readily accessible in this repository to enable us and others to play with them.

@fgmccabe
Copy link
Collaborator

I am referring here to the interpreter changes...
There is only one folder with the interpreter in it.

However, if you would like to merge into a 'wasm-fx' branch of the repo, I can definitely agree with that.

@tlively
Copy link
Member

tlively commented Apr 15, 2024

+1 from me for having WasmFX and bag-o-stacks simultaneously available for experimentation.

@slindley
Copy link
Collaborator

Right. I think @dhil's point is that if there's another variation of stack-switching that somebody wants to implement in the reference interpreter (e.g. a bag-o-stacks variation) then it would probably be more convenient for people to experiment with if it was included alongside the WasmFX one so that it was easy to switch between the two.

Given that the WasmFX reference interpreter has been publicly available and pretty stable for the last 2-3 years, and as far as I'm aware there has been no other alternative stack-switching implementation in the reference interpreter in that time, it seems reasonable to at least consider merging it into main.

@fgmccabe
Copy link
Collaborator

It is not possible to have both wasm fx & another proposal available simultaneously in one folder; unless we use different branches.

@slindley
Copy link
Collaborator

It is possible. We could use a flag to switch between them, or if they use different instructions it should work for them to coexist.

Of course, it's a little hard to judge how viable that would be without seeing what this hypothetical alternative implementation looks like.

@fgmccabe
Copy link
Collaborator

It is possible (in principle) to use switches. However, modulo a change in the way that wasm specs are worked on (which I happen to champion), the most common way seems to be to have separate repos, (in effect, a disjunction rather than a conjunction with multiple designs).
In this case, I think that having branches is the best approach.
Personally, I don't want to stuff checked into the main repo; in part because I am actually working on a bag of stacks interpreter.

@dhil
Copy link
Member Author

dhil commented Apr 17, 2024

@fgmccabe I think having separate branches is suboptimal as it reduces visibility and makes it needlessly complicated for the curious user to play with it (i.e. the user drop off rate is likely to be affected negatively). This repository is one of the top results for the search query "webassembly stack switching", and as such, I think it should be as easy as 1 2 3 for a curious user to get started. In this regard, I think binaryen does it right: clone, build, play. I am strongly advocating for an approach where we have both proposals implemented in main.

I do not find either of your arguments against it convincing:

It is not possible to have both wasm fx & another proposal available simultaneously in one folder; unless we use different branches.
[...]
Personally, I don't want to stuff checked into the main repo; in part because I am actually working on a bag of stacks interpreter.

The implementations can coexist mostly without worrying about each other. The notable exception is the choice of opcodes. In this case, we can either organise the opcode space together, or we can use flags as suggested by @slindley.

@dead-claudia
Copy link

@fgmccabe @dhil Could you all not just do separate non-main branches and just link to them from the README or some other main-ish document?

That way, you all can do your thing, and other interested people could also fork off the main interpreter and do their own experiments without having to worry about stepping on your toes (or vice versa) either.

@titzer
Copy link
Contributor

titzer commented Apr 17, 2024

In the spirit of working together on a shared journey of discovery, I strongly support merging the WasmFX repo into this proposal repo. It is less confusing and fragmenting for the community but also more accurately reflects the current status of work in this space. In Wizard we've been working off the WasmFX repo, which isn't the case for any other proposal and certainly stands out.

@fgmccabe Let's please discuss weighty and important PRs like this one; I feel closing this was premature.

I'd like to feel that this subgroup is working together, even if we have some technical disagreements, and I think having the union of both proposals in the main branch, with different names and opcode assignments, would be the best way to reflect that.

@fgmccabe
Copy link
Collaborator

Apologies, did not mean to actually close the PR itself.

@fgmccabe fgmccabe reopened this Apr 19, 2024
@slindley slindley changed the base branch from main to wasmfx April 19, 2024 18:01
@slindley
Copy link
Collaborator

@fgmccabe and I have discussed this offline. We've agreed to have separate wasmfx and bag-o-stacks branches in this repo. I've added the wasmfx branch and changed the base branch for the PR to that.

@fgmccabe will add the bag-o-stacks branch when he's ready.

For now we'll avoid merging stack-switching functionality for the reference interpreter into the main branch - but we will merge it into the wasmfx and bag-o-stacks branches.

To be clear, I agree with @dhil that having both implementations in the same branch would be convenient for experimentation. However, @rossberg has also argued (offline) that having them both in the main branch here would likely be painful when it comes to trying to upstream the final stack-switching implementation to the offical repo, as we'd probably need to reverse quite a lot of changes.

@slindley slindley merged commit a1ccfe1 into WebAssembly:wasmfx Apr 22, 2024
12 checks passed
@dhil dhil deleted the wasmfx-ref-interp branch April 23, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@dhil @fgmccabe @tlively @slindley @dead-claudia @titzer @rossberg @ani003 @matijapretnar @takikawa