From a0dca15fbc009e28495ceb606f4c7f946d22ddf7 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Fri, 27 Mar 2020 13:25:51 -0700 Subject: [PATCH] Add misc. fixes and updates from feedback --- lib/api-tests/tests/high_level_api.rs | 180 +++++++++++++---------- lib/runtime-core/src/instance.rs | 202 +++++++++++--------------- lib/runtime-core/src/macros.rs | 12 +- lib/runtime-core/src/module.rs | 10 +- src/bin/wasmer.rs | 4 +- 5 files changed, 207 insertions(+), 201 deletions(-) diff --git a/lib/api-tests/tests/high_level_api.rs b/lib/api-tests/tests/high_level_api.rs index 22aaf93b3..a469fae11 100644 --- a/lib/api-tests/tests/high_level_api.rs +++ b/lib/api-tests/tests/high_level_api.rs @@ -19,17 +19,16 @@ static TEST_WAT: &str = r#" "#; fn append_custom_section( - wasm: &[u8], + wasm: &mut Vec, custom_section_name: &str, custom_section_contents: &[u8], -) -> Vec { - let mut out = Vec::with_capacity( +) { + wasm.reserve( // 1 for custom section id, 5 for max length of custom section, 5 for max length of name - wasm.len() + custom_section_name.len() + custom_section_contents.len() + 1 + 5 + 5, + custom_section_name.len() + custom_section_contents.len() + 1 + 5 + 5, ); - out.extend(wasm); - out.push(0); + wasm.push(0); let name_length = custom_section_name.as_bytes().len() as u32; let mut name_length_as_leb128 = vec![]; @@ -42,82 +41,117 @@ fn append_custom_section( let mut custom_section_length_as_leb128 = vec![]; write_u32_leb128(&mut custom_section_length_as_leb128, custom_section_length); - out.extend(&custom_section_length_as_leb128); - out.extend(&name_length_as_leb128); - out.extend(custom_section_name.as_bytes()); - out.extend(custom_section_contents); - - out + wasm.extend(&custom_section_length_as_leb128); + wasm.extend(&name_length_as_leb128); + wasm.extend(custom_section_name.as_bytes()); + wasm.extend(custom_section_contents); } #[test] -fn it_works() { - use wasmer::wasm::{Global, Value}; - use wasmer::{export, func, imports, CompiledModule, Func, Module, Table}; - let wasm_no_custom_section = wabt::wat2wasm(TEST_WAT).unwrap(); - let wasm = append_custom_section( - &wasm_no_custom_section, - "test_custom_section", - b"hello, world!", - ); - let wasm = append_custom_section(&wasm, "test_custom_section", b"goodbye, world!"); - // TODO: review error messages when `CompiledModule` is not in scope. My hypothesis is that they'll be - // misleading, if so we may want to do something about it. +fn custom_section_parsing_works() { + use wasmer::{CompiledModule, Module}; + let wasm = { + let mut wasm = wabt::wat2wasm(TEST_WAT).unwrap(); + append_custom_section(&mut wasm, "test_custom_section", b"hello, world!"); + append_custom_section(&mut wasm, "test_custom_section", b"goodbye, world!"); + append_custom_section(&mut wasm, "different_name", b"different value"); + wasm + }; + let module = Module::new(wasm).unwrap(); - let imports = module.imports(); - assert_eq!(imports.len(), 2); - let num_exports = module.exports().count(); - assert_eq!(num_exports, 6); - let ( - mut test_table_seen, - mut test_global_seen, - mut test_ret_2_seen, - mut test_ret_4_seen, - mut set_test_global_seen, - mut update_outside_global_seen, - ) = (false, false, false, false, false, false); - for export in module.exports() { - match (export.name.as_ref(), export.kind) { - ("test-table", export::ExportKind::Table) => { - assert!(!test_table_seen); - test_table_seen = true; - } - ("test-global", export::ExportKind::Global) => { - assert!(!test_global_seen); - test_global_seen = true; - } - ("ret_2", export::ExportKind::Function) => { - assert!(!test_ret_2_seen); - test_ret_2_seen = true; - } - ("ret_4", export::ExportKind::Function) => { - assert!(!test_ret_4_seen); - test_ret_4_seen = true; - } - ("set_test_global", export::ExportKind::Function) => { - assert!(!set_test_global_seen); - set_test_global_seen = true; - } - ("update_outside_global", export::ExportKind::Function) => { - assert!(!update_outside_global_seen); - update_outside_global_seen = true; - } - _ => { - assert!(false, "Unknown export found!"); - } - } - } - assert!(test_table_seen); - assert!(test_global_seen); - assert!(test_ret_2_seen); - assert!(test_ret_4_seen); - assert!(set_test_global_seen); - assert!(update_outside_global_seen); assert_eq!( module.custom_sections("test_custom_section"), Some(&[b"hello, world!".to_vec(), b"goodbye, world!".to_vec()][..]) ); +} + +#[test] +fn module_exports_are_ordered() { + use wasmer::{export, CompiledModule, Module}; + + let wasm = wabt::wat2wasm(TEST_WAT).unwrap(); + // TODO: review error messages when `CompiledModule` is not in scope. My hypothesis is that they'll be + // misleading, if so we may want to do something about it. + let module = Module::new(wasm).unwrap(); + let exports = module.exports().collect::>(); + assert_eq!( + exports, + vec![ + export::ExportDescriptor { + name: "test-table", + kind: export::ExportKind::Table, + }, + export::ExportDescriptor { + name: "test-global", + kind: export::ExportKind::Global, + }, + export::ExportDescriptor { + name: "ret_2", + kind: export::ExportKind::Function, + }, + export::ExportDescriptor { + name: "ret_4", + kind: export::ExportKind::Function, + }, + export::ExportDescriptor { + name: "set_test_global", + kind: export::ExportKind::Function, + }, + export::ExportDescriptor { + name: "update_outside_global", + kind: export::ExportKind::Function, + }, + ] + ); +} + +#[test] +fn module_imports_are_correct() { + use wasmer::{CompiledModule, Module}; + + let wasm = wabt::wat2wasm(TEST_WAT).unwrap(); + // TODO: review error messages when `CompiledModule` is not in scope. My hypothesis is that they'll be + // misleading, if so we may want to do something about it. + let module = Module::new(wasm).unwrap(); + + // TODO: test this more later + let imports = module.imports(); + assert_eq!(imports.len(), 2); +} + +#[test] +fn module_can_be_instantiated() { + use wasmer::wasm::{Global, Value}; + use wasmer::{func, imports, CompiledModule, Module}; + + let wasm = wabt::wat2wasm(TEST_WAT).unwrap(); + // TODO: review error messages when `CompiledModule` is not in scope. My hypothesis is that they'll be + // misleading, if so we may want to do something about it. + let module = Module::new(wasm).unwrap(); + + let outside_global = Global::new_mutable(Value::I32(15)); + let import_object = imports! { + "env" => { + "update-func" => func!(|x: i32| x * 2), + "outside-global" => outside_global.clone(), + } + }; + let instance = module.instantiate(&import_object); + + assert!(instance.is_ok()); +} + +#[test] +fn exports_work() { + use wasmer::wasm::{Global, Value}; + use wasmer::{func, imports, CompiledModule, Func, Module, Table}; + + let wasm = wabt::wat2wasm(TEST_WAT).unwrap(); + // TODO: review error messages when `CompiledModule` is not in scope. My hypothesis is that they'll be + // misleading, if so we may want to do something about it. + let module = Module::new(wasm).unwrap(); + let outside_global = Global::new_mutable(Value::I32(15)); let import_object = imports! { diff --git a/lib/runtime-core/src/instance.rs b/lib/runtime-core/src/instance.rs index 6233c8777..a7d47ae40 100644 --- a/lib/runtime-core/src/instance.rs +++ b/lib/runtime-core/src/instance.rs @@ -181,7 +181,10 @@ impl Instance { /// # Ok(()) /// # } /// ``` - #[deprecated(since = "0.17.0", note = "Please use `instance.exports.get()` instead")] + #[deprecated( + since = "0.17.0", + note = "Please use `instance.exports.get(name)` instead" + )] pub fn func(&self, name: &str) -> ResolveResult> where Args: WasmTypeList, @@ -192,23 +195,7 @@ impl Instance { /// Resolve a function by name. pub fn resolve_func(&self, name: &str) -> ResolveResult { - let export_index = - self.module - .info - .exports - .get(name) - .ok_or_else(|| ResolveError::ExportNotFound { - name: name.to_string(), - })?; - - if let ExportIndex::Func(func_index) = export_index { - Ok(func_index.index()) - } else { - Err(ResolveError::ExportWrongType { - name: name.to_string(), - } - .into()) - } + resolve_func_index(&*self.module, name).map(|fi| fi.index()) } /// This returns the representation of a function that can be called @@ -225,7 +212,10 @@ impl Instance { /// # Ok(()) /// # } /// ``` - #[deprecated(since = "0.17.0", note = "Please use `instance.exports.get()` instead")] + #[deprecated( + since = "0.17.0", + note = "Please use `instance.exports.get(name)` instead" + )] pub fn dyn_func(&self, name: &str) -> ResolveResult { self.exports.get(name) } @@ -324,16 +314,8 @@ impl Instance { } } -/// Private function implementing the inner logic of `Instance::func` -fn func<'a, Args, Rets>( - module: &'a ModuleInner, - inst_inner: &'a InstanceInner, - name: &str, -) -> ResolveResult> -where - Args: WasmTypeList, - Rets: WasmTypeList, -{ +/// Private function used to find the [`FuncIndex`] of a given export. +fn resolve_func_index(module: &ModuleInner, name: &str) -> ResolveResult { let export_index = module .info @@ -344,95 +326,7 @@ where })?; if let ExportIndex::Func(func_index) = export_index { - let sig_index = *module - .info - .func_assoc - .get(*func_index) - .expect("broken invariant, incorrect func index"); - let signature = SigRegistry.lookup_signature_ref(&module.info.signatures[sig_index]); - - if signature.params() != Args::types() || signature.returns() != Rets::types() { - Err(ResolveError::Signature { - expected: (*signature).clone(), - found: Args::types().to_vec(), - })?; - } - - let ctx = match func_index.local_or_import(&module.info) { - LocalOrImport::Local(_) => inst_inner.vmctx, - LocalOrImport::Import(imported_func_index) => unsafe { - inst_inner.import_backing.vm_functions[imported_func_index] - .func_ctx - .as_ref() - } - .vmctx - .as_ptr(), - }; - - let func_wasm_inner = module - .runnable_module - .get_trampoline(&module.info, sig_index) - .unwrap(); - - let (func_ptr, func_env) = match func_index.local_or_import(&module.info) { - LocalOrImport::Local(local_func_index) => ( - module - .runnable_module - .get_func(&module.info, local_func_index) - .unwrap(), - None, - ), - LocalOrImport::Import(import_func_index) => { - let imported_func = &inst_inner.import_backing.vm_functions[import_func_index]; - - ( - NonNull::new(imported_func.func as *mut _).unwrap(), - unsafe { imported_func.func_ctx.as_ref() }.func_env, - ) - } - }; - - let typed_func: Func = - unsafe { Func::from_raw_parts(func_wasm_inner, func_ptr, func_env, ctx) }; - - Ok(typed_func) - } else { - Err(ResolveError::ExportWrongType { - name: name.to_string(), - } - .into()) - } -} - -/// Private function implementing the inner logic of `Instance::dyn_func` -fn dyn_func<'a>( - module: &'a ModuleInner, - inst_inner: &'a InstanceInner, - name: &str, -) -> ResolveResult> { - let export_index = - module - .info - .exports - .get(name) - .ok_or_else(|| ResolveError::ExportNotFound { - name: name.to_string(), - })?; - - if let ExportIndex::Func(func_index) = export_index { - let sig_index = *module - .info - .func_assoc - .get(*func_index) - .expect("broken invariant, incorrect func index"); - let signature = SigRegistry.lookup_signature_ref(&module.info.signatures[sig_index]); - - Ok(DynFunc { - signature, - module: &module, - instance_inner: &inst_inner, - func_index: *func_index, - }) + Ok(*func_index) } else { Err(ResolveError::ExportWrongType { name: name.to_string(), @@ -910,14 +804,82 @@ impl<'a> Exportable<'a> for Global { impl<'a> Exportable<'a> for DynFunc<'a> { fn get_self(exports: &'a Exports, name: &str) -> ResolveResult { let (inst_inner, module) = exports.get_inner(); - dyn_func(module, inst_inner, name) + let func_index = resolve_func_index(module, name)?; + + let sig_index = *module + .info + .func_assoc + .get(func_index) + .expect("broken invariant, incorrect func index"); + let signature = SigRegistry.lookup_signature_ref(&module.info.signatures[sig_index]); + + Ok(DynFunc { + signature, + module: &module, + instance_inner: &inst_inner, + func_index: func_index, + }) } } impl<'a, Args: WasmTypeList, Rets: WasmTypeList> Exportable<'a> for Func<'a, Args, Rets, Wasm> { fn get_self(exports: &'a Exports, name: &str) -> ResolveResult { let (inst_inner, module) = exports.get_inner(); - func(module, inst_inner, name) + + let func_index = resolve_func_index(module, name)?; + + let sig_index = *module + .info + .func_assoc + .get(func_index) + .expect("broken invariant, incorrect func index"); + let signature = SigRegistry.lookup_signature_ref(&module.info.signatures[sig_index]); + + if signature.params() != Args::types() || signature.returns() != Rets::types() { + Err(ResolveError::Signature { + expected: (*signature).clone(), + found: Args::types().to_vec(), + })?; + } + + let ctx = match func_index.local_or_import(&module.info) { + LocalOrImport::Local(_) => inst_inner.vmctx, + LocalOrImport::Import(imported_func_index) => unsafe { + inst_inner.import_backing.vm_functions[imported_func_index] + .func_ctx + .as_ref() + } + .vmctx + .as_ptr(), + }; + + let func_wasm_inner = module + .runnable_module + .get_trampoline(&module.info, sig_index) + .unwrap(); + + let (func_ptr, func_env) = match func_index.local_or_import(&module.info) { + LocalOrImport::Local(local_func_index) => ( + module + .runnable_module + .get_func(&module.info, local_func_index) + .unwrap(), + None, + ), + LocalOrImport::Import(import_func_index) => { + let imported_func = &inst_inner.import_backing.vm_functions[import_func_index]; + + ( + NonNull::new(imported_func.func as *mut _).unwrap(), + unsafe { imported_func.func_ctx.as_ref() }.func_env, + ) + } + }; + + let typed_func: Func = + unsafe { Func::from_raw_parts(func_wasm_inner, func_ptr, func_env, ctx) }; + + Ok(typed_func) } } diff --git a/lib/runtime-core/src/macros.rs b/lib/runtime-core/src/macros.rs index 17114a91d..2c30ca8ea 100644 --- a/lib/runtime-core/src/macros.rs +++ b/lib/runtime-core/src/macros.rs @@ -137,11 +137,12 @@ macro_rules! namespace { #[cfg(test)] mod test { + fn func(arg: i32) -> i32 { + arg + 1 + } + #[test] fn imports_macro_allows_trailing_comma_and_none() { - fn func(arg: i32) -> i32 { - arg + 1 - } let _ = imports! { "env" => { "func" => func!(func), @@ -182,7 +183,12 @@ mod test { "func2" => func!(func), } }; + } + + #[test] + fn imports_macro_allows_trailing_comma_and_none_with_state() { use std::{ffi, ptr}; + fn dtor(_arg: *mut ffi::c_void) {} fn state_creator() -> (*mut ffi::c_void, fn(*mut ffi::c_void)) { (ptr::null_mut() as *mut ffi::c_void, dtor) diff --git a/lib/runtime-core/src/module.rs b/lib/runtime-core/src/module.rs index d31490e40..1f99fa3c2 100644 --- a/lib/runtime-core/src/module.rs +++ b/lib/runtime-core/src/module.rs @@ -54,6 +54,10 @@ pub struct ModuleInfo { pub imported_globals: Map, /// Map of string to export index. + // Implementation note: this should maintain the order that the exports appear in the + // Wasm module. Be careful not to use APIs that may break the order! + // Side note, because this is public we can't actually guarantee that it will remain + // in order. pub exports: IndexMap, /// Vector of data initializers. @@ -193,7 +197,7 @@ impl Module { .exports .iter() .map(|(name, &ei)| ExportDescriptor { - name: name.clone(), + name, kind: ei.into(), }) } @@ -282,9 +286,9 @@ impl Module { // TODO: review this vs `ExportIndex` /// Type describing an export that the [`Module`] provides. #[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct ExportDescriptor { +pub struct ExportDescriptor<'a> { /// The name identifying the export. - pub name: String, + pub name: &'a str, /// The type of the export. pub kind: ExportKind, } diff --git a/src/bin/wasmer.rs b/src/bin/wasmer.rs index 6ba033a44..7c4b9a346 100644 --- a/src/bin/wasmer.rs +++ b/src/bin/wasmer.rs @@ -31,7 +31,7 @@ use wasmer_llvm_backend::{ }; use wasmer_runtime::{ cache::{Cache as BaseCache, FileSystemCache, WasmHash}, - Backend, DynFunc, Func, Value, VERSION, + Backend, DynFunc, Value, VERSION, }; #[cfg(feature = "managed")] use wasmer_runtime_core::tiering::{run_tiering, InteractiveShellContext, ShellExitOperation}; @@ -437,7 +437,7 @@ fn execute_wasi( .instantiate(&import_object) .map_err(|e| format!("Can't instantiate WASI module: {:?}", e))?; - let start: Func<(), ()> = instance + let start: wasmer_runtime::Func<(), ()> = instance .exports .get("_start") .map_err(|e| format!("{:?}", e))?;