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: Make StoreContextMut accessible in the epoch deadline callback #6075

Merged

Conversation

saulecabrera
Copy link
Member

This commit changes the signature of the Store::epoch_deadline_callback to
take in StoreContextMut instead of a mutable reference to the store's data.

This is useful in cases in which the callback definition needs access to the
Store to be able to use other methods that take in AsContext/AsContextMut,
like for example WasmtimeBacktrace::capture

I decided to only pass in StoreContextMut as the data is accessible through it
via dataand data_mut, but also happy to have them both if there are specific
reasons to do so.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Mar 21, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

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

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

  • peterhuene: wasmtime: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.

Thanks!

My main point of hesitation is that giving out a StoreContextMut is a pretty big "power up" from the prior interface where you only get access to T. Notably the "weird thing" you can do is to recursively invoke more wasm given that you've got the whole store on hand. That's not necessarily a bad thing, though, it's mostly just one where any update to the boundary of Wasmtime and its runtime needs to be carefully scrutinized since we can't rely on Rust's type system to catch errors.

One reason I'm hestitating here is that we don't currently have precedent for this. None of the other libcalls hand out StoreContextMut<T> to the closures provided. Everything else only operates on the &mut T I think which means that this would be the first of its kind in terms of handing out the whole store. The closest analog is host-defined functions which get access to the whole context.

Thinking about this though I don't think that there's any issues here. I believe mutability of the whole store is threaded through correctly so no borrowing violations or such should be happening. Given all that, while it would be weird to recursively invoke wasm in this callback, I believe it's safe to do so.

I've got one minor comment, but otherwise looks good to me 👍

crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved
This commit changes the signature of the `Store::epoch_deadline_callback` to
take in `StoreContextMut` instead of a mutable reference to the store's data.

This is useful in cases in which the callback definition needs access to the
Store to be able to use other methods that take in `AsContext`/`AsContextMut`,
like for example `WasmtimeBacktrace::capture`
@saulecabrera saulecabrera force-pushed the store-in-epoch-callback branch from 1faa806 to 72e48a9 Compare March 23, 2023 14:23
@alexcrichton alexcrichton enabled auto-merge March 23, 2023 14:38
@alexcrichton alexcrichton added this pull request to the merge queue Mar 23, 2023
Merged via the queue into bytecodealliance:main with commit a6925c2 Mar 23, 2023
@saulecabrera saulecabrera deleted the store-in-epoch-callback branch March 23, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants