Wasmtime GitHub notifications bot (Nov 25 2024 at 17:59): Wasmtime GitHub notifications bot (Nov 25 2024 at 17:59):alexcrichton opened PR #9675 from alexcrichton:move-traps-into-abi to bytecodealliance:main:
This commit updates the "array call" ABI of Wasmtime used to transition from wasm to the host to explicitly return a
boolindicating whether the call succeeded or not. Previously functions would implicitly unwind vialongjmpand thus no explicit checks were necessary. The burden of unwinding is now placed on Cranelift-compiled code itself instead of the caller.There are a few pieces of rationale for this change:
Primarily I was implementing initial integration of Pulley where the host
setjmpandlongjmpcannot be used to maintain the Pulley interpreter state. My initial plan for handling this was to handle traps a bit differently in Pulley where having direct access to whether a function trapped or not in the interpreter bytecode is easier to deal with.Additionally it's generally not safe to call
longjmpfrom Rust as it will not run on-stack destructors. This is ok today in the sense that we shouldn't have these in Wasmtime, but directly returning to compiled code improves our safety story here a bit where we just won't have to deal with the problem of skipping destructors.In the future I'd like to move away from
setjmpandlongjmpentirely in the host to a Cranelift-native solution. This change would be required for such a migration anyway so it's my hope to make such a Cranelift-based implementation easier in the future. This might be necessary, for example, when implementing theexception-handlingproposal for Wasmtime.Functionally this commit does not remove all usages of call-
longjmp-from-Rust. Notably all libcalls and builtins still use this helper in the trampolines generated in Rust. I plan on going through the libcalls and updating their ABIs and signatures to reflect this in follow-up commits. As a result a few preexisting functions that should go away are marked#[deprecated]for internal use in this commit. I'll be cleaning that up as follow-ups. For now this commit handles the "hard part" of host functions by ensuring that the newboolreturn value is plumbed in all the locations correctly.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Nov 25 2024 at 17:59):alexcrichton requested wasmtime-core-reviewers for a review on PR #9675.
Wasmtime GitHub notifications bot (Nov 25 2024 at 17:59):alexcrichton requested pchickey for a review on PR #9675.
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:02):alexcrichton commented on PR #9675:
I'll note that this has a bit of https://github.com/bytecodealliance/wasmtime/pull/9673 mixed in but doesn't depend on it.
Also to expand on:
I plan on going through the libcalls and updating their ABIs and signatures to reflect this in follow-up commits
The categories of libcalls are:
- Some libcalls, but not all, need to be able to raise traps. For example we support
memory.growraising a trap because of the embedder API but we don't support table-lazy-init raising a trap.- All libcalls, however, need to catch Rust panics and do something with them.
My plan is to update the ABI of all libcalls that return traps to return some encoding of the current return value plus whether it trapped. That'll suffice for catching panics and handling traps. For all other libcalls which can't actually trap my plan is to abort the process. I plan on doing this by removing the
catch_unwindentirely. That is no longer necessary for safety reasons in recent versions of Rust. That means that only libcalls which can raise a trap will have panics shepherded across them, and panics in other libcalls will abort the process.
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:04):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:37):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:46):pchickey submitted PR review:
Broad strokes looks good, though I'm not quite enough of a cranelift expert to catch anything subtle here.
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:46):pchickey created PR review comment:
Still a FIXME here and below in same file
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:46):pchickey created PR review comment:
FIXME
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:05):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:05):alexcrichton created PR review comment:
Ah yeah this is intentional where it's deferred to a future PR to handle the usages for libcalls
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:18):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:19):alexcrichton requested cfallin for a review on PR #9675.
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:28):cfallin submitted PR review:
Cranelift compilation bits look good to me.
Wasmtime GitHub notifications bot (Nov 26 2024 at 00:54):alexcrichton requested abrown for a review on PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 00:54):alexcrichton requested wasmtime-default-reviewers for a review on PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 00:54):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 01:15):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 01:23):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 03:02):github-actions[bot] commented on PR #9675:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "pulley", "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- fitzgen: pulley
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Nov 26 2024 at 15:38):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 15:43):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 15:55):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 16:05):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 18:14):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 21:52):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 22:00):alexcrichton updated PR #9675.
Wasmtime GitHub notifications bot (Nov 26 2024 at 22:44):alexcrichton merged PR #9675.
Last updated: Dec 23 2024 at 12:05 UTC