From 217b17b75926d9dc01b33d7f2ff652b3ede0c1d3 Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Tue, 17 Oct 2023 01:32:26 +0200 Subject: [PATCH] Apply review --- boa_engine/src/builtins/eval/mod.rs | 10 +-- boa_engine/src/builtins/generator/mod.rs | 2 +- boa_engine/src/builtins/json/mod.rs | 5 +- boa_engine/src/module/source.rs | 5 +- .../src/object/internal_methods/function.rs | 23 +++--- boa_engine/src/object/internal_methods/mod.rs | 2 +- boa_engine/src/object/operations.rs | 4 +- boa_engine/src/script.rs | 5 +- boa_engine/src/vm/call_frame/mod.rs | 71 ++++++++++++++++--- boa_engine/src/vm/mod.rs | 15 ++-- .../src/vm/opcode/control_flow/return.rs | 2 +- boa_engine/src/vm/opcode/generator/mod.rs | 5 +- boa_engine/src/vm/opcode/mod.rs | 4 +- 13 files changed, 104 insertions(+), 49 deletions(-) diff --git a/boa_engine/src/builtins/eval/mod.rs b/boa_engine/src/builtins/eval/mod.rs index b32481f344d..f10068fa271 100644 --- a/boa_engine/src/builtins/eval/mod.rs +++ b/boa_engine/src/builtins/eval/mod.rs @@ -18,7 +18,7 @@ use crate::{ object::JsObject, realm::Realm, string::common::StaticJsStrings, - vm::{CallFrame, Opcode}, + vm::{CallFrame, CallFrameFlags, Opcode}, Context, JsArgs, JsResult, JsString, JsValue, }; use boa_ast::operations::{contains, contains_arguments, ContainsSymbol}; @@ -258,9 +258,11 @@ impl Eval { let env_fp = context.vm.environments.len() as u32; let environments = context.vm.environments.clone(); let realm = context.realm().clone(); - context - .vm - .push_frame(CallFrame::new(code_block, None, environments, realm).with_env_fp(env_fp)); + context.vm.push_frame( + CallFrame::new(code_block, None, environments, realm) + .with_env_fp(env_fp) + .with_flags(CallFrameFlags::EXIT_EARLY), + ); context.vm.push(JsValue::undefined()); // Push `this` value. context.vm.push(JsValue::undefined()); // No function object, so push undefined. diff --git a/boa_engine/src/builtins/generator/mod.rs b/boa_engine/src/builtins/generator/mod.rs index dde61eb23d4..945efd68562 100644 --- a/boa_engine/src/builtins/generator/mod.rs +++ b/boa_engine/src/builtins/generator/mod.rs @@ -101,7 +101,7 @@ impl GeneratorContext { .push_frame(self.call_frame.take().expect("should have a call frame")); context.vm.frame_mut().fp = 0; - context.vm.frame_mut().exit_early = true; + context.vm.frame_mut().set_exit_early(true); if let Some(value) = value { context.vm.push(value); diff --git a/boa_engine/src/builtins/json/mod.rs b/boa_engine/src/builtins/json/mod.rs index f2dafd2d1b9..79ce436503f 100644 --- a/boa_engine/src/builtins/json/mod.rs +++ b/boa_engine/src/builtins/json/mod.rs @@ -29,7 +29,7 @@ use crate::{ string::{common::StaticJsStrings, utf16, CodePoint}, symbol::JsSymbol, value::IntegerOrInfinity, - vm::CallFrame, + vm::{CallFrame, CallFrameFlags}, Context, JsArgs, JsResult, JsString, JsValue, }; use boa_gc::Gc; @@ -134,7 +134,8 @@ impl Json { let env_fp = context.vm.environments.len() as u32; context.vm.push_frame( CallFrame::new(code_block, None, context.vm.environments.clone(), realm) - .with_env_fp(env_fp), + .with_env_fp(env_fp) + .with_flags(CallFrameFlags::EXIT_EARLY), ); context.vm.push(JsValue::undefined()); // Push `this` value. diff --git a/boa_engine/src/module/source.rs b/boa_engine/src/module/source.rs index f990eb694c1..f248f99ed89 100644 --- a/boa_engine/src/module/source.rs +++ b/boa_engine/src/module/source.rs @@ -29,7 +29,7 @@ use crate::{ realm::Realm, vm::{ create_function_object_fast, create_generator_function_object, ActiveRunnable, CallFrame, - CodeBlock, CompletionRecord, Opcode, + CallFrameFlags, CodeBlock, CompletionRecord, Opcode, }, Context, JsArgs, JsError, JsNativeError, JsObject, JsResult, JsString, JsValue, NativeFunction, }; @@ -1738,7 +1738,8 @@ impl SourceTextModule { environments, realm, ) - .with_env_fp(env_fp); + .with_env_fp(env_fp) + .with_flags(CallFrameFlags::EXIT_EARLY); callframe.promise_capability = capability; // 8. Suspend the running execution context. diff --git a/boa_engine/src/object/internal_methods/function.rs b/boa_engine/src/object/internal_methods/function.rs index fd44ca071a9..b4f5d238c1f 100644 --- a/boa_engine/src/object/internal_methods/function.rs +++ b/boa_engine/src/object/internal_methods/function.rs @@ -6,7 +6,7 @@ use crate::{ internal_methods::{InternalObjectMethods, ORDINARY_INTERNAL_METHODS}, JsObject, ObjectData, ObjectKind, }, - vm::CallFrame, + vm::{CallFrame, CallFrameFlags}, Context, JsNativeError, JsResult, JsValue, }; @@ -64,19 +64,16 @@ pub(crate) fn function_call( let env_fp = environments.len() as u32; - let mut frame = CallFrame::new(code.clone(), script_or_module, environments, realm) + let frame = CallFrame::new(code.clone(), script_or_module, environments, realm) .with_argument_count(argument_count as u32) .with_env_fp(env_fp); - frame.exit_early = false; - context.vm.push_frame(frame); - let at = context.vm.stack.len() - argument_count; - - context.vm.frame_mut().fp = at as u32 - 2; + let fp = context.vm.stack.len() - argument_count - CallFrame::FUCNTION_PROLOGUE; + context.vm.frame_mut().fp = fp as u32; - let this = context.vm.stack[at - 2].clone(); + let this = context.vm.stack[fp + CallFrame::THIS_POSITION].clone(); let lexical_this_mode = code.this_mode == ThisMode::Lexical; @@ -134,7 +131,7 @@ pub(crate) fn function_call( // that don't have a rest parameter, any parameter // default value initializers, or any destructured parameters. // ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env). - let args = context.vm.stack[at..].to_vec(); + let args = context.vm.stack[(fp + CallFrame::FIRST_ARGUMENT_POSITION)..].to_vec(); let arguments_obj = if code.strict() || !code.params.is_simple() { Arguments::create_unmapped_arguments_object(&args, context) } else { @@ -213,12 +210,10 @@ fn function_construct( None }; - let mut frame = CallFrame::new(code.clone(), script_or_module, environments, realm) + let frame = CallFrame::new(code.clone(), script_or_module, environments, realm) .with_argument_count(argument_count as u32) - .with_env_fp(env_fp); - - frame.exit_early = false; - frame.construct = true; + .with_env_fp(env_fp) + .with_flags(CallFrameFlags::CONSTRUCT); context.vm.push_frame(frame); diff --git a/boa_engine/src/object/internal_methods/mod.rs b/boa_engine/src/object/internal_methods/mod.rs index e4590d2e743..de77e14a4b4 100644 --- a/boa_engine/src/object/internal_methods/mod.rs +++ b/boa_engine/src/object/internal_methods/mod.rs @@ -305,7 +305,7 @@ pub(crate) struct InternalObjectMethods { /// The return value of an internal method (`[[Call]]` or `[[Construct]]`). /// -/// This is done to avoid recusion. +/// This is done to avoid recursion. pub(crate) enum CallValue { /// Calling is ready, the frames have been setup. /// diff --git a/boa_engine/src/object/operations.rs b/boa_engine/src/object/operations.rs index c4f0c811fcf..c54f0837b61 100644 --- a/boa_engine/src/object/operations.rs +++ b/boa_engine/src/object/operations.rs @@ -335,7 +335,7 @@ impl JsObject { return Ok(context.vm.pop()); } - context.vm.frames[frame_index].exit_early = true; + context.vm.frames[frame_index].set_exit_early(true); let result = context.run().consume(); @@ -385,7 +385,7 @@ impl JsObject { .clone()); } - context.vm.frames[frame_index].exit_early = true; + context.vm.frames[frame_index].set_exit_early(true); let result = context.run().consume(); diff --git a/boa_engine/src/script.rs b/boa_engine/src/script.rs index 0d8e2d7c5cb..95c1d3be3a2 100644 --- a/boa_engine/src/script.rs +++ b/boa_engine/src/script.rs @@ -19,7 +19,7 @@ use rustc_hash::FxHashMap; use crate::{ bytecompiler::ByteCompiler, realm::Realm, - vm::{ActiveRunnable, CallFrame, CodeBlock}, + vm::{ActiveRunnable, CallFrame, CallFrameFlags, CodeBlock}, Context, HostDefined, JsResult, JsString, JsValue, Module, }; @@ -156,7 +156,8 @@ impl Script { context.vm.environments.clone(), self.inner.realm.clone(), ) - .with_env_fp(env_fp), + .with_env_fp(env_fp) + .with_flags(CallFrameFlags::EXIT_EARLY), ); context.vm.push(JsValue::undefined()); // Push `this` value. diff --git a/boa_engine/src/vm/call_frame/mod.rs b/boa_engine/src/vm/call_frame/mod.rs index c53cd3fd7f7..172e2f97570 100644 --- a/boa_engine/src/vm/call_frame/mod.rs +++ b/boa_engine/src/vm/call_frame/mod.rs @@ -15,6 +15,19 @@ use thin_vec::ThinVec; use super::{ActiveRunnable, Vm}; +bitflags::bitflags! { + /// Flags associated with a [`CallFrame`]. + #[derive(Debug, Default, Clone, Copy)] + pub(crate) struct CallFrameFlags: u8 { + /// When we return from this [`CallFrame`] to stop execution and + /// return from [`crate::Context::run()`], and leave the remaining [`CallFrame`]s unchanged. + const EXIT_EARLY = 0b0000_0001; + + /// Was this [`CallFrame`] created from the `__construct__()` internal object method? + const CONSTRUCT = 0b0000_0010; + } +} + /// A `CallFrame` holds the state of a function call. #[derive(Clone, Debug, Finalize, Trace)] pub struct CallFrame { @@ -43,12 +56,15 @@ pub struct CallFrame { /// \[\[ScriptOrModule\]\] pub(crate) active_runnable: Option, + /// \[\[Environment\]\] pub(crate) environments: EnvironmentStack, + /// \[\[Realm\]\] pub(crate) realm: Realm, - pub(crate) exit_early: bool, - pub(crate) construct: bool, + // SAFETY: Nothing in `CallFrameFlags` requires tracing, so this is safe. + #[unsafe_ignore_trace] + pub(crate) flags: CallFrameFlags, } /// ---- `CallFrame` public API ---- @@ -63,8 +79,25 @@ impl CallFrame { /// ---- `CallFrame` creation methods ---- impl CallFrame { - pub(crate) const THIS_POSITION: u32 = 0; - pub(crate) const FUNCTION_POSITION: u32 = 1; + /// This is the size of the function prologue. + /// + /// The position of the elements are relative to the [`CallFrame::fp`]. + /// + /// ```text + /// arguments + /// --- frame pointer ^ + /// / __________________________/ + /// / / \ + /// | 0: this | 1: func | 2: arg1 | ... | (2 + N): argN | + /// \ / + /// ------------ + /// | + /// function prolugue + /// ``` + pub(crate) const FUCNTION_PROLOGUE: usize = 2; + pub(crate) const THIS_POSITION: usize = 0; + pub(crate) const FUNCTION_POSITION: usize = 1; + pub(crate) const FIRST_ARGUMENT_POSITION: usize = 2; /// Creates a new `CallFrame` with the provided `CodeBlock`. pub(crate) fn new( @@ -87,8 +120,7 @@ impl CallFrame { active_runnable, environments, realm, - exit_early: true, - construct: false, + flags: CallFrameFlags::empty(), } } @@ -104,19 +136,38 @@ impl CallFrame { self } + /// Updates a `CallFrame`'s `flags` field with the value provided. + pub(crate) fn with_flags(mut self, flags: CallFrameFlags) -> Self { + self.flags = flags; + self + } + pub(crate) fn this(&self, vm: &Vm) -> JsValue { - let this_index = self.fp + Self::THIS_POSITION; - vm.stack[this_index as usize].clone() + let this_index = self.fp as usize + Self::THIS_POSITION; + vm.stack[this_index].clone() } pub(crate) fn function(&self, vm: &Vm) -> Option { - let function_index = self.fp + Self::FUNCTION_POSITION; - if let Some(object) = vm.stack[function_index as usize].as_object() { + let function_index = self.fp as usize + Self::FUNCTION_POSITION; + if let Some(object) = vm.stack[function_index].as_object() { return Some(object.clone()); } None } + + /// Does this have the [`CallFrameFlags::EXIT_EARLY`] flag. + pub(crate) fn exit_early(&self) -> bool { + self.flags.contains(CallFrameFlags::EXIT_EARLY) + } + /// Set the [`CallFrameFlags::EXIT_EARLY`] flag. + pub(crate) fn set_exit_early(&mut self, early_exit: bool) { + self.flags.set(CallFrameFlags::EXIT_EARLY, early_exit); + } + /// Does this have the [`CallFrameFlags::CONSTRUCT`] flag. + pub(crate) fn construct(&self) -> bool { + self.flags.contains(CallFrameFlags::CONSTRUCT) + } } /// ---- `CallFrame` stack methods ---- diff --git a/boa_engine/src/vm/mod.rs b/boa_engine/src/vm/mod.rs index e36b3d84584..4ba3b9ff699 100644 --- a/boa_engine/src/vm/mod.rs +++ b/boa_engine/src/vm/mod.rs @@ -38,6 +38,7 @@ pub use { }; pub(crate) use { + call_frame::CallFrameFlags, code_block::{ create_function_object, create_function_object_fast, create_generator_function_object, CodeBlockFlags, Handler, @@ -403,7 +404,7 @@ impl Context<'_> { self.vm.stack.truncate(self.vm.frame().fp as usize); let result = self.vm.take_return_value(); - if self.vm.frame().exit_early { + if self.vm.frame().exit_early() { return CompletionRecord::Normal(result); } @@ -413,7 +414,7 @@ impl Context<'_> { Ok(CompletionType::Throw) => { self.vm.stack.truncate(self.vm.frame().fp as usize); - if self.vm.frame().exit_early { + if self.vm.frame().exit_early() { return CompletionRecord::Throw( self.vm .pending_exception @@ -427,7 +428,7 @@ impl Context<'_> { while let Some(frame) = self.vm.frames.last_mut() { let pc = frame.pc; let fp = frame.fp; - let exit_early = frame.exit_early; + let exit_early = frame.exit_early(); if self.vm.handle_exception_at(pc) { continue 'instruction; @@ -449,7 +450,7 @@ impl Context<'_> { // Early return immediately. Ok(CompletionType::Yield) => { let result = self.vm.take_return_value(); - if self.vm.frame().exit_early { + if self.vm.frame().exit_early() { return CompletionRecord::Return(result); } @@ -473,7 +474,7 @@ impl Context<'_> { let mut fp = self.vm.stack.len(); let mut env_fp = self.vm.environments.len(); while let Some(frame) = self.vm.frames.last() { - if frame.exit_early { + if frame.exit_early() { break; } @@ -496,7 +497,7 @@ impl Context<'_> { continue; } - if self.vm.frame().exit_early { + if self.vm.frame().exit_early() { self.vm .environments .truncate(self.vm.frame().env_fp as usize); @@ -513,7 +514,7 @@ impl Context<'_> { let pc = frame.pc; let fp = frame.fp; let env_fp = frame.env_fp; - let exit_early = frame.exit_early; + let exit_early = frame.exit_early(); if self.vm.handle_exception_at(pc) { self.vm.pending_exception = Some(err); diff --git a/boa_engine/src/vm/opcode/control_flow/return.rs b/boa_engine/src/vm/opcode/control_flow/return.rs index c173d213d86..747437cb089 100644 --- a/boa_engine/src/vm/opcode/control_flow/return.rs +++ b/boa_engine/src/vm/opcode/control_flow/return.rs @@ -31,7 +31,7 @@ impl Operation for CheckReturn { const INSTRUCTION: &'static str = "INST - CheckReturn"; fn execute(context: &mut Context<'_>) -> JsResult { - if !context.vm.frame().construct { + if !context.vm.frame().construct() { return Ok(CompletionType::Normal); } let frame = context.vm.frame(); diff --git a/boa_engine/src/vm/opcode/generator/mod.rs b/boa_engine/src/vm/opcode/generator/mod.rs index 243ed6c412d..32408f2407c 100644 --- a/boa_engine/src/vm/opcode/generator/mod.rs +++ b/boa_engine/src/vm/opcode/generator/mod.rs @@ -48,7 +48,10 @@ impl Operation for Generator { dummy_call_frame.pc = pc; let mut call_frame = std::mem::replace(context.vm.frame_mut(), dummy_call_frame); - context.vm.frame_mut().exit_early = call_frame.exit_early; + context + .vm + .frame_mut() + .set_exit_early(call_frame.exit_early()); call_frame.environments = context.vm.environments.clone(); call_frame.realm = context.realm().clone(); diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs index 0ce4d92873e..bc7d73cbfcc 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -536,14 +536,14 @@ pub(crate) trait Operation { /// Execute opcode with [`VaryingOperandKind::U16`] sized [`VaryingOperand`]s. /// - /// By default if not implemented will call [`Reserved::u16_execute()`] which panics. + /// By default if not implemented will call [`Reserved::execute_with_u16_operands()`] which panics. fn execute_with_u16_operands(context: &mut Context<'_>) -> JsResult { Reserved::execute_with_u16_operands(context) } /// Execute opcode with [`VaryingOperandKind::U32`] sized [`VaryingOperand`]s. /// - /// By default if not implemented will call [`Reserved::u32_execute()`] which panics. + /// By default if not implemented will call [`Reserved::execute_with_u32_operands()`] which panics. fn execute_with_u32_operands(context: &mut Context<'_>) -> JsResult { Reserved::execute_with_u32_operands(context) }