diff --git a/runtime/near-vm/compiler-singlepass/src/codegen_x64.rs b/runtime/near-vm/compiler-singlepass/src/codegen_x64.rs index 981e6be969d..70113e14134 100644 --- a/runtime/near-vm/compiler-singlepass/src/codegen_x64.rs +++ b/runtime/near-vm/compiler-singlepass/src/codegen_x64.rs @@ -1104,19 +1104,13 @@ impl<'a> FuncGen<'a> { call_movs.push((*param, x)); } Location::Memory(_, _) => { - match *param { - Location::GPR(_) => {} - Location::XMM(_) => {} - Location::Memory(reg, _) => { - if reg != GPR::RBP { - return Err(CodegenError { - message: "emit_call_native loc param: unreachable code" - .to_string(), - }); - } - // TODO: Read value at this offset + if let Location::Memory(reg, _) = *param { + if reg != GPR::RBP { + return Err(CodegenError { + message: "emit_call_native loc param: unreachable code".to_string(), + }); } - _ => {} + // TODO: Ensure that the two `Location::Memories` point at the same place? } match *param { Location::Imm64(_) => { diff --git a/runtime/near-vm/engine/src/universal/engine.rs b/runtime/near-vm/engine/src/universal/engine.rs index b7874fcf495..64736705aac 100644 --- a/runtime/near-vm/engine/src/universal/engine.rs +++ b/runtime/near-vm/engine/src/universal/engine.rs @@ -611,8 +611,17 @@ impl UniversalEngineInner { PrimaryMap::new(); for (offset, _) in allocated_functions.drain(0..call_trampoline_count) { - // TODO: What in damnation have you done?! – Bannon let trampoline = unsafe { + // SAFETY: The executable code was written at the specified offset just above. + // TODO: Somewhat concerning is that the `VMTrampoline` does not ensure that the + // lifetime of the function pointer is a subset of the lifetime of the + // `code_memory`. Quite conversely, this `transmute` asserts that `VMTrampoline: + // 'static` and thus that this function pointer is callable even after + // `code_memory` is freed. + // + // As lifetime annotations in Rust cannot influence the codegen, this is not a + // source of undefined behaviour but we do lose static lifetime checks that Rust + // enforces. std::mem::transmute::<_, VMTrampoline>(code_memory.executable_address(offset)) }; allocated_function_call_trampolines.push(trampoline); diff --git a/runtime/near-vm/test-api/Cargo.toml b/runtime/near-vm/test-api/Cargo.toml index 38ba702f1ce..e789bba7bb5 100644 --- a/runtime/near-vm/test-api/Cargo.toml +++ b/runtime/near-vm/test-api/Cargo.toml @@ -29,7 +29,6 @@ wat = { workspace = true, optional = true } # Dependencies and Development Dependencies for `sys`. [target.'cfg(not(target_arch = "wasm32"))'.dependencies] # - Mandatory dependencies for `sys`. -# TODO: properly finish crate renaming and hoist dep to workspace (#8834) near-vm-vm.workspace = true near-vm-compiler.workspace = true near-vm-engine.workspace = true diff --git a/runtime/near-vm/tests/compilers/imports.rs b/runtime/near-vm/tests/compilers/imports.rs index f4a678339cf..a640ce061a2 100644 --- a/runtime/near-vm/tests/compilers/imports.rs +++ b/runtime/near-vm/tests/compilers/imports.rs @@ -393,92 +393,3 @@ fn regression_import_trampolines(config: crate::Config) -> Result<()> { assert_eq!(GAS_CALLED.load(SeqCst), true); Ok(()) } - -// TODO(0-copy): no longer possible to get references to exported entities other than functions -// (we don't need that functionality) -// #[compiler_test(imports)] -// fn multi_use_host_fn_manages_memory_correctly(config: crate::Config) -> Result<()> { -// let store = config.store(); -// let module = get_module2(&store)?; -// -// #[allow(dead_code)] -// #[derive(Clone)] -// struct Env { -// memory: LazyInit, -// } -// -// impl WasmerEnv for Env { -// fn init_with_instance(&mut self, instance: &Instance) -> Result<(), HostEnvInitError> { -// let memory = instance.exports.get_memory("memory")?.clone(); -// self.memory.initialize(memory); -// Ok(()) -// } -// } -// -// let env: Env = Env { -// memory: LazyInit::default(), -// }; -// fn host_fn(env: &Env) { -// assert!(env.memory.get_ref().is_some()); -// println!("Hello, world!"); -// } -// -// let imports = imports! { -// "host" => { -// "fn" => Function::new_native_with_env(&store, env.clone(), host_fn), -// }, -// }; -// let instance1 = Instance::new_with_config(&module, InstanceConfig::with_stack_limit(1000000), &imports)?; -// let instance2 = Instance::new_with_config(&module, InstanceConfig::with_stack_limit(1000000), &imports)?; -// { -// let f1: NativeFunc<(), ()> = instance1.get_native_function("main")?; -// f1.call()?; -// } -// drop(instance1); -// { -// let f2: NativeFunc<(), ()> = instance2.get_native_function("main")?; -// f2.call()?; -// } -// drop(instance2); -// Ok(()) -// } -// -// #[compiler_test(imports)] -// fn instance_local_memory_lifetime(config: crate::Config) -> Result<()> { -// let store = config.store(); -// -// let memory: Memory = { -// let wat = r#"(module -// (memory $mem 1) -// (export "memory" (memory $mem)) -// )"#; -// let module = Module::new(&store, wat)?; -// let instance = Instance::new_with_config(&module, InstanceConfig::with_stack_limit(1000000), &imports! {})?; -// instance.exports.get_memory("memory")?.clone() -// }; -// -// let wat = r#"(module -// (import "env" "memory" (memory $mem 1) ) -// (func $get_at (type $get_at_t) (param $idx i32) (result i32) -// (i32.load (local.get $idx))) -// (type $get_at_t (func (param i32) (result i32))) -// (type $set_at_t (func (param i32) (param i32))) -// (func $set_at (type $set_at_t) (param $idx i32) (param $val i32) -// (i32.store (local.get $idx) (local.get $val))) -// (export "get_at" (func $get_at)) -// (export "set_at" (func $set_at)) -// )"#; -// let module = Module::new(&store, wat)?; -// let imports = imports! { -// "env" => { -// "memory" => memory, -// }, -// }; -// let instance = Instance::new_with_config(&module, InstanceConfig::with_stack_limit(1000000), &imports)?; -// let set_at: NativeFunc<(i32, i32), ()> = instance.get_native_function("set_at")?; -// let get_at: NativeFunc = instance.get_native_function("get_at")?; -// set_at.call(200, 123)?; -// assert_eq!(get_at.call(200)?, 123); -// -// Ok(()) -// } diff --git a/runtime/near-vm/vm/src/instance/ref.rs b/runtime/near-vm/vm/src/instance/ref.rs index 550cc2ede17..e5ea81bfa5f 100644 --- a/runtime/near-vm/vm/src/instance/ref.rs +++ b/runtime/near-vm/vm/src/instance/ref.rs @@ -76,7 +76,12 @@ impl Drop for InstanceInner { } } -/// TODO: Review this super carefully. +// TODO: These implementations have been added to enable instances to contain `Arc`s, however it +// isn’t exactly clear that the `InstanceInner` contents are `Send` or `Sync`, thus effectively +// lying to the compiler. Fortunately in actual use this is not a big deal as the near runtime is +// single-threaded anyway (and `Arc` is only necessary to facilitate caching of the loaded +// modules IIRC; an attempt to remove this cache has been made in the past, but had to be +// restored.) unsafe impl Send for InstanceInner {} unsafe impl Sync for InstanceInner {}