-
Notifications
You must be signed in to change notification settings - Fork 1.5k
cranelift: stack-switching support #11003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cranelift: stack-switching support #11003
Conversation
This initial commit represents the "pr2" base commit with minimal merge conflicts resolved. Due to OOB conflicts, this commit is not functional as-is, but using it as a base in order to allow for easier reviewing of the delta from this commit to what will be used for the PR against upstream. Co-authored-by: Daniel Hillerström <[email protected]> Co-authored-by: Paul Osborne <[email protected]>
This first set of changes updates the base pr in order to compiled and pass basic checks (compile, clippy, fmt) with the biggest part of the change being to eliminate injection of tracing/assertions in JIT'ed code.
…c_environ members
At this point, the only bit we really branch on is what we do in order to avoid problems tying into wasmtime_environ. This is basd on the approach and macro used by the gc code for converting presence/absence of the cranelift feature flag to cranelift compile time. This is a bit of a half-measure for now as we still compile most stack-switching code in cranelift, but this does enough to avoid causing problems with missing definitions in wasmtime_environ.
Replace either with infallible From or fallible, panicing TryFrom alternatives where required.
After removing emission of runtime trace logging and assertions, there were several unused parameters. Remove those from the ControlEffect signatures completely.
This matches a change to the mirrored runtime type in the upstream changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Lots of comments below but for the most part these should be pretty straightforward to fix.
Co-authored-by: Daniel Hillerström <[email protected]>
The extra parameters here used to be used for emitting runtime assertions, but with those gone we just had unused params and lifetimes, clean those out.
There's already a stub elsewhere and this is not called, when exceptions are added and it is time to revisit, this method can be restored.
Rename VMHostArray -> VMHostArrayRef Change impl to compute address with offset upfront rather than on each load.
Pushed most of the updates but still need to make the suggested updates around the fat pointer stuff and resolve conflicts with upstream. |
This matches the directory structure for gc and aids in visibility for a few members required by stack-switching code in cranelift.
@fitzgen I did run the test changes now -- a subset of those tests are now failing as it looks like there are some codepaths hit due to upstream changes where we end up calling |
This type is not fully complete until continuation/gc integration is revisited (bytecodealliance#10248) but without these changes, test cases are now failing on panics as we need some representation of continuation references in the runtime Val enumeration. Runtime errors with TODO notes are added for the stubbed code paths to revisit later.
83b22a2
to
7d4e678
Compare
To get tests going with the upstream stuff, I did end up adding a minimal |
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "wasmtime:api", "wasmtime:ref-types"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing", "wasmtime:c-api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the below addressed, thanks!
Disas tests were failing on i686 targeting x86_64 as the size of the host pointer was leaking into what we were using to do codegen in a few paths. This patch is a bit of a hack as it seems like using a generic <T> for T: *mut u8 (as an example) is a bit questionable. To keep things small, I do a hacky typecheck to map pointers to the target pointer size here.
7a29573
to
ed4b010
Compare
// TODO: hack around host pointer size being mixed up with target | ||
let (align, entry_size) = | ||
if core::any::TypeId::of::<T>() == core::any::TypeId::of::<*mut u8>() { | ||
(env.pointer_type().bytes() as u8, env.pointer_type().bytes()) | ||
} else { | ||
( | ||
u8::try_from(std::mem::align_of::<T>()).unwrap(), | ||
u32::try_from(std::mem::size_of::<T>()).unwrap(), | ||
) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff should be a method on the PtrSize
trait that we define in vmoffsets.rs
(maybe fn vm_host_array_layout(&self) -> (u8, u32)
) and ultimately we should have access to that method here via env.offsets.ptr
.
We should definitely avoid any kind of std::mem::{size,align}_of
calls, as that ends up being pretty fragile (doesn't work for f64
across 32- and 64-bit architectures, for example, where the alignment is different despite size being the same).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify a little more:
To avoid the std::mem::{size,align}_of
calls, you may have to have different PtrSize
methods for each concrete T
that is used, and then bound T
by T: HostArrayLayout
and define
trait HostArrayLayout {
fn host_array_layout<P: PtrSize>(p: &P) -> (u8, u32);
}
and implement that for each concreate T
such that it calls the appropriate PtrSize
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll take a look at making changes to revise VMHostArray and any other problematic parts I can find (or see if @dhil can help out). I wanted to get a basic version of the commit up to get this kind of feedback as doing operations on the generic T here (from the original impl) seemed problematic for the reasons you describe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 9343ab9 if my interpretation was correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Revisiting the previous commit with an approach that should be less brittle.
* cranelift: stack-switching support This initial commit represents the "pr2" base commit with minimal merge conflicts resolved. Due to OOB conflicts, this commit is not functional as-is, but using it as a base in order to allow for easier reviewing of the delta from this commit to what will be used for the PR against upstream. Co-authored-by: Daniel Hillerström <[email protected]> Co-authored-by: Paul Osborne <[email protected]> * cranelift: stack-switching updates pass 1 This first set of changes updates the base pr in order to compiled and pass basic checks (compile, clippy, fmt) with the biggest part of the change being to eliminate injection of tracing/assertions in JIT'ed code. * cranelift: stack-switching: restore original visibility for a few func_environ members * cranelift: stack-switching conditional compilation At this point, the only bit we really branch on is what we do in order to avoid problems tying into wasmtime_environ. This is basd on the approach and macro used by the gc code for converting presence/absence of the cranelift feature flag to cranelift compile time. This is a bit of a half-measure for now as we still compile most stack-switching code in cranelift, but this does enough to avoid causing problems with missing definitions in wasmtime_environ. * cranelift: avoid "as" casts in stack-switching Replace either with infallible From or fallible, panicing TryFrom alternatives where required. * cranelift: cleanup stack-switching control_effect signatures After removing emission of runtime trace logging and assertions, there were several unused parameters. Remove those from the ControlEffect signatures completely. * cranelift: rename stack-switching VMArray to VMHostArray This matches a change to the mirrored runtime type in the upstream changes. * stack-switching: fix typo Co-authored-by: Daniel Hillerström <[email protected]> * stack-switching: used Index impl for get_stack_slot_data * stack-switching: use smallvec over vec in several cases * stack-switching: avoid resumetable naming confusion * stack-switching: cleanup unused params from unchecked_get_continuation The extra parameters here used to be used for emitting runtime assertions, but with those gone we just had unused params and lifetimes, clean those out. * stack_switching: simplify store_data_entries assertion * stack-switching: simplify translate_table_{grow,fill} control flow * stack-switching: remove translate_resume_throw stub There's already a stub elsewhere and this is not called, when exceptions are added and it is time to revisit, this method can be restored. * stack-switching: compute control_context_size based on target triple * stack-switching: VMHostArrayRef updates Rename VMHostArray -> VMHostArrayRef Change impl to compute address with offset upfront rather than on each load. * stack-switching: move cranelift code to live under func_environ This matches the directory structure for gc and aids in visibility for a few members required by stack-switching code in cranelift. * stack-switching: formatting fix * stack-switching: reduce visibility on a few additional items * stack-switching: simplify contobj fatptr con/de-struction * stack-switching: add disas tests to cover new instructions * stack-switching: fix layout of VMContObj In the course of the various runtime updates, the layout of the runtime VMContObj got switched around. This resulted in failures when doing certain table operations on continuations. This change fixes that layout problem and adds some tests with offsets to avoid the problem. Due to the way that we interact with the VMContObj in cranelift, we don't use these offsets outside of the tests. * Fix formatting of merge conflict resolution * cranelift: remove ir::function::get_stack_slot_data This method isn't required as sized_stack_slots is already pub. * stack-switching: reduce visibility of a couple func_environ methods * stack-switching: define VMContObj as two words This change migrates VMContObj and its usages in cranelift and runtime to work with the VMContObj fat pointer as two words in order to better target different architectures (still gated to x86_64 for now). To support this, a size type was plumbed into the builtins function signature types (as is done for component types) that maps to usize. * fixup! stack-switching: define VMContObj as two words * stack-switching: add stub Val::ContRef This type is not fully complete until continuation/gc integration is revisited (bytecodealliance#10248) but without these changes, test cases are now failing on panics as we need some representation of continuation references in the runtime Val enumeration. Runtime errors with TODO notes are added for the stubbed code paths to revisit later. * fixup! stack-switching: add stub Val::ContRef * fixup! stack-switching: add stub Val::ContRef * fixup! stack-switching: define VMContObj as two words prtest:full * stack-switching: don't conflate host and target pointer sizes Disas tests were failing on i686 targeting x86_64 as the size of the host pointer was leaking into what we were using to do codegen in a few paths. This patch is a bit of a hack as it seems like using a generic <T> for T: *mut u8 (as an example) is a bit questionable. To keep things small, I do a hacky typecheck to map pointers to the target pointer size here. * stack-switching: VMHostArray entry sizes based off env PtrSize Revisiting the previous commit with an approach that should be less brittle. --------- Co-authored-by: Frank Emrich <[email protected]> Co-authored-by: Daniel Hillerström <[email protected]>
These changes pull in the cranelift changes from #10177 with some additional stacked changes to resolve conflicts, align with previous changes in the stack-switching series, and address feedback items which were raised on previous iterations of the PR (but mostly not changing anything of significant substance). Tracking Issue: #10248.
The
stack-switching
feature flag is retained and used minimally in this changeset in order to avoid compilation problems, but not really used beyond that. There is at least one item in the tracking issue already related to likely finding a way to drop these compilation flags in most places but I think it is worth deferring that here as it will required touching code more broadly.CC @frank-emrich @dhil