From 6b6e13e28c46177617db5b2a25fcebc1a63eaf3c Mon Sep 17 00:00:00 2001
From: Lucien Fiorini <lucienfiorini@gmail.com>
Date: Thu, 27 Mar 2025 12:02:14 +0100
Subject: [PATCH] LibJS: Avoid emptying the return value register in
 try/finally

This works because at the end of the finally chunk, a
ContinuePendingUnwind is generated which copies the saved return value
register into the return value register. In cases where
ContinuePendingUnwind is not generated such as when there is a break
statement in the finally block, the fonction will return undefined which
is consistent with V8 and SpiderMonkey.
---
 Libraries/LibJS/Bytecode/Generator.h       |  4 ----
 Libraries/LibJS/Bytecode/Interpreter.cpp   |  2 --
 Libraries/LibJS/Tests/try-finally-break.js | 14 ++++++++++++++
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Libraries/LibJS/Bytecode/Generator.h b/Libraries/LibJS/Bytecode/Generator.h
index 27ad0efdda2..3d86faa0e87 100644
--- a/Libraries/LibJS/Bytecode/Generator.h
+++ b/Libraries/LibJS/Bytecode/Generator.h
@@ -274,8 +274,6 @@ public:
     void emit_return(ScopedOperand value)
     requires(IsOneOf<OpType, Op::Return, Op::Yield>)
     {
-        // FIXME: Tell the call sites about the `saved_return_value` destination
-        //        And take that into account in the movs below.
         perform_needed_unwinds<OpType>();
         if (must_enter_finalizer()) {
             VERIFY(m_current_basic_block->finalizer() != nullptr);
@@ -289,8 +287,6 @@ public:
             else
                 emit<Bytecode::Op::Mov>(Operand(Register::saved_return_value()), value);
             emit<Bytecode::Op::Mov>(Operand(Register::exception()), add_constant(Value {}));
-            // FIXME: Do we really need to clear the return value register here?
-            emit<Bytecode::Op::Mov>(Operand(Register::return_value()), add_constant(Value {}));
             emit<Bytecode::Op::Jump>(Label { *m_current_basic_block->finalizer() });
             return;
         }
diff --git a/Libraries/LibJS/Bytecode/Interpreter.cpp b/Libraries/LibJS/Bytecode/Interpreter.cpp
index d45d84699e5..35e0fabb6e8 100644
--- a/Libraries/LibJS/Bytecode/Interpreter.cpp
+++ b/Libraries/LibJS/Bytecode/Interpreter.cpp
@@ -778,8 +778,6 @@ Interpreter::ResultAndReturnRegister Interpreter::run_executable(Executable& exe
     auto return_value = js_undefined();
     if (!reg(Register::return_value()).is_empty())
         return_value = reg(Register::return_value());
-    else if (!reg(Register::saved_return_value()).is_empty())
-        return_value = reg(Register::saved_return_value());
     auto exception = reg(Register::exception());
 
     vm().run_queued_promise_jobs();
diff --git a/Libraries/LibJS/Tests/try-finally-break.js b/Libraries/LibJS/Tests/try-finally-break.js
index 162c3ed5da1..c8ed3edaf61 100644
--- a/Libraries/LibJS/Tests/try-finally-break.js
+++ b/Libraries/LibJS/Tests/try-finally-break.js
@@ -490,3 +490,17 @@ test("Break with nested mixed try-catch/finally", () => {
 
     expect(executionOrder).toEqual([1, 2]);
 });
+
+test("Break in finally return in try", () => {
+    function foo() {
+        do {
+            try {
+                return "bar";
+            } finally {
+                break;
+            }
+        } while (expect.fail("Continued after do-while loop"));
+    }
+
+    expect(foo()).toEqual(undefined);
+});