Merge #1929

1929: fix(vm) Update doc', clarify type fields etc. r=Hywan a=Hywan

# Description

Sequel of #1902. This PR updates the documentation, rename some variables, and clarify some type fields.

# Review

- ~[ ] Add a short description of the the change to the CHANGELOG.md file~ not necessary I guess


Co-authored-by: Ivan Enderlin <ivan@mnt.io>

lib/api/src/externals/function.rs

@@ -126,7 +126,7 @@ impl Function {
                     vmctx,
                     signature: ty,
                     call_trampoline: None,
-                    instance_allocator: None,
+                    instance_ref: None,
                 },
             },
         }
@@ -210,7 +210,7 @@ impl Function {
                     vmctx,
                     signature: ty,
                     call_trampoline: None,
-                    instance_allocator: None,
+                    instance_ref: None,
                 },
             },
         }
@@ -264,7 +264,7 @@ impl Function {
                     signature,
                     kind: VMFunctionKind::Static,
                     call_trampoline: None,
-                    instance_allocator: None,
+                    instance_ref: None,
                 },
             },
         }
@@ -334,7 +334,7 @@ impl Function {
                     vmctx,
                     signature,
                     call_trampoline: None,
-                    instance_allocator: None,
+                    instance_ref: None,
                 },
             },
         }
@@ -387,7 +387,7 @@ impl Function {
                     vmctx,
                     signature,
                     call_trampoline: None,
-                    instance_allocator: None,
+                    instance_ref: None,
                 },
             },
         }

lib/api/src/externals/global.rs

@@ -220,7 +220,7 @@ impl<'a> Exportable<'a> for Global {
         ExportGlobal {
             vm_global: VMExportGlobal {
                 from: self.global.clone(),
-                instance_allocator: None,
+                instance_ref: None,
             },
         }
         .into()

lib/api/src/externals/memory.rs

@@ -250,7 +250,7 @@ impl<'a> Exportable<'a> for Memory {
         ExportMemory {
             vm_memory: VMExportMemory {
                 from: self.memory.clone(),
-                instance_allocator: None,
+                instance_ref: None,
             },
         }
         .into()

lib/api/src/externals/table.rs

@@ -158,7 +158,7 @@ impl<'a> Exportable<'a> for Table {
         ExportTable {
             vm_table: VMExportTable {
                 from: self.table.clone(),
-                instance_allocator: None,
+                instance_ref: None,
             },
         }
         .into()

lib/api/src/native.rs

@@ -75,7 +75,7 @@ where
             signature,
             kind: other.arg_kind,
             call_trampoline: None,
-            instance_allocator: None,
+            instance_ref: None,
         }
     }
 }*/

lib/api/src/types.rs

@@ -85,7 +85,7 @@ impl ValFuncRef for Val {
                 kind: wasmer_vm::VMFunctionKind::Static,
                 vmctx: item.vmctx,
                 call_trampoline: None,
-                instance_allocator: None,
+                instance_ref: None,
             },
         };
         let f = Function::from_vm_export(store, export);

lib/vm/src/export.rs

@@ -50,7 +50,7 @@ pub struct VMExportFunction {
 
     /// A “reference” to the instance through the
     /// `InstanceRef`. `None` if it is a host function.
-    pub instance_allocator: Option<InstanceRef>,
+    pub instance_ref: Option<InstanceRef>,
 }
 
 /// # Safety
@@ -74,8 +74,8 @@ pub struct VMExportTable {
     pub from: Arc<dyn Table>,
 
     /// A “reference” to the instance through the
-    /// `InstanceRef`. `None` if it is a host function.
-    pub instance_allocator: Option<InstanceRef>,
+    /// `InstanceRef`. `None` if it is a host table.
+    pub instance_ref: Option<InstanceRef>,
 }
 
 /// # Safety
@@ -120,8 +120,8 @@ pub struct VMExportMemory {
     pub from: Arc<dyn Memory>,
 
     /// A “reference” to the instance through the
-    /// `InstanceRef`. `None` if it is a host function.
-    pub instance_allocator: Option<InstanceRef>,
+    /// `InstanceRef`. `None` if it is a host memory.
+    pub instance_ref: Option<InstanceRef>,
 }
 
 /// # Safety
@@ -166,8 +166,8 @@ pub struct VMExportGlobal {
     pub from: Arc<Global>,
 
     /// A “reference” to the instance through the
-    /// `InstanceRef`. `None` if it is a host function.
-    pub instance_allocator: Option<InstanceRef>,
+    /// `InstanceRef`. `None` if it is a host global.
+    pub instance_ref: Option<InstanceRef>,
 }
 
 /// # Safety

