wasmtime / PR #6119 winch: Initial integration with wasmtime · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6119 winch: Initial integration with wasmtime

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:11):

KevinRizzoTO opened PR #6119 from wasmtime-winch-integration to main:

Another big thank you to @alexcrichton for helping out with this and wiring up some of the initial pieces!

What this includes:

wiring up the wasmtime_environ::Compiler trait for Winch with wasmtime

including a new compilation strategy for winch to be used in the Config

the new compiler flag for the CLI, which will help us a lot in testing winch features

an initial implementation of the host-to-wasm trampoline for winch so it can be called in wasmtime

Things to note:

only supports x86_64, as winch is interacting on it should fill out other architectures

support for Typed functions isn't included, these don't use trampolines which winch requires at this time

no changes to support the Wasmtime* calling conventions

the new trampoline doesn't all callee-saved registers prior to the function call, nor does it restore them prior to the epilogue

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:11):

KevinRizzoTO requested elliottt for a review on PR #6119.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:11):

KevinRizzoTO requested wasmtime-compiler-reviewers for a review on PR #6119.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:11):

KevinRizzoTO requested jameysharp for a review on PR #6119.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:11):

KevinRizzoTO requested wasmtime-core-reviewers for a review on PR #6119.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:11):

KevinRizzoTO requested wasmtime-default-reviewers for a review on PR #6119.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:21):

KevinRizzoTO edited PR #6119 from wasmtime-winch-integration to main:

Another big thank you to @alexcrichton for helping out with this and wiring up some of the initial pieces!

What this includes:

wiring up the wasmtime_environ::Compiler trait for Winch with wasmtime

including a new compilation strategy for winch to be used in the Config

the new compiler flag for the CLI, which will help us a lot in testing winch features

an initial implementation of the host-to-wasm trampoline for winch so it can be called in wasmtime

Things to note:

only supports x86_64, as winch is interacting on it should fill out other architectures

support for Typed functions isn't included, these don't use trampolines which winch requires at this time

no changes to support the Wasmtime* calling conventions

the new trampoline doesn't all callee-saved registers prior to the function call, nor does it restore them prior to the epilogue

The intention is to work on the above iteratively, but this first pass will make it much easier to test winch. We've been manually running it's output in a test environment, but moving that over to wasmtime would greatly simplify the process.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:31):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:31):

jameysharp created PR review comment:

I think the Strategy variants for Cranelift and Winch need to be present even if the corresponding features are disabled. Both cli-flags and the build_compiler function here will fail to build if either is disabled otherwise.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:31):

jameysharp created PR review comment:

Could you put this winch feature declaration next to the cranelift feature above? A brief comment resembling the one for the cranelift feature would be nice too.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:31):

jameysharp created PR review comment:

FYI, @fitzgen and I are working on changing how all the trampolines work. No need to block merging this PR for that, but we may call on you to help with the Winch side of that work once we've sorted out how the Cranelift version needs to go.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:31):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2023 at 19:56):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2023 at 20:56):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2023 at 20:56):

saulecabrera created PR review comment:

Thanks for bringing this up @jameysharp. This is something that we've discussed during the Winch meetings. We are aware that these changes are coming with support for tail calls.

but we may call on you to help with the Winch side of that work once we've sorted out how the Cranelift version needs to go.

Absolutely. I'll note that since Winch is still in development we can easily adjust here moving forward.

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera created PR review comment:

Small nit: can we remove this comment?

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera created PR review comment:

I had introduced https://github.com/bytecodealliance/wasmtime/blob/main/winch/environ/src/lib.rs to be able to share the implementation of this trait with Winch's CLI and wasmtime-winch. Could we use it here instead?

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera created PR review comment:

// For now, winch is only supported on x86_64 when running through Wasmtime.

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera created PR review comment:

        // The host to wasm trampoline is currently hard coded (see vmcontext.rs in the
        // wasmtime-runtime crate, VMTrampoline).
        // The first two parameters are VMContexts (not used at this time).
        // The third parameter is the function pointer to call.
        // The fourth parameter is an address to storage space for both the return value and the
        // arguments to the function.

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera created PR review comment:

        // How much we need to adjust the stack pointer by to account for the alignment
        // required by the ISA.

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera created PR review comment:

        // Hard-coding the size in bytes of the trampoline arguments since it's static, based on
        // the current signature we should always have 4 arguments, each of which is 8 bytes.
