LibJS: Generate unwind chains for break in Bytecode

This uses a newly added instruction `ScheduleJump`
This instruction tells the finally proceeding it, that instead of
jumping to it's next block it should jump to the designated block.

Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp

@@ -2168,17 +2168,11 @@ Bytecode::CodeGenerationErrorOr<void> BreakStatement::generate_bytecode(Bytecode
     //        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(
-            generator.nearest_breakable_scope(),
-            {});
+        generator.generate_break();
         return {};
     }
 
-    auto target_to_jump_to = generator.perform_needed_unwinds_for_labelled_break_and_return_target_block(m_target_label);
-    generator.emit<Bytecode::Op::Jump>().set_targets(
-        target_to_jump_to,
-        {});
+    generator.generate_break(m_target_label);
     return {};
 }
 
@@ -2192,6 +2186,7 @@ Bytecode::CodeGenerationErrorOr<void> TryStatement::generate_bytecode(Bytecode::
     Bytecode::BasicBlock* next_block { nullptr };
 
     if (m_finalizer) {
+        // FIXME: See notes in Op.h->ScheduleJump
         auto& finalizer_block = generator.make_block();
         generator.switch_to_basic_block(finalizer_block);
         generator.emit<Bytecode::Op::LeaveUnwindContext>();

Userland/Libraries/LibJS/Bytecode/Generator.cpp

@@ -262,23 +262,62 @@ CodeGenerationErrorOr<void> Generator::emit_delete_reference(JS::ASTNode const&
     return {};
 }
 
-Label Generator::perform_needed_unwinds_for_labelled_break_and_return_target_block(DeprecatedFlyString const& break_label)
+void Generator::generate_break()
+{
+    bool last_was_finally = false;
+    // FIXME: Reduce code duplication
+    for (size_t i = m_boundaries.size(); i > 0; --i) {
+        auto boundary = m_boundaries[i - 1];
+        using enum BlockBoundaryType;
+        switch (boundary) {
+        case Break:
+            emit<Op::Jump>().set_targets(nearest_breakable_scope(), {});
+            return;
+        case Unwind:
+            if (!last_was_finally)
+                emit<Bytecode::Op::LeaveUnwindContext>();
+            last_was_finally = false;
+            break;
+        case LeaveLexicalEnvironment:
+            emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Lexical);
+            break;
+        case LeaveVariableEnvironment:
+            emit<Bytecode::Op::LeaveEnvironment>(Bytecode::Op::EnvironmentMode::Var);
+            break;
+        case Continue:
+            break;
+        case ReturnToFinally: {
+            auto& block = make_block(DeprecatedString::formatted("{}.break", current_block().name()));
+            emit<Op::ScheduleJump>(Label { block });
+            switch_to_basic_block(block);
+            last_was_finally = true;
+            break;
+        };
+        }
+    }
+    VERIFY_NOT_REACHED();
+}
+
+void Generator::generate_break(DeprecatedFlyString const& break_label)
 {
     size_t current_boundary = m_boundaries.size();
-    for (auto& breakable_scope : m_breakable_scopes.in_reverse()) {
+    bool last_was_finally = false;
+    for (auto const& 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>();
+                if (!last_was_finally)
+                    emit<Bytecode::Op::LeaveUnwindContext>();
+                last_was_finally = false;
             } 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
+                auto& block = make_block(DeprecatedString::formatted("{}.break", current_block().name()));
+                emit<Op::ScheduleJump>(Label { block });
+                switch_to_basic_block(block);
+                last_was_finally = true;
             } 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;
@@ -286,8 +325,10 @@ Label Generator::perform_needed_unwinds_for_labelled_break_and_return_target_blo
             }
         }
 
-        if (breakable_scope.language_label_set.contains_slow(break_label))
-            return breakable_scope.bytecode_target;
+        if (breakable_scope.language_label_set.contains_slow(break_label)) {
+            emit<Op::Jump>().set_targets(breakable_scope.bytecode_target, {});
+            return;
+        }
     }
 
     // We must have a breakable scope available that contains the label, as this should be enforced by the parser.

Userland/Libraries/LibJS/Bytecode/Generator.h

@@ -213,7 +213,9 @@ public:
         }
     }
 
-    Label perform_needed_unwinds_for_labelled_break_and_return_target_block(DeprecatedFlyString const& break_label);
+    void generate_break();
+    void generate_break(DeprecatedFlyString const& break_label);
+
     Label perform_needed_unwinds_for_labelled_continue_and_return_target_block(DeprecatedFlyString const& continue_label);
 
     void start_boundary(BlockBoundaryType type) { m_boundaries.append(type); }

Userland/Libraries/LibJS/Bytecode/Instruction.h

@@ -78,6 +78,7 @@
     O(ResolveThisBinding)            \
     O(Return)                        \
     O(RightShift)                    \
+    O(ScheduleJump)                  \
     O(SetVariable)                   \
     O(Store)                         \
     O(StrictlyEquals)                \

Userland/Libraries/LibJS/Bytecode/Interpreter.cpp

