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

Proposal to remove unwind #153

Closed
tlively opened this issue May 11, 2021 · 4 comments · Fixed by #156
Closed

Proposal to remove unwind #153

tlively opened this issue May 11, 2021 · 4 comments · Fixed by #156

Comments

@tlively
Copy link
Member

tlively commented May 11, 2021

unwind is similar to catch_all and in the semantics we've settled on it is also strictly less expressive than catch_all. unwind can be defined in terms of catch_all and rethrow, so any program that uses unwind could equivalently use catch_all and rethrow instead.

unwind was originally meant to be a forward-compatibility feature bridging the gap between the current EH proposal and some future proposal for additional non-local control flow. My personal concern about this is that we have not discussed making an exception to WebAssembly's usual backwards compatibility guarantees for unwind, and it is not possible to both maintain backwards compatibility and allow for a graceful transition to different (currently unspecified) semantics.

Since unwind does not increase the expressiveness of the proposal and does not seem useful for other reasons such as forward compatibility, I propose that we simplify the proposal by removing it.

Does that sound reasonable, @aheejin, @RossTate, @rossberg, and @ioannad?

@rossberg
Copy link
Member

rossberg commented May 11, 2021

Thanks for bringing this up. Yes, that sounds reasonable to me. Any hypothetical addition of more forms of terminating non-local control transfer (if they're even desirable) would inevitably change the contextual observations of existing code, and thus invalidate assumptions it might have. Unwind can't prevent that -- nothing generally can AFAICT. But I don't think it ever was an assumption that this ought to be possible either. Combining code from different sources/runtimes will always require extra care and possible barriers, handling which is the purpose of e.g. the IT proposal.

Also, if we included a special feature along these lines, then it should at least be designed such that it can effectively express common constructs like finally clauses, with isn't the case with unwind (it would require code duplication that is exponential in the nesting depth of finally constructs). Otherwise, those constructs will likely be compiled to catch_all anyway, and any supposed forward compatibility is already moot.

Finally, unwind has unusual behaviour relative to the Wasm instruction set, since it would be the only block construct where end is not a control no-op but actually has rather non-trivial behaviour. That could be taken as an indication that there is a mismatch in abstraction level.

@lukewagner
Copy link
Member

Oh good; I had only recently noticed unwind (having missed the rather long discussion leading up to it) and I had the same immediate concern. Introducing two ways of doing the same thing today and trying to make one of them behave differently in the future seems guaranteed to fail: if I can't write tests confirming I'm doing the right or wrong thing today, then there's no chance the whole ecosystem is going to get things right for a hypothetical tomorrow.

@aheejin
Copy link
Member

aheejin commented May 11, 2021

I support it as well because of all the reasons I stated in the other discussion thread. I agree the peculiarity of end behavior is not very much in line with other wasm block-like instructions, and also we don't need to have two different ways to achieve the same thing in the current spec.

@eqrion
Copy link
Contributor

eqrion commented May 12, 2021

SpiderMonkey would also be in favor of deferring unwind. Primarily concerned about the feasibility of changing the behavior of unwind in the future in an open-ended way and expected it to be used correctly today, as Luke mentioned.

aheejin added a commit to aheejin/exception-handling that referenced this issue Jun 3, 2021
It looks people have generally agreed on removing `unwind` in the MVP
spec, as it overlaps with `catch_all` in functionality.

Fixes WebAssembly#153.
aheejin added a commit to aheejin/exception-handling that referenced this issue Jun 3, 2021
It looks people have generally agreed on removing `unwind` in the MVP
spec, as it overlaps with `catch_all` in functionality.

Fixes WebAssembly#153.
@aheejin aheejin mentioned this issue Jun 3, 2021
aheejin added a commit that referenced this issue Jun 11, 2021
It looks people have generally agreed on removing `unwind` in the MVP
spec, as it overlaps with `catch_all` in functionality.

Closes #153.
pull bot pushed a commit to p-g-krish/v8 that referenced this issue Jun 14, 2021
Relevant links:
WebAssembly/exception-handling#153
WebAssembly/exception-handling#156

R=clemensb@chromium.org

Bug: v8:8091
Change-Id: I0deeb9665c6648e643d0aa4f310b7676e1c2fa32
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2959624
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75135}
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 a pull request may close this issue.

5 participants