Resolve review comments

lib/compiler-singlepass/src/codegen_x64.rs

@@ -27,9 +27,6 @@ use wasmer_runtime::{
     VMOffsets,
 };
 
-// Placeholder
-use crate::vm::{self, LocalTable};
-
 /// The singlepass per-function code generator.
 pub struct FuncGen<'a> {
     // Immutable properties assigned at creation time.
@@ -1703,8 +1700,6 @@ impl<'a> FuncGen<'a> {
         let state_diff_id = self.fsm.diffs.len();
         self.fsm.diffs.push(diff);
 
-        //println!("initial state = {:?}", self.machine.state);
-
         self.assembler
             .emit_sub(Size::S64, Location::Imm32(32), Location::GPR(GPR::RSP)); // simulate "red zone" if not supported by the platform
 
@@ -1829,9 +1824,9 @@ impl<'a> FuncGen<'a> {
 
         match op {
             Operator::GlobalGet { global_index } => {
-                let global_index = global_index as usize;
+                let global_index = GlobalIndex::new(global_index as usize);
 
-                let ty = type_to_wp_type(self.module.globals[GlobalIndex::new(global_index)].ty);
+                let ty = type_to_wp_type(self.module.globals[global_index].ty);
                 if ty.is_float() {
                     self.fp_stack.push(FloatValue::new(self.value_stack.len()));
                 }
@@ -1844,10 +1839,14 @@ impl<'a> FuncGen<'a> {
 
                 let tmp = self.machine.acquire_temp_gpr().unwrap();
 
-                let src = if global_index < self.module.num_imported_globals {
+                let src = if let Some(local_global_index) = self.module.local_global_index(global_index) {
+                    let offset = self.vmoffsets
+                        .vmctx_vmglobal_definition(local_global_index);
+                    Location::Memory(Machine::get_vmctx_reg(), offset as i32)
+                } else {
                     // Imported globals require one level of indirection.
                     let offset = self.vmoffsets
-                        .vmctx_vmglobal_import_definition(GlobalIndex::new(global_index));
+                        .vmctx_vmglobal_import_definition(global_index);
                     self.emit_relaxed_binop(
                         Assembler::emit_mov,
                         Size::S64,
@@ -1855,13 +1854,8 @@ impl<'a> FuncGen<'a> {
                         Location::GPR(tmp),
                     );
                     Location::Memory(tmp, 0)
-                } else {
-                    let offset = self.vmoffsets
-                        .vmctx_vmglobal_definition(LocalGlobalIndex::new(
-                            global_index - self.module.num_imported_globals,
-                        ));
-                    Location::Memory(Machine::get_vmctx_reg(), offset as i32)
                 };
+
                 self.emit_relaxed_binop(
                     Assembler::emit_mov,
                     Size::S64,
@@ -5534,11 +5528,11 @@ impl<'a> FuncGen<'a> {
                     Location::Memory(
                         Machine::get_vmctx_reg(),
                         self.vmoffsets.vmctx_builtin_function(
-                            if memory_index.index() < self.module.num_imported_memories {
+                            if let Some(_) = self.module.local_memory_index(memory_index) {
-                                VMBuiltinFunctionIndex::get_imported_memory32_size_index()
-                            } else {
                                 VMBuiltinFunctionIndex::get_memory32_size_index()
-                            },
+                            } else {
+                                VMBuiltinFunctionIndex::get_imported_memory32_size_index()
+                            }
                         ) as i32,
                     ),
                     Location::GPR(GPR::RAX),
@@ -5576,11 +5570,11 @@ impl<'a> FuncGen<'a> {
                     Location::Memory(
                         Machine::get_vmctx_reg(),
                         self.vmoffsets.vmctx_builtin_function(
-                            if memory_index.index() < self.module.num_imported_memories {
+                            if let Some(_) = self.module.local_memory_index(memory_index) {
-                                VMBuiltinFunctionIndex::get_imported_memory32_grow_index()
-                            } else {
                                 VMBuiltinFunctionIndex::get_memory32_grow_index()
-                            },
+                            } else {
+                                VMBuiltinFunctionIndex::get_imported_memory32_grow_index()
+                            }
                         ) as i32,
                     ),
                     Location::GPR(GPR::RAX),

lib/compiler-singlepass/src/common_decl.rs

@@ -3,26 +3,6 @@ use std::collections::BTreeMap;
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 pub struct RegisterIndex(pub usize);
 
-/// Information of an inline breakpoint.
-///
-/// TODO: Move this into runtime.
-#[derive(Clone, Debug)]
-pub struct InlineBreakpoint {
-    /// Size in bytes taken by this breakpoint's instruction sequence.
-    pub size: usize,
-
-    /// Type of the inline breakpoint.
-    pub ty: InlineBreakpointType,
-}
-
-/// The type of an inline breakpoint.
-#[repr(u8)]
-#[derive(Copy, Clone, Debug)]
-pub enum InlineBreakpointType {
-    /// A middleware invocation breakpoint.
-    Middleware,
-}
-
 /// A kind of wasm or constant value
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 pub enum WasmAbstractValue {
@@ -44,6 +24,8 @@ pub struct MachineState {
     /// Wasm stack.
     pub wasm_stack: Vec<WasmAbstractValue>,
     /// Private depth of the wasm stack.
+    /// 
+    /// 
     pub wasm_stack_private_depth: usize,
     /// Wasm instruction offset.
     pub wasm_inst_offset: usize,
@@ -52,25 +34,30 @@ pub struct MachineState {
 /// A diff of two `MachineState`s.
 #[derive(Clone, Debug, Default)]
 pub struct MachineStateDiff {
-    /// Last.
+    /// Link to the previous diff this diff is based on, or `None` if this is the first diff.
     pub last: Option<usize>,
-    /// Stack push.
+    
+    /// What values are pushed onto the stack?
     pub stack_push: Vec<MachineValue>,
-    /// Stack pop.
+
+    /// How many values are popped from the stack?
     pub stack_pop: usize,
 
     /// Register diff.
     pub reg_diff: Vec<(RegisterIndex, MachineValue)>,
 
-    /// Previous frame diff.
+    /// Changes in the previous frame's data.
     pub prev_frame_diff: BTreeMap<usize, Option<MachineValue>>, // None for removal
 
-    /// Wasm stack push.
+    /// Values pushed to the Wasm stack.
     pub wasm_stack_push: Vec<WasmAbstractValue>,
-    /// Wasm stack pop.
+
+    /// # of values popped from the Wasm stack.
     pub wasm_stack_pop: usize,
+
     /// Private depth of the wasm stack.
     pub wasm_stack_private_depth: usize, // absolute value; not a diff.
+
     /// Wasm instruction offset.
     pub wasm_inst_offset: usize, // absolute value; not a diff.
 }
@@ -145,15 +132,6 @@ pub struct OffsetInfo {
     pub activate_offset: usize,
 }
 
-/// A map of module state.
-#[derive(Clone, Debug)]
-pub struct ModuleStateMap {
-    /// Local functions.
-    pub local_functions: BTreeMap<usize, FunctionStateMap>,
-    /// Total size.
-    pub total_size: usize,
-}
-
 impl FunctionStateMap {
     /// Creates a new `FunctionStateMap` with the given parameters.
     pub fn new(

lib/compiler-singlepass/src/emitter_x64.rs

@@ -1,4 +1,3 @@
-use crate::common_decl::InlineBreakpointType;
 pub use crate::x64_decl::{GPR, XMM};
 use dynasm::dynasm;
 use dynasmrt::{x64::Assembler, AssemblyOffset, DynamicLabel, DynasmApi, DynasmLabelApi};
@@ -198,7 +197,6 @@ pub trait Emitter {
     fn emit_bkpt(&mut self);
 
     fn emit_host_redirection(&mut self, target: GPR);
-    fn emit_inline_breakpoint(&mut self, ty: InlineBreakpointType);
 
     fn arch_has_itruncf(&self) -> bool {
         false
@@ -1322,15 +1320,6 @@ impl Emitter for Assembler {
         self.emit_jmp_location(Location::GPR(target));
     }
 
-    fn emit_inline_breakpoint(&mut self, ty: InlineBreakpointType) {
-        dynasm!(self
-            ; ud2
-            ; .byte 0x0f ; .byte 0xb9u8 as i8 // ud
-            ; int -1
-            ; .byte ty as u8 as i8
-        );
-    }
-
     fn arch_mov64_imm_offset(&self) -> usize {
         2
     }

lib/compiler-singlepass/src/lib.rs

@@ -18,7 +18,6 @@ mod compiler;
 mod config;
 mod emitter_x64;
 mod machine;
-mod vm;
 mod x64_decl;
 
 pub use crate::compiler::SinglepassCompiler;

lib/compiler-singlepass/src/machine.rs

@@ -288,9 +288,8 @@ impl Machine {
     }
 
     pub fn release_locations_only_osr_state(&mut self, n: usize) {
-        for _ in 0..n {
+        let new_length = self.state.wasm_stack.len().checked_sub(n).expect("release_locations_only_osr_state: length underflow");
-            self.state.wasm_stack.pop().unwrap();
+        self.state.wasm_stack.truncate(new_length);
-        }
     }
 
     pub fn release_locations_keep_state<E: Emitter>(&self, assembler: &mut E, locs: &[Location]) {

lib/compiler-singlepass/src/vm.rs

@@ -1,206 +0,0 @@
-// Placeholder.
-
-use std::{
-    cell::UnsafeCell,
-    ffi::c_void,
-    mem,
-    ptr::{self, NonNull},
-    sync::atomic::{AtomicUsize, Ordering},
-    sync::Once,
-};
-
-pub struct Ctx {}
-
-#[doc(hidden)]
-impl Ctx {
-    #[allow(clippy::erasing_op)] // TODO
-    pub const fn offset_memories() -> u8 {
-        0 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_tables() -> u8 {
-        1 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_globals() -> u8 {
-        2 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_imported_memories() -> u8 {
-        3 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_imported_tables() -> u8 {
-        4 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_imported_globals() -> u8 {
-        5 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_imported_funcs() -> u8 {
-        6 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_signatures() -> u8 {
-        7 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_intrinsics() -> u8 {
-        8 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_stack_lower_bound() -> u8 {
-        9 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_memory_base() -> u8 {
-        10 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_memory_bound() -> u8 {
-        11 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_internals() -> u8 {
-        12 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_interrupt_signal_mem() -> u8 {
-        13 * (mem::size_of::<usize>() as u8)
-    }
-
-    pub const fn offset_local_functions() -> u8 {
-        14 * (mem::size_of::<usize>() as u8)
-    }
-}
-
-pub struct Anyfunc {}
-
-impl Anyfunc {
-    /// Offset to the `func` field.
-    #[allow(clippy::erasing_op)] // TODO
-    pub const fn offset_func() -> u8 {
-        0 * (mem::size_of::<usize>() as u8)
-    }
-
-    /// Offset to the `vmctx` field..
-    pub const fn offset_vmctx() -> u8 {
-        1 * (mem::size_of::<usize>() as u8)
-    }
-
-    /// Offset to the `sig_id` field.
-    pub const fn offset_sig_id() -> u8 {
-        2 * (mem::size_of::<usize>() as u8)
-    }
-
-    /// The size of `Anyfunc`.
-    pub const fn size() -> u8 {
-        mem::size_of::<Self>() as u8
-    }
-}
-
-pub struct LocalTable {}
-
-impl LocalTable {
-    pub fn offset_count() -> usize {
-        0
-    }
-    pub fn offset_base() -> usize {
-        0
-    }
-}
-
-pub struct Intrinsics {}
-
-impl Intrinsics {
-    /// Offset of the `memory_grow` field.
-    #[allow(clippy::erasing_op)]
-    pub const fn offset_memory_grow() -> u8 {
-        (0 * mem::size_of::<usize>()) as u8
-    }
-    /// Offset of the `memory_size` field.
-    pub const fn offset_memory_size() -> u8 {
-        (1 * mem::size_of::<usize>()) as u8
-    }
-}
-
-/// An imported function is a function pointer associated to a
-/// function context.
-#[derive(Debug, Clone)]
-#[repr(C)]
-pub struct ImportedFunc {
-    /// Pointer to the function itself.
-    pub(crate) func: *const Func,
-
-    /// Mutable non-null pointer to [`FuncCtx`].
-    pub(crate) func_ctx: NonNull<FuncCtx>,
-}
-
-// Manually implemented because ImportedFunc contains raw pointers
-// directly; `Func` is marked Send (But `Ctx` actually isn't! (TODO:
-// review this, shouldn't `Ctx` be Send?))
-unsafe impl Send for ImportedFunc {}
-
-impl ImportedFunc {
-    /// Offset to the `func` field.
-    #[allow(clippy::erasing_op)] // TODO
-    pub const fn offset_func() -> u8 {
-        0 * (mem::size_of::<usize>() as u8)
-    }
-
-    /// Offset to the `func_ctx` field.
-    pub const fn offset_func_ctx() -> u8 {
-        1 * (mem::size_of::<usize>() as u8)
-    }
-
-    /// Size of an `ImportedFunc`.
-    pub const fn size() -> u8 {
-        mem::size_of::<Self>() as u8
-    }
-}
-
-/// Represents a function pointer. It is mostly used in the
-/// `typed_func` module within the `wrap` functions, to wrap imported
-/// functions.
-#[repr(transparent)]
-pub struct Func(*mut c_void);
-
-/// Represents a function environment pointer, like a captured
-/// environment of a closure. It is mostly used in the `typed_func`
-/// module within the `wrap` functions, to wrap imported functions.
-#[repr(transparent)]
-pub struct FuncEnv(*mut c_void);
-
-/// Represents a function context. It is used by imported functions
-/// only.
-#[derive(Debug)]
-#[repr(C)]
-pub struct FuncCtx {
-    /// The `Ctx` pointer.
-    pub(crate) vmctx: NonNull<Ctx>,
-
-    /// A pointer to the function environment. It is used by imported
-    /// functions only to store the pointer to the real host function,
-    /// whether it is a regular function, or a closure with or without
-    /// a captured environment.
-    pub(crate) func_env: Option<NonNull<FuncEnv>>,
-}
-
-impl FuncCtx {
-    /// Offset to the `vmctx` field.
-    #[allow(clippy::erasing_op)]
-    pub const fn offset_vmctx() -> u8 {
-        0 * (mem::size_of::<usize>() as u8)
-    }
-
-    /// Offset to the `func_env` field.
-    pub const fn offset_func_env() -> u8 {
-        1 * (mem::size_of::<usize>() as u8)
-    }
-
-    /// Size of a `FuncCtx`.
-    pub const fn size() -> u8 {
-        mem::size_of::<Self>() as u8
-    }
-}