From a3b46545a2a09f9ac81fd83ac6b180934c728f61 Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Fri, 11 Aug 2023 22:01:13 +0200 Subject: [PATCH] Use main stack for calling ordinary functions (#3185) * Use main stack for calling - Move `return_value` to stack * Move return_value to VM * Apply review --- boa_engine/src/builtins/eval/mod.rs | 1 + boa_engine/src/builtins/generator/mod.rs | 13 +++-- .../declaration/declaration_pattern.rs | 16 +++---- boa_engine/src/bytecompiler/expression/mod.rs | 3 +- boa_engine/src/bytecompiler/function.rs | 1 + boa_engine/src/bytecompiler/jump_control.rs | 11 ++++- boa_engine/src/bytecompiler/mod.rs | 3 +- boa_engine/src/bytecompiler/statement/mod.rs | 13 +++-- boa_engine/src/vm/call_frame/mod.rs | 6 --- boa_engine/src/vm/code_block.rs | 47 ++++++------------- boa_engine/src/vm/mod.rs | 12 ++++- boa_engine/src/vm/opcode/await/mod.rs | 6 ++- .../src/vm/opcode/control_flow/return.rs | 4 +- boa_engine/src/vm/opcode/generator/mod.rs | 15 ++++-- 14 files changed, 81 insertions(+), 70 deletions(-) diff --git a/boa_engine/src/builtins/eval/mod.rs b/boa_engine/src/builtins/eval/mod.rs index ab3e25b1b8..caa4baccba 100644 --- a/boa_engine/src/builtins/eval/mod.rs +++ b/boa_engine/src/builtins/eval/mod.rs @@ -258,6 +258,7 @@ impl Eval { .vm .push_frame(CallFrame::new(code_block).with_env_fp(env_fp)); context.realm().resize_global_env(); + let record = context.run(); context.vm.pop_frame(); diff --git a/boa_engine/src/builtins/generator/mod.rs b/boa_engine/src/builtins/generator/mod.rs index cfca602d79..ea9a636e4d 100644 --- a/boa_engine/src/builtins/generator/mod.rs +++ b/boa_engine/src/builtins/generator/mod.rs @@ -85,13 +85,18 @@ impl GeneratorContext { /// Creates a new `GeneratorContext` from the current `Context` state. pub(crate) fn from_current(context: &mut Context<'_>) -> Self { - Self { + let fp = context.vm.frame().fp as usize; + let this = Self { environments: context.vm.environments.clone(), call_frame: Some(context.vm.frame().clone()), - stack: context.vm.stack.clone(), + stack: context.vm.stack[fp..].to_vec(), active_function: context.vm.active_function.clone(), realm: context.realm().clone(), - } + }; + + context.vm.stack.truncate(fp); + + this } /// Resumes execution with `GeneratorContext` as the current execution context. @@ -109,6 +114,8 @@ impl GeneratorContext { .vm .push_frame(self.call_frame.take().expect("should have a call frame")); + context.vm.frame_mut().fp = 0; + if let Some(value) = value { context.vm.push(value); } diff --git a/boa_engine/src/bytecompiler/declaration/declaration_pattern.rs b/boa_engine/src/bytecompiler/declaration/declaration_pattern.rs index ab8149c5c5..37bee0444a 100644 --- a/boa_engine/src/bytecompiler/declaration/declaration_pattern.rs +++ b/boa_engine/src/bytecompiler/declaration/declaration_pattern.rs @@ -191,30 +191,26 @@ impl ByteCompiler<'_, '_> { self.compile_array_pattern_element(element, def); } + let no_exception_thrown = self.jump(); self.patch_handler(handler_index); self.emit_opcode(Opcode::MaybeException); // stack: hasPending, exception? self.current_stack_value_count += 2; - let iterator_close_handler = self.push_handler(); self.iterator_close(false); - - let exit = self.jump(); self.patch_handler(iterator_close_handler); self.current_stack_value_count -= 2; - { - let jump = self.jump_if_false(); - self.emit_opcode(Opcode::Throw); - self.patch_jump(jump); - } - self.emit_opcode(Opcode::ReThrow); - self.patch_jump(exit); let jump = self.jump_if_false(); self.emit_opcode(Opcode::Throw); self.patch_jump(jump); + self.emit_opcode(Opcode::ReThrow); + + self.patch_jump(no_exception_thrown); + + self.iterator_close(false); } } } diff --git a/boa_engine/src/bytecompiler/expression/mod.rs b/boa_engine/src/bytecompiler/expression/mod.rs index 8189e096dd..5f9ee4c3d9 100644 --- a/boa_engine/src/bytecompiler/expression/mod.rs +++ b/boa_engine/src/bytecompiler/expression/mod.rs @@ -204,8 +204,7 @@ impl ByteCompiler<'_, '_> { } self.close_active_iterators(); - self.emit_opcode(Opcode::SetReturnValue); - self.r#return(); + self.r#return(true); self.patch_jump(throw_method_undefined); self.iterator_close(self.in_async()); diff --git a/boa_engine/src/bytecompiler/function.rs b/boa_engine/src/bytecompiler/function.rs index 5418488d3f..b159f21271 100644 --- a/boa_engine/src/bytecompiler/function.rs +++ b/boa_engine/src/bytecompiler/function.rs @@ -159,6 +159,7 @@ impl FunctionCompiler { // Note: We do handle exceptions thrown by generator body in `AsyncGeneratorStart`. if compiler.in_generator() { assert!(compiler.async_handler.is_none()); + if compiler.in_async() { // Patched in `ByteCompiler::finish()`. compiler.async_handler = Some(compiler.push_handler()); diff --git a/boa_engine/src/bytecompiler/jump_control.rs b/boa_engine/src/bytecompiler/jump_control.rs index f98191386a..85708c90a9 100644 --- a/boa_engine/src/bytecompiler/jump_control.rs +++ b/boa_engine/src/bytecompiler/jump_control.rs @@ -66,7 +66,7 @@ pub(crate) enum JumpRecordAction { pub(crate) enum JumpRecordKind { Break, Continue, - Return, + Return { return_value_on_stack: bool }, } /// This represents a local control flow handling. See [`JumpRecordKind`] for types. @@ -122,7 +122,13 @@ impl JumpRecord { match self.kind { JumpRecordKind::Break => compiler.patch_jump(self.label), JumpRecordKind::Continue => compiler.patch_jump_with_target(self.label, start_address), - JumpRecordKind::Return => { + JumpRecordKind::Return { + return_value_on_stack, + } => { + if return_value_on_stack { + compiler.emit_opcode(Opcode::SetReturnValue); + } + match (compiler.in_async(), compiler.in_generator()) { // Taken from: // - 27.6.3.2 AsyncGeneratorStart ( generator, generatorBody ): https://tc39.es/ecma262/#sec-asyncgeneratorstart @@ -137,6 +143,7 @@ impl JumpRecord { (true, false) => compiler.emit_opcode(Opcode::CompletePromiseCapability), (_, _) => {} } + compiler.emit_opcode(Opcode::Return); } } diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index b0364f9f36..2b5d834a90 100644 --- a/boa_engine/src/bytecompiler/mod.rs +++ b/boa_engine/src/bytecompiler/mod.rs @@ -315,6 +315,7 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> { params: FormalParameterList::default(), compile_environments: Vec::default(), current_open_environments_count: 0, + current_stack_value_count: 0, code_block_flags, handlers: ThinVec::default(), @@ -1496,7 +1497,7 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> { if let Some(async_handler) = self.async_handler { self.patch_handler(async_handler); } - self.r#return(); + self.r#return(false); let name = self .context diff --git a/boa_engine/src/bytecompiler/statement/mod.rs b/boa_engine/src/bytecompiler/statement/mod.rs index f1f4a9089a..81f7b3d45a 100644 --- a/boa_engine/src/bytecompiler/statement/mod.rs +++ b/boa_engine/src/bytecompiler/statement/mod.rs @@ -72,9 +72,8 @@ impl ByteCompiler<'_, '_> { } else { self.emit_opcode(Opcode::PushUndefined); } - self.emit_opcode(Opcode::SetReturnValue); - self.r#return(); + self.r#return(true); } Statement::Try(t) => self.compile_try(t, use_expr), Statement::Expression(expr) => { @@ -88,10 +87,16 @@ impl ByteCompiler<'_, '_> { } } - pub(crate) fn r#return(&mut self) { + pub(crate) fn r#return(&mut self, return_value_on_stack: bool) { let actions = self.return_jump_record_actions(); - JumpRecord::new(JumpRecordKind::Return, actions).perform_actions(Self::DUMMY_ADDRESS, self); + JumpRecord::new( + JumpRecordKind::Return { + return_value_on_stack, + }, + actions, + ) + .perform_actions(Self::DUMMY_ADDRESS, self); } fn return_jump_record_actions(&self) -> Vec { diff --git a/boa_engine/src/vm/call_frame/mod.rs b/boa_engine/src/vm/call_frame/mod.rs index 430840434e..9597e3a28a 100644 --- a/boa_engine/src/vm/call_frame/mod.rs +++ b/boa_engine/src/vm/call_frame/mod.rs @@ -36,11 +36,6 @@ pub struct CallFrame { /// How many iterations a loop has done. pub(crate) loop_iteration_count: u64, - - /// The value that is returned from the function. - // - // TODO(HalidOdat): Remove this and put into the stack, maybe before frame pointer. - pub(crate) return_value: JsValue, } /// ---- `CallFrame` public API ---- @@ -68,7 +63,6 @@ impl CallFrame { iterators: ThinVec::new(), binding_stack: Vec::new(), loop_iteration_count: 0, - return_value: JsValue::undefined(), } } diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index e060fcec06..86134adf50 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -1235,22 +1235,7 @@ impl JsObject { } let argument_count = args.len(); - - // Push function arguments to the stack. - let mut args = if code.params.as_ref().len() > args.len() { - let mut v = args.to_vec(); - v.extend(vec![ - JsValue::Undefined; - code.params.as_ref().len() - args.len() - ]); - v - } else { - args.to_vec() - }; - args.reverse(); - let mut stack = args; - - std::mem::swap(&mut context.vm.stack, &mut stack); + let parameters_count = code.params.as_ref().len(); let frame = CallFrame::new(code) .with_argument_count(argument_count as u32) @@ -1260,6 +1245,12 @@ impl JsObject { context.vm.push_frame(frame); + // Push function arguments to the stack. + for _ in argument_count..parameters_count { + context.vm.push(JsValue::undefined()); + } + context.vm.stack.extend(args.iter().rev().cloned()); + let result = context .run() .consume() @@ -1267,7 +1258,6 @@ impl JsObject { context.vm.pop_frame().expect("frame must exist"); std::mem::swap(&mut environments, &mut context.vm.environments); - std::mem::swap(&mut context.vm.stack, &mut stack); std::mem::swap(&mut context.vm.active_runnable, &mut script_or_module); result @@ -1442,22 +1432,7 @@ impl JsObject { } let argument_count = args.len(); - - // Push function arguments to the stack. - let args = if code.params.as_ref().len() > args.len() { - let mut v = args.to_vec(); - v.extend(vec![ - JsValue::Undefined; - code.params.as_ref().len() - args.len() - ]); - v - } else { - args.to_vec() - }; - - for arg in args.iter().rev() { - context.vm.push(arg.clone()); - } + let parameters_count = code.params.as_ref().len(); let has_binding_identifier = code.has_binding_identifier(); @@ -1469,6 +1444,12 @@ impl JsObject { .with_env_fp(environments_len as u32), ); + // Push function arguments to the stack. + for _ in argument_count..parameters_count { + context.vm.push(JsValue::undefined()); + } + context.vm.stack.extend(args.iter().rev().cloned()); + let record = context.run(); context.vm.pop_frame(); diff --git a/boa_engine/src/vm/mod.rs b/boa_engine/src/vm/mod.rs index 8ddef1b24d..9e3c4d7de9 100644 --- a/boa_engine/src/vm/mod.rs +++ b/boa_engine/src/vm/mod.rs @@ -53,6 +53,7 @@ mod tests; pub struct Vm { pub(crate) frames: Vec, pub(crate) stack: Vec, + pub(crate) return_value: JsValue, /// When an error is thrown, the pending exception is set. /// @@ -94,6 +95,7 @@ impl Vm { Self { frames: Vec::with_capacity(16), stack: Vec::with_capacity(1024), + return_value: JsValue::undefined(), environments: EnvironmentStack::new(global), pending_exception: None, runtime_limits: RuntimeLimits::default(), @@ -181,6 +183,14 @@ impl Vm { true } + + pub(crate) fn get_return_value(&self) -> JsValue { + self.return_value.clone() + } + + pub(crate) fn set_return_value(&mut self, value: JsValue) { + self.return_value = value; + } } #[derive(Debug, Clone, Copy, PartialEq)] @@ -329,7 +339,7 @@ impl Context<'_> { Ok(CompletionType::Normal) => {} Ok(CompletionType::Return) => { self.vm.stack.truncate(self.vm.frame().fp as usize); - let execution_result = self.vm.frame_mut().return_value.clone(); + let execution_result = std::mem::take(&mut self.vm.return_value); return CompletionRecord::Normal(execution_result); } Ok(CompletionType::Throw) => { diff --git a/boa_engine/src/vm/opcode/await/mod.rs b/boa_engine/src/vm/opcode/await/mod.rs index f952580c39..c39e42128a 100644 --- a/boa_engine/src/vm/opcode/await/mod.rs +++ b/boa_engine/src/vm/opcode/await/mod.rs @@ -189,14 +189,16 @@ impl Operation for CompletePromiseCapability { .call(&JsValue::undefined(), &[error.to_opaque(context)], context) .expect("cannot fail per spec"); } else { - let return_value = context.vm.frame().return_value.clone(); + let return_value = context.vm.get_return_value(); promise_capability .resolve() .call(&JsValue::undefined(), &[return_value], context) .expect("cannot fail per spec"); }; - context.vm.frame_mut().return_value = promise_capability.promise().clone().into(); + context + .vm + .set_return_value(promise_capability.promise().clone().into()); Ok(CompletionType::Normal) } diff --git a/boa_engine/src/vm/opcode/control_flow/return.rs b/boa_engine/src/vm/opcode/control_flow/return.rs index 17e8898390..8db9cab870 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 GetReturnValue { const INSTRUCTION: &'static str = "INST - GetReturnValue"; fn execute(context: &mut Context<'_>) -> JsResult { - let value = context.vm.frame().return_value.clone(); + let value = context.vm.get_return_value(); context.vm.push(value); Ok(CompletionType::Normal) } @@ -50,7 +50,7 @@ impl Operation for SetReturnValue { fn execute(context: &mut Context<'_>) -> JsResult { let value = context.vm.pop(); - context.vm.frame_mut().return_value = value; + context.vm.set_return_value(value); Ok(CompletionType::Normal) } } diff --git a/boa_engine/src/vm/opcode/generator/mod.rs b/boa_engine/src/vm/opcode/generator/mod.rs index 34d4242a7a..41ce8dd001 100644 --- a/boa_engine/src/vm/opcode/generator/mod.rs +++ b/boa_engine/src/vm/opcode/generator/mod.rs @@ -16,7 +16,7 @@ use crate::{ opcode::{Operation, ReThrow}, CallFrame, CompletionType, }, - Context, JsError, JsObject, JsResult, + Context, JsError, JsObject, JsResult, JsValue, }; pub(crate) use yield_stm::*; @@ -41,7 +41,14 @@ impl Operation for Generator { let pc = context.vm.frame().pc; let mut dummy_call_frame = CallFrame::new(code_block); dummy_call_frame.pc = pc; - let call_frame = std::mem::replace(context.vm.frame_mut(), dummy_call_frame); + let mut call_frame = std::mem::replace(context.vm.frame_mut(), dummy_call_frame); + + let fp = call_frame.fp as usize; + + let stack = context.vm.stack[fp..].to_vec(); + context.vm.stack.truncate(fp); + + call_frame.fp = 0; let this_function_object = context .vm @@ -69,7 +76,6 @@ impl Operation for Generator { &mut context.vm.environments, EnvironmentStack::new(global_environement), ); - let stack = std::mem::take(&mut context.vm.stack); let data = if r#async { ObjectData::async_generator(AsyncGenerator { @@ -154,7 +160,8 @@ impl Operation for AsyncGeneratorClose { .expect("must have item in queue"); drop(generator_object_mut); - let return_value = std::mem::take(&mut context.vm.frame_mut().return_value); + let return_value = context.vm.get_return_value(); + context.vm.set_return_value(JsValue::undefined()); if let Some(error) = context.vm.pending_exception.take() { AsyncGenerator::complete_step(&next, Err(error), true, None, context);