From a7da92ab71f3d45667e4583d838caacb43ca408a Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Fri, 1 Dec 2023 21:12:02 +0100 Subject: [PATCH] Fix panics and refactor code --- core/engine/src/bytecompiler/mod.rs | 29 ++++++------ core/engine/src/vm/code_block.rs | 67 ++++++++++++++++++++------- core/engine/src/vm/opcode/call/mod.rs | 46 +++++++----------- 3 files changed, 81 insertions(+), 61 deletions(-) diff --git a/core/engine/src/bytecompiler/mod.rs b/core/engine/src/bytecompiler/mod.rs index 385ec317af..5320deb008 100644 --- a/core/engine/src/bytecompiler/mod.rs +++ b/core/engine/src/bytecompiler/mod.rs @@ -1512,24 +1512,25 @@ impl<'ctx> ByteCompiler<'ctx> { fn optimize(bytecode: &mut Vec, flags: CodeBlockFlags) { // only perform Tail Call Optimisation in strict mode - if flags.contains(CodeBlockFlags::STRICT) { + if flags.contains(CodeBlockFlags::STRICT) && flags.is_ordinary() { // if a sequence of Call, SetReturnValue, CheckReturn, Return is found // replace Call with TailCall to allow frame elimination for i in 0..bytecode.len() { let opcode: Opcode = bytecode[i].into(); - if matches!(opcode, Opcode::Call) { - if bytecode.len() > i + 5 { - let second = bytecode[i + 2].into(); - let third = bytecode[i + 3].into(); - let fourth = bytecode[i + 4].into(); - if let (Opcode::SetReturnValue, Opcode::CheckReturn, Opcode::Return) = - (second, third, fourth) - { - bytecode[i] = Opcode::TailCall as u8; - } - } else { - return; - } + if !matches!(opcode, Opcode::Call) { + continue; + } + if bytecode.len() <= i + 5 { + return; + } + + let second = bytecode[i + 2].into(); + let third = bytecode[i + 3].into(); + let fourth = bytecode[i + 4].into(); + if let (Opcode::SetReturnValue, Opcode::CheckReturn, Opcode::Return) = + (second, third, fourth) + { + bytecode[i] = Opcode::TailCall as u8; } } } diff --git a/core/engine/src/vm/code_block.rs b/core/engine/src/vm/code_block.rs index f69f728335..f236fc988d 100644 --- a/core/engine/src/vm/code_block.rs +++ b/core/engine/src/vm/code_block.rs @@ -76,6 +76,47 @@ bitflags! { } } +impl CodeBlockFlags { + /// Check if the function is traced. + #[cfg(feature = "trace")] + pub(crate) fn traceable(&self) -> bool { + self.contains(CodeBlockFlags::TRACEABLE) + } + /// Check if the function is a class constructor. + pub(crate) fn is_class_constructor(&self) -> bool { + self.contains(CodeBlockFlags::IS_CLASS_CONSTRUCTOR) + } + /// Check if the function is in strict mode. + pub(crate) fn strict(&self) -> bool { + self.contains(CodeBlockFlags::STRICT) + } + /// Indicates if the function is an expression and has a binding identifier. + pub(crate) fn has_binding_identifier(&self) -> bool { + self.contains(CodeBlockFlags::HAS_BINDING_IDENTIFIER) + } + /// Does this function have the `[[ClassFieldInitializerName]]` internal slot set to non-empty value. + pub(crate) fn in_class_field_initializer(&self) -> bool { + self.contains(CodeBlockFlags::IN_CLASS_FIELD_INITIALIZER) + } + /// Returns true if this function is a derived constructor. + pub(crate) fn is_derived_constructor(&self) -> bool { + self.contains(CodeBlockFlags::IS_DERIVED_CONSTRUCTOR) + } + /// Returns true if this function an async function. + pub(crate) fn is_async(self) -> bool { + self.contains(CodeBlockFlags::IS_ASYNC) + } + + /// Returns true if this function an generator function. + pub(crate) fn is_generator(self) -> bool { + self.contains(CodeBlockFlags::IS_GENERATOR) + } + /// Returns true if this function an async function. + pub(crate) fn is_ordinary(self) -> bool { + !self.is_async() && !self.is_generator() + } +} + // SAFETY: Nothing in CodeBlockFlags needs tracing, so this is safe. unsafe impl Trace for CodeBlockFlags { empty_trace!(); @@ -232,7 +273,7 @@ impl CodeBlock { /// Check if the function is traced. #[cfg(feature = "trace")] pub(crate) fn traceable(&self) -> bool { - self.flags.get().contains(CodeBlockFlags::TRACEABLE) + self.flags.get().traceable() } /// Enable or disable instruction tracing to `stdout`. #[cfg(feature = "trace")] @@ -245,50 +286,42 @@ impl CodeBlock { /// Check if the function is a class constructor. pub(crate) fn is_class_constructor(&self) -> bool { - self.flags - .get() - .contains(CodeBlockFlags::IS_CLASS_CONSTRUCTOR) + self.flags.get().is_class_constructor() } /// Check if the function is in strict mode. pub(crate) fn strict(&self) -> bool { - self.flags.get().contains(CodeBlockFlags::STRICT) + self.flags.get().strict() } /// Indicates if the function is an expression and has a binding identifier. pub(crate) fn has_binding_identifier(&self) -> bool { - self.flags - .get() - .contains(CodeBlockFlags::HAS_BINDING_IDENTIFIER) + self.flags.get().has_binding_identifier() } /// Does this function have the `[[ClassFieldInitializerName]]` internal slot set to non-empty value. pub(crate) fn in_class_field_initializer(&self) -> bool { - self.flags - .get() - .contains(CodeBlockFlags::IN_CLASS_FIELD_INITIALIZER) + self.flags.get().in_class_field_initializer() } /// Returns true if this function is a derived constructor. pub(crate) fn is_derived_constructor(&self) -> bool { - self.flags - .get() - .contains(CodeBlockFlags::IS_DERIVED_CONSTRUCTOR) + self.flags.get().is_derived_constructor() } /// Returns true if this function an async function. pub(crate) fn is_async(&self) -> bool { - self.flags.get().contains(CodeBlockFlags::IS_ASYNC) + self.flags.get().is_async() } /// Returns true if this function an generator function. pub(crate) fn is_generator(&self) -> bool { - self.flags.get().contains(CodeBlockFlags::IS_GENERATOR) + self.flags.get().is_generator() } /// Returns true if this function an async function. pub(crate) fn is_ordinary(&self) -> bool { - !self.is_async() && !self.is_generator() + self.flags.get().is_ordinary() } /// Returns true if this function has the `"prototype"` property when function object is created. diff --git a/core/engine/src/vm/opcode/call/mod.rs b/core/engine/src/vm/opcode/call/mod.rs index d722f0587d..60228d389f 100644 --- a/core/engine/src/vm/opcode/call/mod.rs +++ b/core/engine/src/vm/opcode/call/mod.rs @@ -3,7 +3,7 @@ use crate::{ error::JsNativeError, module::{ModuleKind, Referrer}, object::FunctionObjectBuilder, - vm::{opcode::Operation, CompletionType}, + vm::{opcode::Operation, CallFrame, CompletionType}, Context, JsObject, JsResult, JsValue, NativeFunction, }; @@ -210,7 +210,7 @@ pub(crate) struct TailCall; impl TailCall { fn operation(context: &mut Context, argument_count: usize) -> JsResult { let at = context.vm.stack.len() - argument_count; - let func = &context.vm.stack[at - 1]; + let func = context.vm.stack[at - 1].clone(); let Some(object) = func.as_object() else { return Err(JsNativeError::typ() @@ -218,39 +218,25 @@ impl TailCall { .into()); }; - let is_ordinary_function = object.is::(); - object.__call__(argument_count).resolve(context)?; - - // only tail call for ordinary functions - // don't tail call on the main script - // TODO: the 3 needs to be reviewed - if is_ordinary_function && context.vm.frames.len() > 3 { - // check that caller is also ordinary function - let frames = &context.vm.frames; - let caller_frame = &frames[frames.len() - 2]; - let caller_function = caller_frame - .function(&context.vm) - .expect("there must be a caller function"); - if caller_function.is::() { - // remove caller's CallFrame - let frames = &mut context.vm.frames; - let caller_frame = frames.swap_remove(frames.len() - 2); - - // remove caller's prologue from stack - // this + func + arguments - let to_remove = 1 + 1 + caller_frame.argument_count as usize; + let mut early_exit = false; + if object.is::() { + if let Some(caller) = context.vm.pop_frame() { + let fp = caller.fp as usize; context .vm .stack - .drain((caller_frame.fp as usize)..(caller_frame.fp as usize + to_remove)); - - // update invoked function's fp - let frames = &mut context.vm.frames; - let invoked_frame = frames.last_mut().expect("invoked frame must exist"); - invoked_frame.set_exit_early(caller_frame.exit_early()); - invoked_frame.fp -= to_remove as u32; + .drain(fp..(at - CallFrame::FUNCTION_PROLOGUE)); + early_exit = caller.exit_early(); } } + + object.__call__(argument_count).resolve(context)?; + + // If the caller is early exit, since it has been poped from the frames + // we have to mark the current frame as early exit. + if early_exit { + context.vm.frame_mut().set_exit_early(true); + } Ok(CompletionType::Normal) } }