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

Support try-catch blocks in Asyncify. #5143

Closed
wants to merge 2 commits into from

Conversation

jashug
Copy link

@jashug jashug commented Oct 16, 2022

Previously, when the Asyncify pass ran on a wasm file that includes try blocks (in a position such that asyncify has to un/re-wind through those try blocks), an error would be produced:
WASM_UNREACHABLE("unexpected expression type");

This PR adds support for the try case in Asyncify.

Fixes #4470

I was not sure how to add tests for this feature to the repository; it runs without crashing and produces reasonable-looking output on one sample file I have. This feature deserves tests: if someone will explain how to add them I will.

This should have no effect on previously successful applications of Asyncify (the behavior change here is from a crash to producing code), but if the implementation is wrong could produce broken code.

Convert

(try
  (...)
 catch $i
  (...))

into

(try
  (...)
 catch $i
  (set tag 1))
(if (or (eq tag 1) (rewinding))
 (...))

Convert

(try
  (...)
 catch $i
  (...))

into

(try
  (...)
 catch $i
  (set tag 1))
(if (or (eq tag 1) (rewinding))
 (...))
@kripken
Copy link
Member

kripken commented Oct 17, 2022

Thanks for the PR @jashug !

I think this almost works, but I believe in a recent discussion with @aheejin and @tlively it came up that it isn't quite enough, since moving code out of the catch block means that rethrow would work differently.

@jashug
Copy link
Author

jashug commented Oct 17, 2022

Good point. I think converting all the rethrow instructions targeting a converted catch block into throw instructions would fix this? Extra data on the exception like a stack trace will be lost, but I don't see any way of preserving that in Asyncify.

Edit: except this doesn't work for rethrow in a catch_all block. In fact I can't see any way for Asyncify to work in the presence of rethrow in a catch_all block if the catch_all block can be paused before the rethrow: the exception needs to be saved but we can't get access to it.

@kripken
Copy link
Member

kripken commented Oct 18, 2022

One idea I had (not sure if it make sense) is to re-run the throw. We sort of do that for calls - we have to re-run calls so that the call stack is rewound, and only after that can we resume execution from there. So perhaps similarly we could re-run relevant throws, which would have the effect of recreating the "exception state"..?

A rethrow in an important catch_all block is a lost cause.

This mishandles rethrows in catch blocks when the exception has
arguments. Need to futz around with multivalues.
@tlively
Copy link
Member

tlively commented Oct 18, 2022

Every throw would have to throw the original value and also a full set of Asyncify unwind data describing how to get back to the throw (including the data for any previous throws). This data would have to be collected eagerly, since there would be no way to go back and collect it at the suspension point. I think it would be simpler to lower away EH entirely before applying the Asyncify transformation.

@jashug
Copy link
Author

jashug commented Oct 18, 2022

For catch_all, the tricky case is when an exception is thrown without a wasm tag to be caught, such as an exception thrown from an imported Javascript function. In that case, Javascript has to be the one to save the exception, since it might not be representable/savable in wasm. If all the exceptions come from wasm, they have exception tags to be caught, and we could convert catch_all into forall i. catch i.

If we could save catch_all exceptions on a Javascript stack data structure, we could "just" pass around the pair of Asyncify unwind data and that Javascript stack, and avoid having to get back to the throw. "Exception reference" types look like they might solve this problem, but I'm having trouble finding their specification.

WebAssembly/exception-handling#30

@tlively
Copy link
Member

tlively commented Oct 18, 2022

Yeah, exception references would have made this simpler, but they were removed from the final EH spec and do not exist.

@jashug
Copy link
Author

jashug commented Oct 18, 2022

I pushed a sketch dealing with rethrows in catch but not catch_all blocks, but I don't see a way to handle the full exception handling WebAssembly feature, so I'm closing this PR.

@jashug jashug closed this Oct 18, 2022
@kripken
Copy link
Member

kripken commented Oct 19, 2022

Makes sense. This is very hard to do in Asyncify. I guess the solutions are either

  1. Wait for wasm stack switching, which can replace Asyncify. That's testable now but will take time to be standardized.
  2. As @tlively said, I think it would be simpler to lower away EH entirely before applying the Asyncify transformation - we could write an EH lowering pass to wasm MVP.

@aheejin
Copy link
Member

aheejin commented Oct 19, 2022

How do you lower away EH? Do you mean removing landingpads like -fignore-exceptions so if there's an exception we just crash, or do you mean something else?

@tlively
Copy link
Member

tlively commented Oct 19, 2022

No, I mean a binaryen pass to lower away EH but preserve semantics as much as possible, probably by instrumenting functions to return an extra value signaling an exception and instrumenting all function calls to check that value. Exception payloads could go on a stack in a second memory or could be stored in GC data. This lowering could not possibly do the right thing for exceptions entering the module from the outside, but that might be ok for some use cases where the only exceptions are expected to come from the Wasm module itself.

@aheejin
Copy link
Member

aheejin commented Oct 19, 2022

You think we can handle rethrow too?

@tlively
Copy link
Member

tlively commented Oct 19, 2022

Yeah, I think so. If we take all the engine-internal state from the EH proposal and put it in a secondary memory or in GC objects, then we should have everything we need to implement rethrow correctly as well.

@aheejin
Copy link
Member

aheejin commented Oct 19, 2022

How can we access the engine-internal state from user code?

@tlively
Copy link
Member

tlively commented Oct 20, 2022

If we lower away EH in a Binaryen pass, there won't be any EH-internal state at runtime. It will all be stored in data locations introduced by the Binaryen pass instead.

@aheejin
Copy link
Member

aheejin commented Oct 20, 2022

To lower away EH in Binaryen, we need to access the engine-internal state to make rethrow work. I think it's what you said, no?

@tlively
Copy link
Member

tlively commented Oct 20, 2022

By “take all the engine-internal state from the EH proposal and put it in a secondary memory or in GC objects” I meant “lower EH operations so that what would have been engine-internal state at runtime will instead be user-level state in a secondary memory or in GC objects.”

@caiiiycuk
Copy link
Contributor

So, exceptions is not supported with asyncify?

@kripken
Copy link
Member

kripken commented Dec 13, 2022

Wasm exceptions don't work with Asyncify, correct. The older "emscripten exceptions" (using JS) do work.

Reopening as this is worth working on.

@kripken kripken reopened this Dec 13, 2022
@kripken
Copy link
Member

kripken commented Dec 13, 2022

Actually we should have an issue open for this, not a PR. Closing this, will open an issue.

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.

Support Asyncify + Try-Catch [Can't build web-gphoto2 with -fwasm-exceptions instead of -fexceptions]
5 participants