From 887a3f454a4f2b511d82b190abd9a5b014faff68 Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Wed, 20 Dec 2023 12:19:02 +0100 Subject: [PATCH] Apply review and some clean-up --- core/engine/src/builtins/function/mod.rs | 6 +- core/engine/src/builtins/generator/mod.rs | 10 ++-- core/engine/src/vm/call_frame/mod.rs | 58 ++++++++++--------- core/engine/src/vm/mod.rs | 21 ++++--- core/engine/src/vm/opcode/generator/mod.rs | 2 +- .../src/vm/opcode/rest_parameter/mod.rs | 6 +- 6 files changed, 57 insertions(+), 46 deletions(-) diff --git a/core/engine/src/builtins/function/mod.rs b/core/engine/src/builtins/function/mod.rs index cc466f1bfc0..45b3358c1af 100644 --- a/core/engine/src/builtins/function/mod.rs +++ b/core/engine/src/builtins/function/mod.rs @@ -1035,12 +1035,12 @@ fn function_construct( .with_env_fp(env_fp) .with_flags(CallFrameFlags::CONSTRUCT); - context.vm.push_frame(frame); - let len = context.vm.stack.len(); + context.vm.push_frame(frame); + // NOTE(HalidOdat): +1 because we insert `this` value below. - context.vm.frame_mut().fp = len as u32 + 1; + context.vm.frame_mut().rp = len as u32 + 1; let mut last_env = 0; diff --git a/core/engine/src/builtins/generator/mod.rs b/core/engine/src/builtins/generator/mod.rs index a06515cc3b3..c9760405df5 100644 --- a/core/engine/src/builtins/generator/mod.rs +++ b/core/engine/src/builtins/generator/mod.rs @@ -69,13 +69,13 @@ impl GeneratorContext { let mut frame = context.vm.frame().clone(); frame.environments = context.vm.environments.clone(); frame.realm = context.realm().clone(); - let fp = frame.restore_fp() as usize; + let fp = frame.fp() as usize; let stack = context.vm.stack.split_off(fp); - frame.fp = CallFrame::FUNCTION_PROLOGUE + frame.argument_count; + frame.rp = CallFrame::FUNCTION_PROLOGUE + frame.argument_count; // NOTE: Since we get a pre-built call frame with stack, and we reuse them. - // So we don't need to push the locals in subsuquent calls. + // So we don't need to push the locals in subsequent calls. frame.flags |= CallFrameFlags::LOCALS_ALREADY_PUSHED; Self { @@ -93,10 +93,10 @@ impl GeneratorContext { ) -> CompletionRecord { std::mem::swap(&mut context.vm.stack, &mut self.stack); let frame = self.call_frame.take().expect("should have a call frame"); - let fp = frame.fp; + let rp = frame.rp; context.vm.push_frame(frame); - context.vm.frame_mut().fp = fp; + context.vm.frame_mut().rp = rp; context.vm.frame_mut().set_exit_early(true); if let Some(value) = value { diff --git a/core/engine/src/vm/call_frame/mod.rs b/core/engine/src/vm/call_frame/mod.rs index 40c66087551..856a339f9fc 100644 --- a/core/engine/src/vm/call_frame/mod.rs +++ b/core/engine/src/vm/call_frame/mod.rs @@ -36,11 +36,10 @@ bitflags::bitflags! { pub struct CallFrame { pub(crate) code_block: Gc, pub(crate) pc: u32, - pub(crate) fp: u32, - pub(crate) env_fp: u32, - // Tracks the number of environments in environment entry. - // On abrupt returns this is used to decide how many environments need to be pop'ed. + /// The register pointer, points to the first register in the stack. + pub(crate) rp: u32, pub(crate) argument_count: u32, + pub(crate) env_fp: u32, pub(crate) promise_capability: Option, // Iterators and their `[[Done]]` flags that must be closed when an abrupt completion is thrown. @@ -80,16 +79,18 @@ impl CallFrame { impl CallFrame { /// This is the size of the function prologue. /// - /// The position of the elements are relative to the [`CallFrame::fp`] (frame pointer). + /// The position of the elements are relative to the [`CallFrame::fp`] (register pointer). /// /// ```text /// Setup by the caller - /// ┌─────────────────────────────────────────────────────────┐ ┌───── frame pointer + /// ┌─────────────────────────────────────────────────────────┐ ┌───── register pointer /// ▼ ▼ ▼ /// | -(2 + N): this | -(1 + N): func | -N: arg1 | ... | -1: argN | 0: local1 | ... | K: localK | /// ▲ ▲ ▲ ▲ ▲ ▲ /// └──────────────────────────────┘ └──────────────────────┘ └─────────────────────────┘ - /// function prolugue arguments Setup by the callee + /// function prologue arguments Setup by the callee + /// ▲ + /// └─ Frame pointer /// ``` /// /// ### Example @@ -107,17 +108,22 @@ impl CallFrame { /// ``` /// /// ```text - /// caller prolugue caller arguments callee prolugue callee arguments - /// ______|____________ _____|_____ _________|_________ __|_ - /// / \ / \ / \ / \ - /// | 0: undefined | 1: y | 2: 1 | 3: 2 | 4: undefined | 5: x | 6: 3 | - /// / ^ - /// caller frame pointer --+ callee frame pointer + /// caller prologue caller arguments callee prologue callee arguments + /// ┌─────────────────┐ ┌─────────┐ ┌─────────────────┐ ┌──────┐ + /// ▼ ▼ ▼ ▼ │ ▼ ▼ ▼ + /// | 0: undefined | 1: y | 2: 1 | 3: 2 | 4: undefined | 5: x | 6: 3 | + /// ▲ ▲ ▲ + /// │ caller register pointer ────┤ │ + /// │ │ callee register pointer + /// │ callee frame pointer + /// │ + /// └───── caller frame pointer + /// /// ``` /// /// Some questions: /// - /// - Who is responsible for cleaning up the stack after a call? The caller. + /// - Who is responsible for cleaning up the stack after a call? The rust caller. /// pub(crate) const FUNCTION_PROLOGUE: u32 = 2; pub(crate) const THIS_POSITION: u32 = 2; @@ -134,7 +140,7 @@ impl CallFrame { Self { code_block, pc: 0, - fp: 0, + rp: 0, env_fp: 0, argument_count: 0, promise_capability: None, @@ -167,12 +173,12 @@ impl CallFrame { } pub(crate) fn this(&self, vm: &Vm) -> JsValue { - let this_index = self.fp - self.argument_count - Self::THIS_POSITION; + let this_index = self.rp - self.argument_count - Self::THIS_POSITION; vm.stack[this_index as usize].clone() } pub(crate) fn function(&self, vm: &Vm) -> Option { - let function_index = self.fp - self.argument_count - Self::FUNCTION_POSITION; + let function_index = self.rp - self.argument_count - Self::FUNCTION_POSITION; if let Some(object) = vm.stack[function_index as usize].as_object() { return Some(object.clone()); } @@ -181,22 +187,22 @@ impl CallFrame { } pub(crate) fn arguments<'stack>(&self, vm: &'stack Vm) -> &'stack [JsValue] { - let fp = self.fp as usize; + let rp = self.rp as usize; let argument_count = self.argument_count as usize; - let arguments_start = fp - argument_count; - &vm.stack[arguments_start..fp] + let arguments_start = rp - argument_count; + &vm.stack[arguments_start..rp] } pub(crate) fn argument<'stack>(&self, index: usize, vm: &'stack Vm) -> Option<&'stack JsValue> { self.arguments(vm).get(index) } - pub(crate) fn restore_fp(&self) -> u32 { - self.fp - self.argument_count - Self::FUNCTION_PROLOGUE + pub(crate) fn fp(&self) -> u32 { + self.rp - self.argument_count - Self::FUNCTION_PROLOGUE } pub(crate) fn restore_stack(&self, vm: &mut Vm) { - let fp = self.restore_fp(); + let fp = self.fp(); vm.stack.truncate(fp as usize); } @@ -218,7 +224,7 @@ impl CallFrame { /// If the index is out of bounds. pub(crate) fn local<'stack>(&self, index: u32, stack: &'stack [JsValue]) -> &'stack JsValue { debug_assert!(index < self.code_block().locals_count); - let at = self.fp + index; + let at = self.rp + index; &stack[at as usize] } @@ -242,8 +248,8 @@ impl CallFrame { /// ---- `CallFrame` stack methods ---- impl CallFrame { - pub(crate) fn set_frame_pointer(&mut self, pointer: u32) { - self.fp = pointer; + pub(crate) fn set_register_pointer(&mut self, pointer: u32) { + self.rp = pointer; } } diff --git a/core/engine/src/vm/mod.rs b/core/engine/src/vm/mod.rs index 94c12a8a0b3..cf02d8b0b30 100644 --- a/core/engine/src/vm/mod.rs +++ b/core/engine/src/vm/mod.rs @@ -158,7 +158,7 @@ impl Vm { pub(crate) fn push_frame(&mut self, mut frame: CallFrame) { let current_stack_length = self.stack.len(); - frame.set_frame_pointer(current_stack_length as u32); + frame.set_register_pointer(current_stack_length as u32); std::mem::swap(&mut self.environments, &mut frame.environments); std::mem::swap(&mut self.realm, &mut frame.realm); @@ -210,7 +210,7 @@ impl Vm { let catch_address = handler.handler(); let environment_sp = frame.env_fp + handler.environment_count; - let sp = frame.fp + handler.stack_count; + let sp = frame.rp + handler.stack_count; // Go to handler location. frame.pc = catch_address; @@ -325,7 +325,12 @@ impl Context { let result = self.execute_instruction(f); let duration = instant.elapsed(); - let fp = self.vm.frames.last().map(|frame| frame.fp as usize); + let fp = self + .vm + .frames + .last() + .map(CallFrame::fp) + .map(|fp| fp as usize); let stack = { let mut stack = String::from("[ "); @@ -427,7 +432,7 @@ impl Context { break; } - fp = frame.restore_fp() as usize; + fp = frame.fp() as usize; env_fp = frame.env_fp as usize; self.vm.pop_frame(); } @@ -454,8 +459,8 @@ impl Context { match result { CompletionType::Normal => {} CompletionType::Return => { - let restore_fp = self.vm.frame().restore_fp() as usize; - self.vm.stack.truncate(restore_fp); + let fp = self.vm.frame().fp() as usize; + self.vm.stack.truncate(fp); let result = self.vm.take_return_value(); if self.vm.frame().exit_early() { @@ -466,7 +471,7 @@ impl Context { self.vm.pop_frame(); } CompletionType::Throw => { - let mut fp = self.vm.frame().restore_fp(); + let mut fp = self.vm.frame().fp(); let mut env_fp = self.vm.frame().env_fp; if self.vm.frame().exit_early() { self.vm.environments.truncate(env_fp as usize); @@ -482,7 +487,7 @@ impl Context { self.vm.pop_frame(); while let Some(frame) = self.vm.frames.last_mut() { - fp = frame.restore_fp(); + fp = frame.fp(); env_fp = frame.env_fp; let pc = frame.pc; let exit_early = frame.exit_early(); diff --git a/core/engine/src/vm/opcode/generator/mod.rs b/core/engine/src/vm/opcode/generator/mod.rs index 8dbe8a525ba..989c0c45f13 100644 --- a/core/engine/src/vm/opcode/generator/mod.rs +++ b/core/engine/src/vm/opcode/generator/mod.rs @@ -82,7 +82,7 @@ impl Operation for Generator { let fp = frame .call_frame .as_ref() - .map_or(0, |frame| frame.fp as usize); + .map_or(0, |frame| frame.rp as usize); frame.stack[fp] = generator.clone().into(); let mut gen = generator diff --git a/core/engine/src/vm/opcode/rest_parameter/mod.rs b/core/engine/src/vm/opcode/rest_parameter/mod.rs index 280c2951df9..eda0764c610 100644 --- a/core/engine/src/vm/opcode/rest_parameter/mod.rs +++ b/core/engine/src/vm/opcode/rest_parameter/mod.rs @@ -17,11 +17,11 @@ impl Operation for RestParameterInit { const COST: u8 = 6; fn execute(context: &mut Context) -> JsResult { - let arg_count = context.vm.frame().argument_count as usize; + let argument_count = context.vm.frame().argument_count as usize; let param_count = context.vm.frame().code_block().params.as_ref().len(); - let array = if arg_count >= param_count { - let rest_count = arg_count - param_count + 1; + let array = if argument_count >= param_count { + let rest_count = argument_count - param_count + 1; let args = context.vm.pop_n_values(rest_count); Array::create_array_from_list(args, context) } else {