~~~

Wasmtime GitHub notifications bot (Mar 29 2023 at 21:07):

saulecabrera created PR review comment:

Could you apply the same suggestion to the rest of the comments in the file?

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:19):

KevinRizzoTO updated PR #6119 from wasmtime-winch-integration to main.

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:34):

KevinRizzoTO updated PR #6119 from wasmtime-winch-integration to main.

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:37):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:37):

KevinRizzoTO created PR review comment:

I was able to use this with a few modifications! I had to change the signature to accept a reference to a box so it could e used from the wasmtime-winch side.

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:37):

KevinRizzoTO updated PR #6119 from wasmtime-winch-integration to main.

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:38):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:38):

KevinRizzoTO created PR review comment:

Good catch here! I've removed the feature gates, so we can let the errors throw when building the compiler if the right features aren't set.

Wasmtime GitHub notifications bot (Mar 30 2023 at 01:48):

KevinRizzoTO updated PR #6119 from wasmtime-winch-integration to main.

Wasmtime GitHub notifications bot (Mar 30 2023 at 14:09):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 30 2023 at 14:15):

KevinRizzoTO edited PR #6119:

Another big thank you to @alexcrichton for helping out with this and wiring up some of the initial pieces!

What this includes:

wiring up the wasmtime_environ::Compiler trait for Winch with wasmtime

including a new compilation strategy for winch to be used in the Config

the new compiler flag for the CLI, which will help us a lot in testing winch features

an initial implementation of the host-to-wasm trampoline for winch so it can be called in wasmtime

Things to note:

only supports x86_64, as winch is iterating on it should fill out other architectures

support for Typed functions isn't included, these don't use trampolines which winch requires at this time

no changes to support the Wasmtime* calling conventions

the new trampoline doesn't all callee-saved registers prior to the function call, nor does it restore them prior to the epilogue

The intention is to work on the above iteratively, but this first pass will make it much easier to test winch. We've been manually running it's output in a test environment, but moving that over to wasmtime would greatly simplify the process.

Wasmtime GitHub notifications bot (Mar 30 2023 at 14:16):

KevinRizzoTO edited PR #6119:

Another big thank you to @alexcrichton for helping out with this and wiring up some of the initial pieces!

What this includes:

wiring up the wasmtime_environ::Compiler trait for Winch with wasmtime

including a new compilation strategy for winch to be used in the Config

the new compiler flag for the CLI, which will help us a lot in testing winch features

an initial implementation of the host-to-wasm trampoline for winch so it can be called in wasmtime

Things to note:

only supports x86_64, as winch is iterating on it should fill out other architectures

support for Typed functions isn't included, these don't use trampolines which winch requires at this time

no changes to support the Wasmtime* calling conventions

the new trampoline doesn't save all callee-saved registers prior to the function call, nor does it restore them prior to the epilogue

The intention is to work on the above iteratively, but this first pass will make it much easier to test winch. We've been manually running it's output in a test environment, but moving that over to wasmtime would greatly simplify the process.

Wasmtime GitHub notifications bot (Mar 30 2023 at 18:31):

KevinRizzoTO updated PR #6119.

Wasmtime GitHub notifications bot (Mar 30 2023 at 18:33):

KevinRizzoTO updated PR #6119.

Wasmtime GitHub notifications bot (Mar 30 2023 at 18:34):

KevinRizzoTO updated PR #6119.

Wasmtime GitHub notifications bot (Mar 30 2023 at 18:42):

KevinRizzoTO updated PR #6119.

Wasmtime GitHub notifications bot (Mar 30 2023 at 18:43):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 30 2023 at 18:43):

KevinRizzoTO created PR review comment:

@jameysharp (and possibly @alexcrichton) for additional context, adding this feature here allows the docs creation to pass. Attempting to add it the the wasmtime/Cargo.toml doesn't for some reason :thinking: (at least when run locally)

