Wasmtime GitHub notifications bot (Mar 15 2024 at 20:34): Wasmtime GitHub notifications bot (Mar 15 2024 at 20:34):alexcrichton opened PR #8152 from alexcrichton:builtin-trampolines-through-cranelift to bytecodealliance:main:
This commit changes how builtin functions in Wasmtime (think
memory.grow) are implemented. These functions are required to exit through some manner of trampoline to handle runtime requirements for backtracing right now. Currently this is done via inline assembly for each architecture (or external assembly for s390x). This is a bit unfortunate as it's a lot of hand-coding and making sure everything is right, and it's not easy to update as it's multiple platforms to update.The change in this commit is to instead use Cranelift-generated trampolines for this purpose instead. The path for invoking a builtin function now looks like:
- Wasm code calls a statically known symbol for each builtin.
- The statically known symbol will perform exit trampoline duties (e.g. pc/fp/etc) and then load a function pointer to the host implementation.
- The host implementation is invoked and then proceeds as usual.
The main new piece for this PR is that all wasm modules and functions are compiled in parallel but an output of this compilation phase is what builtin functions are required. All builtin functions are then unioned together into one set and then anything required is generated just afterwards. That means that only one builtin-trampoline per-module is generated per-builtin.
This work is inspired by #8135 and my own personal desire to have as much about our ABI details flowing through Cranelift as we can. This in theory makes it more flexible to deal with future improvements to our ABI.
<!--
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 (Mar 15 2024 at 20:34):alexcrichton requested fitzgen for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 15 2024 at 20:34):alexcrichton requested wasmtime-core-reviewers for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 15 2024 at 20:35):alexcrichton commented on PR #8152:
I'll note that I'm opening this up as a draft because Winch doesn't work yet. I'll need to update invocation of those libcalls in addition to updating how trampolines are required. I plan on waiting until https://github.com/bytecodealliance/wasmtime/pull/8109 lands to fully implement that though to avoid implementing this new trampoline in Winch.
Wasmtime GitHub notifications bot (Mar 15 2024 at 20:44):github-actions[bot] commented on PR #8152:
Subscribe to Label Action
cc @peterhuene, @saulecabrera
<details>
This issue or pull request has been labeled: "wasmtime:api", "winch"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
- saulecabrera: winch
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 (Mar 18 2024 at 18:34):fitzgen submitted PR review:
Looks great! Excited to remove the last vestiges of hand-written asm.
Wasmtime GitHub notifications bot (Mar 18 2024 at 18:34):fitzgen submitted PR review:
Looks great! Excited to remove the last vestiges of hand-written asm.
Wasmtime GitHub notifications bot (Mar 18 2024 at 18:34):fitzgen created PR review comment:
Should required be a set of some sort?
Wasmtime GitHub notifications bot (Mar 18 2024 at 18:34):fitzgen created PR review comment:
/// * A wasm function calls a cranelift-compiled tramopline that's generated /// once-per-builtin.
Wasmtime GitHub notifications bot (Mar 18 2024 at 18:34):fitzgen created PR review comment:
We could probably define a
fn builtin_name(builtin: BuiltinFunctionIndex) -> &'static strmethod via the macros to get better names here, which could be useful in debugging/profiling.
Wasmtime GitHub notifications bot (Mar 19 2024 at 21:36):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 19 2024 at 21:36):alexcrichton created PR review comment:
I think I'll actually refactor and remove this since I realized it's also present via relocations, so I'll handle this when I rebase over the winch trampolines going through Cranelift.
Wasmtime GitHub notifications bot (Mar 21 2024 at 17:56):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 21 2024 at 17:59):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 21 2024 at 18:02):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 21 2024 at 18:03):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 21 2024 at 18:04):alexcrichton has marked PR #8152 as ready for review.
Wasmtime GitHub notifications bot (Mar 21 2024 at 18:04):alexcrichton requested abrown for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 21 2024 at 18:04):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:40):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:40):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:40):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:40):alexcrichton created PR review comment:
@fitzgen do you remember why this was here? The new builtin functions trip this assertion and I couldn't find anything that broke if I remove this so I didn't try to hard to fix it, but wanted to flag this regardless
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:44):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:44):fitzgen created PR review comment:
I think because we rely on the order for when we resolve relocs? So if they are out of order, then we could resolve relocs to the wrong function, I think?
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:46):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2024 at 19:46):fitzgen created PR review comment:
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:00):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:00):alexcrichton requested wasmtime-default-reviewers for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:06):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:06):alexcrichton created PR review comment:
Ok if that was historically the case then that's no longer needed. Definitely makes sense the order maps to the layout in the object itself, but for relocations order no longer matters since all relocations are tracked by an ID that's not an index.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:06):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:08):alexcrichton commented on PR #8152:
Ok various prerequisites are now landed on
mainand I've additionally implemented this for Winch too. The Winch changes are pretty nontrivial, so I'm going to request review from @saulecabrera and @elliottt, so if one of y'all could look at those and make sure I didn't do anything too weird it'd be much appreciated!
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:08):alexcrichton requested saulecabrera for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:08):alexcrichton requested elliottt for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:09):alexcrichton requested fitzgen for a review on PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:49):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:59):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 22 2024 at 15:59):elliottt created PR review comment:
/// * A wasm function calls a cranelift-compiled trampoline that's generated
Wasmtime GitHub notifications bot (Mar 22 2024 at 16:46):saulecabrera submitted PR review:
Winch pieces look great to me, thanks!
Wasmtime GitHub notifications bot (Mar 22 2024 at 16:46):saulecabrera submitted PR review:
Winch pieces look great to me, thanks!
Wasmtime GitHub notifications bot (Mar 22 2024 at 16:46):saulecabrera created PR review comment:
Nice!
Wasmtime GitHub notifications bot (Mar 22 2024 at 16:46):saulecabrera created PR review comment:
Do you mind updating the comment here and in
libcallbelow?
Wasmtime GitHub notifications bot (Mar 22 2024 at 17:06):elliottt submitted PR review:
Awesome!
Wasmtime GitHub notifications bot (Mar 22 2024 at 20:09):alexcrichton updated PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 20:10):alexcrichton has enabled auto merge for PR #8152.
Wasmtime GitHub notifications bot (Mar 22 2024 at 21:00):alexcrichton merged PR #8152.
Last updated: Nov 22 2024 at 17:03 UTC