From c8e4499b087b2562dce90f97bf48fa2cc86fb102 Mon Sep 17 00:00:00 2001 From: Hendiadyoin1 Date: Sun, 12 May 2024 11:03:26 +0200 Subject: [PATCH] LibJS: Make return control flow more static With this only `ContinuePendingUnwind` needs to dynamically check if a scheduled return needs to go through a `finally` block, making the interpreter loop a bit nicer --- .../Libraries/LibJS/Bytecode/ASTCodegen.cpp | 20 ++++------ .../Libraries/LibJS/Bytecode/Generator.cpp | 2 +- Userland/Libraries/LibJS/Bytecode/Generator.h | 30 ++++++++++++++ .../Libraries/LibJS/Bytecode/Interpreter.cpp | 40 ++++++++----------- 4 files changed, 54 insertions(+), 38 deletions(-) diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index c078fc9ac7b..d78141f53b3 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -1766,13 +1766,10 @@ Bytecode::CodeGenerationErrorOr> ReturnStatement::genera return_value = generator.add_constant(js_undefined()); } - if (generator.is_in_generator_or_async_function()) { - generator.perform_needed_unwinds(); - generator.emit(nullptr, *return_value); - } else { - generator.perform_needed_unwinds(); - generator.emit(return_value.has_value() ? return_value->operand() : Optional {}); - } + if (generator.is_in_generator_or_async_function()) + generator.emit_return(return_value.value()); + else + generator.emit_return(return_value.value()); return return_value; } @@ -2117,8 +2114,7 @@ Bytecode::CodeGenerationErrorOr> YieldExpression::genera // 2. Return ? received. // NOTE: This will always be a return completion. - generator.perform_needed_unwinds(); - generator.emit(nullptr, received_completion_value); + generator.emit_return(received_completion_value); generator.switch_to_basic_block(return_is_defined_block); @@ -2155,8 +2151,7 @@ Bytecode::CodeGenerationErrorOr> YieldExpression::genera generator.emit_iterator_value(inner_return_result_value, inner_return_result); // 2. Return Completion Record { [[Type]]: return, [[Value]]: value, [[Target]]: empty }. - generator.perform_needed_unwinds(); - generator.emit(nullptr, inner_return_result_value); + generator.emit_return(inner_return_result_value); generator.switch_to_basic_block(type_is_return_not_done_block); @@ -2239,8 +2234,7 @@ Bytecode::CodeGenerationErrorOr> YieldExpression::genera generator.emit(received_completion_value); generator.switch_to_basic_block(return_value_block); - generator.perform_needed_unwinds(); - generator.emit(nullptr, received_completion_value); + generator.emit_return(received_completion_value); generator.switch_to_basic_block(normal_completion_continuation_block); return received_completion_value; diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.cpp b/Userland/Libraries/LibJS/Bytecode/Generator.cpp index 3dafc51bc22..aeaf5956446 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Generator.cpp @@ -232,7 +232,7 @@ CodeGenerationErrorOr> Generator::emit_function_body_by if (block->is_terminated()) continue; generator.switch_to_basic_block(*block); - generator.emit(nullptr, generator.add_constant(js_undefined())); + generator.emit_return(generator.add_constant(js_undefined())); } } diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.h b/Userland/Libraries/LibJS/Bytecode/Generator.h index 88782c21ab2..8d4fb234c92 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.h +++ b/Userland/Libraries/LibJS/Bytecode/Generator.h @@ -261,6 +261,7 @@ public: } bool is_in_finalizer() const { return m_boundaries.contains_slow(BlockBoundaryType::LeaveFinally); } + bool must_enter_finalizer() const { return m_boundaries.contains_slow(BlockBoundaryType::ReturnToFinally); } void generate_break(); void generate_break(DeprecatedFlyString const& break_label); @@ -268,6 +269,35 @@ public: void generate_continue(); void generate_continue(DeprecatedFlyString const& continue_label); + template + void emit_return(ScopedOperand value) + requires(IsOneOf) + { + // FIXME: Tell the call sites about the `saved_return_value` destination + // And take that into account in the movs below. + perform_needed_unwinds(); + if (must_enter_finalizer()) { + VERIFY(m_current_basic_block->finalizer() != nullptr); + // Compare to: + // * Interpreter::do_return + // * Interpreter::run_bytecode::handle_ContinuePendingUnwind + // * Return::execute_impl + // * Yield::execute_impl + emit(Operand(Register::saved_return_value()), value); + emit(Operand(Register::exception()), add_constant(Value {})); + // FIXME: Do we really need to clear the return value register here? + emit(Operand(Register::return_value()), add_constant(Value {})); + + emit(Label { *m_current_basic_block->finalizer() }); + return; + } + + if constexpr (IsSame) + emit(value); + else + emit(nullptr, value); + } + void start_boundary(BlockBoundaryType type) { m_boundaries.append(type); } void end_boundary(BlockBoundaryType type) { diff --git a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp index 6a0a9e62bcc..76ebf165af6 100644 --- a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp @@ -356,8 +356,6 @@ FLATTEN_ON_CLANG void Interpreter::run_bytecode(size_t entry_point) goto* bytecode_dispatch_table[static_cast(next_instruction.type())]; \ } while (0) - bool will_yield = false; - for (;;) { start: for (;;) { @@ -486,7 +484,19 @@ FLATTEN_ON_CLANG void Interpreter::run_bytecode(size_t entry_point) } if (!saved_return_value().is_empty()) { do_return(saved_return_value()); - goto run_finalizer_and_return; + if (auto handlers = executable.exception_handlers_for_offset(program_counter); handlers.has_value()) { + if (auto finalizer = handlers.value().finalizer_offset; finalizer.has_value()) { + VERIFY(!running_execution_context.unwind_contexts.is_empty()); + auto& unwind_context = running_execution_context.unwind_contexts.last(); + VERIFY(unwind_context.executable == m_current_executable); + reg(Register::saved_return_value()) = reg(Register::return_value()); + reg(Register::return_value()) = {}; + program_counter = finalizer.value(); + // the unwind_context will be pop'ed when entering the finally block + goto start; + } + } + return; } auto const old_scheduled_jump = running_execution_context.previously_scheduled_jumps.take_last(); if (m_scheduled_jump.has_value()) { @@ -639,14 +649,13 @@ FLATTEN_ON_CLANG void Interpreter::run_bytecode(size_t entry_point) handle_Await: { auto& instruction = *reinterpret_cast(&bytecode[program_counter]); instruction.execute_impl(*this); - will_yield = true; - goto run_finalizer_and_return; + return; } handle_Return: { auto& instruction = *reinterpret_cast(&bytecode[program_counter]); instruction.execute_impl(*this); - goto run_finalizer_and_return; + return; } handle_Yield: { @@ -657,27 +666,10 @@ FLATTEN_ON_CLANG void Interpreter::run_bytecode(size_t entry_point) // but we generate a Yield Operation in the case of returns in // generators as well, so we need to check if it will actually // continue or is a `return` in disguise - will_yield = instruction.continuation().has_value(); - goto run_finalizer_and_return; + return; } } } - -run_finalizer_and_return: - if (!will_yield) { - if (auto handlers = executable.exception_handlers_for_offset(program_counter); handlers.has_value()) { - if (auto finalizer = handlers.value().finalizer_offset; finalizer.has_value()) { - VERIFY(!running_execution_context.unwind_contexts.is_empty()); - auto& unwind_context = running_execution_context.unwind_contexts.last(); - VERIFY(unwind_context.executable == m_current_executable); - reg(Register::saved_return_value()) = reg(Register::return_value()); - reg(Register::return_value()) = {}; - program_counter = finalizer.value(); - // the unwind_context will be pop'ed when entering the finally block - goto start; - } - } - } } Interpreter::ResultAndReturnRegister Interpreter::run_executable(Executable& executable, Optional entry_point, Value initial_accumulator_value)