From 8ca2adcadac72f1f70d9fab269eaf9a7cc698a73 Mon Sep 17 00:00:00 2001
From: Jonathan Coates <jonathan.coates@oxionics.com>
Date: Tue, 19 Nov 2024 17:24:53 +0000
Subject: [PATCH] Correctly handle try/catch try/finally blocks

If we have a try/catch block nested inside a try/finally, then the
finally block would not be executed in the event of the exception.
This is because the landing pad of the inner try is marked as having no
cleanup clause.

We now set has_cleanup to False only if there is no outer try block.

Signed-off-by: Jonathan Coates <jonathan.coates@oxionics.com>
---
 .../compiler/transforms/artiq_ir_generator.py |  4 +---
 .../test/lit/exceptions/finally_catch_try.py  | 19 ++++++++++++++++++
 artiq/test/lit/exceptions/finally_try.py      | 20 +++++++++++++++++++
 3 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100644 artiq/test/lit/exceptions/finally_catch_try.py
 create mode 100644 artiq/test/lit/exceptions/finally_try.py

diff --git a/artiq/compiler/transforms/artiq_ir_generator.py b/artiq/compiler/transforms/artiq_ir_generator.py
index a7177210b..d3d348b12 100644
--- a/artiq/compiler/transforms/artiq_ir_generator.py
+++ b/artiq/compiler/transforms/artiq_ir_generator.py
@@ -703,7 +703,7 @@ class ARTIQIRGenerator(algorithm.Visitor):
                 return_action.append(ir.Return(value))
                 final_branch(return_action, return_proxy)
         else:
-            landingpad.has_cleanup = False
+            landingpad.has_cleanup = self.unwind_target is not None
 
         # we should propagate the clauses to nested try catch blocks
         # so nested try catch will jump to our clause if the inner one does not
@@ -767,7 +767,6 @@ class ARTIQIRGenerator(algorithm.Visitor):
                 self.continue_target = old_continue
             self.return_target = old_return
 
-        if any(node.finalbody):
             # create new unwind target for cleanup
             final_dispatcher = self.add_block("try.final.dispatch")
             final_landingpad = ir.LandingPad(cleanup)
@@ -776,7 +775,6 @@ class ARTIQIRGenerator(algorithm.Visitor):
             # make sure that exception clauses are unwinded to the finally block
             old_unwind, self.unwind_target = self.unwind_target, final_dispatcher
 
-        if any(node.finalbody):
             # if we have a while:try/finally continue must execute finally
             # before continuing the while
             redirect = final_branch
diff --git a/artiq/test/lit/exceptions/finally_catch_try.py b/artiq/test/lit/exceptions/finally_catch_try.py
new file mode 100644
index 000000000..c1b500a78
--- /dev/null
+++ b/artiq/test/lit/exceptions/finally_catch_try.py
@@ -0,0 +1,19 @@
+# RUN: %python -m artiq.compiler.testbench.jit %s >%t
+# RUN: OutputCheck %s --file-to-check=%t
+# REQUIRES: exceptions
+
+def doit():
+    try:
+        try:
+            raise RuntimeError("Error")
+        except ValueError:
+            print("ValueError")
+    except RuntimeError:
+        print("Caught")
+    finally:
+        print("Cleanup")
+
+doit()
+
+# CHECK-L: Caught
+# CHECK-NEXT-L: Cleanup
diff --git a/artiq/test/lit/exceptions/finally_try.py b/artiq/test/lit/exceptions/finally_try.py
new file mode 100644
index 000000000..377ca1c06
--- /dev/null
+++ b/artiq/test/lit/exceptions/finally_try.py
@@ -0,0 +1,20 @@
+# RUN: %python -m artiq.compiler.testbench.jit %s >%t
+# RUN: OutputCheck %s --file-to-check=%t
+# REQUIRES: exceptions
+
+def doit():
+    try:
+        try:
+            raise RuntimeError("Error")
+        except ValueError:
+            print("ValueError")
+    finally:
+        print("Cleanup")
+
+try:
+    doit()
+except RuntimeError:
+    print("Caught")
+
+# CHECK-L: Cleanup
+# CHECK-NEXT-L: Caught