Wasmtime GitHub notifications bot (Mar 30 2023 at 19:00):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 30 2023 at 19:00):

jameysharp created PR review comment:

I have a hypothesis about proximate cause, but there are several things that don't make sense about this to me.

It seems like different crates are disagreeing about whether the component-model feature should be enabled.

I think wasmtime-winch has it disabled, while wasmtime-environ and wasmtime have it enabled. Adding the winch feature to the wasmtime crate causes it to forward its value for this feature to wasmtime-winch, making it agree with wasmtime-environ.

But I don't see why either wasmtime or wasmtime-environ would have this feature enabled given that cargo doc invocation.

Since Alex is out of the office right now, I'm wondering if @fitzgen can figure anything out about this.

Wasmtime GitHub notifications bot (Mar 30 2023 at 19:23):

KevinRizzoTO created PR review comment:

Ah yeah the only thing that I see that could possibly explain it is this. The curious part is adding winch to this doesn't fix the error when I run the cargo doc command locally :thinking:

Wasmtime GitHub notifications bot (Mar 30 2023 at 19:23):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 01:31):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 01:31):

saulecabrera created PR review comment:

@jameysharp I spent some time on this issue today, but still can't quite figure out why wasmtime-environ is expecting a component_compiler implementation even though the feature is not explicitly enabled for cargo doc and as far as I can tell, it's not set as default feature anywhere in the workspace. One thing that I discovered though, is that everything works as expected if the component-model feature is passed to the cargo doc invocation in the github workflow, which would match the invocation that doc.rs will make according to docs metadata entry:
[package.metadata.docs.rs]
rustdoc-args = ["--cfg", "nightlydoc"]
features = ["component-model"]
So I think that using the component-model feature in the CI workflow is preferred over using winch since this is what docs.rs will use for the documentation anyway.

I was wondering if you'd be good with this change as a short term fix and I'll circle back with Alex when is back.

Wasmtime GitHub notifications bot (Apr 04 2023 at 01:32):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Apr 04 2023 at 11:06):

saulecabrera updated PR #6119.

Wasmtime GitHub notifications bot (Apr 04 2023 at 11:27):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 11:27):

saulecabrera created PR review comment:

Your previous comment around the doc attribute was added in https://github.com/bytecodealliance/wasmtime/pull/6119/commits/4d16393b202f594c79eca7ea080a4a9bf65e6fc6 and the approach described in my previous comment was added in https://github.com/bytecodealliance/wasmtime/pull/6119/commits/82e119485ed68dba122655139e9c6d2393aa6b64.

Wasmtime GitHub notifications bot (Apr 04 2023 at 11:27):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Apr 04 2023 at 17:58):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 17:58):

jameysharp created PR review comment:

Yup, that feels entirely reasonable to me. I appreciate that you also added comments to the CI config mentioning the docs.rs configuration. Would you also add the reverse direction—a brief comment in the docs.rs metadata pointing to the CI config? With that, I think this is good to merge.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:08):

saulecabrera updated PR #6119.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:10):

saulecabrera has enabled auto merge for PR #6119.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:53):

saulecabrera updated PR #6119.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:55):

saulecabrera has enabled auto merge for PR #6119.

Wasmtime GitHub notifications bot (Apr 04 2023 at 19:38):

saulecabrera updated PR #6119.

Wasmtime GitHub notifications bot (Apr 04 2023 at 19:42):

saulecabrera has enabled auto merge for PR #6119.

Wasmtime GitHub notifications bot (Apr 04 2023 at 20:01):

saulecabrera updated PR #6119.

Wasmtime GitHub notifications bot (Apr 05 2023 at 00:10):

saulecabrera updated PR #6119.

Wasmtime GitHub notifications bot (Apr 05 2023 at 00:13):

saulecabrera has enabled auto merge for PR #6119.

Wasmtime GitHub notifications bot (Apr 05 2023 at 01:06):

saulecabrera merged PR #6119.

Last updated: Nov 22 2024 at 16:03 UTC