Skip to content
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

Merged
merged 9 commits into from
Jul 2, 2024
Merged

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Dec 4, 2023

This PR adds -target=wasip2 support to TinyGo. It no longer depends on wasi-libc, but. has its own miniature libc that turns the calls into appropriate component calls.

@aykevl
Copy link
Member

aykevl commented Dec 5, 2023

Interesting!
Suggestion: use -target=wasip2 to more clearly specify the WASI version.

@dgryski dgryski force-pushed the dgryski/wasi-preview-2 branch 2 times, most recently from 399c173 to eeb8f09 Compare December 8, 2023 02:42
@dgryski
Copy link
Member Author

dgryski commented Dec 8, 2023

Now we just need to unstub the 29 calls in libc_wasip2.go with Go implementations of the associated C calls using preview2 calls under the hood.

@dgryski dgryski force-pushed the dgryski/wasi-preview-2 branch 2 times, most recently from 4288422 to 681281a Compare December 16, 2023 06:42
@ydnar
Copy link
Contributor

ydnar commented Feb 22, 2024

@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

@dgryski dgryski force-pushed the dgryski/wasi-preview-2 branch 2 times, most recently from 630f6e5 to dcea7bb Compare February 28, 2024 22:51
@dgryski dgryski force-pushed the dgryski/wasi-preview-2 branch 9 times, most recently from 567da72 to b2ae42f Compare April 2, 2024 21:34
@dgryski dgryski marked this pull request as ready for review April 2, 2024 21:37
@dgryski
Copy link
Member Author

dgryski commented Apr 2, 2024

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.

src/syscall/libc_wasip2.go Outdated Show resolved Hide resolved
src/syscall/libc_wasip2.go Outdated Show resolved Hide resolved
src/syscall/libc_wasip2.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ydnar ydnar left a 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,
Copy link
Contributor

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?

loader/goroot.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@aykevl
Copy link
Member

aykevl commented Jun 24, 2024

I'd like to take another look once that PR is merged, but initially it seems to resolve most of my concerns.

@deadprogram
Copy link
Member

@aykevl please take a look now.

Copy link
Member

@aykevl aykevl left a 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?

case types.Uintptr, types.UnsafePointer:
return true
case types.String:
return isPointerOrField
Copy link
Member

@aykevl aykevl Jun 26, 2024

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).

Copy link
Contributor

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/symbol.go Outdated Show resolved Hide resolved
Comment on lines 19 to 25
// 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)
Copy link
Member

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.

@deadprogram
Copy link
Member

@dgryski please resolve merge conflicts. Hopefully for the last time!

@rajatjindal
Copy link
Contributor

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 target wasip2, I am getting following error:

# os
/Users/rajatjindal/go/src/github.com/tinygo-org/tinygo/build/release/tinygo/src/os/stat_unix.go:28:6: statNolog redeclared in this block
/Users/rajatjindal/go/src/github.com/tinygo-org/tinygo/build/release/tinygo/src/os/stat_other.go:15:6:  other declaration of statNolog
/Users/rajatjindal/go/src/github.com/tinygo-org/tinygo/build/release/tinygo/src/os/stat_unix.go:41:6: lstatNolog redeclared in this block
/Users/rajatjindal/go/src/github.com/tinygo-org/tinygo/build/release/tinygo/src/os/stat_other.go:20:6:  other declaration of lstatNolog
/Users/rajatjindal/go/src/github.com/tinygo-org/tinygo/build/release/tinygo/src/os/stat_unix.go:15:16: method File.Stat already declared at /Users/rajatjindal/go/src/github.com/tinygo-org/tinygo/build/release/tinygo/src/os/stat_other.go:10:16

which I was able to fix by making following change to stat_other.go:

diff --git a/src/os/stat_other.go b/src/os/stat_other.go
index d3e0af6e..234c59db 100644
--- a/src/os/stat_other.go
+++ b/src/os/stat_other.go
@@ -1,4 +1,4 @@
-//go:build baremetal || (tinygo.wasm && !wasip1)
+//go:build baremetal || (tinygo.wasm && !wasip1 && !wasip2)
 
 // Copyright 2016 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style

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

@dgryski
Copy link
Member Author

dgryski commented Jun 30, 2024

@rajatjindal Thanks. Those build tags must have broken on one my rebases. Fixed now.

Copy link
Member

@aykevl aykevl left a 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.

Comment on lines +137 to +138
with:
submodules: true
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member

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
Copy link
Member

@aykevl aykevl Jul 1, 2024

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

Copy link
Member

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?

Copy link
Member Author

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.

@ydnar
Copy link
Contributor

ydnar commented Jul 1, 2024

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.

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 (wasm-tools), and the standard library wouldn’t build for GOOS=wasip2 unless those files are present.

