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

Update to recent Wasmtime C API changes regarding values #318

Merged
merged 5 commits into from
Jun 28, 2024

Conversation

kpreisser
Copy link
Contributor

@kpreisser kpreisser commented Jun 24, 2024

Update to recent Wasmtime C API changes regarding values.

This includes updates for:

TODOs:

  • Allocating an externref can now fail (by wasmtime_externref_new returning false). With this PR, we will throw a WasmtimeException in that case. We need to check where that exception can be thrown, and whether we need to do any additional clean-up (e.g. when converting arguments for a function call).
  • Check whether we can avoid directly accessing the __private field of externs (which has been remaned in the C API, previously it was index).

Note: The anyref type is not yet supported; it was introduced by the GC Proposal. Support for this type can be added separately, after deciding how the API should look like.

Fixes part of #315 (the WASI part is fixed by #316).

This includes updates for:
- bytecodealliance/wasmtime#8451
- bytecodealliance/wasmtime#8461
- bytecodealliance/wasmtime#8011

TODOs:
- Allocating an `externref` can now fail (by `wasmtime_externref_new` returning `false`). Currently, we throw a `WasmtimeException` in that case. We need to check where that exception can be thrown, and whether we need to do any additional clean-up (e.g. when converting arguments for a function call).
- Check whether it's ok to compare the `__private` field of externs (which has been remaned in the C API, previously it was `index`).
- `anyref` type is not yet supported, but I'm not sure what exactly it is and whether we need to add it.

Fixes bytecodealliance#315
…l, and in results for an untyped callback) when e.g. allocating an `externref` fails.

We don't need to do such a clean-up for unchecked function calls that use `ValueRaw` because in that case we don't own `externref` values.
… struct for equality (which is the case when all members are equal).
…al` objects in the `Store`, which avoids having to explicitly accessing the `__private` field (because the whole struct is now compared).

Additionally, it is more type-safe (since we don't need to cast the `object`).
@kpreisser
Copy link
Contributor Author

I changed the cache in the Store from #235 to use separate ConcurrentDictionary instances for each type, to avoid having to explicitly access the __private field of the externs (instead the whole struct is now compared), and is a bit more type-safe since we don't need to use a cast.

@kpreisser kpreisser marked this pull request as ready for review June 28, 2024 10:58
Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

Spent the afternoon reviewing and understand the changes. I am fairly new to the project but these changes look sane to me.

Thanks for helping catch the project up to the latest changes in wasmtime!

@jsturtevant jsturtevant merged commit 560a13b into bytecodealliance:main Jun 28, 2024
6 checks passed
@kpreisser kpreisser deleted the c-api-fixes-refvalues branch July 1, 2024 07:02
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