wasmtime / PR #10177 Support for stack switching proposal · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10177 Support for stack switching proposal

Wasmtime GitHub notifications bot (Feb 03 2025 at 18:37):

frank-emrich opened PR #10177 from dhil:stack-switching to bytecodealliance:main:

This PR adds support for the Wasm stack switching proposal. The explainer document for the proposal is here.

This means that the following instructions are added: cont.new, resume, suspend, cont.bind.
The instruction resume.throw, which is part of the proposal but relies on exception handling, is currently unsupported.

This was developed together with @dhil.

General implementation notes

In Wasm, continuations are represented by values of type (ref $ct), where $ct is a new composite type/heap type for continuations.
In the implementation, these are represented by values of type VMContObj. These are fat pointers, consisting of a sequence counter, and a pointer to a VMContRef. The latter type is used for the actual representation of continuations.
The sequence counter part of VMContObjs is used to check that every continuation value can only be used once.

The StoreOpaque is extended to contain a "stack chain": It indicates what stack we are currently executing on. Logically, this is a linked list of stacks, since each continuation has a parent field. The chain stored in the StoreOpaque always ends with a value representing the initial stack. This is the stack we were on when entering Wasm, which will usually be the main stack.

Memory Management

Currently, memory management is very basic: The StoreOpaque provides a new method for allocation a new VMContRef, and keeps track of all continuations created this way. Continuations are never deallocated at the moment, meaning that they live until the store itself is deallocated.

The stack memory used by each allocation (represented by type ContinuationStack) is always mmap-ed, with a guard page at the end. There is currently no support for growing stacks, or segmented stacks.

Backtrace generation

The existing backtrace generation mechanism is extended to be fully aware of continuations: When generating backtraces, the entire chain of continuation is traversed, not just the stack frames of the currently active stack/continuation.

Integration with GC

Integration with the GC proposal is limited, but should at least be safe:

Programs where continuation objects would be stored inside GC objects (structs, arrays, ...) are rejected

During GC, all stack frames of all continuations are inspected when looking for GC roots.

Entering/Exiting Wasm

Before this PR, there are two separate mechanism that save and restore some state of the runtime when entering/exiting Wasm:

The functions enter_wasm and exit_wasm in func.rs

CallThreadState saves and restores (on drop) parts of the VMRuntimeLimits

This PR consolidates these two mechanism, because it requires some additional state to be updated and restored on enter/exit:
the type wasmtime::runtime::func::RuntimeEntryState now stores all of the required runtime state and ensures that it's restored when exiting Wasm.

Tags

Since the stack switching proposal extends the notion of tags introduced by the exception handling proposal, it adds the necessary machinery for importing, exporting, and defining tags.

Wasmtime GitHub notifications bot (Feb 03 2025 at 19:01):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 03 2025 at 19:05):

frank-emrich edited PR #10177:

This PR adds support for the Wasm stack switching proposal. The explainer document for the proposal is here.

This means that the following instructions are added: cont.new, resume, suspend, cont.bind.
The instruction resume.throw, which is part of the proposal but relies on exception handling, is currently unsupported.

This was developed together with @dhil.

General implementation notes

In Wasm, continuations are represented by values of type (ref $ct), where $ct is a new composite type/heap type for continuations.
In the implementation, these are represented by values of type VMContObj. These are fat pointers, consisting of a sequence counter, and a pointer to a VMContRef. The latter type is used for the actual representation of continuations.
The sequence counter part of VMContObjs is used to check that every continuation value can only be used once.

The StoreOpaque is extended to contain a "stack chain": It indicates what stack we are currently executing on. Logically, this is a linked list of stacks, since each continuation has a parent field. The chain stored in the StoreOpaque always ends with a value representing the initial stack. This is the stack we were on when entering Wasm, which will usually be the main stack.

Memory Management

Currently, memory management is very basic: The StoreOpaque provides a new method for allocation a new VMContRef, and keeps track of all continuations created this way. Continuations are never deallocated at the moment, meaning that they live until the store itself is deallocated.

The stack memory used by each allocation (represented by type ContinuationStack) is always mmap-ed, with a guard page at the end. There is currently no support for growing stacks, or segmented stacks.

Backtrace generation

The existing backtrace generation mechanism is extended to be fully aware of continuations: When generating backtraces, the entire chain of continuation is traversed, not just the stack frames of the currently active stack/continuation.

Integration with GC

Integration with the GC proposal is limited, but should at least be safe:

Programs where continuation objects would be stored inside GC objects (structs, arrays, ...) are rejected

During GC, all stack frames of all continuations are inspected when looking for GC roots.

Entering/Exiting Wasm

Prior to this PR, there were two separate mechanisms that save and restore some state of the runtime when entering/exiting Wasm:

The functions enter_wasm and exit_wasm in func.rs

CallThreadState saves and restores (on drop) parts of the VMRuntimeLimits

This PR consolidates these two mechanism, because it requires some additional state to be updated and restored on enter/exit:
the type wasmtime::runtime::func::RuntimeEntryState now stores all of the required runtime state and ensures that it's restored when exiting Wasm.

Tags

Since the stack switching proposal extends the notion of tags introduced by the exception handling proposal, it adds the necessary machinery for importing, exporting, and defining tags.

Wasmtime GitHub notifications bot (Feb 03 2025 at 20:44):

github-actions[bot] commented on PR #10177:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "wasmtime:c-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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 04 2025 at 10:21):

frank-emrich edited PR #10177:

This PR adds support for the Wasm stack switching proposal. The explainer document for the proposal is here.

This means that the following instructions are added: cont.new, resume, suspend, switch, and cont.bind.
The instruction resume.throw, which is part of the proposal but relies on exception handling, is currently unsupported.

This was developed together with @dhil.

General implementation notes

