Gut vm::Export to mostly be crate::Extern
#11229
Conversation
This commit removes the unsafe function `Table::from_wasmtime_table`. This goes a bit further and removes `ExportTable` entirely as well which means that table lookup on a `vm::Instance` directly returns a `wasmtime::Table` without any need to translate back-and-forth.
Like the previous commit, but for tags.
Like the previous commit, but for globals.
Like the previous commit, but for memories.
Like previous commits, but for functions.
github-actions
bot
added
wasmtime:api
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
LGTM modulo one safety comment/question below that should be addressed before merging
| debug_assert!( | ||
| wasmtime_export | ||
| .table | ||
| .ref_type | ||
| .is_canonicalized_for_runtime_usage() | ||
| ); |
| let func_ref = self.get_func_ref(index).unwrap(); | ||
| ExportFunction { func_ref } | ||
| unsafe { crate::Func::from_vm_func_ref(store, func_ref) } |
There was a problem hiding this comment.
Can we actually encapsulate the unsafety here? Because from_vm_func_ref requires that the store owns the given funcref, but because we take the store as an argument but get the funcref from self, we don't technically have the guarantee that the given store owns this funcref. So I think either this function should be unsafe because it relies on passing the correct StoreId in, or we need to get that StoreId from self somehow.
There was a problem hiding this comment.
Good point yeah, I meant to come back and fix this up but forgot to do so. It's a bit unfortunate how infectious the unsafety is here but I agree it's required to be sound regardless.
* Remove `Table::from_wasmtime_table` This commit removes the unsafe function `Table::from_wasmtime_table`. This goes a bit further and removes `ExportTable` entirely as well which means that table lookup on a `vm::Instance` directly returns a `wasmtime::Table` without any need to translate back-and-forth. * Remove `Tag::from_wasmtime_tag` Like the previous commit, but for tags. * Remove `Global::from_wasmtime_global` Like the previous commit, but for globals. * Remove `Memory::from_wasmtime_memory` Like the previous commit, but for memories. * Remove `Func::from_wasmtime_function` Like previous commits, but for functions. * Fix lints * Fill out missing safety comment * Review comments
2 participants
This series of commits removes all of the
vm::Export*types from Wasmtime. These were all relatively unsafe types which were schlepping around unsafe pointers from point A to point B. In lieu of this and given prior refactorings this commit replaces all of these items with the top-level crate definitions (e.g.crate::Funcinstead ofExportFunc). The goal here is to improve safety and generally reduceunsafefunctions and blocks necessary. A nice benefit is that from avm::Instanceyou can now directly load all top-level types such ascrate::Table.