From 37a333b33afd5489418b262761c19c00ee2fde4e Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Fri, 27 Oct 2023 15:01:50 +0200 Subject: [PATCH] Move `arguments` object creation to opcode --- boa_engine/src/bytecompiler/declarations.rs | 38 +++++++---- boa_engine/src/bytecompiler/mod.rs | 2 +- boa_engine/src/context/mod.rs | 5 +- .../src/object/internal_methods/function.rs | 68 +------------------ boa_engine/src/vm/code_block.rs | 28 ++++---- boa_engine/src/vm/flowgraph/mod.rs | 8 +-- boa_engine/src/vm/opcode/arguments.rs | 64 +++++++++++++++++ boa_engine/src/vm/opcode/define/mod.rs | 53 +++++++++++++++ boa_engine/src/vm/opcode/mod.rs | 38 +++++++++-- 9 files changed, 192 insertions(+), 112 deletions(-) create mode 100644 boa_engine/src/vm/opcode/arguments.rs diff --git a/boa_engine/src/bytecompiler/declarations.rs b/boa_engine/src/bytecompiler/declarations.rs index ffb97dbb013..d6b24bef57e 100644 --- a/boa_engine/src/bytecompiler/declarations.rs +++ b/boa_engine/src/bytecompiler/declarations.rs @@ -3,6 +3,7 @@ use std::rc::Rc; use crate::{ bytecompiler::{ByteCompiler, FunctionCompiler, FunctionSpec, NodeKind}, environments::CompileTimeEnvironment, + js_string, vm::{create_function_object_fast, BindingOpcode, CodeBlockFlags, Opcode}, JsNativeError, JsResult, }; @@ -285,6 +286,7 @@ impl ByteCompiler<'_, '_> { let function = create_function_object_fast(code, self.context); // c. Perform ? env.CreateGlobalFunctionBinding(fn, fo, false). + let name = js_string!(self.interner().resolve_expect(name.sym()).utf16()); self.context .create_global_function_binding(name, function, false)?; } @@ -726,14 +728,17 @@ impl ByteCompiler<'_, '_> { // c. If varEnv is a Global Environment Record, then if var_env.is_global() { // Ensures global functions are printed when generating the global flowgraph. - let _ = self.push_function_to_constants(code.clone()); + let index = self.push_function_to_constants(code.clone()); // b. Let fo be InstantiateFunctionObject of f with arguments lexEnv and privateEnv. - let function = create_function_object_fast(code, self.context); + self.emit_with_varying_operand(Opcode::GetFunction, index); // i. Perform ? varEnv.CreateGlobalFunctionBinding(fn, fo, true). - self.context - .create_global_function_binding(name, function, true)?; + let name_index = self.get_or_insert_name(name); + self.emit( + Opcode::CreateGlobalFunctionBinding, + &[Operand::Bool(true), Operand::Varying(name_index)], + ); } // d. Else, else { @@ -915,14 +920,19 @@ impl ByteCompiler<'_, '_> { // NOTE(HalidOdat): Has been moved up, so "arguments" gets registed as // the first binding in the environment with index 0. if arguments_object_needed { - // Note: This happens at runtime. // a. If strict is true or simpleParameterList is false, then - // i. Let ao be CreateUnmappedArgumentsObject(argumentsList). + if strict || !formals.is_simple() { + // i. Let ao be CreateUnmappedArgumentsObject(argumentsList). + self.emit_opcode(Opcode::CreateUnmappedArgumentsObject); + } // b. Else, - // i. NOTE: A mapped argument object is only provided for non-strict functions - // 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). + else { + // i. NOTE: A mapped argument object is only provided for non-strict functions + // 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). + self.emit_opcode(Opcode::CreateMappedArgumentsObject); + } // c. If strict is true, then if strict { @@ -937,7 +947,8 @@ impl ByteCompiler<'_, '_> { env.create_mutable_binding(arguments, false); } - self.code_block_flags |= CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT; + // e. Perform ! env.InitializeBinding("arguments", ao). + self.emit_binding(BindingOpcode::InitLexical, arguments); } // 21. For each String paramName of parameterNames, do @@ -961,13 +972,10 @@ impl ByteCompiler<'_, '_> { // 22. If argumentsObjectNeeded is true, then if arguments_object_needed { - // MOVED: a-d. + // MOVED: a-e. // // NOTE(HalidOdat): Has been moved up, see comment above. - // Note: This happens at runtime. - // e. Perform ! env.InitializeBinding("arguments", ao). - // f. Let parameterBindings be the list-concatenation of parameterNames and « "arguments" ». parameter_names.push(arguments); } diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index 5901f5e821f..ddce437535c 100644 --- a/boa_engine/src/bytecompiler/mod.rs +++ b/boa_engine/src/bytecompiler/mod.rs @@ -275,7 +275,7 @@ pub struct ByteCompiler<'ctx, 'host> { json_parse: bool, // TODO: remove when we separate scripts from the context - context: &'ctx mut Context<'host>, + pub(crate) context: &'ctx mut Context<'host>, #[cfg(feature = "annex-b")] annex_b_function_names: Vec, diff --git a/boa_engine/src/context/mod.rs b/boa_engine/src/context/mod.rs index dc78b21beb0..3aa8617fce2 100644 --- a/boa_engine/src/context/mod.rs +++ b/boa_engine/src/context/mod.rs @@ -718,7 +718,7 @@ impl Context<'_> { /// [spec]: https://tc39.es/ecma262/#sec-createglobalfunctionbinding pub(crate) fn create_global_function_binding( &mut self, - name: Identifier, + name: JsString, function: JsObject, configurable: bool, ) -> JsResult<()> { @@ -727,8 +727,7 @@ impl Context<'_> { let global_object = self.realm().global_object().clone(); // 3. Let existingProp be ? globalObject.[[GetOwnProperty]](N). - let name = PropertyKey::from(self.interner().resolve_expect(name.sym()).utf16()); - let existing_prop = global_object.__get_own_property__(&name, self)?; + let existing_prop = global_object.__get_own_property__(&name.clone().into(), self)?; // 4. If existingProp is undefined or existingProp.[[Configurable]] is true, then let desc = if existing_prop.is_none() diff --git a/boa_engine/src/object/internal_methods/function.rs b/boa_engine/src/object/internal_methods/function.rs index d5061b4365b..91a76142c7e 100644 --- a/boa_engine/src/object/internal_methods/function.rs +++ b/boa_engine/src/object/internal_methods/function.rs @@ -1,5 +1,5 @@ use crate::{ - builtins::function::{arguments::Arguments, ThisMode}, + builtins::function::ThisMode, context::intrinsics::StandardConstructors, environments::{FunctionSlots, ThisBindingStatus}, native_function::NativeFunctionObject, @@ -121,39 +121,6 @@ pub(crate) fn function_call( .push_lexical(code.constant_compile_time_environment(last_env)); } - // Taken from: `FunctionDeclarationInstantiation` abstract function. - // - // Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation - // - // 22. If argumentsObjectNeeded is true, then - if code.needs_arguments_object() { - // a. If strict is true or simpleParameterList is false, then - // i. Let ao be CreateUnmappedArgumentsObject(argumentsList). - // b. Else, - // i. NOTE: A mapped argument object is only provided for non-strict functions - // 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[(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 { - let env = context.vm.environments.current(); - Arguments::create_mapped_arguments_object( - function_object, - &code.params, - &args, - env.declarative_expect(), - context, - ) - }; - let env_index = context.vm.environments.len() as u32 - 1; - context - .vm - .environments - .put_lexical_value(env_index, 0, arguments_obj.into()); - } - Ok(CallValue::Ready) } @@ -257,39 +224,6 @@ fn function_construct( .push_lexical(code.constant_compile_time_environment(last_env)); } - // Taken from: `FunctionDeclarationInstantiation` abstract function. - // - // Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation - // - // 22. If argumentsObjectNeeded is true, then - if code.needs_arguments_object() { - // a. If strict is true or simpleParameterList is false, then - // i. Let ao be CreateUnmappedArgumentsObject(argumentsList). - // b. Else, - // i. NOTE: A mapped argument object is only provided for non-strict functions - // 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 arguments_obj = if code.strict() || !code.params.is_simple() { - Arguments::create_unmapped_arguments_object(&args, context) - } else { - let env = context.vm.environments.current(); - Arguments::create_mapped_arguments_object( - this_function_object, - &code.params, - &args, - env.declarative_expect(), - context, - ) - }; - let env_index = context.vm.environments.len() as u32 - 1; - context - .vm - .environments - .put_lexical_value(env_index, 0, arguments_obj.into()); - } - // Insert `this` value context .vm diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index 64b1ce1c5d1..904f63fe9fe 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -59,11 +59,6 @@ bitflags! { /// Does this function have a parameters environment. const PARAMETERS_ENV_BINDINGS = 0b0000_1000; - /// Does this function need a `"arguments"` object. - /// - /// The `"arguments"` binding is the first binding. - const NEEDS_ARGUMENTS_OBJECT = 0b0001_0000; - /// The `[[ClassFieldInitializerName]]` internal slot. const IN_CLASS_FIELD_INITIALIZER = 0b0010_0000; @@ -233,13 +228,6 @@ impl CodeBlock { .contains(CodeBlockFlags::PARAMETERS_ENV_BINDINGS) } - /// Does this function need a `"arguments"` object. - pub(crate) fn needs_arguments_object(&self) -> bool { - self.flags - .get() - .contains(CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT) - } - /// Does this function have the `[[ClassFieldInitializerName]]` internal slot set to non-empty value. pub(crate) fn in_class_field_initializer(&self) -> bool { self.flags @@ -526,6 +514,15 @@ impl CodeBlock { Instruction::CreateIteratorResult { done } => { format!("done: {done}") } + Instruction::CreateGlobalFunctionBinding { + name_index, + configurable, + } => { + let name = self + .constant_string(name_index.value() as usize) + .to_std_string_escaped(); + format!("name: {name}, configurable: {configurable}") + } Instruction::Pop | Instruction::Dup | Instruction::Swap @@ -646,6 +643,8 @@ impl CodeBlock { | Instruction::GetReturnValue | Instruction::SetReturnValue | Instruction::BindThisValue + | Instruction::CreateMappedArgumentsObject + | Instruction::CreateUnmappedArgumentsObject | Instruction::Nop => String::new(), Instruction::U16Operands @@ -707,10 +706,7 @@ impl CodeBlock { | Instruction::Reserved55 | Instruction::Reserved56 | Instruction::Reserved57 - | Instruction::Reserved58 - | Instruction::Reserved59 - | Instruction::Reserved60 - | Instruction::Reserved61 => unreachable!("Reserved opcodes are unrechable"), + | Instruction::Reserved58 => unreachable!("Reserved opcodes are unrechable"), } } } diff --git a/boa_engine/src/vm/flowgraph/mod.rs b/boa_engine/src/vm/flowgraph/mod.rs index 5a8db1a0024..fc8c65527b4 100644 --- a/boa_engine/src/vm/flowgraph/mod.rs +++ b/boa_engine/src/vm/flowgraph/mod.rs @@ -451,6 +451,9 @@ impl CodeBlock { | Instruction::MaybeException | Instruction::CheckReturn | Instruction::BindThisValue + | Instruction::CreateMappedArgumentsObject + | Instruction::CreateUnmappedArgumentsObject + | Instruction::CreateGlobalFunctionBinding { .. } | Instruction::Nop => { graph.add_node(previous_pc, NodeShape::None, label.into(), Color::None); graph.add_edge(previous_pc, pc, None, Color::None, EdgeStyle::Line); @@ -517,10 +520,7 @@ impl CodeBlock { | Instruction::Reserved55 | Instruction::Reserved56 | Instruction::Reserved57 - | Instruction::Reserved58 - | Instruction::Reserved59 - | Instruction::Reserved60 - | Instruction::Reserved61 => unreachable!("Reserved opcodes are unrechable"), + | Instruction::Reserved58 => unreachable!("Reserved opcodes are unrechable"), } } diff --git a/boa_engine/src/vm/opcode/arguments.rs b/boa_engine/src/vm/opcode/arguments.rs new file mode 100644 index 00000000000..80c399f3cf2 --- /dev/null +++ b/boa_engine/src/vm/opcode/arguments.rs @@ -0,0 +1,64 @@ +use crate::{ + builtins::function::arguments::Arguments, + vm::{CallFrame, CompletionType}, + Context, JsResult, +}; + +use super::Operation; + +/// `CreateMappedArgumentsObject` implements the Opcode Operation for `Opcode::CreateMappedArgumentsObject` +/// +/// Operation: +/// - TODO: doc +#[derive(Debug, Clone, Copy)] +pub(crate) struct CreateMappedArgumentsObject; + +impl Operation for CreateMappedArgumentsObject { + const NAME: &'static str = "CreateMappedArgumentsObject"; + const INSTRUCTION: &'static str = "INST - CreateMappedArgumentsObject"; + const COST: u8 = 8; + + fn execute(context: &mut Context<'_>) -> JsResult { + let arguments_start = context.vm.frame().fp as usize + CallFrame::FIRST_ARGUMENT_POSITION; + let function_object = context + .vm + .frame() + .function(&context.vm) + .clone() + .expect("there should be a function object"); + let code = context.vm.frame().code_block().clone(); + let args = context.vm.stack[arguments_start..].to_vec(); + + let env = context.vm.environments.current(); + let arguments = Arguments::create_mapped_arguments_object( + &function_object, + &code.params, + &args, + env.declarative_expect(), + context, + ); + context.vm.push(arguments); + Ok(CompletionType::Normal) + } +} + +/// `CreateUnmappedArgumentsObject` implements the Opcode Operation for `Opcode::CreateUnmappedArgumentsObject` +/// +/// Operation: +/// - TODO: doc +#[derive(Debug, Clone, Copy)] +pub(crate) struct CreateUnmappedArgumentsObject; + +impl Operation for CreateUnmappedArgumentsObject { + const NAME: &'static str = "CreateUnmappedArgumentsObject"; + const INSTRUCTION: &'static str = "INST - CreateUnmappedArgumentsObject"; + const COST: u8 = 4; + + fn execute(context: &mut Context<'_>) -> JsResult { + let arguments_start = context.vm.frame().fp as usize + CallFrame::FIRST_ARGUMENT_POSITION; + let args = context.vm.stack[arguments_start..].to_vec(); + let arguments = Arguments::create_unmapped_arguments_object(&args, context); + context.vm.push(arguments); + Ok(CompletionType::Normal) + } +} diff --git a/boa_engine/src/vm/opcode/define/mod.rs b/boa_engine/src/vm/opcode/define/mod.rs index e555f545eab..57fd59cf996 100644 --- a/boa_engine/src/vm/opcode/define/mod.rs +++ b/boa_engine/src/vm/opcode/define/mod.rs @@ -141,3 +141,56 @@ impl Operation for PutLexicalValue { Self::operation(context, index as usize) } } + +/// `CreateGlobalFunctionBinding` implements the Opcode Operation for `Opcode::CreateGlobalFunctionBinding` +/// +/// Operation: +/// - Performs [`CreateGlobalFunctionBinding ( N, V, D )`][spec] +/// +/// [spec]: https://tc39.es/ecma262/#sec-createglobalfunctionbinding +#[derive(Debug, Clone, Copy)] +pub(crate) struct CreateGlobalFunctionBinding; + +impl CreateGlobalFunctionBinding { + #[allow(clippy::unnecessary_wraps)] + fn operation( + context: &mut Context<'_>, + index: usize, + configurable: bool, + ) -> JsResult { + let name = context.vm.frame().code_block().constant_string(index); + let value = context.vm.pop(); + + let function = value + .as_object() + .expect("valeu should be an function") + .clone(); + context.create_global_function_binding(name, function, configurable)?; + + Ok(CompletionType::Normal) + } +} + +impl Operation for CreateGlobalFunctionBinding { + const NAME: &'static str = "CreateGlobalFunctionBinding"; + const INSTRUCTION: &'static str = "INST - CreateGlobalFunctionBinding"; + const COST: u8 = 2; + + fn execute(context: &mut Context<'_>) -> JsResult { + let configurable = context.vm.read::() != 0; + let index = context.vm.read::() as usize; + Self::operation(context, index, configurable) + } + + fn execute_with_u16_operands(context: &mut Context<'_>) -> JsResult { + let configurable = context.vm.read::() != 0; + let index = context.vm.read::() as usize; + Self::operation(context, index, configurable) + } + + fn execute_with_u32_operands(context: &mut Context<'_>) -> JsResult { + let configurable = context.vm.read::() != 0; + let index = context.vm.read::() as usize; + Self::operation(context, index, configurable) + } +} diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs index cc2dcbd4dcf..e134792252a 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -4,6 +4,7 @@ use std::iter::FusedIterator; use crate::{vm::CompletionType, Context, JsResult, JsValue}; // Operation modules +mod arguments; mod r#await; mod binary_ops; mod call; @@ -35,6 +36,8 @@ mod value; // Operation structs #[doc(inline)] +pub(crate) use arguments::*; +#[doc(inline)] pub(crate) use binary_ops::*; #[doc(inline)] pub(crate) use call::*; @@ -2053,6 +2056,35 @@ generate_opcodes! { /// Stack: **=>** PopPrivateEnvironment, + /// Creates a mapped `arguments` object. + /// + /// Performs [`10.4.4.7 CreateMappedArgumentsObject ( func, formals, argumentsList, env )`] + /// + /// [spec]: https://tc39.es/ecma262/#sec-createunmappedargumentsobject + /// + /// Operands: + /// + /// Stack: **=>** `arguments` + CreateMappedArgumentsObject, + + /// Creates an unmapped `arguments` object. + /// + /// Performs: [`10.4.4.6 CreateUnmappedArgumentsObject ( argumentsList )`] + /// + /// [spec]: https://tc39.es/ecma262/#sec-createmappedargumentsobject + /// + /// Stack: **=>** `arguments` + CreateUnmappedArgumentsObject, + + /// Performs [`CreateGlobalFunctionBinding ( N, V, D )`][spec] + /// + /// Operands: configurable: `bool`, `name_index`: `VaryingOperand` + /// + /// Stack: `value` **=>** + /// + /// [spec]: https://tc39.es/ecma262/#sec-createglobalfunctionbinding + CreateGlobalFunctionBinding { configurable: bool, name_index: VaryingOperand }, + /// No-operation instruction, does nothing. /// /// Operands: @@ -2190,12 +2222,6 @@ generate_opcodes! { Reserved57 => Reserved, /// Reserved [`Opcode`]. Reserved58 => Reserved, - /// Reserved [`Opcode`]. - Reserved59 => Reserved, - /// Reserved [`Opcode`]. - Reserved60 => Reserved, - /// Reserved [`Opcode`]. - Reserved61 => Reserved, } /// Specific opcodes for bindings.