In Wasm, continuations are represented by values of type (ref $ct), where $ct is a new composite type/heap type for continuations.
In the implementation, these are represented by values of type VMContObj. These are fat pointers, consisting of a sequence counter, and a pointer to a VMContRef. The latter type is used for the actual representation of continuations.
The sequence counter part of VMContObjs is used to check that every continuation value can only be used once.

The StoreOpaque is extended to contain a "stack chain": It indicates what stack we are currently executing on. Logically, this is a linked list of stacks, since each continuation has a parent field. The chain stored in the StoreOpaque always ends with a value representing the initial stack. This is the stack we were on when entering Wasm, which will usually be the main stack.

Memory Management

Currently, memory management is very basic: The StoreOpaque provides a new method for allocation a new VMContRef, and keeps track of all continuations created this way. Continuations are never deallocated at the moment, meaning that they live until the store itself is deallocated.

The stack memory used by each allocation (represented by type ContinuationStack) is always mmap-ed, with a guard page at the end. There is currently no support for growing stacks, or segmented stacks.

Backtrace generation

The existing backtrace generation mechanism is extended to be fully aware of continuations: When generating backtraces, the entire chain of continuation is traversed, not just the stack frames of the currently active stack/continuation.

Integration with GC

Integration with the GC proposal is limited, but should at least be safe:

Programs where continuation objects would be stored inside GC objects (structs, arrays, ...) are rejected

During GC, all stack frames of all continuations are inspected when looking for GC roots.

Entering/Exiting Wasm

Prior to this PR, there were two separate mechanisms that save and restore some state of the runtime when entering/exiting Wasm:

The functions enter_wasm and exit_wasm in func.rs

CallThreadState saves and restores (on drop) parts of the VMRuntimeLimits

This PR consolidates these two mechanism, because it requires some additional state to be updated and restored on enter/exit:
the type wasmtime::runtime::func::RuntimeEntryState now stores all of the required runtime state and ensures that it's restored when exiting Wasm.

Tags

Since the stack switching proposal extends the notion of tags introduced by the exception handling proposal, it adds the necessary machinery for importing, exporting, and defining tags.

Wasmtime GitHub notifications bot (Feb 04 2025 at 14:55):

frank-emrich edited PR #10177:

This PR adds support for the Wasm stack switching proposal. The explainer document for the proposal is here.

This means that the following instructions are added: cont.new, resume, suspend, switch, and cont.bind.
The instruction resume.throw, which is part of the proposal but relies on exception handling, is currently unsupported.

This was developed together with @dhil.

General implementation notes

In Wasm, continuations are represented by values of type (ref $ct), where $ct is a new composite type/heap type for continuations.
In the implementation, these are represented by values of type VMContObj. These are fat pointers, consisting of a sequence counter, and a pointer to a VMContRef. The latter type is used for the actual representation of continuations.
The sequence counter part of VMContObjs is used to check that every continuation value can only be used once.

The StoreOpaque is extended to contain a "stack chain": It indicates what stack we are currently executing on. Logically, this is a linked list of stacks, since each continuation has a parent field. The chain stored in the StoreOpaque always ends with a value representing the initial stack. This is the stack we were on when entering Wasm, which will usually be the main stack.

Memory Management

Currently, memory management is very basic: The StoreOpaque provides a new method for allocation a new VMContRef, and keeps track of all continuations created this way. Continuations are never deallocated at the moment, meaning that they live until the store itself is deallocated.

The stack memory used by each allocation (represented by type ContinuationStack) is always mmap-ed, with a guard page at the end. There is currently no support for growing stacks, or segmented stacks.

Backtrace generation

The existing backtrace generation mechanism is extended to be fully aware of continuations: When generating backtraces, the entire chain of continuation is traversed, not just the stack frames of the currently active stack/continuation.

Integration with GC

Integration with the GC proposal is limited, but should at least be safe:

Programs where continuation objects would be stored inside GC objects (structs, arrays, ...) are rejected

During GC, all stack frames of all continuations are inspected when looking for GC roots.

Entering/Exiting Wasm

Prior to this PR, there were two separate mechanisms that save and restore some state of the runtime when entering/exiting Wasm:

The functions enter_wasm and exit_wasm in func.rs

CallThreadState saves and restores (on drop) parts of the VMRuntimeLimits

This PR consolidates these two mechanism, because it requires some additional state to be updated and restored on enter/exit:
the type wasmtime::runtime::func::RuntimeEntryState now stores all of the required runtime state and ensures that it's restored when exiting Wasm.

Tags

Since the stack switching proposal extends the notion of tags introduced by the exception handling proposal, it adds the necessary machinery for importing, exporting, and defining tags.

Limitations

The limitations are as follows, some of which were mentioned above:

resume.throw needs to be implemented once exception handling is there

Full integration with GC and support for deallocating continuations

The only supported platform at the moment are unix-like x64 systems. I'm planning to change that soon, this won't require big changes to what's in this PR at the moment.

Wasmtime GitHub notifications bot (Feb 04 2025 at 15:04):

frank-emrich commented on PR #10177:

There are currently a few test failures that I 'm hoping to get some help with:

I don't understand cargo vet well enough to know what's up, but the problem should be fairly benign: We are using memoffset in a different crate now, but from what I can tell, it has already been vetted for use in this project.

There are some build failures due to the new instructions not being supported on all platforms. In general, we've added a Wasm feature and a cargo feature for stack switching. My plan was that all stack switching tests would be skipped if the stack-switching cargo feature is not enabled. That's easy to do for the all test suite, but I'm not sure what the best approach is for the wast tests to achieve this. Am I understanding correctly that the wast test suite always currently runs all tests, independently from what cargo features are enabled?

As a general, so far most of the stack switching code is compiled even if the cargo feature is disabled. It would be possible to get more aggressive with conditional compilation, but this requires some ugly special casing in backtrace.rs. Happy for suggestions if people would like to see more conditional compilation or are happy with keeping it minimal.

