Skip to content

rust: module inspection prototype #728

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.

Sign up for GitHub

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

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

rust: module inspection prototype #728

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fa608b3
f
axic Feb 4, 2021
c55edd3
f
axic Feb 4, 2021
6e2803e
f
axic Feb 5, 2021
33ff96c
f
axic Feb 5, 2021
e8eb116
Ref type
axic Feb 5, 2021
d57eb01
f
axic May 21, 2022
ff41a4f
f
axic May 21, 2022
6fc8787
f
axic May 28, 2022
4f6b02e
f
axic May 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 72 additions & 19 deletions bindings/rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,20 +146,28 @@ pub fn validate<T: AsRef<[u8]>>(input: T) -> Result<(), Error> {
}

/// A parsed and validated WebAssembly 1.0 module.
pub struct Module(ConstNonNull<sys::FizzyModule>);
pub struct Module {
ptr: ConstNonNull<sys::FizzyModule>,
owned: bool,
Copy link
Member Author

Choose a reason for hiding this comment

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

So I think these are the options for having inspection on the module (which is still available through an instance):
a) either we have this owned flag here and disallow instantiation of a non-owned module
b) introduce a ModuleInspection or some other abstraction which has it, and that can be obtained as module.inspect() or instance.inspect() (this can be seen in one of the earlier commits)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This look ok, although fragile. And I'd rather name the flag instantiated so it is clear it can be instantiated once.

Can't Rust magic memory annotations handle this?

Copy link
Member Author

@axic axic Feb 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can't Rust magic memory annotations handle this?

One workaround, where I maintain an "instance of module" in the instance and return the reference on it. Since instantiate() requires to own the input, a reference can't be passed so it will not compile.

i.e.

struct Instance {
  instance: NonNull<sys::FizzyInstance>,
  module: Module,
}

impl Instance {
    pub fn get_module(&self) -> &Module {
       &self.module
    }
}

// Then this will be a compilation error: instance.get_module().instantiate()

Added nastyness is that in instantiate() need to drop module from being managed (i.e. core::mem::forget(instance.module)), otherwise we end up with the same problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessary to save module in Instance, you can call fizzy_get_instance_module

Copy link
Member Author

Choose a reason for hiding this comment

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

The downside of the ModuleInspection option is that need to assign its lifetime bound to the underlying Module/Instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary to save module in Instance, you can call fizzy_get_instance_module

That is what is being done in this PR. For that one needs to extend the Module as in this PR.

An alternative approach is to save the module in the instance and return it as a reference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I imagined I guess some combination of your approaches: don't change Module, but have Instance::get_module(&self) -> Module that will call fizzy_get_instance_module, wrap in Module and call core::mem::forget before returning it.

}

impl Drop for Module {
fn drop(&mut self) {
unsafe { sys::fizzy_free_module(self.0.as_ptr()) }
if self.owned {
unsafe { sys::fizzy_free_module(self.ptr.as_ptr()) }
}
}
}

impl Clone for Module {
fn clone(&self) -> Self {
let ptr = unsafe { sys::fizzy_clone_module(self.0.as_ptr()) };
let ptr = unsafe { sys::fizzy_clone_module(self.ptr.as_ptr()) };
// TODO: this can be zero in case of memory allocation error, should this be gracefully handled?
assert!(!ptr.is_null());
Module(unsafe { ConstNonNull::new_unchecked(ptr) })
Module {
ptr: unsafe { ConstNonNull::new_unchecked(ptr) },
owned: true,
}
}
}

Expand All @@ -178,27 +186,47 @@ pub fn parse<T: AsRef<[u8]>>(input: &T) -> Result<Module, Error> {
Err(err.error().unwrap())
} else {
debug_assert!(err.code() == 0);
Ok(Module(unsafe { ConstNonNull::new_unchecked(ptr) }))
Ok(Module {
ptr: unsafe { ConstNonNull::new_unchecked(ptr) },
owned: true,
})
}
}

/// A view of a module for inspection.
pub struct ModuleInspector(*const sys::FizzyModule);

impl ModuleInspector {
pub fn has_start_function(&self) -> bool {
unsafe { sys::fizzy_module_has_start_function(self.0) }
}
}

/// An instance of a module.
pub struct Instance(NonNull<sys::FizzyInstance>);
pub struct Instance {
instance: NonNull<sys::FizzyInstance>,
module: Module,
}

impl Drop for Instance {
fn drop(&mut self) {
unsafe { sys::fizzy_free_instance(self.0.as_ptr()) }
unsafe { sys::fizzy_free_instance(self.instance.as_ptr()) }
}
}

