You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR is part of a series that adds support for the Wasm stack switching proposal. The explainer document for the proposal is here. There's a tracking issue for the overall progress: #10248
This particular PR does one small change: It adds a single pointer-sized field, called stack_chain to the fixed-width part of VMContext.
Note that this PR only adds the field in the layout of the VMContext, but does not actually read or write any data in that field. The reason for doing this change in a separate PR is simply that the change to the layout of the VMContext requires updates to 700 disas tests.
In subsequent PRs, the field is used as follows: In the future, the StoreOpaque will contain a stack_chain field as well, which indicates the currently running continuation, and logically represents a linked list continuations.
The stack_chain field in the VMContext added in this PR will merely be a pointer to that stack chain in the StoreOpaque. This is due to the fact that the stack chain is shared by all instances running inside a store, and we need access to the stack chain in the StoreOpaque from Cranelift-generated code. This is similar to how the VMRuntimeLimits are accessed.
The stack_chain field in the VMContext added in this PR will merely be a pointer to that stack chain in the StoreOpaque. This is due to the fact that the stack chain is shared by all instances running inside a store, and we need access to the stack chain in the StoreOpaque from Cranelift-generated code. This is similar to how the VMRuntimeLimits are accessed.
Would it make sense to put the stack chain in the VMRuntimeLimits (which I have been meaning to rename to something like VMStoreContext for forever) rather than in StoreOpaque? That is generally the location where we put all shared-across-the-whole-store-and-accessed-by-JIT-code data (as opposed to instance-specific data that is accessed by JIT code, which goes in VMContext). There is no precedent, afaik, of JIT code directly accessing anything in StoreOpaque thus far, and I'd sort of like to keep it that way so that we have that clean separation between what is vs isn't accessed by JIT code and because StoreOpaque is not repr(C).
(Apologies for all these late comments, I've been at the CG and then on vacation and I'm just catching up now)
@fitzgen
I don't have strong feelings about this, but from what you describe it sounds like a good idea to move the stack chain there. It would also mean that the new field added in this PR can be removed again (its only purpose is to achieve being able to access that particular field in the StoreOpaque from jitted code. If we move the stack chain into the VMRuntimeLimits, we can just rely on the fact that we already got a pointer from the VMContext to the VMRuntimeLimits).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a series that adds support for the Wasm stack switching proposal. The explainer document for the proposal is here. There's a tracking issue for the overall progress: #10248
This particular PR does one small change: It adds a single pointer-sized field, called
stack_chainto the fixed-width part ofVMContext.Note that this PR only adds the field in the layout of the
VMContext, but does not actually read or write any data in that field. The reason for doing this change in a separate PR is simply that the change to the layout of theVMContextrequires updates to 700 disas tests.In subsequent PRs, the field is used as follows: In the future, the
StoreOpaquewill contain astack_chainfield as well, which indicates the currently running continuation, and logically represents a linked list continuations.The
stack_chainfield in theVMContextadded in this PR will merely be a pointer to that stack chain in theStoreOpaque. This is due to the fact that the stack chain is shared by all instances running inside a store, and we need access to the stack chain in theStoreOpaquefrom Cranelift-generated code. This is similar to how theVMRuntimeLimitsare accessed.