Wasmtime GitHub notifications bot (Feb 04 2025 at 15:05):

frank-emrich edited a comment on PR #10177:

There are currently a few test failures that I 'm hoping to get some help with:

I don't understand cargo vet well enough to know what's up, but the problem should be fairly benign: We are using memoffset in a different crate now, but from what I can tell, it has already been vetted for use in this project.

There are some build failures due to the new instructions not being supported on all platforms. In general, we've added a Wasm feature and a cargo feature for stack switching. My plan was that all stack switching tests would be skipped if the stack-switching cargo feature is not enabled. That's easy to do for the all test suite, but I'm not sure what the best approach is for the wast tests to achieve this. Am I understanding correctly that the wast test suite always currently runs all tests, independently from what cargo features are enabled?

As a general, so far most of the stack switching code is compiled even if the cargo feature is disabled. It would be possible to get more aggressive with conditional compilation, but this requires some ugly special casing in backtrace.rs. Happy for suggestions if people would like to see more conditional compilation or are happy with keeping it minimal.

Also, I had to re-bless most of the disas tests since the layout of the VMContext change due to the addition of some new fields. That has increased the surface area of this PR even more.

Wasmtime GitHub notifications bot (Feb 04 2025 at 15:06):

frank-emrich edited a comment on PR #10177:

There are currently a few test failures that I 'm hoping to get some help with:

I don't understand cargo vet well enough to know what's up, but the problem should be fairly benign: We are using memoffset in a different crate now, but from what I can tell, it has already been vetted for use in this project.

There are some build failures due to the new instructions not being supported on all platforms. In general, we've added a Wasm feature and a cargo feature for stack switching. My plan was that all stack switching tests would be skipped if the stack-switching cargo feature is not enabled. That's easy to do for the all test suite, but I'm not sure what the best approach is for the wast tests to achieve this. Am I understanding correctly that the wast test suite always currently runs all tests, independently from what cargo features are enabled?

As a general, so far most of the stack switching code is compiled even if the cargo feature is disabled. It would be possible to get more aggressive with conditional compilation, but this requires some ugly special casing in backtrace.rs. Happy for suggestions if people would like to see more conditional compilation or are happy with keeping it minimal.

Also, I had to re-bless most of the disas tests since the layout of the VMContext change due to the addition of some new fields. That has increased the surface area of this PR even more.

Wasmtime GitHub notifications bot (Feb 04 2025 at 18:00):

frank-emrich edited a comment on PR #10177:

There are currently a few test failures that I 'm hoping to get some help with:

I don't understand cargo vet well enough to know what's up, but the problem should be fairly benign: We are using memoffset in a different crate now, but from what I can tell, it has already been vetted for use in this project.

There are some build failures due to the new instructions not being supported on all platforms. In general, we've added a Wasm feature and a cargo feature for stack switching. My plan was that all stack switching tests would be skipped if the stack-switching cargo feature is not enabled. That's easy to do for the all test suite, but I'm not sure what the best approach is for the wast tests to achieve this. Am I understanding correctly that the wast test suite always currently runs all tests, independently from what cargo features are enabled?

As a general note, so far most of the stack switching code is compiled even if the cargo feature is disabled. It would be possible to get more aggressive with conditional compilation, but this requires some ugly special casing in backtrace.rs. Happy for suggestions if people would like to see more conditional compilation or are happy with keeping it minimal.

Also, I had to re-bless most of the disas tests since the layout of the VMContext change due to the addition of some new fields. That has increased the surface area of this PR even more.

Wasmtime GitHub notifications bot (Feb 04 2025 at 18:55):

frank-emrich edited PR #10177:

This PR adds support for the Wasm stack switching proposal. The explainer document for the proposal is here.

This means that the following instructions are added: cont.new, resume, suspend, switch, and cont.bind.
The instruction resume.throw, which is part of the proposal but relies on exception handling, is currently unsupported.

This was developed together with @dhil.

General implementation notes

In Wasm, continuations are represented by values of type (ref $ct), where $ct is a new composite type/heap type for continuations.
In the implementation, these are represented by values of type VMContObj. These are fat pointers, consisting of a sequence counter, and a pointer to a VMContRef. The latter type is used for the actual representation of continuations.
The sequence counter part of VMContObjs is used to check that every continuation value can only be used once.

The StoreOpaque is extended to contain a "stack chain": It indicates what stack we are currently executing on. Logically, this is a linked list of stacks, since each continuation has a parent field. The chain stored in the StoreOpaque always ends with a value representing the initial stack. This is the stack we were on when entering Wasm, which will usually be the main stack.

Memory Management

Currently, memory management is very basic: The StoreOpaque provides a new method for allocation a new VMContRef, and keeps track of all continuations created this way. Continuations are never deallocated at the moment, meaning that they live until the store itself is deallocated.

The stack memory used by each allocation (represented by type ContinuationStack) is always mmap-ed, with a guard page at the end. There is currently no support for growing stacks, or segmented stacks.

Backtrace generation

The existing backtrace generation mechanism is extended to be fully aware of continuations: When generating backtraces, the entire chain of continuation is traversed, not just the stack frames of the currently active stack/continuation.

Integration with GC

Integration with the GC proposal is limited, but should at least be safe:

Programs where continuation objects would be stored inside GC objects (structs, arrays, ...) are rejected

During GC, all stack frames of all continuations are inspected when looking for GC roots.

Entering/Exiting Wasm

Prior to this PR, there were two separate mechanisms that save and restore some state of the runtime when entering/exiting Wasm:

The functions enter_wasm and exit_wasm in func.rs

CallThreadState saves and restores (on drop) parts of the VMRuntimeLimits