impl Module {
/// Create an instance of a module.
// TODO: support imported functions
pub fn instantiate(self) -> Result<Instance, Error> {
if !self.owned {
return Err(Error::InstantiationFailed(
"Trying to instantiate not owned module".into(),
));
}
let mut err = FizzyErrorBox::new();
let ptr = unsafe {
sys::fizzy_instantiate(
self.0.as_ptr(),
self.ptr.as_ptr(),
std::ptr::null(),
0,
std::ptr::null(),
Expand All @@ -216,9 +244,26 @@ impl Module {
Err(err.error().unwrap())
} else {
debug_assert!(err.code() == 0);
Ok(Instance(unsafe { NonNull::new_unchecked(ptr) }))
let instance = unsafe { NonNull::new_unchecked(ptr) };
Ok(Instance {
instance: instance,
module: Module {
ptr: unsafe {
ConstNonNull::new_unchecked(sys::fizzy_get_instance_module(
instance.as_ptr(),
))
},
owned: false,
},
})
}
}

/// Returns true if the module has a start function defined.
/// This function will be executed automatically as part of instantiation.
pub fn has_start_function(&self) -> bool {
unsafe { sys::fizzy_module_has_start_function(self.ptr.as_ptr()) }
}
}

/// A WebAssembly value of i32/i64/f32/f64.
Expand Down Expand Up @@ -408,8 +453,8 @@ impl Instance {
/// # Safety
/// These slices turn invalid if the memory is resized (i.e. via the WebAssembly `memory.grow` instruction)
pub unsafe fn checked_memory_slice(&self, offset: u32, size: usize) -> Result<&[u8], Error> {
let memory_data = sys::fizzy_get_instance_memory_data(self.0.as_ptr());
let memory_size = sys::fizzy_get_instance_memory_size(self.0.as_ptr());
let memory_data = sys::fizzy_get_instance_memory_data(self.instance.as_ptr());
let memory_size = sys::fizzy_get_instance_memory_size(self.instance.as_ptr());
let range = Instance::checked_memory_range(memory_data, memory_size, offset, size)?;
// Slices allow empty length, but data must be a valid pointer.
debug_assert!(!memory_data.is_null());
Expand All @@ -426,8 +471,8 @@ impl Instance {
offset: u32,
size: usize,
) -> Result<&mut [u8], Error> {
let memory_data = sys::fizzy_get_instance_memory_data(self.0.as_ptr());
let memory_size = sys::fizzy_get_instance_memory_size(self.0.as_ptr());
let memory_data = sys::fizzy_get_instance_memory_data(self.instance.as_ptr());
let memory_size = sys::fizzy_get_instance_memory_size(self.instance.as_ptr());
let range = Instance::checked_memory_range(memory_data, memory_size, offset, size)?;
// Slices allow empty length, but data must be a valid pointer.
debug_assert!(!memory_data.is_null());
Expand All @@ -437,7 +482,7 @@ impl Instance {

/// Returns the current memory size, in bytes.
pub fn memory_size(&self) -> usize {
unsafe { sys::fizzy_get_instance_memory_size(self.0.as_ptr()) }
unsafe { sys::fizzy_get_instance_memory_size(self.instance.as_ptr()) }
}

/// Copies memory from `offset` to `target`, for the length of `target.len()`.
Expand All @@ -455,13 +500,21 @@ impl Instance {
}

/// Get a read-only pointer to the module.
unsafe fn get_module(&self) -> *const sys::FizzyModule {
sys::fizzy_get_instance_module(self.0.as_ptr())
unsafe fn get_module_ptr(&self) -> *const sys::FizzyModule {
let ptr = sys::fizzy_get_instance_module(self.instance.as_ptr());
debug_assert!(!ptr.is_null());
ptr
}

/// Get a non-owned module instance.
pub fn get_module(&self) -> &Module {
debug_assert!(!self.module.owned);
&self.module
}

/// Find index of exported function by name.
pub fn find_exported_function_index(&self, name: &str) -> Option<u32> {
let module = unsafe { self.get_module() };
let module = unsafe { self.get_module_ptr() };
let name = CString::new(name).expect("CString::new failed");
let mut func_idx: u32 = 0;
let found = unsafe {
Expand All @@ -482,7 +535,7 @@ impl Instance {
/// This function expects a valid `func_idx` and appropriate number of `args`.
pub unsafe fn unsafe_execute(&mut self, func_idx: u32, args: &[Value]) -> ExecutionResult {
ExecutionResult(sys::fizzy_execute(
self.0.as_ptr(),
self.instance.as_ptr(),
func_idx,
args.as_ptr(),
std::ptr::null_mut(),
Expand All @@ -491,7 +544,7 @@ impl Instance {

/// Find function type for a given index. Must be a valid index otherwise behaviour is undefined.
unsafe fn get_function_type(&self, func_idx: u32) -> sys::FizzyFunctionType {
let module = self.get_module();
let module = self.get_module_ptr();
sys::fizzy_get_function_type(module, func_idx)
}

Expand Down