-
Notifications
You must be signed in to change notification settings - Fork 855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wasi preview 2 support #4027
base: dev
Are you sure you want to change the base?
wasi preview 2 support #4027
Conversation
Interesting! |
399c173
to
eeb8f09
Compare
Now we just need to unstub the 29 calls in |
4288422
to
681281a
Compare
1cb14ca
to
90028f7
Compare
2279be1
to
6b0b0ef
Compare
@dgryski the HOWTO in this PR might need to be updated to reflect the new build steps (and no need for a Preview 1 adapter!): tinygo build -target=wasip2 -x -o main.wasm ./cmd/wasip2-test
wasm-tools component embed -w wasi:cli/command $(tinygo env TINYGOROOT)/lib/wasi-cli/wit/ main.wasm -o embedded.wasm
wasm-tools component new embedded.wasm -o component.wasm
wasmtime run --wasm component-model component.wasm |
630f6e5
to
dcea7bb
Compare
567da72
to
b2ae42f
Compare
This is now in a state where it's roughly equivalent to wasip1 support. I still plan to add networking support, but that can come in a later PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phone review, WIP
loader/goroot.go
Outdated
@@ -240,6 +240,7 @@ func pathsToOverride(goMinor int, needsSyscallPackage bool) map[string]bool { | |||
"internal/fuzz/": false, | |||
"internal/reflectlite/": false, | |||
"internal/task/": false, | |||
"internal/wasm/": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
532f31d
to
72e73c9
Compare
e984598
to
79b158e
Compare
@dgryski please see this error: main.go:1344: error: wasm-tools failed: exec: "wasm-tools": executable file not found in $PATH |
@deadprogram Yup, I'm looking at the linux.yml workflow now.. |
cd32f3b
to
589e1ec
Compare
3e61972
to
19780b4
Compare
19780b4
to
8f9b569
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very large, but it is also the result of a lot of iterating and is covered by tests.
I say we merge and then continue from there.
@aykevl any comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through most of the PR, and skimmed libc_wasip2.go. Looks reasonable to me though I have a few concerns here and there.
And some more comments that I can't really do inline:
- Can you add wasip2 to builder/builder_test.go? That way we can be sure the LLVM flags etc are the same between Clang and TinyGo.
- I see a lot of places where you use
//go:export
. This is equivalent to//export
, and deviates to from upstream Go. I'd prefer using//export
everywhere.
(I should never have added//go:export
in the first place, I was just being lazy...)
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type int | ||
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type string | ||
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type []byte | ||
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type *int32 | ||
// | ||
//go:wasmimport modulename invalidparam | ||
func invalidparam(a int, b string, c []byte, d *int32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove these tests? I think they're important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're mostly no longer valid with the relaxed types in wasm args. We can re-add the slice test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if these tests were updated instead of removed altogether. ABIs are tricky to get right, and I'd like to have some tests for them.
For example: slices, maps, structs, and func values are still not allowed.
To make it really perfect, it'd be nice to add some tests to compiler/testdata/pragma.go to show that the compiler indeed produces the expected LLVM IR for the given types. Not a blocker though.
Fixes feedback in tinygo-org#4027.
Addresses PR feedback in tinygo-org#4027.
@aykevl PTAL. I think we've addressed everything. |
@aykevl any other comments before we merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a proposal eases my worries about relaxing the //go:wasmimport
rules, though I still have a few small things about the code that I'd like to see improved. (I'm careful with this because experience tells me that once the compiler allows something, it's very difficult to restrict it again in the future - it's much easier the other way round if needed!).
Other than that, it's mostly just minor nits.
case *types.Struct: | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually not part of golang/go#66984 anymore (though it is when it's a pointer-to-struct and pointer-to-array, this probably needs some more logic):
Supporting
struct
and[...]T
by valueA previous version of this proposal included support for passing
struct
and[...]T
types by value by expanding each field recursively into call parameters. This was removed in favor of a simpler initial implementation but could be readded if users require it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, now I think about it, we should definitely be stricter here and also check every field in the struct. Otherwise func(x *struct{field [2]int})
would be legal which it shouldn't be.
case types.String: | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this works more or less by accident (because the way both Go and TinyGo represent strings is the most obvious way to do it so they match).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also matches how WASI 0.2 represents strings (ptr, len pair)
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type int | ||
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type string | ||
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type []byte | ||
// ERROR: //go:wasmimport modulename invalidparam: unsupported parameter type *int32 | ||
// | ||
//go:wasmimport modulename invalidparam | ||
func invalidparam(a int, b string, c []byte, d *int32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if these tests were updated instead of removed altogether. ABIs are tricky to get right, and I'd like to have some tests for them.
For example: slices, maps, structs, and func values are still not allowed.
To make it really perfect, it'd be nice to add some tests to compiler/testdata/pragma.go to show that the compiler indeed produces the expected LLVM IR for the given types. Not a blocker though.
@@ -16,14 +16,6 @@ type Uint uint32 | |||
//go:wasmimport modulename validparam | |||
func validparam(a int32, b uint64, c float64, d unsafe.Pointer, e Uint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good idea to update this to add some new types (like string
and *struct{}
).
src/runtime/runtime_wasm_wasip2.go
Outdated
// libc constructors | ||
// | ||
//export __wasm_call_ctors | ||
func __wasm_call_ctors() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if it was removed then (though that's not a blocker, it just removes noise).
Is there anything else left unaddressed before we merge? |
@dgryski yes there is, namely with
We could decide to deviate from the upstream proposal (especially when it comes to passing structs by value) but if we do that I'd like to see at least a good explanation for that (and the comment should be more than just "this reflects the relaxed type restrictions proposed here" because it doesn't). |
@dgryski can you please rebase against the latest |
3ef711a
to
784eba8
Compare
Rebased against dev and squashed. |
@aykevl So what's the easiest set of short-term hacks to get this merged? Merge with a note indicating the current incompatibilities but that we will update it to match upstream Go before the next Go release that properly includes support for wasip2? |
@dgryski please rebase against
IMO if we do that, we can merge this right now. wasi preview in the naming even is a signal to developers that they may have to deal with at least some breaking changes along the way. And as long as we are compatible with the lastest shipping Go releases then we have done the correct thing. |
@dgryski what did you think about my last request/comment? |
@deadprogram Yeah, I think that's fine. Big Go doesn't have wasip2 support yet, so we're leading the charge here. There are proposals (some of whic have been accepted) but we're still in unknown territory. "Nobody" should be using this yet for any real workload. |
@dgryski but you said yourself that passing structs wasn't going to work with WASI and that more work was needed to lower to primitive types? What about a different solution: the relaxed To be clear, it's these two lines that are a problem: case *types.Struct:
return true This needs to be more strict. We can't just pass any struct, because there is no defined ABI for passing structs and there is no checking whether these fields are allowed. The other changes (loosening integers and pointers) seem safe to me.
The relaxed constraints of |
This PR adds
-target=wasip2
support to TinyGo. It no longer depends onwasi-libc
, but. has its own miniature libc that turns the calls into appropriate component calls.