Change proc_exit to unwind the stack rather than exiting the host process.#1646
Conversation
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @bnjbvr, @kubkon, @peterhuene DetailsThis issue or pull request has been labeled: "cranelift", "wasi", "wasmtime:api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
I would personally prefer to keep imported functions having the signature Result<T, Trap> instead of anyhow::Error as the error payload. That feels more closely aligned with the wasm spec and avoids us having weird questions about how to interpret various kinds of errors from imported functions. (to me it feels pretty straightforward that returning any sort of "trap" results in a wasm trap immediately)
Could this instead be extended to avoid special-casing on proc_exit and changing the error type? Perhaps Trap could have a new constructor representing "wasm exited" or something like that? Basically I'm thinking we could soup up the Trap type to have more information inside it, and one of the variants of information is "the program requested an exit". That way the trap could propagate all the way up to the wasmtime CLI, which could catch the error, and then exit the process accordingly. The worst-case scenario would be handling the trap as a usual wasm trap, which typically results in killing the wasm anyway right now.
sunfishcode
force-pushed
the
sunfishcode/exit-unwind
branch
from
eb3837b to
b4aae4b
Compare
|
Thanks for the feedback @pchickey and @alexcrichton! I've now reworked the patch here to avoid making any changes to wiggle, and to go back to |
Subscribe to Label Actioncc @peterhuene DetailsThis issue or pull request has been labeled: "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Is the spec testsuite change perhaps accidental?
|
|
||
| if let TrapReason::I32Exit(status) = trap.reason() { | ||
| // On Windows, exit status 3 indicates an abort (see below), | ||
| // so just return 1 indicating a non-zero status. |
There was a problem hiding this comment.
This seems a bit odd to me, why not expose exiting with all sorts of error codes on Windows?
There was a problem hiding this comment.
(or put another way I don't fully understand the comment as to why Windows is special here)
There was a problem hiding this comment.
I don't have a strong sense of what Windows wants here; I just didn't want to overload exit status 3 as meaning both "program trapped" and "program explicitly exited with status 3".
Am I reading too much into the docs by assuming that using 3 for an abort is a meaningful convention?
pchickey
left a comment
pchickey
left a comment
There was a problem hiding this comment.
Thanks, this approach looks much better
This implements the semantics in WebAssembly/WASI#235. Fixes bytecodealliance#783. Fixes bytecodealliance#993.
sunfishcode
force-pushed
the
sunfishcode/exit-unwind
branch
from
03c9321 to
59e816d
Compare
4 participants
This allows programs which call
proc_exitto coexist with other programs in the same host process. This also implements the semantics in WebAssembly/WASI#235.Fixes #783.
Fixes #993.