@@ -44,6 +44,7 @@ Interpreter::ValueAndFrame Interpreter::run_and_return_frame(Executable const& e
     dbgln_if(JS_BYTECODE_DEBUG, "Bytecode::Interpreter will run unit {:p}", &executable);
 
     TemporaryChange restore_executable { m_current_executable, &executable };
+    TemporaryChange restore_saved_jump { m_scheduled_jump, static_cast<BasicBlock const*>(nullptr) };
     VERIFY(m_saved_exception.is_null());
 
     bool pushed_execution_context = false;
@@ -217,7 +218,14 @@ ThrowCompletionOr<void> Interpreter::continue_pending_unwind(Label const& resume
         return {};
     }
 
-    jump(resume_label);
+    if (m_scheduled_jump) {
+        // FIXME: If we `break` or `continue` in the finally, we need to clear
+        //        this field
+        jump(Label { *m_scheduled_jump });
+        m_scheduled_jump = nullptr;
+    } else {
+        jump(resume_label);
+    }
     return {};
 }
 

Userland/Libraries/LibJS/Bytecode/Interpreter.h

@@ -59,6 +59,12 @@ public:
     {
         m_pending_jump = &label.block();
     }
+    void schedule_jump(Label const& label)
+    {
+        m_scheduled_jump = &label.block();
+        VERIFY(unwind_contexts().last().finalizer);
+        jump(Label { *unwind_contexts().last().finalizer });
+    }
     void do_return(Value return_value) { m_return_value = return_value; }
 
     void enter_unwind_context(Optional<Label> handler_target, Optional<Label> finalizer_target);
@@ -102,6 +108,7 @@ private:
     Realm& m_realm;
     Vector<Variant<NonnullOwnPtr<RegisterWindow>, RegisterWindow*>> m_register_windows;
     Optional<BasicBlock const*> m_pending_jump;
+    BasicBlock const* m_scheduled_jump { nullptr };
     Value m_return_value;
     Handle<Value> m_saved_return_value;
     Executable const* m_current_executable { nullptr };

Userland/Libraries/LibJS/Bytecode/Op.cpp

@@ -790,6 +790,12 @@ void Call::replace_references_impl(Register from, Register to)
         m_this_value = to;
 }
 
+ThrowCompletionOr<void> ScheduleJump::execute_impl(Bytecode::Interpreter& interpreter) const
+{
+    interpreter.schedule_jump(m_target);
+    return {};
+}
+
 ThrowCompletionOr<void> LeaveEnvironment::execute_impl(Bytecode::Interpreter& interpreter) const
 {
     if (m_mode == EnvironmentMode::Lexical)
@@ -1265,6 +1271,11 @@ DeprecatedString EnterUnwindContext::to_deprecated_string_impl(Bytecode::Executa
     return DeprecatedString::formatted("EnterUnwindContext handler:{} finalizer:{} entry:{}", handler_string, finalizer_string, m_entry_point);
 }
 
+DeprecatedString ScheduleJump::to_deprecated_string_impl(Bytecode::Executable const&) const
+{
+    return DeprecatedString::formatted("ScheduleJump {}", m_target);
+}
+
 DeprecatedString LeaveEnvironment::to_deprecated_string_impl(Bytecode::Executable const&) const
 {
     auto mode_string = m_mode == EnvironmentMode::Lexical

Userland/Libraries/LibJS/Bytecode/Op.h

@@ -887,6 +887,41 @@ private:
     Optional<Label> m_finalizer_target;
 };
 
+class ScheduleJump final : public Instruction {
+public:
+    // Note: We use this instruction to tell the next `finally` block to
+    //       continue execution with a specific break/continue target;
+    // FIXME: We currently don't clear the interpreter internal flag, when we change
+    //        the control-flow (`break`, `continue`) in a finally-block,
+    // FIXME: .NET on x86_64 uses a call to the finally instead, which could make this
+    //        easier, at the cost of making control-flow changes (`break`, `continue`, `return`)
+    //        in the finally-block more difficult, but as stated above, those
+    //        aren't handled 100% correctly at the moment anyway
+    //        It might be worth investigating a similar mechanism
+    constexpr static bool IsTerminator = true;
+
+    ScheduleJump(Label target)
+        : Instruction(Type::ScheduleJump)
+        , m_target(target)
+    {
+    }
+
+    Label target() const { return m_target; }
+
+    ThrowCompletionOr<void> execute_impl(Bytecode::Interpreter&) const;
+    void replace_references_impl(BasicBlock const& from, BasicBlock const& to)
+    {
+        if (&m_target.block() == &from)
+            m_target = Label { to };
+    }
+    void replace_references_impl(Register, Register) { }
+
+    DeprecatedString to_deprecated_string_impl(Bytecode::Executable const&) const;
+
+private:
+    Label m_target;
+};
+
 class LeaveEnvironment final : public Instruction {
 public:
     LeaveEnvironment(EnvironmentMode mode)

Userland/Libraries/LibJS/Bytecode/Pass/GenerateCFG.cpp

@@ -134,6 +134,10 @@ static void generate_cfg_for_block(BasicBlock const& current_block, PassPipeline
             if (auto const* finalizer = unwind_frames.last()->finalizer)
                 enter_label(Label { *finalizer }, current_block);
             return;
+        case ScheduleJump:
+            enter_label(Label { *unwind_frames.last()->finalizer }, current_block);
+            enter_label(static_cast<Op::ScheduleJump const&>(instruction).target(), *unwind_frames.last()->finalizer);
+            return;
         default:
             dbgln("Unhandled terminator instruction: `{}`", instruction.to_deprecated_string(executable.executable));
             VERIFY_NOT_REACHED();