near-vm: review some of the TODOs (#10167)

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(_) => {

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);

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

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<Memory>,
-//     }
-//
-//     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<i32, i32> = instance.get_native_function("get_at")?;
-//     set_at.call(200, 123)?;
-//     assert_eq!(get_at.call(200)?, 123);
-//
-//     Ok(())
-// }

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 {}