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.
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.
pchickey submitted PR Review.
pchickey submitted PR Review.
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 justpanic!
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?
pchickey submitted PR Review.
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 onwasmtime
inlucet_runtime
, which would not be good.
alexcrichton submitted PR Review.
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 thanResult<T, Trap>
, which means that any arbitrary error type can be returned. Could we perhaps special case when the error isExit
and propagate that through, removing the need for an extra constructor here?
alexcrichton submitted PR Review.
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 typeNonZeroI32
?
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.
sunfishcode submitted PR Review.
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 plainextern "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.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
This code already depends on
wasmtime::Func::wrap
; how does that work in Lucet?
pchickey submitted PR Review.
pchickey created PR Review Comment:
I missed that in my first look, any dep inside
wasi-common
onwasmtime
(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)
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
@kubkon
define_struct
has the comment "This is a single-use macro intended to be used in thewasmtime-wasi
crate.". Do you know if that's intended to be the case fordefine_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.
kubkon submitted PR Review.
kubkon created PR Review Comment:
I’m pretty sure
wig
is fully internal toWasmtime
, and so isdefine_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 :-)
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.
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Should this perhaps just be deleted?
alexcrichton created PR Review Comment:
This seems a bit odd to me, why not expose exiting with all sorts of error codes on Windows?
alexcrichton created PR Review Comment:
Exposing an
enum
often makes pretty hard guarantees about representations which are tough to change. Could we perhaps retainfn message()
with a canned string for exit statuses, and then add an extra accessor returningOption<i32>
for reading the exit code?
alexcrichton created PR Review Comment:
(or put another way I don't fully understand the comment as to why Windows is special here)
pchickey submitted PR Review.
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.
pchickey submitted PR Review.
pchickey submitted PR Review.
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")
?
sunfishcode submitted PR Review.
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?
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.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Done.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Done
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Done
alexcrichton submitted PR Review.
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.
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.
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.
sunfishcode merged PR #1646.
Last updated: Dec 23 2024 at 12:05 UTC