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

wasmtime-c-api: Add support for GC references to the wasm_* C API functions #8326

Closed

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Apr 10, 2024

Does not add support for the wasmtime_* functions yet.

@fitzgen fitzgen requested a review from a team as a code owner April 10, 2024 22:01
@fitzgen fitzgen requested review from alexcrichton and removed request for a team April 10, 2024 22:01
@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Apr 10, 2024
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

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.

Personally I'm feeling pretty uneasy about this having a self-referencing store. This is exclusively for the wasm_* API which none of our wasmtime-* bindings are using as well. Given that my gut would actually be the opposite of what you're thinking which is to basically stop supporting reference types in the wasm_* APIs and only support them in wasmtime_* APIs. The wasm.h header feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references and unsafe and bugs here. Given the fact that we'll have a full implementation available with wasmtime_* I'd be tempted to tailor those to what we need and only have that for now.

Comment on lines -68 to 86
pub type CStoreContext<'a> = StoreContext<'a, StoreData>;
pub type CStoreContextMut<'a> = StoreContextMut<'a, StoreData>;
pub type WasmtimeStoreContext<'a> = StoreContext<'a, WasmtimeStoreData>;
pub type WasmtimeStoreContextMut<'a> = StoreContextMut<'a, WasmtimeStoreData>;

Copy link
Member

Choose a reason for hiding this comment

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

Could these renamings perhaps be split out either commit-wise or PR-wise? It's a bit difficult to disentangle what's going on here with simultaneous renamings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Split out the renaming to #8344

@fitzgen
Copy link
Member Author

fitzgen commented Apr 11, 2024

Personally I'm feeling pretty uneasy about this having a self-referencing store. This is exclusively for the wasm_* API which none of our wasmtime-* bindings are using as well. Given that my gut would actually be the opposite of what you're thinking which is to basically stop supporting reference types in the wasm_* APIs and only support them in wasmtime_* APIs. The wasm.h header feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references and unsafe and bugs here. Given the fact that we'll have a full implementation available with wasmtime_* I'd be tempted to tailor those to what we need and only have that for now.

Yeah, that's fair. I have a branch with wasmtime_* working, and it is indeed a lot cleaner than wasm_* so I'll close this PR in favor of #8344 and then open another new PR on top of that one with the wasmtime_* APIs.

@fitzgen fitzgen closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants