Fix finally-region cloning losing break/continue/parentTryOp wiring

docs/LowerToAffineLoops-LowerToLLVM-review.md

@@ -181,15 +181,70 @@ the fix was written.
     (silent no-op in release builds, leaving a malformed CFG) replaced with
     `return rewriter.notifyMatchFailure(...)`.
 
-## Remaining known issues (not yet fixed, tracked here for follow-up)
+## Third pass (2026-07-14) — item 5 fixed, plus two bugs found while testing it
+
+Fixed on branch `fix/lowering-passes-bugs-3`, verified via full ctest suite
+(690/690 passed, JIT + AOT).
 
 5. **`LowerToAffineLoops.cpp:1351,1358,1370`** (`TryOpLowering`, `finally`
-   region cloning) — the `finally` region is cloned twice (normal-exit and
-   return-cleanup copies) *after* the `parentTryOp`/`jumps` side-tables were
-   populated by walking only the original region. A nested `try` or a
-   `break`/`continue` inside a `finally` block loses correct wiring in the
-   cloned copies. PLAUSIBLE, not yet fixed — needs a test case (`grep` of
-   `test/` found no try/finally-nesting coverage).
+   region cloning) — FIXED. Confirmed: `cloneRegionBefore` produces brand-new
+   `Operation*` instances (verified against MLIR's `Region::cloneInto`/
+   `IRMapping` machinery), so `tsContext->jumps`/`parentTryOp` entries
+   populated by walking the *original* `finally` region (line ~1260, and by
+   an enclosing loop's break/continue-scope walk that runs before this try
+   is lowered) were silently absent for both of the two cloned copies
+   (normal-exit and return-cleanup) — only the third, *inlined* copy (which
+   preserves original `Operation*` identity) kept valid wiring. Fixed by
+   passing an explicit `mlir::IRMapping` to both `cloneRegionBefore` calls
+   and re-propagating any `jumps`/`parentTryOp` entry found for an old op
+   onto its corresponding new op via the mapping's `getOperationMap()`,
+   immediately after each clone.
+
+   While building a regression test for this, found and fixed two more bugs
+   in the same area:
+
+   - **Pre-existing, unrelated to this fix**: a `try` nested directly inside
+     a `finally` block failed to compile at all (`error: failed to legalize
+     operation 'ts.Try' that was explicitly marked illegal`), regardless of
+     break/continue. Root cause: `cloneRegionBefore` correctly notifies the
+     conversion driver of the cloned nested `TryOp`, which then legalizes it
+     *recursively while the outer TryOp's own pattern application is still on
+     the stack* (`legalizePatternCreatedOperations`) — but MLIR's
+     `OperationLegalizer::canApplyPattern` refuses to reapply the same shared
+     `Pattern` instance recursively unless it opts in via
+     `setHasBoundedRewriteRecursion()`, which no `TsPattern` did. Fixed by
+     giving `TryOpLowering` its own constructor that calls
+     `setHasBoundedRewriteRecursion()` — safe here since each recursive
+     application strictly consumes one `TryOp`.
+   - **Pre-existing, separate, NOT fixed — tracked here for follow-up**:
+     `break`/`continue` inside a `try` *body* (as opposed to directly inside
+     `finally`) does not run the enclosing `finally` block before jumping —
+     `jumps[breakOp]`/`jumps[continueOp]` are set by the loop's own
+     break/continue-scope walk (`visitBreakContinueInScope`) to point
+     *directly* at the loop's continuation/increment block, with no
+     awareness that a `try/finally` sits in between. Reproduces a JIT crash
+     (`0x80000003`) on unmodified `main` (i.e. predates all three passes of
+     this review) with a minimal `for { try { if (...) continue; } finally
+     { ... } }`. Not fixed in this pass — would require threading
+     finally-block awareness into the jump-target logic for every loop
+     lowering, larger in scope than this pass's fixes. The new regression
+     test (`00try_finally_break_continue.ts`) deliberately only covers
+     break/continue placed directly *inside* `finally` (which does work
+     correctly) to avoid this separate gap.
+
+   Added `00try_finally_break_continue.ts` (break in finally, continue in
+   finally, nested try/catch in finally, each inside a loop, each run 3-5
+   times to catch wrong-copy-used-once-vs-every-iteration bugs), JIT +
+   AOT green.
+
+## Remaining known issues (not yet fixed, tracked here for follow-up)
+
+5b. **`LowerToAffineLoops.cpp`** (loop `jumps[]` target computation) —
+    `break`/`continue` inside a `try` body does not run an enclosing
+    `finally` block before jumping to the loop's continuation/increment.
+    See "Third pass" above for the repro and root cause. Needs the
+    break/continue lowering (or the `TryOp` lowering) to detect that the
+    jump target crosses a `finally` boundary and route through it first.
 
 8. **`LowerToLLVM.cpp:4159`** (`LandingPadOpLowering`, Windows path) —
    cleanup-only branch reuses the typed-catch filter value instead of an

tslang/lib/TypeScript/LowerToAffineLoops.cpp

@@ -21,6 +21,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Sequence.h"
 #include "mlir/IR/Dialect.h"
+#include "mlir/IR/IRMapping.h"
 #include "llvm/Support/Debug.h"
 
 #include "scanner_enums.h"
@@ -1219,6 +1220,19 @@ struct TryOpLowering : public TsPattern<mlir_ts::TryOp>
 {
     using TsPattern<mlir_ts::TryOp>::TsPattern;
 
+    TryOpLowering(MLIRContext *context, TSContext *tsContext, TSFunctionContext *tsFuncContext,
+                  PatternBenefit benefit = 1)
+        : TsPattern<mlir_ts::TryOp>(context, tsContext, tsFuncContext, benefit)
+    {
+        // a `try` nested inside its own `finally` block gets cloned (not just moved) as part
+        // of lowering the outer TryOp, so the dialect-conversion driver legalizes the inner
+        // TryOp recursively while the outer TryOp's own pattern application is still on the
+        // stack. Without this, the conversion driver's single-pattern-instance recursion guard
+        // refuses to reapply this same pattern object and the whole outer TryOp fails to
+        // legalize. Recursion here is bounded: each application strictly consumes one TryOp.
+        setHasBoundedRewriteRecursion();
+    }
+
     // TODO: set 'loc' correctly to newly created ops
     LogicalResult matchAndRewrite(mlir_ts::TryOp tryOp, PatternRewriter &rewriter) const final
     {
@@ -1357,28 +1371,58 @@ struct TryOpLowering : public TsPattern<mlir_ts::TryOp>
         mlir::Block *exitBlockLast = nullptr;
         if (finallyHasOps)
         {
+            // cloneRegionBefore makes brand-new Operation* instances, so any tsContext
+            // side-table entry (jumps[], parentTryOp[]) keyed on the original finally
+            // region's ops -- populated by visitorTryOps above, or by an enclosing loop's
+            // break/continue-scope walk that ran before this try was lowered -- would
+            // silently be missing for the clones. Re-propagate those entries onto each
+            // clone via the IRMapping so a nested try/break/continue inside `finally`
+            // keeps correct wiring in all copies, not just the final inlined one.
+            auto propagateTsContextEntries = [&](const mlir::IRMapping &mapping) {
+                for (auto &[oldOp, newOp] : mapping.getOperationMap())
+                {
+                    auto jumpIt = tsContext->jumps.find(oldOp);
+                    if (jumpIt != tsContext->jumps.end())
+                    {
+                        tsContext->jumps[newOp] = jumpIt->second;
+                    }
+
+                    auto parentIt = tsContext->parentTryOp.find(oldOp);
+                    if (parentIt != tsContext->parentTryOp.end())
+                    {
+                        tsContext->parentTryOp[newOp] = parentIt->second;
+                    }
+                }
+            };
+
             auto beforeFinallyBlock = continuation->getPrevNode();
-            rewriter.cloneRegionBefore(tryOp.getFinally(), continuation);
+            mlir::IRMapping finallyMapping;
+            rewriter.cloneRegionBefore(tryOp.getFinally(), *continuation->getParent(), continuation->getIterator(),
+                                        finallyMapping);
+            propagateTsContextEntries(finallyMapping);
             finallyBlock = beforeFinallyBlock->getNextNode();
             finallyBlockLast = continuation->getPrevNode();
 
             // add clone for 'return'
             if (returns.size() > 0)
             {
-                rewriter.cloneRegionBefore(tryOp.getFinally(), continuation);
+                mlir::IRMapping returnFinallyMapping;
+                rewriter.cloneRegionBefore(tryOp.getFinally(), *continuation->getParent(), continuation->getIterator(),
+                                            returnFinallyMapping);
+                propagateTsContextEntries(returnFinallyMapping);
                 auto returnFinallyBlockLast = continuation->getPrevNode();
                 rewriter.setInsertionPoint(returnFinallyBlockLast->getTerminator());
                 auto resultOpOfReturnFinallyBlock = cast<mlir_ts::ResultOp>(returnFinallyBlockLast->getTerminator());
                 rewriter.replaceOpWithNewOp<mlir_ts::ReturnOp>(resultOpOfReturnFinallyBlock);
                 // if has returns we need to create return cleanup block
                 for (auto retOp : returns)
                 {
-                    tsContext->cleanup[retOp] = returnFinallyBlockLast;                
+                    tsContext->cleanup[retOp] = returnFinallyBlockLast;
                 }
             }
 
             rewriter.inlineRegionBefore(tryOp.getFinally(), continuation);
-            exitBlockLast = continuation->getPrevNode();            
+            exitBlockLast = continuation->getPrevNode();
         }
         else
         {

tslang/test/tester/CMakeLists.txt

@@ -309,6 +309,7 @@ add_test(NAME test-compile-00-try-catch-return COMMAND test-runner "${PROJECT_SO
 add_test(NAME test-compile-00-try-finally COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_finally.ts")
 add_test(NAME test-compile-01-try-finally COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/01try_finally.ts")
 add_test(NAME test-compile-00-try-finally-return COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_finally_return.ts")
+add_test(NAME test-compile-00-try-finally-break-continue COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_finally_break_continue.ts")
 add_test(NAME test-compile-00-try-catch-rethrow COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_catch_rethrow.ts")
 add_test(NAME test-compile-00-try-catch-mismatch-rethrow COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_catch_mismatch_rethrow.ts")
 add_test(NAME test-compile-00-try-catch-return-dispose COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_catch_return_dispose.ts")
@@ -645,6 +646,7 @@ add_test(NAME test-jit-00-try-catch-return COMMAND test-runner -jit "${PROJECT_S
 add_test(NAME test-jit-00-try-finally COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_finally.ts")
 add_test(NAME test-jit-01-try-finally COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/01try_finally.ts")
 add_test(NAME test-jit-00-try-finally-return COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_finally_return.ts")
+add_test(NAME test-jit-00-try-finally-break-continue COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_finally_break_continue.ts")
 add_test(NAME test-jit-00-try-catch-rethrow COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_catch_rethrow.ts")
 # pre-existing JIT limitation (reproduces on unmodified main too): the JIT's
 # Windows SEH landing pad/personality plumbing exits with code 3 and no

tslang/test/tester/tests/00try_finally_break_continue.ts

@@ -0,0 +1,84 @@
+// regression test for TryOpLowering's finally-region cloning: break/continue and a
+// nested try inside `finally` must keep correct wiring in all 3 copies of the region
+// (the two cloned exception-path copies, and the inlined normal-exit copy).
+//
+// Note: break/continue inside a try *body* (rather than directly in `finally`) is a
+// separate, pre-existing bug tracked in docs/LowerToAffineLoops-LowerToLLVM-review.md --
+// intervening finally blocks are not run before the jump, so those cases are not covered
+// here.
+
+function test_break_in_finally() {
+    let iterations = 0;
+    let finallyRuns = 0;
+
+    for (let i = 0; i < 5; i++) {
+        iterations++;
+        try {
+            print("try ", i);
+        } finally {
+            finallyRuns++;
+            if (i == 2) {
+                break;
+            }
+        }
+    }
+
+    assert(iterations == 3, "break-in-finally: wrong iteration count");
+    assert(finallyRuns == 3, "break-in-finally: wrong finally-run count");
+
+    return iterations * 10 + finallyRuns;
+}
+
+function test_continue_in_finally() {
+    let sum = 0;
+    let finallyRuns = 0;
+
+    for (let i = 0; i < 5; i++) {
+        try {
+            print("try ", i);
+        } finally {
+            finallyRuns++;
+            if (i == 2) {
+                continue;
+            }
+            sum += i;
+        }
+    }
+
+    assert(finallyRuns == 5, "continue-in-finally: wrong finally-run count");
+
+    // sum should skip i == 2 (continue in finally happens before sum += i runs for that
+    // iteration): 0 + 1 + 3 + 4 = 8
+    return sum;
+}
+
+function test_nested_try_in_finally() {
+    let caught = 0;
+    let finallyRuns = 0;
+
+    for (let i = 0; i < 3; i++) {
+        try {
+            print("outer try ", i);
+        } finally {
+            finallyRuns++;
+            try {
+                throw "inner";
+            } catch (e: string) {
+                caught++;
+            }
+        }
+    }
+
+    assert(finallyRuns == 3, "nested-try-in-finally: wrong finally-run count");
+    assert(caught == 3, "nested-try-in-finally: inner catch did not run every time");
+
+    return caught;
+}
+
+function main() {
+    assert(test_break_in_finally() == 33, "failed. 1");
+    assert(test_continue_in_finally() == 8, "failed. 2");
+    assert(test_nested_try_in_finally() == 3, "failed. 3");
+
+    print("done.");
+}