lib/vm/src/instance/allocator.rs

@@ -14,18 +14,29 @@ use wasmer_types::{LocalMemoryIndex, LocalTableIndex};
 /// This type will free the allocated memory if it's dropped before
 /// being used.
 ///
+/// It is important to remind that [`Instance`] is dynamically-sized
+/// based on `VMOffsets`: The `Instance.vmctx` field represents a
+/// dynamically-sized array that extends beyond the nominal end of the
+/// type. So in order to create an instance of it, we must:
+///
+/// 1. Define the correct layout for `Instance` (size and alignment),
+/// 2. Allocate it properly.
+///
 /// The [`InstanceAllocator::instance_layout`] computes the correct
 /// layout to represent the wanted [`Instance`].
 ///
 /// Then we use this layout to allocate an empty `Instance` properly.
 pub struct InstanceAllocator {
     /// The buffer that will contain the [`Instance`] and dynamic fields.
     instance_ptr: NonNull<Instance>,
+
     /// The layout of the `instance_ptr` buffer.
     instance_layout: Layout,
+
     /// Information about the offsets into the `instance_ptr` buffer for
     /// the dynamic fields.
     offsets: VMOffsets,
+
     /// Whether or not this type has transferred ownership of the
     /// `instance_ptr` buffer. If it has not when being dropped,
     /// the buffer should be freed.
@@ -38,6 +49,7 @@ impl Drop for InstanceAllocator {
             // If `consumed` has not been set, then we still have ownership
             // over the buffer and must free it.
             let instance_ptr = self.instance_ptr.as_ptr();
+
             unsafe {
                 std::alloc::dealloc(instance_ptr as *mut u8, self.instance_layout);
             }
@@ -56,7 +68,7 @@ impl InstanceAllocator {
     pub fn new(
         module: &ModuleInfo,
     ) -> (
-        InstanceAllocator,
+        Self,
         Vec<NonNull<VMMemoryDefinition>>,
         Vec<NonNull<VMTableDefinition>>,
     ) {
@@ -110,9 +122,10 @@ impl InstanceAllocator {
     /// memory in the VM.
     ///
     /// # Safety
-    /// - `instance_ptr` must point to enough memory that all of the
-    ///   offsets in `offsets` point to valid locations in memory,
-    ///   i.e. `instance_ptr` must have been allocated by
+    ///
+    /// - `Self.instance_ptr` must point to enough memory that all of
+    ///   the offsets in `Self.offsets` point to valid locations in
+    ///   memory, i.e. `Self.instance_ptr` must have been allocated by
     ///   `Self::new`.
     unsafe fn memory_definition_locations(&self) -> Vec<NonNull<VMMemoryDefinition>> {
         let num_memories = self.offsets.num_local_memories;
@@ -143,9 +156,10 @@ impl InstanceAllocator {
     /// memory in the VM.
     ///
     /// # Safety
-    /// - `instance_ptr` must point to enough memory that all of the
-    ///   offsets in `offsets` point to valid locations in memory,
-    ///   i.e. `instance_ptr` must have been allocated by
+    ///
+    /// - `Self.instance_ptr` must point to enough memory that all of
+    ///   the offsets in `Self.offsets` point to valid locations in
+    ///   memory, i.e. `Self.instance_ptr` must have been allocated by
     ///   `Self::new`.
     unsafe fn table_definition_locations(&self) -> Vec<NonNull<VMTableDefinition>> {
         let num_tables = self.offsets.num_local_tables;
@@ -169,16 +183,17 @@ impl InstanceAllocator {
         out
     }
 
-    /// Finish preparing by writing the [`Instance`] into memory.
+    /// Finish preparing by writing the [`Instance`] into memory, and
+    /// consume this `InstanceAllocator`.
     pub(crate) fn write_instance(mut self, instance: Instance) -> InstanceRef {
-        // prevent the old state's drop logic from being called as we
+        // Prevent the old state's drop logic from being called as we
         // transition into the new state.
         self.consumed = true;
 
         unsafe {
-            // `instance` is moved at `instance_ptr`. This pointer has
-            // been allocated by `Self::allocate_instance` (so by
-            // `InstanceRef::allocate_instance`.
+            // `instance` is moved at `Self.instance_ptr`. This
+            // pointer has been allocated by `Self::allocate_instance`
+            // (so by `InstanceRef::allocate_instance`).
             ptr::write(self.instance_ptr.as_ptr(), instance);
             // Now `instance_ptr` is correctly initialized!
         }

lib/vm/src/instance/mod.rs

@@ -692,27 +692,18 @@ impl Instance {
     }
 }
 
-/// TODO: update these docs
-/// An `InstanceRef` is responsible to allocate, to deallocate,
+/// An `InstanceRef` is responsible to properly deallocate,
 /// and to give access to an `Instance`, in such a way that `Instance`
 /// is unique, can be shared, safely, across threads, without
 /// duplicating the pointer in multiple locations. `InstanceRef`
 /// must be the only “owner” of an `Instance`.
 ///
 /// Consequently, one must not share `Instance` but
-/// `InstanceRef`. It acts like an Atomically Reference Counted
+/// `InstanceRef`. It acts like an Atomically Reference Counter
 /// to `Instance`. In short, `InstanceRef` is roughly a
 /// simplified version of `std::sync::Arc`.
 ///
-/// It is important to remind that `Instance` is dynamically-sized
-/// based on `VMOffsets`: The `Instance.vmctx` field represents a
-/// dynamically-sized array that extends beyond the nominal end of the
-/// type. So in order to create an instance of it, we must:
-///
-/// 1. Define the correct layout for `Instance` (size and alignment),
-/// 2. Allocate it properly.
-///
-/// This allocation must be freed with [`InstanceRef::deallocate_instance`]
+/// This `InstanceRef` must be freed with [`InstanceRef::deallocate_instance`]
 /// if and only if it has been set correctly. The `Drop` implementation of
 /// [`InstanceRef`] calls its `deallocate_instance` method without
 /// checking if this  property holds, only when `Self.strong` is equal to 1.
@@ -752,20 +743,17 @@ pub struct InstanceRef {
 }
 
 impl InstanceRef {
-    /// Create a new `InstanceRef`. It allocates nothing. It
-    /// fills nothing. The `Instance` must be already valid and
-    /// filled. `self_ptr` and `self_layout` must be the pointer and
-    /// the layout returned by `Self::allocate_self` used to build
-    /// `Self`.
+    /// Create a new `InstanceRef`. It allocates nothing. It fills
+    /// nothing. The `Instance` must be already valid and
+    /// filled.
     ///
     /// # Safety
     ///
     /// `instance` must a non-null, non-dangling, properly aligned,
     /// and correctly initialized pointer to `Instance`. See
-    /// [`InstanceAllocator::new`] for an example of how to correctly use
+    /// [`InstanceAllocator`] for an example of how to correctly use
     /// this API.
-    /// TODO: update docs
-    pub(crate) unsafe fn new(instance: NonNull<Instance>, instance_layout: Layout) -> Self {
+    pub(self) unsafe fn new(instance: NonNull<Instance>, instance_layout: Layout) -> Self {
         Self {
             strong: Arc::new(atomic::AtomicUsize::new(1)),
             instance_layout,
@@ -917,12 +905,12 @@ impl Drop for InstanceRef {
 /// providing useful higher-level API.
 #[derive(Debug, PartialEq)]
 pub struct InstanceHandle {
-    /// The `InstanceRef`. See its documentation to learn more.
+    /// The [`InstanceRef`]. See its documentation to learn more.
     instance: InstanceRef,
 }
 
 impl InstanceHandle {
-    /// Create a new `InstanceHandle` pointing at a new `InstanceRef`.
+    /// Create a new `InstanceHandle` pointing at a new [`InstanceRef`].
     ///
     /// # Safety
     ///
@@ -983,10 +971,10 @@ impl InstanceHandle {
                 vmctx: VMContext {},
             };
 
-            let instance_allocator = allocator.write_instance(instance);
+            let instance_ref = allocator.write_instance(instance);
 
             Self {
-                instance: instance_allocator,
+                instance: instance_ref,
             }
         };
         let instance = handle.instance().as_ref();
@@ -1131,7 +1119,7 @@ impl InstanceHandle {
                     signature,
                     vmctx,
                     call_trampoline,
-                    instance_allocator: Some(instance),
+                    instance_ref: Some(instance),
                 }
                 .into()
             }
@@ -1144,7 +1132,7 @@ impl InstanceHandle {
                 };
                 VMExportTable {
                     from,
-                    instance_allocator: Some(instance),
+                    instance_ref: Some(instance),
                 }
                 .into()
             }
@@ -1157,7 +1145,7 @@ impl InstanceHandle {
                 };
                 VMExportMemory {
                     from,
-                    instance_allocator: Some(instance),
+                    instance_ref: Some(instance),
                 }
                 .into()
             }
@@ -1172,7 +1160,7 @@ impl InstanceHandle {
                 };
                 VMExportGlobal {
                     from,
-                    instance_allocator: Some(instance),
+                    instance_ref: Some(instance),
                 }
                 .into()
             }