winch: Implement new trampolines#6358
Merged
saulecabrera merged 3 commits intobytecodealliance:mainfrom May 16, 2023
Merged
Conversation
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Member
|
Random thought about pinned registers -- while this isn't a problem for winch yet the final set of trampolines are the libcall trampolines which enter the runtime for instructions like |
f3cf780 to
becb391
Compare
Member
Author
|
@alexcrichton I've addressed all of your comments; I think this is ready for another pass. I've opted to leave the translation between Regarding your other comments, I've introduced |
Member
Author
|
I'm not exactly sure why the MIRI check is failing. |
e0c7b64 to
5677405
Compare
Member
Author
I realized that all the other tests are ignoring miri, so I applied the same attribute to Winch's tests. |
5677405 to
f0e2850
Compare
This change is a follow-up to bytecodealliance#6262, in which the new trampolines, described [here](https://github.com/bytecodealliance/rfcs/blob/main/accepted/tail-calls.md#new-trampolines-and-vmcallercheckedanyfunc-changes), were introduced to Wasmtime. This change, focuses on the `array-to-wasm`, `native-to-wasm` and `wasm-to-native` trampolines to restore Winch's working state prior to the introduction of the new trampolines. It's worth noting that the new approach for trampolines make it easier to support the `TypedFunc` API in Winch. Prior to the introduction of the new trampolines, it was not obvious how to approach it. This change also introduces a pinned register that will hold the `VMContext` pointer, which is loaded in the `*-to-wasm` trampolines; the `VMContext` register is a pre-requisite to this change to support the `wasm-to-native` trampolines. Lastly, with the introduction of the `VMContext` register and the `wasm-to-native` trampolines, this change also introduces support for calling function imports, which is a variation of the already existing calls to locally defined functions. The other notable piece of this change aside from the trampolines is `winch-codegen`'s dependency on `wasmtime-environ`. Winch is so closely tied to the concepts exposed by the wasmtime crates that it makes sense to tie them together, even though the separation provides some advantages like easier testing in some cases, in the long run, there's probably going to be less need to test Winch in isolation and rather we'd rely more on integration style tests which require all of Wasmtime pieces anyway (fuzzing, spec tests, etc). This change doesn't update the existing implmenetation of `winch_codegen::FuncEnv`, but the intention is to update that part after this change. prtest:full
f0e2850 to
e9fecc4
Compare
alexcrichton
approved these changes
May 16, 2023
Member
alexcrichton
left a comment
There was a problem hiding this comment.
Looks good to me! @cfallin may want to also take a look at some of the assembly bits too though?
0fc38d5 to
0eff833
Compare
Member
Author
|
Agreed; @cfallin, re-added you as a reviewer, let me know if you have any thoughts! |
Member
Yep, I'll take a look now! |
cfallin
approved these changes
May 16, 2023
Member
cfallin
left a comment
There was a problem hiding this comment.
Overall this looks very reasonable -- no objections (and thanks Alex for doing the detailed review here)!
saulecabrera
added a commit
to saulecabrera/wasmtime
that referenced
this pull request
May 17, 2023
This change is a follow-up to bytecodealliance#6358 This change implements the necessary stores of SP, FP and return address for fast stack walking.
saulecabrera
added a commit
that referenced
this pull request
May 22, 2023
* winch(trampolines): Save SP, FP and return address This change is a follow-up to #6358 This change implements the necessary stores of SP, FP and return address for fast stack walking. * Ignore backtrace test on Windows Temporarily ignoring Winch's trap test on Windows while support for unwind information is added.
saulecabrera
added a commit
to saulecabrera/wasmtime
that referenced
this pull request
May 24, 2023
This commit is a small cleanup to drop the usage of the `FuncEnv` trait. In bytecodealliance#6358, we agreed on making `winch-codegen` directly depend on `wasmtime-environ`. Introducing a direct relatioship between `winch-codegen` and `wasmtime-environ` means that the `FuncEnv` trait is no longer serving its original purpose, and we can drop the usage of the trait and use the types exposed from `winch-codegen` directly instead. Even though this change drops the `FuncEnv` trait, it still keeps a `FuncEnv` struct, which is used during code generation.
saulecabrera
added a commit
to saulecabrera/wasmtime
that referenced
this pull request
May 24, 2023
This commit is a small cleanup to drop the usage of the `FuncEnv` trait. In bytecodealliance#6358, we agreed on making `winch-codegen` directly depend on `wasmtime-environ`. Introducing a direct relatioship between `winch-codegen` and `wasmtime-environ` means that the `FuncEnv` trait is no longer serving its original purpose, and we can drop the usage of the trait and use the types exposed from `winch-codegen` directly instead. Even though this change drops the `FuncEnv` trait, it still keeps a `FuncEnv` struct, which is used during code generation.
saulecabrera
added a commit
to saulecabrera/wasmtime
that referenced
this pull request
May 24, 2023
This commit is a small cleanup to drop the usage of the `FuncEnv` trait. In bytecodealliance#6358, we agreed on making `winch-codegen` directly depend on `wasmtime-environ`. Introducing a direct relatioship between `winch-codegen` and `wasmtime-environ` means that the `FuncEnv` trait is no longer serving its original purpose, and we can drop the usage of the trait and use the types exposed from `winch-codegen` directly instead. Even though this change drops the `FuncEnv` trait, it still keeps a `FuncEnv` struct, which is used during code generation.
saulecabrera
added a commit
that referenced
this pull request
May 24, 2023
This commit is a small cleanup to drop the usage of the `FuncEnv` trait. In #6358, we agreed on making `winch-codegen` directly depend on `wasmtime-environ`. Introducing a direct relatioship between `winch-codegen` and `wasmtime-environ` means that the `FuncEnv` trait is no longer serving its original purpose, and we can drop the usage of the trait and use the types exposed from `winch-codegen` directly instead. Even though this change drops the `FuncEnv` trait, it still keeps a `FuncEnv` struct, which is used during code generation.
salewski
pushed a commit
to salewski/wasmtime
that referenced
this pull request
Jun 30, 2023
…6400) This change is a follow-up to bytecodealliance#6358 This change implements the necessary stores of SP, FP and return address for fast stack walking.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 change is a follow-up to
#6262, in which the new trampolines, described here, were introduced to Wasmtime.
This change, focuses on the
array-to-wasm,native-to-wasmandwasm-to-nativetrampolines to restore Winch's working state prior to the introduction of the new trampolines. It's worth noting that the new approach for trampolines make it easier to support theTypedFuncAPI in Winch. Prior to the introduction of the new trampolines, it was not obvious how to approach it.This change also introduces a pinned register that will hold the
VMContextpointer, which is loaded in the*-to-wasmtrampolines; theVMContextregister is a pre-requisite to this change to support thewasm-to-nativetrampolines.Lastly, with the introduction of the
VMContextregister and thewasm-to-nativetrampolines, this change also introduces support for calling function imports, which is a variation of the already existing calls to locally defined functions.The other notable piece of this change aside from the trampolines is
winch-codegen's dependency onwasmtime-environ. Winch is so closely tied to the concepts exposed by the wasmtime crates that it makes sense to tie them together, even though the separation provides some advantages like easier testing in some cases, in the long run, there's probably going to be less need to test Winch in isolation and rather we'd rely more on integration style tests which require all of Wasmtimepieces anyway (fuzzing, spec tests, etc).
This change doesn't update the existing implmenetation of
winch_codegen::FuncEnv, but the intention is to update that part after this change.