Delete vm::Instance::table_grow (#11216)

This commit is a further inch towards #11179 by removing internal
reliance on reading `VMContext` pointers and casting them to `Pin<&mut
Instance>` in various contexts. Notably now only a defined table needs
to be grown, which simplifies the internals of `vm::Instance` ever so
slightly. This is a similar change as #11211 which transitions libcalls
to using `DefinedTableIndex` instead of `TableIndex`.

Author: alexcrichton

crates/cranelift/src/func_environ.rs

@@ -1814,14 +1814,14 @@ impl FuncEnvironment<'_> {
             self.builtin_functions.table_grow_func_ref(&mut pos.func)
         };
 
-        let vmctx = self.vmctx_val(&mut pos);
+        let (table_vmctx, defined_table_index) =
+            self.table_vmctx_and_defined_index(&mut pos, table_index);
 
         let index_type = table.idx_type;
         let delta = self.cast_index_to_i64(&mut pos, delta, index_type);
-        let table_index_arg = pos.ins().iconst(I32, table_index.as_u32() as i64);
         let call_inst = pos
             .ins()
-            .call(grow, &[vmctx, table_index_arg, delta, init_value]);
+            .call(grow, &[table_vmctx, defined_table_index, delta, init_value]);
         let result = pos.func.dfg.first_result(call_inst);
         Ok(self.convert_pointer_to_index_type(builder.cursor(), result, index_type, false))
     }
@@ -2772,6 +2772,42 @@ impl FuncEnvironment<'_> {
         }
     }
 
+    /// Returns two `ir::Value`s, the first of which is the vmctx for the table
+    /// `index` and the second of which is the `DefinedTableIndex` for `index`.
+    ///
+    /// Handles internally whether `index` is an imported table or not.
+    fn table_vmctx_and_defined_index(
+        &mut self,
+        pos: &mut FuncCursor,
+        index: TableIndex,
+    ) -> (ir::Value, ir::Value) {
+        // NB: the body of this method is similar to
+        // `memory_vmctx_and_defined_index` above.
+        let cur_vmctx = self.vmctx_val(pos);
+        match self.module.defined_table_index(index) {
+            Some(index) => (cur_vmctx, pos.ins().iconst(I32, i64::from(index.as_u32()))),
+            None => {
+                let vmimport = self.offsets.vmctx_vmtable_import(index);
+
+                let vmctx = pos.ins().load(
+                    self.isa.pointer_type(),
+                    ir::MemFlags::trusted(),
+                    cur_vmctx,
+                    i32::try_from(vmimport + u32::from(self.offsets.vmtable_import_vmctx()))
+                        .unwrap(),
+                );
+                let index = pos.ins().load(
+                    ir::types::I32,
+                    ir::MemFlags::trusted(),
+                    cur_vmctx,
+                    i32::try_from(vmimport + u32::from(self.offsets.vmtable_import_index()))
+                        .unwrap(),
+                );
+                (vmctx, index)
+            }
+        }
+    }
+
     pub fn translate_memory_grow(
         &mut self,
         builder: &mut FunctionBuilder<'_>,

crates/environ/src/vmoffsets.rs

@@ -706,6 +706,18 @@ impl<P: PtrSize> VMOffsets<P> {
         0 * self.pointer_size()
     }
 
+    /// The offset of the `vmctx` field.
+    #[inline]
+    pub fn vmtable_import_vmctx(&self) -> u8 {
+        1 * self.pointer_size()
+    }
+
+    /// The offset of the `index` field.
+    #[inline]
+    pub fn vmtable_import_index(&self) -> u8 {
+        2 * self.pointer_size()
+    }
+
     /// Return the size of `VMTableImport`.
     #[inline]
     pub fn size_of_vmtable_import(&self) -> u8 {

crates/wasmtime/src/runtime/vm/instance.rs

@@ -719,19 +719,7 @@ impl Instance {
     ///
     /// Returns `None` if table can't be grown by the specified amount of
     /// elements, or if `init_value` is the wrong type of table element.
-    pub(crate) fn table_grow(
-        self: Pin<&mut Self>,
-        store: &mut dyn VMStore,
-        table_index: TableIndex,
-        delta: u64,
-        init_value: TableElement,
-    ) -> Result<Option<usize>, Error> {
-        self.with_defined_table_index_and_instance(table_index, |i, instance| {
-            instance.defined_table_grow(store, i, delta, init_value)
-        })
-    }
-
-    fn defined_table_grow(
+    pub(crate) fn defined_table_grow(
         mut self: Pin<&mut Self>,
         store: &mut dyn VMStore,
         table_index: DefinedTableIndex,

crates/wasmtime/src/runtime/vm/libcalls.rs

@@ -70,7 +70,8 @@ use core::ptr::NonNull;
 #[cfg(feature = "threads")]
 use core::time::Duration;
 use wasmtime_environ::{
-    DataIndex, DefinedMemoryIndex, ElemIndex, FuncIndex, MemoryIndex, TableIndex, Trap,
+    DataIndex, DefinedMemoryIndex, DefinedTableIndex, ElemIndex, FuncIndex, MemoryIndex,
+    TableIndex, Trap,
 };
 #[cfg(feature = "wmemcheck")]
 use wasmtime_wmemcheck::AccessError::{
@@ -223,20 +224,19 @@ unsafe impl HostResultHasUnwindSentinel for Option<AllocationSize> {
 unsafe fn table_grow_func_ref(
     store: &mut dyn VMStore,
     mut instance: Pin<&mut Instance>,
-    table_index: u32,
+    defined_table_index: u32,
     delta: u64,
     init_value: *mut u8,
 ) -> Result<Option<AllocationSize>> {
-    let table_index = TableIndex::from_u32(table_index);
-
-    let element = match instance.as_mut().table_element_type(table_index) {
-        TableElementType::Func => NonNull::new(init_value.cast::<VMFuncRef>()).into(),
-        TableElementType::GcRef => unreachable!(),
-        TableElementType::Cont => unreachable!(),
-    };
-
+    let defined_table_index = DefinedTableIndex::from_u32(defined_table_index);
+    let table_index = instance.env_module().table_index(defined_table_index);
+    debug_assert!(matches!(
+        instance.as_mut().table_element_type(table_index),
+        TableElementType::Func,
+    ));
+    let element = NonNull::new(init_value.cast::<VMFuncRef>()).into();
     let result = instance
-        .table_grow(store, table_index, delta, element)?
+        .defined_table_grow(store, defined_table_index, delta, element)?
         .map(AllocationSize);
     Ok(result)
 }
@@ -246,27 +246,28 @@ unsafe fn table_grow_func_ref(
 unsafe fn table_grow_gc_ref(
     store: &mut dyn VMStore,
     mut instance: Pin<&mut Instance>,
-    table_index: u32,
+    defined_table_index: u32,
     delta: u64,
     init_value: u32,
 ) -> Result<Option<AllocationSize>> {
-    let table_index = TableIndex::from_u32(table_index);
-
-    let element = match instance.as_mut().table_element_type(table_index) {
-        TableElementType::Func => unreachable!(),
-        TableElementType::GcRef => VMGcRef::from_raw_u32(init_value)
-            .map(|r| {
-                store
-                    .store_opaque_mut()
-                    .unwrap_gc_store_mut()
-                    .clone_gc_ref(&r)
-            })
-            .into(),
-        TableElementType::Cont => unreachable!(),
-    };
+    let defined_table_index = DefinedTableIndex::from_u32(defined_table_index);
+    let table_index = instance.env_module().table_index(defined_table_index);
+    debug_assert!(matches!(
+        instance.as_mut().table_element_type(table_index),
+        TableElementType::GcRef,
+    ));
+
+    let element = VMGcRef::from_raw_u32(init_value)
+        .map(|r| {
+            store
+                .store_opaque_mut()
+                .unwrap_gc_store_mut()
+                .clone_gc_ref(&r)
+        })
+        .into();
 
     let result = instance
-        .table_grow(store, table_index, delta, element)?
+        .defined_table_grow(store, defined_table_index, delta, element)?
         .map(AllocationSize);
     Ok(result)
 }
@@ -275,24 +276,22 @@ unsafe fn table_grow_gc_ref(
 unsafe fn table_grow_cont_obj(
     store: &mut dyn VMStore,
     mut instance: Pin<&mut Instance>,
-    table_index: u32,
+    defined_table_index: u32,
     delta: u64,
     // The following two values together form the initial Option<VMContObj>.
     // A None value is indicated by the pointer being null.
     init_value_contref: *mut u8,
     init_value_revision: u64,
 ) -> Result<Option<AllocationSize>> {
-    let init_value = VMContObj::from_raw_parts(init_value_contref, init_value_revision);
-
-    let table_index = TableIndex::from_u32(table_index);
-
-    let element = match instance.as_mut().table_element_type(table_index) {
-        TableElementType::Cont => init_value.into(),
-        _ => panic!("Wrong table growing function"),
-    };
-
+    let defined_table_index = DefinedTableIndex::from_u32(defined_table_index);
+    let table_index = instance.env_module().table_index(defined_table_index);
+    debug_assert!(matches!(
+        instance.as_mut().table_element_type(table_index),
+        TableElementType::Cont,
+    ));
+    let element = VMContObj::from_raw_parts(init_value_contref, init_value_revision).into();
     let result = instance
-        .table_grow(store, table_index, delta, element)?
+        .defined_table_grow(store, defined_table_index, delta, element)?
         .map(AllocationSize);
     Ok(result)
 }

crates/wasmtime/src/runtime/vm/vmcontext.rs

@@ -179,6 +179,14 @@ mod test_vmtable {
             offset_of!(VMTableImport, from),
             usize::from(offsets.vmtable_import_from())
         );
+        assert_eq!(
+            offset_of!(VMTableImport, vmctx),
+            usize::from(offsets.vmtable_import_vmctx())
+        );
+        assert_eq!(
+            offset_of!(VMTableImport, index),
+            usize::from(offsets.vmtable_import_index())
+        );
     }
 
     #[test]

winch/codegen/src/codegen/mod.rs

@@ -1590,6 +1590,36 @@ where
             }
         }
     }
+
+    /// Same as `prepare_builtin_defined_memory_arg`, but for tables.
+    pub fn prepare_builtin_defined_table_arg(
+        &mut self,
+        table: TableIndex,
+        defined_index_at: usize,
+        builtin: BuiltinFunction,
+    ) -> Result<Callee> {
+        match self.env.translation.module.defined_table_index(table) {
+            Some(defined) => {
+                self.context
+                    .stack
+                    .insert_many(defined_index_at, &[defined.as_u32().try_into()?]);
+                Ok(Callee::Builtin(builtin))
+            }
+            None => {
+                let vmimport = self.env.vmoffsets.vmctx_vmtable_import(table);
+                let vmctx_offset = vmimport + u32::from(self.env.vmoffsets.vmtable_import_vmctx());
+                let index_offset = vmimport + u32::from(self.env.vmoffsets.vmtable_import_index());
+                let index_addr = self.masm.address_at_vmctx(index_offset)?;
+                let index_dst = self.context.reg_for_class(RegClass::Int, self.masm)?;
+                self.masm
+                    .load(index_addr, writable!(index_dst), OperandSize::S32)?;
+                self.context
+                    .stack
+                    .insert_many(defined_index_at, &[Val::reg(index_dst, WasmValType::I32)]);
+                Ok(Callee::BuiltinWithDifferentVmctx(builtin, vmctx_offset))
+            }
+        }
+    }
 }
 
 /// Returns the index of the [`ControlStackFrame`] for the given

winch/codegen/src/visitor.rs

@@ -1718,14 +1718,10 @@ where
         // but the builtin function expects the init value as the last
         // argument.
         self.context.stack.inner_mut().swap(len - 1, len - 2);
-        self.context.stack.insert_many(at, &[table.try_into()?]);
 
-        FnCall::emit::<M>(
-            &mut self.env,
-            self.masm,
-            &mut self.context,
-            Callee::Builtin(builtin.clone()),
-        )?;
+        let builtin = self.prepare_builtin_defined_table_arg(table_index, at, builtin)?;
+
+        FnCall::emit::<M>(&mut self.env, self.masm, &mut self.context, builtin)?;
 
         Ok(())
     }