cranelift: stack-switching support #11003

posborne · 2025-06-10T17:47:43Z

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

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 <daniel.hillerstrom@ed.ac.uk> Co-authored-by: Paul Osborne <paul.osborne@fastly.com>

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.

crates/cranelift/src/stack_switching/instructions.rs

fitzgen

Thanks! Lots of comments below but for the most part these should be pretty straightforward to fix.

cranelift/codegen/src/ir/function.rs

crates/cranelift/src/stack_switching/mod.rs

crates/cranelift/src/func_environ.rs

crates/cranelift/src/translate/code_translator.rs

crates/cranelift/src/stack_switching/fatpointer.rs

crates/cranelift/src/stack_switching/instructions.rs

crates/cranelift/src/stack_switching/mod.rs

Co-authored-by: Daniel Hillerström <daniel.hillerstrom@ed.ac.uk>

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.

posborne · 2025-06-16T16:29:24Z

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.

posborne · 2025-08-27T19:19:55Z

@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 Val::null_top which currently panics for continuation types.

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.

posborne · 2025-08-27T22:54:18Z

@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 Val::null_top which currently panics for continuation types.

To get tests going with the upstream stuff, I did end up adding a minimal Val::ContRef member which doesn't seem avoidable. I tried to keep this as small and I could bundle it with the test changes, but it does appear to be required in order to have ref.null globals for continuations and some other cases.

github-actions · 2025-08-28T02:14:46Z

Subscribe to Label Action

cc @fitzgen

Details

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:

fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2025-08-28T17:44:49Z

Subscribe to Label Action

cc @fitzgen

Details

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:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

fitzgen

LGTM with the below addressed, thanks!

crates/wasmtime/src/runtime/externals/global.rs

crates/wasmtime/src/runtime/trampoline/global.rs

prtest:full

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.

fitzgen · 2025-08-29T18:19:19Z

crates/cranelift/src/func_environ/stack_switching/instructions.rs

+            // 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(),
+                    )
+                };


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).

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.

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.

Addressed in 9343ab9 if my interpretation was correct.

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 <daniel.hillerstrom@ed.ac.uk> Co-authored-by: Paul Osborne <paul.osborne@fastly.com> * 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 <daniel.hillerstrom@ed.ac.uk> * 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 <git@emrich.io> Co-authored-by: Daniel Hillerström <daniel.hillerstrom@ed.ac.uk>

frank-emrich and others added 7 commits June 10, 2025 16:17

cranelift: stack-switching updates pass 1

0853a16

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 fun…

858b22a

…c_environ members

cranelift: avoid "as" casts in stack-switching

100d621

Replace either with infallible From or fallible, panicing TryFrom alternatives where required.

cranelift: cleanup stack-switching control_effect signatures

00c2b56

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

e9fa92d

This matches a change to the mirrored runtime type in the upstream changes.

posborne requested review from a team as code owners June 10, 2025 17:47

posborne requested review from alexcrichton and removed request for a team June 10, 2025 17:47

posborne commented Jun 10, 2025

View reviewed changes

crates/cranelift/src/stack_switching/instructions.rs Outdated Show resolved Hide resolved

fitzgen requested review from fitzgen and removed request for alexcrichton June 10, 2025 18:13

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Jun 10, 2025

fitzgen reviewed Jun 11, 2025

View reviewed changes

dhil reviewed Jun 11, 2025

View reviewed changes

crates/cranelift/src/stack_switching/fatpointer.rs Outdated Show resolved Hide resolved

crates/cranelift/src/stack_switching/instructions.rs Outdated Show resolved Hide resolved

crates/cranelift/src/stack_switching/mod.rs Show resolved Hide resolved

posborne and others added 10 commits June 11, 2025 13:27

stack-switching: fix typo

866fbff

Co-authored-by: Daniel Hillerström <daniel.hillerstrom@ed.ac.uk>

stack-switching: used Index impl for get_stack_slot_data

115e503

stack-switching: use smallvec over vec in several cases

11c6b4a

stack-switching: avoid resumetable naming confusion

fa8001e

stack-switching: cleanup unused params from unchecked_get_continuation

51017f2

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

44c2b34

stack-switching: simplify translate_table_{grow,fill} control flow

b8fb8b3

stack-switching: remove translate_resume_throw stub

a693350

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

eeab1b1

stack-switching: VMHostArrayRef updates

3bd138f

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

49313bc

This matches the directory structure for gc and aids in visibility for a few members required by stack-switching code in cranelift.

posborne force-pushed the stack-switching-cranelift branch from 83b22a2 to 7d4e678 Compare August 27, 2025 22:51

github-actions bot added the wasmtime:ref-types Issues related to reference types and GC in Wasmtime label Aug 28, 2025

fixup! stack-switching: add stub Val::ContRef

e929cb7

posborne requested a review from a team as a code owner August 28, 2025 16:21

posborne requested review from alexcrichton and removed request for a team August 28, 2025 16:21

github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:c-api Issues pertaining to the C API. labels Aug 28, 2025

fitzgen approved these changes Aug 28, 2025

View reviewed changes

crates/wasmtime/src/runtime/externals/global.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/runtime/trampoline/global.rs Outdated Show resolved Hide resolved

fixup! stack-switching: add stub Val::ContRef

e258e35

posborne enabled auto-merge August 28, 2025 20:09

posborne added this pull request to the merge queue Aug 28, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 28, 2025

posborne added 2 commits August 28, 2025 23:39

fixup! stack-switching: define VMContObj as two words

5e24337

prtest:full

posborne force-pushed the stack-switching-cranelift branch from 7a29573 to ed4b010 Compare August 29, 2025 18:22

fitzgen requested changes Aug 29, 2025

View reviewed changes

stack-switching: VMHostArray entry sizes based off env PtrSize

9343ab9

Revisiting the previous commit with an approach that should be less brittle.

posborne requested a review from fitzgen September 4, 2025 19:54

fitzgen approved these changes Sep 4, 2025

View reviewed changes

posborne added this pull request to the merge queue Sep 4, 2025

Merged via the queue into bytecodealliance:main with commit a631d20 Sep 4, 2025
168 checks passed

dhil mentioned this pull request Sep 16, 2025

Tracking issue: Stack switching proposal implementation status #10248

Open

26 tasks

cranelift: stack-switching support #11003

cranelift: stack-switching support #11003

Uh oh!

Conversation

posborne commented Jun 10, 2025

Uh oh!

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

posborne commented Jun 16, 2025

Uh oh!

posborne commented Aug 27, 2025

Uh oh!

posborne commented Aug 27, 2025

Uh oh!

github-actions bot commented Aug 28, 2025

Subscribe to Label Action

Uh oh!

github-actions bot commented Aug 28, 2025

Subscribe to Label Action

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fitzgen Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

fitzgen Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

posborne Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

posborne Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

fitzgen Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants