From a601f7f0a7665159234bff70971459be4a196e59 Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Tue, 14 Jul 2026 22:21:03 +0100 Subject: [PATCH] Fix bugs in TryOpLowering and add regression tests for finally-region cloning --- docs/LowerToAffineLoops-LowerToLLVM-review.md | 69 +++++++++++++-- tslang/lib/TypeScript/LowerToAffineLoops.cpp | 52 +++++++++++- tslang/test/tester/CMakeLists.txt | 2 + .../tests/00try_finally_break_continue.ts | 84 +++++++++++++++++++ 4 files changed, 196 insertions(+), 11 deletions(-) create mode 100644 tslang/test/tester/tests/00try_finally_break_continue.ts diff --git a/docs/LowerToAffineLoops-LowerToLLVM-review.md b/docs/LowerToAffineLoops-LowerToLLVM-review.md index 7542fd4e..79952caf 100644 --- a/docs/LowerToAffineLoops-LowerToLLVM-review.md +++ b/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 diff --git a/tslang/lib/TypeScript/LowerToAffineLoops.cpp b/tslang/lib/TypeScript/LowerToAffineLoops.cpp index 9c101114..e73b072a 100644 --- a/tslang/lib/TypeScript/LowerToAffineLoops.cpp +++ b/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 { using TsPattern::TsPattern; + TryOpLowering(MLIRContext *context, TSContext *tsContext, TSFunctionContext *tsFuncContext, + PatternBenefit benefit = 1) + : TsPattern(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,15 +1371,45 @@ struct TryOpLowering : public TsPattern 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(returnFinallyBlockLast->getTerminator()); @@ -1373,12 +1417,12 @@ struct TryOpLowering : public TsPattern // 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 { diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 598b19c4..8bdf8b21 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/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 diff --git a/tslang/test/tester/tests/00try_finally_break_continue.ts b/tslang/test/tester/tests/00try_finally_break_continue.ts new file mode 100644 index 00000000..728184da --- /dev/null +++ b/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."); +}