-
Notifications
You must be signed in to change notification settings - Fork 252
reboot wit-bindgen-go
#1447
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
reboot wit-bindgen-go
#1447
Conversation
|
I just realized I need to make CI run the go tests. I'll push an update. |
alexcrichton
left a comment
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.
Agreed we'll want to ge this running on CI, but overall looking great! I only skimmed codegen though and would mostly lean on tests to exercise bits and bobs
|
I was so focused on the runtime tests that I never ran the codegen ones. Looks like I have more work to do. I'll switch this to draft mode while I do that. |
This commit switches the way tests are built such that all binaries produced are "reactors" or those without a `main` function. This ensures that all imports/exports are fully described with WIT in this repository as opposed to implicitly relying on `wasi:cli/run`. This additionally means that testing in this repository no longer relies on toolchain conventions for the `wasi:cli/run` world making testing a bit more self-contained here, especially in the context of async. This notably came up on bytecodealliance#1447 which should make some of the tests there easier to write and more idiomatic.
This commit switches the way tests are built such that all binaries produced are "reactors" or those without a `main` function. This ensures that all imports/exports are fully described with WIT in this repository as opposed to implicitly relying on `wasi:cli/run`. This additionally means that testing in this repository no longer relies on toolchain conventions for the `wasi:cli/run` world making testing a bit more self-contained here, especially in the context of async. This notably came up on #1447 which should make some of the tests there easier to write and more idiomatic.
77b27d5 to
03bd027
Compare
990c86b to
5602d38
Compare
This re-introduces Go support, with a new implementation written from scratch. While the previous incarnation targeted TinyGo and reused the C generator for lifting and lowering, this one targets "big" Go and handles lifting and lowering itself. In addition, it supports idiomatic, goroutine-based concurrency on top of the component model async ABI, including streams and futures. This implementation is also distinct from the [go-modules](https://github.com/bytecodealliance/go-modules) project, which has a number of limitations (e.g. no async support, not safe for use with "big" Go) and is no longer being actively maintained. Note that the async support currently requires [a small patch](dicej/go@a1c8322) to the Go `runtime` library. I plan to work with the upstream project to make that patch unecessary in the future. Components that don't use async features should work with stock, unpatched Go. One of the tricky parts about lowering values and passing pointers to e.g. `{stream,future}.{read,write}` is that we must tell the Go garbage collector to pin the pointers (i.e. mark the pointee as both immovable and uncollectable) for as long as the host has access to them, but then unpin them promptly afterward to avoid leaks. We do this using `runtime.Pinner` instances, most of which are locally scoped and easy to reason about. However, we must use a couple of globally-scoped pinners as well: one for `cabi_realloc` allocations by the host when it lowers parameters (which we unpin after lifting those parameters) and another for returning values from sync-lifted exports (which we unpin in post-return functions). There are a couple of known missing features which I'll open GitHub issues for: 1. Resource handles are not being restored for unwritten items in the case of an incomplete stream or future write. 2. Importing and/or exporting multiple versions of the same package will cause name clashes. In addition, I plan to expand the test coverage beyond the basics covered in this commit. Signed-off-by: Joel Dice <[email protected]> add ping-pong Go tests Signed-off-by: Joel Dice <[email protected]> address review feedback Signed-off-by: Joel Dice <[email protected]> add wit-bindgen-go to ci/publish.rs Signed-off-by: Joel Dice <[email protected]> enable Go in test CI matrix I'm assuming the default version of Go on the GitHub worker image will be sufficient; if not, we can install a different one explicitly. I'm not enabling the async tests for Go yet, since that will require publishing and installing a build of Go with [this patch](dicej/go@a1c8322); I'll need to do some homework to find out the best way to do that. Signed-off-by: Joel Dice <[email protected]> fix codegen tests Signed-off-by: Joel Dice <[email protected]> update Go runtime tests to use `run` export Signed-off-by: Joel Dice <[email protected]> run cargo fmt Signed-off-by: Joel Dice <[email protected]> remove `publish = false` from crates/go/Cargo.toml Signed-off-by: Joel Dice <[email protected]> temporarily skip async codegen tests for Go Signed-off-by: Joel Dice <[email protected]> add `async = true` config directive to `async-trait-function.wit` Signed-off-by: Joel Dice <[email protected]> indicate that `async-trait-function.wit-no-std` works for Rust Signed-off-by: Joel Dice <[email protected]> add error contexts to `Go::compile` Signed-off-by: Joel Dice <[email protected]> add `actions/setup-go` to workflow Signed-off-by: Joel Dice <[email protected]> add finalizers where appropriate; tweak `StreamReader` API Signed-off-by: Joel Dice <[email protected]> update workspace to 2024 edition Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Good idea. Currently we generate package names like
Yes, currently. I've opened golang/go#76775 as a baby step towards adding BTW, here's an example that shows off concurrent outbound requests, etc. Eventually we'll want to do something like you did here to make that use |
|
I haven’t dug too deeply into the code—how does it handle the lifting/lowering of pointer types? Big Go uses 64-bit pointers, which means that Our plan with the go-modules repo and |
It doesn't assume the canonical ABI layout of a given |
|
Got it. So when a pointer to a list containing pointers is passed, does that require copying on every function call? We were able to guarantee the Go binary representation matched the canonical ABI, to avoid copying. |
Yes, and not only that. We must also pin those pointers for GC safety when converting them to the canonical ABI, and also unpin them when converting from the ABI. The pin/unpin requirement will remain even if/when
Indeed; Big Go required a different approach, though. |
|
What needs to be done to this implementation to allow this to work with TinyGo? |
I don't yet have a great mental model of how TinyGo differs from Big Go, so I don't know. Does TinyGo support e.g. runtime.Pinner? Also, IIRC, TinyGo relies on Asyncify to support concurrency. We'd need to hook into that somehow to bridge goroutines with the component model async ABI and thereby ensure that I/O operations only block the calling goroutine, allowing others to continue running. For Big Go, I had to patch the One way to start would be to make an example app based on bindings generated by this tool and try building it with TinyGo to see what breaks. |
|
BTW, how does |
|
We have a partial fix here tinygo-org/tinygo#4897 but will need a better one. We don't support pinning (yet) but we also don't have a moving collector. (There three garbage collectors available in TinyGo.). Supporting "don't free this yet" flags would fall under something similar to the above PR. Yes, moving to wasip3 would mean moving away from asyncify, which is something we're looking forward to getting rid of. |
You might already know this, but |
|
Yes, that's what I meant by the "don't free this yet" flag. The PR I linked to attempts to avoid that by storing pointers in a map, but having wit-bindgen-go explicity pin/unpin allocations would solve some of the issues that PR had. |
This re-introduces Go support, with a new implementation written from scratch. While the previous incarnation targeted TinyGo and reused the C generator for lifting and lowering, this one targets "big" Go and handles lifting and lowering itself. In addition, it supports idiomatic, goroutine-based concurrency on top of the component model async ABI, including streams and futures.
This implementation is also distinct from the
go-modules project, which has a number of limitations (e.g. no async support, not safe for use with "big" Go) and is no longer being actively maintained.
Note that the async support currently requires a small patch to the Go
runtimelibrary. I plan to work with the upstream project to make that patch unecessary in the future. Components that don't use async features should work with stock, unpatched Go.One of the tricky parts about lowering values and passing pointers to e.g.
{stream,future}.{read,write}is that we must tell the Go garbage collector to pin the pointers (i.e. mark the pointee as both immovable and uncollectable) for as long as the host has access to them, but then unpin them promptly afterward to avoid leaks. We do this usingruntime.Pinnerinstances, most of which are locally scoped and easy to reason about. However, we must use a couple of globally-scoped pinners as well: one forcabi_reallocallocations by the host when it lowers parameters (which we unpin after lifting those parameters) and another for returning values from sync-lifted exports (which we unpin in post-return functions).There are a couple of known missing features which I'll open GitHub issues for:
In addition, I plan to expand the test coverage beyond the basics covered in this commit.