Stream: git-wasmtime

Topic: wasmtime / PR #1646 Change `proc_exit` to unwind the stac...

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2020 at 23:19):

sunfishcode opened PR #1646 from sunfishcode/exit-unwind to master:

This allows programs which call proc_exit to coexist with other programs in the same host process. This also implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2020 at 23:50):

sunfishcode updated PR #1646 from sunfishcode/exit-unwind to master:

This allows programs which call proc_exit to coexist with other programs in the same host process. This also implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2020 at 23:54):

pchickey submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2020 at 23:54):

pchickey submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2020 at 23:54):

pchickey created PR Review Comment:

This would break Lucet, where we deal with proc_exit in a totally different manner. We install a panic handler at the wasm->host boundary that terminates the instance, and then just panic! in our proc_exit trait method.

If that doesn't work for wasmtime, can we figure out a way to deal with noreturn functions in wiggle that works for both runtimes?

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2020 at 23:59):

pchickey submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2020 at 23:59):

pchickey created PR Review Comment:

Even though we're not exposing the wig generated snapshot 0 code to users over in the lucet embedding, this would make us take a dep on wasmtime in lucet_runtime, which would not be good.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:32):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:32):

alexcrichton created PR Review Comment:

I'm curious if we can find a bit of a cleaner API to export this?

It looks like host functions have been updated to return Result<T> rather than Result<T, Trap>, which means that any arbitrary error type can be returned. Could we perhaps special case when the error is Exit and propagate that through, removing the need for an extra constructor here?

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:32):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:32):

alexcrichton created PR Review Comment:

This feels like it's somewhat dangerous here to me, should this be a panic or should status already have the type NonZeroI32?

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:32):

alexcrichton created PR Review Comment:

Given that we only really have "trusted" code calling this, could this be a panic or something more general? This seems a bit low-level to be dealing with WASI-specific codes and things.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:50):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:50):

sunfishcode created PR Review Comment:

I considered using panics here, but didn't because of the possibility of users embedding Wasmtime in programs built with panic = "abort".

My main idea is that proc_exit doesn't access memory, take any strings, use any handles, etc., so it doesn't need any of the wig machinery. It can just be a plain extern "C" function somewhere, and different runtimes can still do different things in the implementation -- panic as Lucet does, or trigger a trap-like unwind as this PR does. And that way, it can trap without having to worry about unwinding through wig's Rust code.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:51):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2020 at 20:51):

sunfishcode created PR Review Comment:

This code already depends on wasmtime::Func::wrap; how does that work in Lucet?

view this post on Zulip Wasmtime GitHub notifications bot (May 05 2020 at 22:40):

pchickey submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 05 2020 at 22:40):

pchickey created PR Review Comment:

I missed that in my first look, any dep inside wasi-common on wasmtime (or anything besides wig/wiggle) would be a showstopper for integration with Lucet. (I'm on vacation this week and will not be around much, just popped in from a lack of discipline)

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 05:34):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 05:34):

sunfishcode created PR Review Comment:

@kubkon define_struct has the comment "This is a single-use macro intended to be used in the wasmtime-wasi crate.". Do you know if that's intended to be the case for define_struct_for_wiggle? define_struct_for_wiggle currently has Wasmtime-specific code in it, so if it's meant to be usable outside of Wasmtime, we'll need to figure out how to factor that out.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 05:53):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 05:53):

kubkon created PR Review Comment:

I’m pretty sure wig is fully internal to Wasmtime, and so is define_struct_for_wiggle. As far as I remember, lucet had their own set of magical macros, but @pchickey can correct me here if I’m wrong :-)

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 14:28):

sunfishcode updated PR #1646 from sunfishcode/exit-unwind to master:

This allows programs which call proc_exit to coexist with other programs in the same host process. This also implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:12):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:12):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:12):

alexcrichton created PR Review Comment:

Should this perhaps just be deleted?

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:12):

alexcrichton created PR Review Comment:

This seems a bit odd to me, why not expose exiting with all sorts of error codes on Windows?

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:12):

alexcrichton created PR Review Comment:

Exposing an enum often makes pretty hard guarantees about representations which are tough to change. Could we perhaps retain fn message() with a canned string for exit statuses, and then add an extra accessor returning Option<i32> for reading the exit code?

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:12):

alexcrichton created PR Review Comment:

(or put another way I don't fully understand the comment as to why Windows is special here)

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:50):

pchickey submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:50):

pchickey created PR Review Comment:

There's no dep on this code in Lucet, so if Wasmtime doesn't intend to use it, lets delete it.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:53):

pchickey submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:53):

pchickey submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 18:53):

pchickey created PR Review Comment:

If both wasmtime and lucet override this implementation, maybe we could instead replace it with unimplemented!("runtimes are expected to override this implementation")?

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:46):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:46):

sunfishcode created PR Review 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?

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:52):

sunfishcode updated PR #1646 from sunfishcode/exit-unwind to master:

This allows programs which call proc_exit to coexist with other programs in the same host process. This also implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:54):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:54):

sunfishcode created PR Review Comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:54):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:54):

sunfishcode created PR Review Comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:55):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 19:55):

sunfishcode created PR Review Comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 21:24):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2020 at 21:26):

sunfishcode updated PR #1646 from sunfishcode/exit-unwind to master:

This allows programs which call proc_exit to coexist with other programs in the same host process. This also implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2020 at 20:06):

sunfishcode updated PR #1646 from sunfishcode/exit-unwind to master:

This allows programs which call proc_exit to coexist with other programs in the same host process. This also implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2020 at 22:02):

sunfishcode updated PR #1646 from sunfishcode/exit-unwind to master:

This allows programs which call proc_exit to coexist with other programs in the same host process. This also implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2020 at 22:59):

sunfishcode merged PR #1646.

Last updated: Nov 22 2024 at 17:03 UTC