This PR consolidates these two mechanism, because it requires some additional state to be updated and restored on enter/exit:
the type wasmtime::runtime::func::RuntimeEntryState now stores all of the required runtime state and ensures that it's restored when exiting Wasm.

Tags

Since the stack switching proposal extends the notion of tags introduced by the exception handling proposal, it adds the necessary machinery for importing, exporting, and defining tags.

Limitations

The limitations are as follows, some of which were mentioned above:

resume.throw needs to be implemented once exception handling is there

Full integration with GC and support for deallocating continuations

The only supported platform at the moment are unix-like x64 systems. I'm planning to change that soon as follow-up PRs, this won't require big changes to what's in this PR at the moment.

Neither Winch or Pulley are supported at the moment.

Wasmtime GitHub notifications bot (Feb 05 2025 at 10:48):

frank-emrich edited a comment on PR #10177:

There are currently a few test failures that I 'm hoping to get some help with:

I don't understand cargo vet well enough to know what's up, but the problem should be fairly benign: We are using memoffset in a different crate now, but from what I can tell, it has already been vetted for use in this project.

There are some build failures due to the new instructions not being supported on all platforms. In general, we've added a Wasm feature and a cargo feature for stack switching. My plan was that all stack switching tests would be skipped if the stack-switching cargo feature is not enabled. That's easy to do for the all test suite, but I'm not sure what the best approach is for the wast tests to achieve this. Am I understanding correctly that the wast test suite always currently runs all tests, independently from what cargo features are enabled?

As a general note, so far most of the stack switching code is compiled even if the cargo feature is disabled. It would be possible to get more aggressive with conditional compilation, but this requires some ugly special casing in backtrace.rs. Happy for suggestions if people would like to see more conditional compilation or are happy with keeping it minimal.

Also, I had to re-bless most of the disas tests since the layout of the VMContext changed due to the addition of some new fields. That has increased the surface area of this PR even more.

Wasmtime GitHub notifications bot (Feb 05 2025 at 16:50):

fitzgen commented on PR #10177:

Am I understanding correctly that the wast test suite always currently runs all tests, independently from what cargo features are enabled?

That is correct, and we assert that tests that "should" fail do in fact fail. This way, if we fix bugs or expand support, we won't accidentally and silently miss out on tests for those things.

This should be fine for your stack switching tests, as long as you

mark the tests as "should fail" on supported platforms, and

return an error or some sort (rather than unsafely segfault or whatever) when processing Wasm that uses this proposal on an unsupported platform/configuration.

See https://github.com/bytecodealliance/wasmtime/blob/d7d605c236a857a4019aa39850e9dd6a6597ece1/crates/wast-util/src/lib.rs#L280

Wasmtime GitHub notifications bot (Feb 07 2025 at 14:03):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 07 2025 at 14:49):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 11 2025 at 20:58):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 11 2025 at 22:14):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 17:12):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 17:37):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:07):

frank-emrich has marked PR #10177 as ready for review.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:07):

frank-emrich requested alexcrichton for a review on PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:07):

frank-emrich requested wasmtime-fuzz-reviewers for a review on PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:07):

frank-emrich requested fitzgen for a review on PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:07):

frank-emrich requested wasmtime-compiler-reviewers for a review on PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:07):

frank-emrich requested wasmtime-core-reviewers for a review on PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:07):

frank-emrich requested wasmtime-default-reviewers for a review on PR #10177.

Wasmtime GitHub notifications bot (Feb 12 2025 at 18:40):

alexcrichton commented on PR #10177:

Oh sorry I should have left a note here but I think I forgot. I was out last week for a company offsite and I'm out this week for the CG meeting, but I plan on getting to this next week. I want to make sure I've got plenty of time to sit down and go through this, but wanted to give a heads-up that I'll be looking into it next week at the earliest.

Also to reiterate: thank you so much for this! Thanks for wrestling with CI as well, much appreciated!

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton submitted PR review:

Ok I think I've seen about half of the files of this PR (excluding auto-updates in tests) so wanted to leave an initial round of comments/thoughts.

At a high-level everything here looks reasonable to me so far. I haven't dug into the actual implementation details of stack switching though and have for this been focusing on the integration points with the rest of Wasmtime. I left a lot of comments along the lines of "file an issue to point a comment to this" and I think it might make the most sense to have a single tracking issue for follow-up work on stack-switching. That can have a lot of various checkboxes for the logical work items remaining (e.g. contref in the gc heap, in the embedding API, windows support, etc).

Additionally at a high-level I want to also ask you what your own time allowance is for landing this? This is, as expected, a large PR which is going to take some time to land. I suspect it'll take me on the order of ~days of review to get through the the real "meat" of stack switching and I'll likely have comments from all that as well. If, however, you're running short on time for this that's ok too!

Basically what I want to ask is: do you have time allocated for the back-and-forth of review here? It's hard to predict exactly how long it will take but my gut is that it'll be a nontrivial amount of time. If you don't have time for this I think that's also totally ok. I'll probably still continue to review all this and I'll try to find time in the coming future to work on addressing many of these items myself.

In an ideal world this PR would be split up into chunks and landed piecemeal, for example parsing support, then compiling support, then maybe an instruction-at-a-time for each piece, etc. Splitting this up is no easy task though so I don't want to ask this lightly of you, and you also know much better than I about if it even could be split up and where it could be split. Part of this is that GitHub does not make reviewing large PRs easy (e.g. every time I add a comment it takes ~5 seconds for the comment box to appear), and overall it's easier to focus on smaller changes to ensure that as little as possible slips through the cracks.

Basically tl;dr; I've left a lot of comments below but it doesn't have to be you alone addressing them. If you're running low on time to allocate to this work I think we can try to be accomodating in assisting to landing this.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind filing an issue about this and tagging with with // FIXME(#NNN): ...

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind translating this TODO and the above one to a FIXME with a filed issue?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

