tinygo icon indicating copy to clipboard operation
tinygo copied to clipboard

wasi preview 2 support

Open dgryski opened this issue 8 months ago • 28 comments

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.

dgryski avatar Dec 04 '23 21:12 dgryski

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

aykevl avatar Dec 05 '23 12:12 aykevl

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 avatar Dec 08 '23 03:12 dgryski

@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

ydnar avatar Feb 22 '24 20:02 ydnar

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.

dgryski avatar Apr 02 '24 21:04 dgryski

Seems like this PR has some actual fail on the CI https://github.com/tinygo-org/tinygo/actions/runs/8623266604/job/23785693630

deadprogram avatar Apr 13 '24 13:04 deadprogram

@dgryski this PR has a merge conflict that needs to be resolved, if you please.

deadprogram avatar Apr 22 '24 17:04 deadprogram

Just rebased against dev with no conflicts; let see what happes...

dgryski avatar Apr 22 '24 18:04 dgryski

@dgryski looks like actual test failure of some kind here https://github.com/tinygo-org/tinygo/actions/runs/8788784842/job/24119419435#step:18:36

deadprogram avatar Apr 22 '24 19:04 deadprogram

@dgryski please see this error:

main.go:1344: error: wasm-tools failed: exec: "wasm-tools": executable file not found in $PATH

deadprogram avatar Apr 24 '24 05:04 deadprogram

@deadprogram Yup, I'm looking at the linux.yml workflow now..

dgryski avatar Apr 24 '24 05:04 dgryski

@aykevl PTAL. I think we've addressed everything.

dgryski avatar May 07 '24 16:05 dgryski

@dgryski see https://github.com/tinygo-org/tinygo/pull/4027/files#r1589100941

deadprogram avatar May 07 '24 17:05 deadprogram

@aykevl any other comments before we merge?

deadprogram avatar May 07 '24 19:05 deadprogram

Is there anything else left unaddressed before we merge?

dgryski avatar May 16 '24 21:05 dgryski

@dgryski yes there is, namely with //go:wasmimport. We are currently incompatible with the upstream proposal for loosening //go:wasmimport:

  • The upstream proposal doesn't allow passing structs as-is.
  • Also, the proposal now requires structs.HostLayout markers in every struct to ensure they match the underlying platform. The only thing TinyGo needs to do here is to check that the field is present: I haven't checked specifically but I'm pretty sure we already match the Canonical ABI for the in-memory layout.
  • We don't validate whether struct fields are allowed. So for example the following code would be allowed, even though it clearly is invalid:
    //go:wasmimport foo bar
    func bar(x *struct{a func()})
    

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

aykevl avatar May 17 '24 14:05 aykevl

@dgryski can you please rebase against the latest dev so this branch can build using LLVM 18?

deadprogram avatar May 25 '24 15:05 deadprogram

Rebased against dev and squashed.

dgryski avatar May 27 '24 16:05 dgryski

@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 avatar May 29 '24 21:05 dgryski

@dgryski please rebase against dev again and correct the merge conflict.

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?

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.

deadprogram avatar Jun 02 '24 07:06 deadprogram

@dgryski what did you think about my last request/comment?

deadprogram avatar Jun 06 '24 17:06 deadprogram

@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 avatar Jun 06 '24 18:06 dgryski

@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 //go:wasmimport rules are reverted and instead the specific packages in-tree (internal/wasi/*) are whitelisted to allow any type?

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.

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?

The relaxed constraints of //go:wasmimport aren't limited to wasip2, but apply to any wasm target. So then we'd be allowing things now and possibly breaking programs again in the future once we (inevitably) restrict things again. A short term hack would be to revert these changes to //go:wasmimport and instead disable checking for internal/wasi/* packages (they're internal so we can do with them whatever we want).

aykevl avatar Jun 07 '24 12:06 aykevl

@dgryski @aykevl from what I can tell, all of the outstanding comments have been addressed, the branch is rebased, and is passing all tests.

Are we ready to merge?

deadprogram avatar Jun 24 '24 11:06 deadprogram

@deadprogram I still need to merge @ydnar 's https://github.com/dgryski/tinygo/pull/16 PR and rebase, then I think we're good.

dgryski avatar Jun 24 '24 17:06 dgryski

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

aykevl avatar Jun 24 '24 18:06 aykevl

@aykevl please take a look now.

deadprogram avatar Jun 25 '24 07:06 deadprogram

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

deadprogram avatar Jun 28 '24 07:06 deadprogram

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

rajatjindal avatar Jun 29 '24 01:06 rajatjindal

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

dgryski avatar Jun 30 '24 18:06 dgryski

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?

ydnar avatar Jul 01 '24 19:07 ydnar