diff --git a/core/engine/src/bytecompiler/expression/mod.rs b/core/engine/src/bytecompiler/expression/mod.rs index c387b3ffbd3..2c993c8d093 100644 --- a/core/engine/src/bytecompiler/expression/mod.rs +++ b/core/engine/src/bytecompiler/expression/mod.rs @@ -17,7 +17,7 @@ use std::ops::Deref; use super::{Access, CallResultDest, Callable, NodeKind, Register, ToJsString}; use crate::{ - bytecompiler::{ByteCompiler, Literal}, + bytecompiler::{ByteCompiler, Literal, ReturnValueLocation}, vm::{CallFrame, GeneratorResumeKind}, }; use boa_ast::{ @@ -236,7 +236,7 @@ impl ByteCompiler<'_> { } self.close_active_iterators(); - self.r#return(true); + self.r#return(ReturnValueLocation::OnStack); self.patch_jump(throw_method_undefined); diff --git a/core/engine/src/bytecompiler/jump_control.rs b/core/engine/src/bytecompiler/jump_control.rs index 4fc1020a306..3c27e6334d6 100644 --- a/core/engine/src/bytecompiler/jump_control.rs +++ b/core/engine/src/bytecompiler/jump_control.rs @@ -67,12 +67,34 @@ pub(crate) enum JumpRecordAction { }, } +/// Where an explicit `return` value is stored while `finally` blocks execute. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ReturnValueLocation { + /// On the value stack. + /// + /// Used when the `return` doesn't pass through any `finally` block, so no + /// abrupt completion can interleave and leave stale values behind. + OnStack, + /// In the function-level pending-return register (see + /// [`ByteCompiler::pending_return_slot`]). + /// + /// Used when the `return` passes through a `finally` block, since abrupt + /// completions inside `finally` (e.g. `break`) skip the jump-table handler + /// that would pop a stack slot. + InSlot(u32), + /// In the accumulator. + /// + /// Used for implicit function returns, where the completion value is + /// already in the accumulator. + InAccumulator, +} + /// Local Control flow type. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum JumpRecordKind { Break, Continue, - Return { return_value_on_stack: bool }, + Return { value_location: ReturnValueLocation }, } /// This represents a local control flow handling. See [`JumpRecordKind`] for types. @@ -135,14 +157,18 @@ 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 { - return_value_on_stack, - } => { - if return_value_on_stack { - let value = compiler.register_allocator.alloc(); - compiler.pop_into_register(&value); - compiler.bytecode.emit_set_accumulator(value.variable()); - compiler.register_allocator.dealloc(value); + JumpRecordKind::Return { value_location } => { + match value_location { + ReturnValueLocation::OnStack => { + let value = compiler.register_allocator.alloc(); + compiler.pop_into_register(&value); + compiler.bytecode.emit_set_accumulator(value.variable()); + compiler.register_allocator.dealloc(value); + } + ReturnValueLocation::InSlot(slot) => { + compiler.bytecode.emit_set_accumulator(slot.into()); + } + ReturnValueLocation::InAccumulator => {} } match (compiler.is_async(), compiler.is_generator()) { diff --git a/core/engine/src/bytecompiler/mod.rs b/core/engine/src/bytecompiler/mod.rs index 3159e2f062d..93fbbb079bb 100644 --- a/core/engine/src/bytecompiler/mod.rs +++ b/core/engine/src/bytecompiler/mod.rs @@ -88,7 +88,7 @@ pub(crate) use declarations::{ global_declaration_instantiation_context, prepare_eval_declaration_instantiation, }; pub(crate) use function::FunctionCompiler; -pub(crate) use jump_control::JumpControlInfo; +pub(crate) use jump_control::{JumpControlInfo, ReturnValueLocation}; pub(crate) use register::*; pub(crate) trait ToJsString { @@ -544,6 +544,14 @@ pub struct ByteCompiler<'ctx> { jump_info: Vec, + /// Register that holds an explicit `return` value while `finally` blocks execute. + /// + /// Values are kept in a dedicated register instead of the value stack so that + /// abrupt completions inside a `finally` block (e.g. `break`) cannot leave + /// stale values behind for an outer pending `return` to read. + /// Allocated lazily by the first `return` that passes through a `finally`. + pending_return_slot: Option, + /// Used to handle exception throws that escape the async function types. /// /// Async functions and async generator functions, need to be closed and resolved. @@ -670,6 +678,7 @@ impl<'ctx> ByteCompiler<'ctx> { bindings_map: FxHashMap::default(), const_binding_cache: FxHashMap::default(), jump_info: Vec::new(), + pending_return_slot: None, async_handler: None, json_parse, variable_scope, @@ -713,6 +722,18 @@ impl<'ctx> ByteCompiler<'ctx> { self.interner } + /// Returns the register used to hold an explicit `return` value while + /// `finally` blocks execute, allocating it on first use. + pub(crate) fn pending_return_slot(&mut self) -> u32 { + if let Some(slot) = self.pending_return_slot { + slot + } else { + let slot = self.register_allocator.alloc_persistent().index(); + self.pending_return_slot = Some(slot); + slot + } + } + fn get_or_insert_literal(&mut self, literal: Literal) -> u32 { if let Some(index) = self.literals_map.get(&literal) { return *index; @@ -2757,7 +2778,7 @@ impl<'ctx> ByteCompiler<'ctx> { if let Some(async_handler) = self.async_handler { self.patch_handler(async_handler); } - self.r#return(false); + self.r#return(ReturnValueLocation::InAccumulator); let final_bytecode_len = self.next_opcode_location(); diff --git a/core/engine/src/bytecompiler/statement/mod.rs b/core/engine/src/bytecompiler/statement/mod.rs index 0b10b4ba224..86d9e78c2e0 100644 --- a/core/engine/src/bytecompiler/statement/mod.rs +++ b/core/engine/src/bytecompiler/statement/mod.rs @@ -1,4 +1,6 @@ -use super::jump_control::{JumpRecord, JumpRecordAction, JumpRecordKind}; +use super::jump_control::{ + JumpControlInfo, JumpRecord, JumpRecordAction, JumpRecordKind, ReturnValueLocation, +}; use crate::{bytecompiler::ByteCompiler, vm::CallFrame}; use boa_ast::Statement; @@ -65,6 +67,19 @@ impl ByteCompiler<'_> { self.compile_switch(switch, use_expr); } Statement::Return(ret) => { + // If the `return` passes through a `finally` block, keep the value in + // a dedicated register instead of the value stack, so that abrupt + // completions inside `finally` (e.g. `break`) cannot leave a stale + // value behind for an outer pending `return` to read. + let slot = if self + .jump_info + .iter() + .any(JumpControlInfo::is_try_with_finally_block) + { + Some(self.pending_return_slot()) + } else { + None + }; if let Some(expr) = ret.target() { if self.is_async_generator() { let value = self.register_allocator.alloc(); @@ -75,16 +90,32 @@ impl ByteCompiler<'_> { self.pop_into_register(&value); self.generator_next(&value, &resume_kind); self.register_allocator.dealloc(resume_kind); - self.push_from_register(&value); + if let Some(slot) = slot { + self.bytecode.emit_move(slot.into(), value.variable()); + } else { + self.push_from_register(&value); + } + self.register_allocator.dealloc(value); + } else if let Some(slot) = slot { + let value = self.register_allocator.alloc(); + self.compile_expr(expr, &value); + self.bytecode.emit_move(slot.into(), value.variable()); self.register_allocator.dealloc(value); } else { self.compile_expr_to_stack(expr); } + } else if let Some(slot) = slot { + self.bytecode + .emit_move(slot.into(), CallFrame::undefined_register().variable()); } else { self.push_from_register(&CallFrame::undefined_register()); } - self.r#return(true); + if let Some(slot) = slot { + self.r#return(ReturnValueLocation::InSlot(slot)); + } else { + self.r#return(ReturnValueLocation::OnStack); + } } Statement::Try(t) => self.compile_try(t, use_expr), Statement::Expression(expr) => { @@ -102,16 +133,11 @@ impl ByteCompiler<'_> { } } - pub(crate) fn r#return(&mut self, return_value_on_stack: bool) { + pub(crate) fn r#return(&mut self, value_location: ReturnValueLocation) { let actions = self.return_jump_record_actions(); - JumpRecord::new( - JumpRecordKind::Return { - return_value_on_stack, - }, - actions, - ) - .perform_actions(Self::DUMMY_ADDRESS, self); + JumpRecord::new(JumpRecordKind::Return { value_location }, actions) + .perform_actions(Self::DUMMY_ADDRESS, self); } fn return_jump_record_actions(&self) -> Vec { diff --git a/core/engine/src/bytecompiler/statement/try.rs b/core/engine/src/bytecompiler/statement/try.rs index dc2cf51eb62..6a75ef6c1e8 100644 --- a/core/engine/src/bytecompiler/statement/try.rs +++ b/core/engine/src/bytecompiler/statement/try.rs @@ -202,12 +202,28 @@ impl ByteCompiler<'_> { pub(crate) fn compile_finally_stmt(&mut self, finally: &Finally) { // TODO: We could probably remove the Get/SetAccumulatorFromStack if we check that there is no break/continues statements. + // + // Preserve a pending explicit `return` value across the `finally` block. + // Abrupt completions inside `finally` skip the restore, which correctly + // discards the pending value. + let slot_save = self.pending_return_slot.map(|slot| { + let saved = self.register_allocator.alloc(); + self.bytecode.emit_move(saved.variable(), slot.into()); + saved + }); let value = self.register_allocator.alloc(); self.bytecode .emit_set_register_from_accumulator(value.variable()); self.compile_catch_finally_block(finally.block(), false); self.bytecode.emit_set_accumulator(value.variable()); self.register_allocator.dealloc(value); + if let Some(saved) = slot_save { + let slot = self + .pending_return_slot + .expect("pending return slot must still exist"); + self.bytecode.emit_move(slot.into(), saved.variable()); + self.register_allocator.dealloc(saved); + } } /// Compile a catch or finally block. diff --git a/core/engine/src/tests/control_flow/mod.rs b/core/engine/src/tests/control_flow/mod.rs index 13b4b7f79b7..6c6064491c2 100644 --- a/core/engine/src/tests/control_flow/mod.rs +++ b/core/engine/src/tests/control_flow/mod.rs @@ -255,6 +255,42 @@ fn finally_with_loop_break() { )]); } +#[test] +fn nested_finally_break_discards_inner_return() { + run_test_actions([ + TestAction::assert_eq( + indoc! {r#" + function f() { + try { + return 42; + } finally { + do try { + return 43; + } finally { + break; + } while (0); + } + } + f(); + "#}, + 42, + ), + TestAction::assert_eq( + indoc! {r#" + function f() { + try { + return 42; + } finally { + return 43; + } + } + f(); + "#}, + 43, + ), + ]); +} + #[test] fn single_case_switch() { run_test_actions([TestAction::assert_eq(