I think this comment is a copy/paste from a historical version, mind updating it to just say that pulley doesn't support stack-switching/exceptions at this time?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

I think it's fine to leave this as-is, the #[cfg] here is only to handle the conditionally defined functions on wasmtime::Config but it looks like wasm_stack_switching is unconditionally defined so it's ok to unconditionally thread through the boolean to there here. The wasmtime::Config can then handle validation of ensuring various dependent proposals are all enabled.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Like above, mind tagging this with // FIXME(#nnn): ...?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Given that tags are "just" a type, while this follows the pattern of other imports I think this might be able to be deleted to use VMTagDefinition instead? Or basically otherwise only contain a VMSharedTypeIndex. Although I haven't reviewed the rest of this PR yet so I'm not sure if the pointers here are necessary for something else.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

copy/pasting the other unsafe impl VmSafe comments should be sufficient here, basically it's #[repr(C)] and the internals are all VmSafe

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind folding these all up into the arm above as more | ... patterns?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Still getting to the rest of the review in this PR, but for this NOTE is that still applicable? (or maybe just a vestige of an old iteration?)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Is this comment still applicable? My guess is probably "no" and it could be dropped, but wanted to confirm.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

For this we'll want to return a first-class error instead of panicking to gracefully handle modules that do this.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

For this I'd expect "yes" if continuations are stored on the GC heap, but if they're not stored on the GC heap then this is correctly false

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Same as above, we'll want to return a gracefull error on this.

Also as I'm typing this commet out I'm realizing that it'd be good to have // FIXME(#NNN) issue references pointing to an open issue for these bits and pieces of stack switching that aren't supported yet. (e.g. an open issue for GC/stack-switching integration)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind fleshing out this TODO comment? (a FIXME is fine, no need to fix it in this PR per se)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Instead of taking index: u32 here and below could this take TypeIndex directly?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Historically for me at least "LSB" typically means "least significant bits" as opposed to "least significant byte" which I think you're referring to here. Mind switching this to talk about the "low 32 bits" and the "high 32 bits"?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

For this I might also recommend the isplit instruction which in effect does the same thing but is a bit more efficient to represent. (that takes a 128-bit value and splits it into low/high 64-bit halves)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind folding this arm into the previous one to make it clear it's all handled the same way?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Tiny note, but we try to avoid as casts where possible because they can be lossy. I haven't made it to stack_switching_environ-the-module yet but to avoid the as cast here you can:

If the type is u32-or-smaller, use i64::from(...) instead

If the type is u64, then use CONTROL_EFFECT_RESUME_DISCRIMINANT.signed() to avoid as

(just a minor nit here, not major)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

This I think will need some updates. Endianness should only come into play when storing/loading from memory as opposed to the in-register 128-bit logical value. A CLIF i128 should have the same logical value regardless of platform endianness.

I'm also a bit suspicious of the handling of smaller-than-64-bit platforms both here and below. Overall it seems like this may need some refactoring? To know for sure I'll need to read more of this PR msyelf

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind fleshing out the "todo"s here? Here instead of panicking we'll want to return a first-class error to gracefully reject modules that contain resume.throw instructions (but returning an error is of course ok for now)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

For this, similar to above, I'd recommend using iconcat instead of a sequence of or/shift/bor/etc. That should be a bit easier to generate good code for and have the same logical result.

Also, like above, ideally endianness shouldn't factor into this helper at all. I suspect that endianness came into the picture during CI getting s390x working, and if that's the case I can try to help out to debug where the endianness needs to be factored in otherwise.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

For this mind returning a first-class error with a // FIXME(...)?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind expanding this comment? (I'm not familiar myself with set_branched_to_exit so I'm not actually sure what this is doing myself)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

For this, and the new intrinsics below, mind adding #[cfg(feature = "stack-switching")] to avoid needing to define the intrinsics if it's disabled at compile time?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

I suspect it's probably ok to remove this TODO now?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Given the comment I suspect these were intended to be deleted

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

I commented in a few places above this as well, but mind tagging this with a // FIXME pointing to an open issue?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Should be safe to delete now

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

I think these two cases might be dead code now?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Stray change?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Adding a note on these that they might be removable.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind folding these two arms together?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Question on this: we've already got wasmtime::Config::wasm_stack_size, so given that do you feel it's still necessary to have a separate config knob for continuation stacks? For example do continuation stacks want to be smaller than wasm stacks most of the time? Or was this mostly for testing?

