LibJS: Intercept returns through finally blocks in Bytecode

This is still not perfect, as we now actually crash in the
`try-finally-continue` tests, while we now succeed all
`try-catch-finally-*` tests.

Note that we do not yet go through the finally block when exiting the
unwind context through a break or continue.

Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp

@@ -1707,6 +1707,9 @@ Bytecode::CodeGenerationErrorOr<void> IfStatement::generate_bytecode(Bytecode::G
 
 Bytecode::CodeGenerationErrorOr<void> ContinueStatement::generate_bytecode(Bytecode::Generator& generator) const
 {
+    // FIXME: Handle finally blocks in a graceful manner
+    //        We need to execute the finally block, but tell it to resume
+    //        execution at the designated block
     if (m_target_label.is_null()) {
         generator.perform_needed_unwinds<Bytecode::Op::Jump>();
         generator.emit<Bytecode::Op::Jump>().set_targets(
@@ -1900,6 +1903,9 @@ Bytecode::CodeGenerationErrorOr<void> ThrowStatement::generate_bytecode(Bytecode
 
 Bytecode::CodeGenerationErrorOr<void> BreakStatement::generate_bytecode(Bytecode::Generator& generator) const
 {
+    // FIXME: Handle finally blocks in a graceful manner
+    //        We need to execute the finally block, but tell it to resume
+    //        execution at the designated block
     if (m_target_label.is_null()) {
         generator.perform_needed_unwinds<Bytecode::Op::Jump>(true);
         generator.emit<Bytecode::Op::Jump>().set_targets(
@@ -1937,9 +1943,15 @@ Bytecode::CodeGenerationErrorOr<void> TryStatement::generate_bytecode(Bytecode::
         finalizer_target = Bytecode::Label { finalizer_block };
     }
 
+    if (m_finalizer)
+        generator.start_boundary(Bytecode::Generator::BlockBoundaryType::ReturnToFinally);
     if (m_handler) {
         auto& handler_block = generator.make_block();
         generator.switch_to_basic_block(handler_block);
+
+        if (!m_finalizer)
+            generator.emit<Bytecode::Op::LeaveUnwindContext>();
+
         generator.begin_variable_scope(Bytecode::Generator::BindingMode::Lexical, Bytecode::Generator::SurroundingScopeKind::Block);
         TRY(m_handler->parameter().visit(
             [&](FlyString const& parameter) -> Bytecode::CodeGenerationErrorOr<void> {
@@ -1974,11 +1986,15 @@ Bytecode::CodeGenerationErrorOr<void> TryStatement::generate_bytecode(Bytecode::
             }
         }
     }
+    if (m_finalizer)
+        generator.end_boundary(Bytecode::Generator::BlockBoundaryType::ReturnToFinally);
 
     auto& target_block = generator.make_block();
     generator.switch_to_basic_block(saved_block);
     generator.emit<Bytecode::Op::EnterUnwindContext>(Bytecode::Label { target_block }, handler_target, finalizer_target);
     generator.start_boundary(Bytecode::Generator::BlockBoundaryType::Unwind);
+    if (m_finalizer)
+        generator.start_boundary(Bytecode::Generator::BlockBoundaryType::ReturnToFinally);
 
     generator.switch_to_basic_block(target_block);
     TRY(m_block->generate_bytecode(generator));
@@ -1992,6 +2008,9 @@ Bytecode::CodeGenerationErrorOr<void> TryStatement::generate_bytecode(Bytecode::
             next_block = &block;
         }
     }
+
+    if (m_finalizer)
+        generator.end_boundary(Bytecode::Generator::BlockBoundaryType::ReturnToFinally);
     generator.end_boundary(Bytecode::Generator::BlockBoundaryType::Unwind);
 
     generator.switch_to_basic_block(next_block ? *next_block : saved_block);

Userland/Libraries/LibJS/Bytecode/Generator.cpp

@@ -268,12 +268,17 @@ Label Generator::perform_needed_unwinds_for_labelled_break_and_return_target_blo
     for (auto& breakable_scope : m_breakable_scopes.in_reverse()) {
         for (; current_boundary > 0; --current_boundary) {
             auto boundary = m_boundaries[current_boundary - 1];
+            // FIXME: Handle ReturnToFinally in a graceful manner
+            //        We need to execute the finally block, but tell it to resume
+            //        execution at the designated label
             if (boundary == BlockBoundaryType::Unwind) {
                 emit<Bytecode::Op::LeaveUnwindContext>();
             } else if (boundary == BlockBoundaryType::LeaveLexicalEnvironment) {
                 emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Lexical);
             } else if (boundary == BlockBoundaryType::LeaveVariableEnvironment) {
                 emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Var);
+            } else if (boundary == BlockBoundaryType::ReturnToFinally) {
+                // FIXME: We need to enter the `finally`, while still scheduling the break to happen
             } else if (boundary == BlockBoundaryType::Break) {
                 // Make sure we don't process this boundary twice if the current breakable scope doesn't contain the target label.
                 --current_boundary;
@@ -295,12 +300,17 @@ Label Generator::perform_needed_unwinds_for_labelled_continue_and_return_target_
     for (auto& continuable_scope : m_continuable_scopes.in_reverse()) {
         for (; current_boundary > 0; --current_boundary) {
             auto boundary = m_boundaries[current_boundary - 1];
+            // FIXME: Handle ReturnToFinally in a graceful manner
+            //        We need to execute the finally block, but tell it to resume
+            //        execution at the designated label
             if (boundary == BlockBoundaryType::Unwind) {
                 emit<Bytecode::Op::LeaveUnwindContext>();
             } else if (boundary == BlockBoundaryType::LeaveLexicalEnvironment) {
                 emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Lexical);
             } else if (boundary == BlockBoundaryType::LeaveVariableEnvironment) {
                 emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Var);
+            } else if (boundary == BlockBoundaryType::ReturnToFinally) {
+                // FIXME: We need to enter the `finally`, while still scheduling the continue to happen
             } else if (boundary == BlockBoundaryType::Continue) {
                 // Make sure we don't process this boundary twice if the current continuable scope doesn't contain the target label.
                 --current_boundary;

Userland/Libraries/LibJS/Bytecode/Generator.h

@@ -169,6 +169,7 @@ public:
         Break,
         Continue,
         Unwind,
+        ReturnToFinally,
         LeaveLexicalEnvironment,
         LeaveVariableEnvironment,
     };
@@ -188,12 +189,27 @@ public:
             auto boundary = m_boundaries[i - 1];
             if (boundary_to_stop_at.has_value() && boundary == *boundary_to_stop_at)
                 break;
-            if (boundary == BlockBoundaryType::Unwind)
+            using enum BlockBoundaryType;
+            switch (boundary) {
+            case Unwind:
                 emit<Bytecode::Op::LeaveUnwindContext>();
-            else if (boundary == BlockBoundaryType::LeaveLexicalEnvironment)
+                break;
+            case LeaveLexicalEnvironment:
                 emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Lexical);
-            else if (boundary == BlockBoundaryType::LeaveVariableEnvironment)
+                break;
+            case LeaveVariableEnvironment:
                 emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Var);
+                break;
+            case Break:
+            case Continue:
+                break;
+            case ReturnToFinally:
+                // FIXME: In the case of breaks/continues we need to tell the `finally` to break/continue
+                //        For now let's ignore the finally to avoid a crash
+                if (IsSame<OpType, Bytecode::Op::Jump>)
+                    break;
+                return;
+            };
         }
     }
 

Userland/Libraries/LibJS/Bytecode/Interpreter.cpp

@@ -117,14 +117,28 @@ Interpreter::ValueAndFrame Interpreter::run_and_return_frame(Executable const& e
             ++pc;
         }
 
-        if (will_return)
-            break;
+        if (will_jump)
+            continue;
+
+        if (!unwind_contexts().is_empty()) {
+            auto& unwind_context = unwind_contexts().last();
+            if (unwind_context.executable == m_current_executable && unwind_context.finalizer) {
+                m_saved_return_value = make_handle(m_return_value);
+                m_return_value = {};
+                m_current_block = unwind_context.finalizer;
+                // the unwind_context will be pop'ed when entering the finally block
+                continue;
+            }
+        }
 
-        if (pc.at_end() && !will_jump)
+        if (pc.at_end())
             break;
 
         if (!m_saved_exception.is_null())
             break;
+
+        if (will_return)
+            break;
     }
 
     dbgln_if(JS_BYTECODE_DEBUG, "Bytecode::Interpreter did run unit {:p}", &executable);
@@ -142,8 +156,14 @@ Interpreter::ValueAndFrame Interpreter::run_and_return_frame(Executable const& e
 
     auto frame = m_register_windows.take_last();
 
-    auto return_value = m_return_value.value_or(js_undefined());
-    m_return_value = {};
+    Value return_value = js_undefined();
+    if (!m_return_value.is_empty()) {
+        return_value = m_return_value;
+        m_return_value = {};
+    } else if (!m_saved_return_value.is_null()) {
+        return_value = m_saved_return_value.value();
+        m_saved_return_value = {};
+    }
 
     // NOTE: The return value from a called function is put into $0 in the caller context.
     if (!m_register_windows.is_empty())
@@ -168,7 +188,7 @@ Interpreter::ValueAndFrame Interpreter::run_and_return_frame(Executable const& e
         return { throw_completion(thrown_value), nullptr };
     }
 
-    if (auto register_window = frame.get_pointer<NonnullOwnPtr<RegisterWindow>>())
+    if (auto* register_window = frame.get_pointer<NonnullOwnPtr<RegisterWindow>>())
         return { return_value, move(*register_window) };
     return { return_value, nullptr };
 }
@@ -191,6 +211,12 @@ ThrowCompletionOr<void> Interpreter::continue_pending_unwind(Label const& resume
         return result;
     }
 
+    if (!m_saved_return_value.is_null()) {
+        do_return(m_saved_return_value.value());
+        m_saved_return_value = {};
+        return {};
+    }
+
     jump(resume_label);
     return {};
 }

Userland/Libraries/LibJS/Bytecode/Interpreter.h

@@ -103,6 +103,7 @@ private:
     Vector<Variant<NonnullOwnPtr<RegisterWindow>, RegisterWindow*>> m_register_windows;
     Optional<BasicBlock const*> m_pending_jump;
     Value m_return_value;
+    Handle<Value> m_saved_return_value;
     Executable const* m_current_executable { nullptr };
     Handle<Value> m_saved_exception;
     OwnPtr<JS::Interpreter> m_ast_interpreter;