-
Notifications
You must be signed in to change notification settings - Fork 913
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
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
I'd like to take another look once that PR is merged, but initially it seems to resolve most of my concerns. |
11a6e1d
to
13e93c8
Compare
@aykevl please take a look now. |
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 mostly looked at compiler/symbol.go which looks good to go (but with some optional suggestions).
Were there any other changes?
compiler/symbol.go
Outdated
case types.Uintptr, types.UnsafePointer: | ||
return true | ||
case types.String: | ||
return isPointerOrField |
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.
Passing strings as parameters is allowed by the spec (and will do the right thing if I'm not mistaken).
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.
Fixed in dgryski#17
compiler/testdata/errors.go
Outdated
// 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.
It would still be nice to update these tests instead of removing them.
@dgryski please resolve merge conflicts. Hopefully for the last time! |
13e93c8
to
08a2541
Compare
hello folks, thank you for your amazing work on this project as well as this feature. 👍 I was trying out the latest changes in this branch, and ran into a problem that I thought I will mention here. when building a project for
which I was able to fix by making following change to
could you please confirm if that is the right fix (and if so, if you could pls include that fix in changes) or kindly point me in the right direction. thank you |
Address PR feedback.
Address PR feedback.
compiler: refine wasm types proposal
@rajatjindal Thanks. Those build tags must have broken on one my rebases. Fixed now. |
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 did an in-depth review and it looks good to me.
This is probably far too late (I don't want to delay this PR any further), but what about not checking in all those generated files?
Alternatives:
- generating the files on build, like with
gen-device
- putting those files in a separate repository
It's not a big issue, but I feel like adding so many generated files in the repo leads to churn. But maybe that's just me.
with: | ||
submodules: 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 should not be necessary.
The whole point of test-linux-build is that it uses the release tarball created in build-linux and tests whether it works. If you need these submodules, it seems like some files aren't included in the release tarball that should be in there.
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 probably needed for the generated files which were originally a submodule.
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.
Package cm
is a submodule in vendor
. Should we hard vendor 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.
@ydnar no that seems fine, that directory is in src/ and is already copied to the release directory.
//go:build darwin || tinygo.wasm | ||
//go:build darwin || wasip1 || wasip2 || wasm |
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 removes support for crypto/rand in wasm_unknown, which is probably a good thing (since it doesn't have an underlying system to read random values from).
@@ -0,0 +1,76 @@ | |||
//go:build wasip2 | |||
|
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.
What about merging this file and runtime_wasm_wasip2.go?
Or was there a reason you kept them separate?
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.
Probably just an artifact of how the port was initially written. We can do a bit more cleanup later.
The generated files are part of (and compiled with every program that uses) the standard library. I guess we could generate them in CI and build. We’d need to add Rust dependencies ( On the other hand, the diff history of the generated files might be useful over time. Do you have an opinion on whether package |
Hi folks! I've been doing some experiments using this branch and it's some awesome work. I noticed Here's an example and the stacktrace https://github.com/adamreese/component-example/blob/stdout-issue/adder/main.go
|
We could have the code export both _start and _initialize, but it would break on certain runtimes (e.g. Node). This is a separate area of development (see golang/go#42372 and #4082). Ideally it could detect missing func main() and export _initialize instead of _start. |
I would like to propose that we squash/merge this PR now, and make any further changes in smaller PRs. @dgryski do you want to do this? 😸 |
@dgryski @ydnar I think this is working as intended? I think |
_start should call init and then main. _initialize should call init (and initialize the runtime) but not call main. Host expects to call other exported functions. |
Yeah I think we should just squash+merge this. |
🎉 |
Hey folks, this is huge! Thank y'all for your dedication and effort 🎉🥂 |
Congrats everyone on hitting this milestone!!! |
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.