On the other hand, the diff history of the generated files might be useful over time.

Do you have an opinion on whether package cm should be copied into src/vendor, rather than a submodule?

@adamreese
Copy link

Hi folks! I've been doing some experiments using this branch and it's some awesome work.

I noticed init() aren't called when using export functions other than _start. When using Stdin/Stdout/Stderr I'm getting a panic because they're being initialized inside an init() https://github.com/tinygo-org/tinygo/pull/4027/files#diff-519198709ce5f198021f73cc0136481092b1104ed96e2f21c3bbbd0ffe819d9dR132

Here's an example and the stacktrace https://github.com/adamreese/component-example/blob/stdout-issue/adder/main.go

$ wasmtime run final.wasm 1 2 add
Error: failed to run main module `final.wasm`

Caused by:
    0: failed to invoke `run` function
    1: error while executing at wasm backtrace:
           0: 0xdf617 - wit-component:shim!indirect-wasi:io/streams@0.2.0-[method]output-stream.blocking-write-and-flush
           1: 0x9aab2 - (internal/wasi/io/v0.2.0/streams.OutputStream).BlockingWriteAndFlush
                           at /Users/areese/p/src/github.com/tinygo-org/tinygo/src/internal/wasi/io/v0.2.0/streams/streams.wit.go:314:46
           2: 0x9fb18 - syscall.writeStream
                           at /Users/areese/p/src/github.com/tinygo-org/tinygo/src/syscall/libc_wasip2.go:205:45
           3: 0x9f7cb - write
                           at /Users/areese/p/src/github.com/tinygo-org/tinygo/src/syscall/libc_wasip2.go:46:21
           4: 0x9a66d - syscall.Write
                           at /Users/areese/p/src/github.com/tinygo-org/tinygo/src/syscall/syscall_libc.go:32:16              - (os.unixFileHandle).Write
                           at /Users/areese/p/src/github.com/tinygo-org/tinygo/src/os/file_anyos.go:110:24
           5: 0x9a0ac - docs:adder/add@0.1.0#add
                           at /Users/areese/p/src/github.com/tinygo-org/tinygo/src/os/file.go:162:14
           6:   0xc4 - <unknown>!<wasm function 2>
           7: 0x94f84 - calculator.wasm!docs:calculator/calculate@0.1.0#eval-expression
           8:   0xad - <unknown>!<wasm function 1>
           9: 0x60c0 - command-6059807ff4c3095a.wasm!command::main::h86d1f8485843e4d5
          10: 0x3182 - command-6059807ff4c3095a.wasm!std::sys_common::backtrace::__rust_begin_short_backtrace::h61307f8ecbd66741
          11: 0x3191 - command-6059807ff4c3095a.wasm!std::rt::lang_start::{{closure}}::h19e864a4fb53b568
          12: 0x54ba2 - command-6059807ff4c3095a.wasm!std::rt::lang_start_internal::hdc5a291adcbf2591
          13: 0x787e - command-6059807ff4c3095a.wasm!__main_void
          14: 0x253a - command-6059807ff4c3095a.wasm!_start
          15: 0x8ddd3 - wit-component:adapter:wasi_snapshot_preview1!wasi:cli/run@0.2.0#run
    2: unknown handle index 0
make: *** [run] Error 1

@ydnar
Copy link
Contributor

ydnar commented Jul 2, 2024

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.

@deadprogram
Copy link
Member

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? 😸

@aykevl
Copy link
Member

aykevl commented Jul 2, 2024

I noticed init() aren't called when using export functions other than _start. When using Stdin/Stdout/Stderr I'm getting a panic because they're being initialized inside an init() https://github.com/tinygo-org/tinygo/pull/4027/files#diff-519198709ce5f198021f73cc0136481092b1104ed96e2f21c3bbbd0ffe819d9dR132

This is a separate issue, can you create a new issue if needed?
But to explain this issue: this is working as intended. You always need to call _start before calling anything else: _start initializes the runtime (among other things).
For some background, see: WebAssembly/WASI#19

@dgryski @ydnar I think this is working as intended? I think _start is still the function that must be called on init to initialize the runtime and such?

@ydnar
Copy link
Contributor

ydnar commented Jul 2, 2024

_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.

@aykevl
Copy link
Member

aykevl commented Jul 2, 2024

Yeah I think we should just squash+merge this.

@dgryski dgryski merged commit 9cb2634 into tinygo-org:dev Jul 2, 2024
17 checks passed
@dgryski
Copy link
Member Author

dgryski commented Jul 2, 2024

🎉

@Mossaka
Copy link

Mossaka commented Jul 2, 2024

Hey folks, this is huge! Thank y'all for your dedication and effort 🎉🥂

@johanbrandhorst
Copy link
Member

Congrats everyone on hitting this milestone!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.