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

feat: add guest binding generator for TinyGo/Go #471

Merged
merged 73 commits into from
Feb 16, 2023

Conversation

Mossaka
Copy link
Member

@Mossaka Mossaka commented Jan 31, 2023

This PR contains changes related to the implementation of a wit-bindgen-gen-guest-go crate, which based on the TinyGo/Go compiler. The codegen.rs was added and all the "tests/codegen" tests have been passed, except a few that include floating numbers. The crate takes a dependency on the C bindgen crate.

Why choosing TinyGo?

Fermyon has an execllent guide that tracks a list of programming languages that compile to wasm. The Go in WebAssembly mentions currently only TinyGo compiler supports wasm32-wasi as a build target.

I expect to rework on this crate to switch to the mainline Go compiler when the upstream Go supports wasm/wasi.

Go's proposal to add wasi target

Why depending on C APIs instead of Wasm ABI?

I chose to implement the Go bindings for the C APIs rather than the Wasm ABIs without a compelling reason. This project started as a one-week hackathon project in September 2022, and I wanted to choose something simple to get started. The CGo and the generated C bindings seemed like a good option as they covered lower-level pointer arithmetic. At the end of the week, I had a codegen that can generate numbers for Go #321. However, I later realized that the implementation of converting Go types to C types and vice versa was not as straightforward as I had initially thought. The optimizations that made the C APIs more idiomatic to C users made the type conversions more challenging.

If there is a strong need to implement the Go bindings using Wasm ABIs in the future, I am open to refactoring the code to accommodate this change.

What is the issue with floating numbers?

It is a known issue in TinyGo and has been fixed in the dev branch. See here for details. I am expecting the next release (0.27) will fix this issue.

The CI failures are all due to this issue. https://github.com/bytecodealliance/wit-bindgen/actions/runs/4051619869/jobs/6970134812

TinyGo has released v0.27.0 which fixed the floating numbers issue. If you are using the latest TinyGo version then you should not run into the issue described above.

What's next?

  • I have not added any guest Go applications in the tests/runtime folder. This will be the next step.
  • I'd also love to work on improving go bindings ergonomics.
  • Use Ns to create local variables to improve readable on the genereated Go code.

Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>

cargo clippy

Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
The issue with "fragment is larger than or outside of variable" is
a known issue and has been resolved in tinygo v0.26.

However, tinygo v0.26 introduces a regression for wasm/wasi target
where the compiler panics when size 0 passed to malloc. See
tinygo-org/tinygo#3303 for details

Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for this @Mossaka! Was there anything else you wanted to add or is this good to go?

@Mossaka
Copy link
Member Author

Mossaka commented Feb 13, 2023

Thanks again for this @Mossaka! Was there anything else you wanted to add or is this good to go?

I'd prefer to run the runtime tests a few more time. I've observed some flakiness in the tests.

@Mossaka Mossaka mentioned this pull request Feb 13, 2023
11 tasks
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka Mossaka closed this Feb 13, 2023
@Mossaka Mossaka reopened this Feb 13, 2023
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@alexcrichton
Copy link
Member

Thanks again for this @Mossaka! I'm going to go ahead and merge this and it can continue to be iterated on in-tree.

@alexcrichton alexcrichton merged commit 16994da into bytecodealliance:main Feb 16, 2023
@Mossaka Mossaka deleted the go-guest-new-2 branch February 16, 2023 20:19
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.

2 participants