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

wasi_unstable!proc_exit new trap type to force return to entry function 'nicely' #993

Closed
rene-fonseca opened this issue Feb 26, 2020 · 6 comments · Fixed by #1646
Closed

Comments

@rene-fonseca
Copy link

I was thinking about support for multiple WASM instances in the same host process. But ran into a problem with functions like wasi_unstable!proc_exit which are meant to kill the process rather than returning to the main entry function. Killing the process would not be desired in this case. A general use-case would be if a fork() feature was added which make use of the same host process to spawn copies of the running WASM instance - but when terminating an instance - it should then not impact the remaining instances. I'm looking for just running 'fully' independent instances in the same process for now, though.

But to make it possible to run multiple WASM instances - we really need a way to force a return to the instance entry function caller - and ensure the native function stack - but NOT the WASM stack - gets cleaned up. So I think we need a way to create a special termination/not-catchable trap - that just triggers jumps to the next native function call on the stack - but skipping any WASM frames for clean up. The new trap type ensures that WASM cannot recover from the termination request.

This does involve WASM exception handling and WASM C-lib. But I'll start here to see if this would be viable to solve for wasmtime.

@sunfishcode
Copy link
Member

Yes, this lines up with what we're thinking in WASI too: WebAssembly/WASI#26. And I think you're right, this is viable to solve now, with our current trap infrastructure, rather than waiting for full unwind support. Some of the the key pieces are raise_user_trap in traphandlers.rs for raising a trap, and the Trap enum where it probably makes sense to add an arm for explicit "exits", which can hold the exit status passed to proc_exit.

One potentially tricky bit is if there's wasm code calling native code which calls wasm code, then this kind of "exit" will only unwind until the native code, and not exit all wasm code. However, I think that's ok for now -- programs with extra exports that can be called from the outside don't fit into the usual "command" pattern anyway.

@rene-fonseca
Copy link
Author

Nice, great to see discussion is already going on.

@peterhuene
Copy link
Member

peterhuene commented Feb 26, 2020

Regarding wasm -> native host fn -> wasm -> trap: it'll unwind to the native host function, but the host function should receive a trap and must be expected to immediately return the trap to its caller to resume the trap, otherwise I think we should consider it undefined behavior (i.e. as if an "uncatchable" trap were "caught") and document this expectation of host functions accordingly.

Also, be aware that currently panics in Rust host functions (including Wasmtime's WASI implementation) are not converted to traps. So currently it is possible for one panicking instance to unwind and abort your process if the panic isn't handled at the root Func::call (or Instance::new if a start function caused a panic).

@peterhuene
Copy link
Member

peterhuene commented Feb 26, 2020

Also note: if you're using the C API, wasm_func_call will convert a panic to a trap because we're not allowed to unwind through foreign (not Rust or Wasm) frames. If you're using the Rust API, we will unwind a panic past Func::call.

@rene-fonseca
Copy link
Author

As long as it is impossible to trigger panic from within WASM context it should be safe, right.

Need to make sure std::terminate() is handled similar to proc_exit() - e.g. gets called when throwing exceptions during unwind for C++.

One other thing that might cause bigger problems is the potential for deadlocks if mutices are not properly released. WASM instance would need to manage the system locks and ensure they are released. Instead of just letting OS managing these. Its kind of equivalent to killing an OS thread - which has the same issue with unreleased resources - if problem is ignored.

@sunfishcode
Copy link
Member

In libcxxabi, std::terminate() ultimately calls abort(), which in WASI libc is currently implemented as a wasm unreachable instruction, which is a normal trap.

sunfishcode added a commit to sunfishcode/wasmtime that referenced this issue May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this issue May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this issue May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this issue May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this issue May 12, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this issue May 13, 2020
sunfishcode added a commit that referenced this issue May 13, 2020
…rocess. (#1646)

* Remove Cranelift's OutOfBounds trap, which is no longer used.

* Change proc_exit to unwind instead of exit the host process.

This implements the semantics in WebAssembly/WASI#235.

Fixes #783.
Fixes #993.

* Fix exit-status tests on Windows.

* Revert the wiggle changes and re-introduce the wasi-common implementations.

* Move `wasi_proc_exit` into the wasmtime-wasi crate.

* Revert the spec_testsuite change.

* Remove the old proc_exit implementations.

* Make `TrapReason` an implementation detail.

* Allow exit status 2 on Windows too.

* Fix a documentation link.

* Really fix a documentation link.
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.

3 participants