(I'm fine either way just curious to poke at this a bit)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind fleshing this out? It should be ok to drop the _idx parameter as well if not needed

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Stray change? (maybe necessary during some refactoring but perhaps no longer?)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

For this I'm not sure how easy it will be but we'll want to return an error here instead of panicking. That would involve changing this function to return Result<u32>, however, and then updating all callers to propagate errors too. Depending on how big a refactoring that is it's also fine to leave this tagged as // FIXME(...) pointing to an issue.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

At least in theory these are all already in the prelude of Rust itself so I don't think you should need to explicitly import them.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Since this is more of a runtime thing than a compile-time thing, which is mostly what wasmtime-environ is concerned with, could this move to wasmtime::runtime::vm or somewhere internally there?

That would then also enable using SendSyncPtr<T> which would avoid the need for the unsafe impl {Send,Sync} below.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

I think nowadays it should be safe to remove this since StackLimits will naturally implement these traits.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Would it make sense to move these into enum State directly as fields on the Parent variant?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

You can also do this without unsafe as *self as i32

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind fleshing out this comment?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

To confirm, is this read by Cranelift-generated code? If not, it should be safe to drop the #[repr(C)]. If so, more questions!

Could this move to wasmtime::runtime::vm::*? Most runtime-y things live there instead of in wasmtime-environ which is more-or-less "stuff needed at compile time" but that's it.

Data structures read by Cranelift-generated code we idiomatically try to prefix with "VM", so this'd be called VMCommonStackInformation

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Ah ok I see that this might be why the structures above are defined in this module rather than in the wasmtime crate.

For this though I think we'll want to replace this infrastructure with what we have in VMOffsets. To handle cross-compilation correctly we can't use utilities like size_of or offset_of! here. Instead we have to manually calculate sizes in VMOffsets and then in the wasmtime crate in vmcontext.rs we test all the methods to ensure that the offsets calculated actually match the current host.

This is a relatively large refactoring to all of this support (e.g. instead of const-per-offset it'd be method-per-offset on VMOffsets instead, or HostPtr). In that sense I can also help take care of this too.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Similar to above this ideally would live in some place like vmcontext/stack_switching.rs and would have a "VM" prefix to indicate it's read from compiled wasm code (if it's read from compiled code)

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

One thing perhaps worth pointing out here is that one issue we've seen in the past is that using usize in wasmtime-environ can be a bit funky because wasmtime-environ is used in situations such as cross-compiling where the host/target have differing pointer sizes. If possible this'd ideally use something fixed-size like a u32, but I'm not sure how many other casts that would then require.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind tagging this with a // FIXME and an issue to touch it up later?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

This'll definitely want to avoid pub, but pub(crate) is fine to access it throughout this crate.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind prefixing this with unwrap_* to signal that it's a panicking method? Also the todo!() below might be able to switch to a panic!(...) nowadays

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

This ends up affecting the public API of Wasmtime, so would it be possible to leave this out? E.g. was this only needed during development? Or still needed?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

This feels a bit suspicious to me so I'm leaving a note to myself here to revisit this once I've seen more of this PR

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

One other piece of validation (which might belong in Engine::_check_compatible_with_native_host perhaps) is to ensure that if stack-switching is enabled that #[cfg(feature = "stack-switching")] is also enabled. Otherwise that represents a host that has enabled the wasm feature but not compile-time support so it'd probably be good to return a first-class error in such a situation.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Would it make sense to tag this with #[cfg(feature = "stack-switching")]?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind tagging this with a FIXME pointing to an issue?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Artifact of development?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Like above, mind tagging this as #[cfg(feature = "stack-switching")]?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind adding some docs above this explaining what this is gating?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

I haven't gotten to the full PR yet but from some of the translation methods below it looks like the fat pointers of continuations aren't stored on the GC heap so this is correctly false

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Since this is just an internal method it might make sense to delete this and update callers to not use it?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Like above for globals the panic here is fine for now but this'd be good to tag with a // FIXME pointing to an issue

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

As a heads-up I think this may no longer be necessary after https://github.com/bytecodealliance/wasmtime/pull/10223 landed.

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

Mind tagging this with a // FIXME pointing to an issue?

Wasmtime GitHub notifications bot (Feb 14 2025 at 20:26):

alexcrichton created PR review comment:

This location unfortunately can't easily be switched to returning a Result so panicking makes sense to me, but mind tagging this with // FIXME(...) pointing to an issue?

Wasmtime GitHub notifications bot (Feb 17 2025 at 13:08):

frank-emrich commented on PR #10177:

@alexcrichton

Thanks so much for having a look at this already!

Regarding my own time commitment for this PR: I'm starting a new job in mid March, so things are looking as follows:

Until end of February: Focusing exclusively on getting this PR in shape

First two weeks of March: Working on this PR, but some distractions due to moving, etc.

From mid March: There will be an initial period of me settling in at my new job during which I can't do much on this. After that the Wasmtime/stack switching work will not part of my day job anymore, but I'll still be around to help out, address bugs, etc. I also have a few things on top of the current PR already done or planned that I want to add at that stage (aarch64 support, Windows support, better DWARF support, ....).

@dhil should also be able to help out here and there.

In an ideal world this PR would be split up into chunks and landed piecemeal, for example parsing support, then compiling support, then maybe an instruction-at-a-time for each piece, etc.

I think there was just a bit of a miscommunication between me and @dhil. I thought you wanted to see this as one big chunk. There's a simple way in which this PR could be split up, leading to a sequence of PRs as follows:

Integrate tag support, with nothing stack switching related, yet.

Add dummy field to VMContext with the size and position that the stack switching field will later occupy. This way, all the changes to the disas tests will happen in this PR. I'm not sure if the disas tests actually contribute to Github choking on this PR, but at least getting them out of the way will reduce the amount of conflicts coming in.

Everything except the actual Wasm -> CLIF translation. This will be a big chunk where most of the integration of stack switching with the rest of Wasmtime goes.

The Wasm -> CLIF translation. This will mostly be what's currently in crates/cranelift/src/stack_switching/, in particular instructions.rs. The latter is 2500 LOC, so reviewing that separately seems worthwhile. I may even be able to split this up further.

This is far from perfect because PR 3 and 4 will still be large and not really split along separate features of the stack switching implementation, but at least that split up would be easy for me to do. I may discover some more chunks that can be factored out into separate PRs on the way.

My only concerns about this is the following: Given that Github doesn't really support stacked PRs, we would work through these 4+ PRs linearly, making it difficult to point at why we need something in the next PR.
On the other hand, I could just put a branch somewhere that shows everything that's still missing at any point so you can have a look at how something will be used in the remaining PRs.

Please let me know what you think. I will also make sure to work through the comments you already added to this PR so they don't get lost if we do decide to split this PR.

Wasmtime GitHub notifications bot (Feb 17 2025 at 14:50):

frank-emrich edited a comment on PR #10177:

@alexcrichton

Thanks so much for having a look at this already!

Regarding my own time commitment for this PR: I'm starting a new job in mid March, so things are looking as follows:

Until end of February: Focusing exclusively on getting this PR in shape

First two weeks of March: Working on this PR, but some distractions due to moving, etc.

From mid March: There will be an initial period of me settling in at my new job during which I can't do much on this. After that the Wasmtime/stack switching work will not part of my day job anymore, but I'll still be around to help out, address bugs, etc. I also have a few things on top of the current PR already done or planned that I want to add at that stage (aarch64 support, Windows support, better DWARF support, ....).

@dhil should also be able to help out here and there.

In an ideal world this PR would be split up into chunks and landed piecemeal, for example parsing support, then compiling support, then maybe an instruction-at-a-time for each piece, etc.

I think there was just a bit of a miscommunication between me and @dhil. I thought you wanted to see this as one big chunk. There's a simple way in which this PR could be split up, leading to a sequence of PRs as follows:

Integrate tag support, with nothing stack switching related, yet.

Add dummy field to VMContext with the size and position that the stack switching field will later occupy. This way, all the changes to the disas tests will happen in this PR. I'm not sure if the disas tests actually contribute to Github choking on this PR, but at least getting them out of the way will reduce the amount of conflicts coming in.

Everything except the actual Wasm -> CLIF translation. This will be a big chunk where most of the integration of stack switching with the rest of Wasmtime goes.

The Wasm -> CLIF translation. This will mostly be what's currently in crates/cranelift/src/stack_switching/, in particular instructions.rs. The latter is 2500 LOC, so reviewing that separately seems worthwhile. I may even be able to split this up further.

This is far from perfect because PR 3 and 4 will still be large and not really split along separate features of the stack switching implementation, but at least that split up would be easy for me to do. I may discover some more chunks that can be factored out into separate PRs on the way.

My only concerns about this is the following: Given that Github doesn't really support stacked PRs, we would work through these 4+ PRs linearly, making it difficult to point at why we need something in the next PR.
On the other hand, I could just put a branch somewhere that shows everything that's still missing at any point so you can have a look at how something will be used in the remaining PRs.

Please let me know what you think. I will also make sure to work through the comments you already added to this PR so they don't get lost if we do decide to split this PR.

Edit: One concern I forgot to mention regarding splitting this PR up is testing: We only have a few scattered unit tests, most of our testing happens through whole Wasm programs. That means that if we split the PR, we will only be able to add tests in the end, when all pieces are in place.

Wasmtime GitHub notifications bot (Feb 17 2025 at 21:40):

frank-emrich submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2025 at 21:40):

frank-emrich created PR review comment:

This was just a note to ourselves that we made this field pub(crate) because we access it from our code. For example, we call crate::value_type a few times, which requires a &dyn TargetIsa. But I'm happy to leave a mental note here that we should double-check if that's really necessary

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:00):

frank-emrich submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:00):

