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
-->
alexcrichton requested fitzgen for a review on PR #8152.
alexcrichton requested wasmtime-core-reviewers for a review on PR #8152.
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.
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>
fitzgen submitted PR review:
Looks great! Excited to remove the last vestiges of hand-written asm.
fitzgen submitted PR review:
Looks great! Excited to remove the last vestiges of hand-written asm.
fitzgen created PR review comment:
Should required be a set of some sort?
fitzgen created PR review comment:
/// * A wasm function calls a cranelift-compiled tramopline that's generated /// once-per-builtin.
fitzgen created PR review comment:
We could probably define a
fn builtin_name(builtin: BuiltinFunctionIndex) -> &'static str
method via the macros to get better names here, which could be useful in debugging/profiling.
alexcrichton submitted PR review.
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.
alexcrichton updated PR #8152.
alexcrichton updated PR #8152.
alexcrichton updated PR #8152.
alexcrichton updated PR #8152.
alexcrichton has marked PR #8152 as ready for review.
alexcrichton requested abrown for a review on PR #8152.
alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8152.
alexcrichton updated PR #8152.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
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
fitzgen submitted PR review.
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?
fitzgen submitted PR review.
fitzgen created PR review comment:
alexcrichton updated PR #8152.
alexcrichton requested wasmtime-default-reviewers for a review on PR #8152.
alexcrichton submitted PR review.
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.
alexcrichton updated PR #8152.
alexcrichton commented on PR #8152:
Ok various prerequisites are now landed on
main
and 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!
alexcrichton requested saulecabrera for a review on PR #8152.
alexcrichton requested elliottt for a review on PR #8152.
alexcrichton requested fitzgen for a review on PR #8152.
fitzgen submitted PR review.
elliottt submitted PR review.
elliottt created PR review comment:
/// * A wasm function calls a cranelift-compiled trampoline that's generated
saulecabrera submitted PR review:
Winch pieces look great to me, thanks!
saulecabrera submitted PR review:
Winch pieces look great to me, thanks!
saulecabrera created PR review comment:
Nice!
saulecabrera created PR review comment:
Do you mind updating the comment here and in
libcall
below?
elliottt submitted PR review:
Awesome!
alexcrichton updated PR #8152.
alexcrichton has enabled auto merge for PR #8152.
alexcrichton merged PR #8152.
Last updated: Dec 23 2024 at 12:05 UTC