Support Func imports with zero shims
#839
Conversation
|
|
Oh I should clarify in that I didn't actually try I was initially hesitant to longjmp since there's some other trap state exposed, but now that I think more about it I think it would be fine to do so, so I'll try to get that implemented, surely makes more sense than a segfault :) |
alexcrichton
force-pushed
the
func-pointers
branch
from
1edac91
to
9886fac
Compare
There was a problem hiding this comment.
Overall, this looks good.
But it all depends on us compiling exported wasm functions with the system ABI, and I'm not sure that we actually always do that or not, and it is a little scary that safety of this relies on something that is so far away and also isn't commented or documented anywhere as far as I can tell. I think we should do something to improve this, but I'm not sure exactly what yet. Thinking.
|
That's a good point yeah. Currently it boils down to this line which indicates that the ABI for the function that we're creating has, at least if the function name does what it says, the native calling convention. There's certainly a lot of dots to connect here, though, and I doubt that this is 100% right just yet. I'd hope though that this is something we can continue to improve over time and fix through bug reports, this is in general a feature I think we want to work towards to enable fast imports by ensuring there are zero wasmtime-inserted shims. Currently there's a lot of indirection as arguments are moved through memory, vectors are allocated, etc. |
alexcrichton
force-pushed
the
func-pointers
branch
2 times, most recently
from
b61a393
to
a48865e
Compare
This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit bytecodealliance#839 and otherwise will... Closes bytecodealliance#848
This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit bytecodealliance#839 and otherwise will... Closes bytecodealliance#848
This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit bytecodealliance#839 and otherwise will... Closes bytecodealliance#848
This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit bytecodealliance#839 and otherwise will... Closes bytecodealliance#848
alexcrichton
force-pushed
the
func-pointers
branch
from
a48865e
to
b79bb78
Compare
This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit bytecodealliance#839 and otherwise will... Closes bytecodealliance#848
|
Ok after talking more with @sunfishcode I think this is ready to go once approved. To answer some of my original questions:
With @pepyakin's suggestion I feel much better about this. With #857 we're doing the strategy of an "explicit raise" function anyway, so I think this is the way to go.
ABI-wise we're good to go today. Cranelift's "default system ABI" does indeed match the default system for arguments, and so we should be covered in that regard. Multi-value is fundamentally not matched to the C ABI today, and would require other fixes. I'll file an issue shortly about this on the cranelift repository.
That's the review approval part :) |
|
I've opened https://github.com/bytecodealliance/cranelift/issues/1369 about cranelift support for multi-value for this feature. |
This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit bytecodealliance#839 and otherwise will... Closes bytecodealliance#848
This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit bytecodealliance#839 and otherwise will... Closes bytecodealliance#848
* Improve panics/traps from imported functions This commit performs a few refactorings and fixes a bug as well. The changes here are: * The `thread_local!` in the `wasmtime` crate for trap information is removed. The thread local in the `wasmtime_runtime` crate is now leveraged to transmit trap information. * Panics in user-provided functions are now caught explicitly to be carried across JIT code manually. Getting Rust panics unwinding through JIT code is pretty likely to be super tricky and difficult to do, so in the meantime we can get by with catching panics and resuming the panic once we've resumed in Rust code. * Various take/record trap apis have all been removed in favor of working directly with `Trap` objects, where the internal trap object has been expanded slightly to encompass user-provided errors as well. This borrows a bit #839 and otherwise will... Closes #848 * Rename `r#return` to `ret`
alexcrichton
force-pushed
the
func-pointers
branch
from
b79bb78
to
27407a5
Compare
| { | ||
| let ret = { | ||
| let instance = InstanceHandle::from_vmctx(vmctx); | ||
| let func = instance.host_state().downcast_ref::<F>().expect("state"); |
There was a problem hiding this comment.
Not to block the merge, but I wonder what would your thoughts be on this. Here and below, if this expect triggers a panic is raised and the unwinding will reach the FFI boundary. On one hand, it feels that this code is somewhat trusted and we certainly never trigger this assert ( : ) ) and not worth the hassle addressing this, but on the other hand "with UB anything is possible"
This commit extends the `Func` type in the `wasmtime` crate with static `wrap*` constructors. The goal of these constructors is to create a `Func` type which has zero shims associated with it, creating as small of a layer as possible between wasm code and calling imported Rust code. This is achieved by creating an `extern "C"` shim function which matches the ABI of what Cranelift will generate, and then the host function is passed directly into an `InstanceHandle` to get called later. This also enables enough inlining opportunities that LLVM will be able to see all functions and inline everything to the point where your function is called immediately from wasm, no questions asked.
alexcrichton
force-pushed
the
func-pointers
branch
from
a824d21
to
695289d
Compare
|
Sure thing, added a comment! I'm gonna go ahead and merge this once green and we can continue to iterate on it as necessary. |
This commit extends the
Functype in thewasmtimecrate with staticwrap*constructors. The goal of these constructors is to create aFunctype which has zero shims associated with it, creating as smallof a layer as possible between wasm code and calling imported Rust code.
This is achieved by creating an
extern "C"shim function which matchesthe ABI of what Cranelift will generate, and then the host function is
passed directly into an
InstanceHandleto get called later. This alsoenables enough inlining opportunities that LLVM will be able to see all
functions and inline everything to the point where your function is
called immediately from wasm, no questions asked.
There's a few items which I think need some more discussion on this PR before we land it, and I'm also just curious how others think of an API like this for embedding!
Figuring out how traps work. Right now
Result<T, Trap>is intended to be supported as a return from a Rust closure, but without generating a function shim it's not trivial to generate a trap. Currently the code sort of cops out by manually doing a segfault viawrite_volatile, but that feels pretty brittle. I'm curious if others have a good idea of how to trigger a trap from Rust code?Dealing with multi-value isn't supported yet. I talked with @fitzgen briefly but it sounds like we may not have a native ABI in Rust that matches up with multi-value returns. I'd ideally like to support Rust closures returning, for example,
(i32, i32)to automatically get wired up to a multi-value return import, but the ABI of that Rust function I don't think matches what Cranelift currently does. I suspect that this will require some work in Cranelift to (maybe?) add a new ABI that matches theextern "C"abi for the native platform.I'm also curious to largely just get some more eyes on this. I'm shooting for an extremely low overhead
Funcwrapper, but naturally there's a lot of possible unsafety here if things don't line up precisely. Close review would definitely be appreciated!I've wanted to do this for some time but recently I've been thinking that the
wasmtime-wasicrate should move over to thewasmtimeAPI, but to preserve the current functional semantics where it has zero shims at runtime I felt was important, so I wanted to try to pursue this style of creating a low-overheadFunc.