frank-emrich created PR review comment:

Sorry, there's one thing I forgot to mention in the overall PR description: There is currently some debugging logic left in this PR, you will run into it when looking at instructions.rs: We have added emit_debug_println! and emit_debug_assert!. Their implementation is too hacky to keep in the final version that we merge, but I'm hoping to keep them until the very end and only rip them out when the rest of the code is ready. They are incredibly helpful while I may need to make any kind of changes to the code.

Anything that has something like "delete me" in the name is related to this debugging code.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:01):

frank-emrich submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:01):

frank-emrich created PR review comment:

This is also an instance of "debugging code that I would like to keep around until the rest of the PR is ready to merge"

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:08):

frank-emrich submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:08):

frank-emrich created PR review comment:

Yes, that's absolutely the reason why some of the types that should morally live in the runtime/wasmtime crate are defined here, just to make these offset definitions easier.

I actually suspected that the module here would have to be merge into (something like) VMOffsets, there's a comment a few lines above saying that.

I'm happy to do that refactoring, but just a few questions:
Should I actually integrate all the offsets for these types here into VMOffsets (or more accurately, the PtrSize trait)? Or was your suggestion to re-create similar infrastructure elsewhere? I'm mostly asking because AFAIK all the types that vmoffsets.rs talks about have the VM* prefix. There's nothing really stopping us from adding that prefix to our own types, but I'm unsure if that prefix carries more meaning and may be inappropriate for our types.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:09):

frank-emrich submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:09):

frank-emrich created PR review comment:

Yes, definitely! See my comment on the delete_me_* libcalls

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:09):

frank-emrich edited PR review comment.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:18):

frank-emrich submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2025 at 22:18):

frank-emrich created PR review comment:

Yes, this is ugly. This is basically just a way of saying "give me the size of a ValRaw", without having access to that type here.
The reason we need that size is that all continuation payloads (arguments to continuations, return values from continuations, values provided by suspend, etc) are currently written to and then read from buffers containing ValRaw. Let's keep this comment open, we will certainly be re-visiting that discussion when you have a look at instructions.rs.

Wasmtime GitHub notifications bot (Feb 18 2025 at 17:33):

alexcrichton commented on PR #10177:

Regarding my own time commitment for this PR

Ok that all sounds good, thanks! (very helpful to set expectations!)

I think there was just a bit of a miscommunication between me and @dhil.

Oh no that's on me. I remember telling @dhil that and I think I was just basically wrong, so that's on me for leading y'all astray. Given the size of this (not just the auto-updated tests but also the scale of the implementation) along with the subtelty of the implementation in retrospect I just didn't account for that and misled y'all. If possible I think the best way to handle this would be to peel off PRs from this one and submit them independently.

Overall my goal is to ensure that we get a good chance to review everything coming in and reducing the chance of something being lost by accident due to being overlooked. For coordination what I would imagine is that it's completely reasonable to have lightly, if at all, tested intermediate states. Additionally it's totally reasonable to leave a "hole" in the code with something like // more coming soon ... (e.g. making a hole in the VMContext for the stack-switching state necessary for a future PR). Let's keep this PR open so it can be cross-referenced if needed, and this can be rebased occasionally as smaller pieces are landed.

Does that seem ok? It's a similar strategy we're taking for landing async component model support in Wasmtime where it's landing piece-by-piece. Each piece is not 100% thoroughly tested but the "final PR" has lots of tests. We try to keep track of outstanding TODO items in issues occasionally and otherwise are ok deferring work to a future PR which has more tests and more fully justifies a prior design.

Your sequence of PRs makes sense to me as well. I'd mostly defer to y'all for pieces to split up as you're the most familiar with the implementation, and if something seems too big I can bring it up during the review but by naturally splitting this up I think most slicing/dicing will be quite reasonable.

Wasmtime GitHub notifications bot (Feb 18 2025 at 21:11):

frank-emrich commented on PR #10177:

Overall my goal is to ensure that we get a good chance to review everything coming in and reducing the chance of something being lost by accident due to being overlooked.

Yes, absolutely. I'm happy to do the necessary chopping. I'll create a tracking issue describing the bigger picture and where we currently are regarding the PRs that have and haven't landed at any time.

The outline of PRs I mentioned in my previous comment will still lead to fairly large ones, but I will see if there's potential to split this up further and wait for your feedback once I've split off a candidate for a PR.

Also, sorry for making you wade through lots of todo!(), unimplemented!() and other little things that we hadn't resolved before creating this PR.

Wasmtime GitHub notifications bot (Feb 18 2025 at 23:43):

alexcrichton commented on PR #10177:

Sounds good! It's ok to skip the step of writing out a plan in an issue and then executing on the plan, we can work on making sure things are documented before you wind down but up until then it's ok if it's just in a few heads. In that sense no need to exhaustively document "here's the round peg that's gonna fit in that round hole" and we can just have one issue for tracking known items that are already in-tree.

And please no apologies necessary! Landing big changes I've found is always a bit of an art and if anything I'm the one who led you astray by being overconfident!

Wasmtime GitHub notifications bot (Feb 19 2025 at 13:00):

frank-emrich submitted PR review.

Wasmtime GitHub notifications bot (Feb 19 2025 at 13:00):

frank-emrich created PR review comment:

This comment is actually still applicable, but it's a relatively minor point.

What's happening here is the following: For the translation of some instructions, like resume, we need access to the VMRuntimeLimits. That means that if a Wasm function contains certain instructions, we would like to use the the vmruntime_limits_ptr ir::Value in FuncEnvironment.
However, the code here must decide whether or not to call declare_vmruntime_limits_ptr without knowing if the function body contains such an instruction (and it seems like a bad idea to traversing the body of the function just for this).

What we currently do is just to re-load the pointer to the VMRuntimeLimits every time we need it (basically just inlining the same load as the one in declare_vmruntime_limits_ptr) and hoping that the optimizer may catch cases where we do this more than once per function.

I'm not sure if it's worth to update declare_vmruntime_limits_ptr so that it doesn't define vmruntime_limits_ptr in the function prelude, but instead make it an Option that's set the first time someone needs the value.

Wasmtime GitHub notifications bot (Feb 19 2025 at 19:00):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 19 2025 at 19:44):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 19 2025 at 19:44):

dhil created PR review comment:

A tag is more than a wasm type. It is a nominal entity and its unique identity is important. Basically we get a unique identity by picking backing on pointer equality.

I think we can get rid of the vmctx, potentially. Though we can discuss the best course of action here. I have pulled the tag stuff into its own patch which I will open tomorrow when I have had a chance to look at the tests.

Wasmtime GitHub notifications bot (Feb 19 2025 at 21:44):

github-actions[bot] commented on PR #10177:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

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 (Feb 19 2025 at 23:15):

frank-emrich commented on PR #10177:

Converting this back to a draft now because it will be split up into multiple PRs.

Over the next few weeks, I expect to rebase this PR whenever parts of it have landed. This way, this PR will always contain whatever is left.

I've also created issue #10248 to track the overall status. I will also use that issue in various FIXMEs for missing features.

Wasmtime GitHub notifications bot (Feb 19 2025 at 23:20):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:39):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 20 2025 at 22:24):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 20 2025 at 23:07):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 21 2025 at 02:09):

github-actions[bot] commented on PR #10177:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Feb 21 2025 at 14:46):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 21 2025 at 15:13):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 21 2025 at 15:30):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2025 at 15:30):

alexcrichton created PR review comment:

Should I actually integrate all the offsets for these types here into VMOffsets (or more accurately, the PtrSize trait)?

Yeah let's do that. That's the conventional location we have for dealing with offsets-of-things in compiled code, so I think keeping it all in one place is best.

There's nothing really stopping us from adding that prefix to our own types, but I'm unsure if that prefix carries more meaning and may be inappropriate for our types.

Oh no special meaning yeah, just our convention for "this datatype is read by both compiled code and the host" and then we try to group things together where it makes sense

Wasmtime GitHub notifications bot (Feb 21 2025 at 15:55):

dhil created PR review comment:

I think we need some knobs for continuation stacks, e.g. limit the number of allocated stacks. I think the stack size can be useful, but perhaps its use is rather limited until we have implemented growable stacks. I think we also want to land a pooling allocator for continuation stacks, as it is necessary to obtain decent performance on the workloads we have been benchmarking.

Though, I'd be quite happy to simply use wasm_stack_size and keep the specific stack switching config downstream for now.

Wasmtime GitHub notifications bot (Feb 21 2025 at 15:55):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 25 2025 at 01:14):

frank-emrich updated PR #10177.

Wasmtime GitHub notifications bot (Feb 25 2025 at 16:02):

frank-emrich updated PR #10177.

Last updated: Feb 28 2025 at 03:10 UTC