Skip to content

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

Merged
ASDAlexander77 merged 1 commit into
mainfrom
fix/lowering-passes-bugs-3
Jul 14, 2026
Merged

Fix finally-region cloning losing break/continue/parentTryOp wiring#233
ASDAlexander77 merged 1 commit into
mainfrom
fix/lowering-passes-bugs-3

Conversation

@ASDAlexander77

Copy link
Copy Markdown
Owner

Summary

Follow-up to #232, addressing the last actionable finding (item 5) from the LowerToAffineLoops.cpp/LowerToLLVM.cpp lowering-passes review (docs/LowerToAffineLoops-LowerToLLVM-review.md).

  • TryOpLowering's finally region cloningcloneRegionBefore is called twice to build the normal-exit and return-cleanup copies of finally, producing entirely new Operation* instances each time. Any tsContext side-table entry (jumps[] for break/continue, parentTryOp[] for a nested try) keyed on the original region's ops — populated by walking that region before cloning — was silently missing for both clones; only the third, inlined copy (which preserves original identity) kept correct wiring. Fixed by passing an explicit mlir::IRMapping to each cloneRegionBefore call and re-propagating matching side-table entries from each old op onto its clone via the mapping's getOperationMap().
  • Bonus fix, found while building the regression test: 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), independent of break/continue. Root cause: the dialect-conversion driver legalizes a cloned nested TryOp recursively while the outer TryOp's own pattern application is still on the stack, and refuses to reapply the same shared Pattern instance recursively without an explicit setHasBoundedRewriteRecursion() opt-in — which no TsPattern had. Fixed by giving TryOpLowering its own constructor that opts in (safe here: each recursive application strictly consumes one TryOp).
  • Found but deliberately NOT fixed, tracked in the review doc (item 5b): break/continue inside a try body (as opposed to directly inside finally) does not run the enclosing finally block before jumping — reproduces a JIT crash on unmodified main, predates all three passes of this review. Out of scope for this change; the new test only covers break/continue placed directly inside finally, which works correctly.

Test plan

  • cmake --build __build/tslang/windows-msbuild-2026-debug --config Debug -j8 — clean build
  • ctest --test-dir __build/tslang/windows-msbuild-2026-debug -C Debug -j8 — 690/690 passed (JIT + AOT)
  • New 00try_finally_break_continue.ts: break directly in finally, continue directly in finally, and a nested try/catch inside finally — each exercised across multiple loop iterations to catch "only the first/last clone copy is wired correctly" bugs. JIT + AOT green.

@ASDAlexander77 ASDAlexander77 merged commit 8eaf13a into main Jul 14, 2026
2 checks passed
@ASDAlexander77 ASDAlexander77 deleted the fix/lowering-passes-bugs-3 branch July 14, 2026 21:53
ASDAlexander77 added a commit that referenced this pull request Jul 15, 2026
A follow-up static audit of LowerToAffineLoops.cpp, LowerToLLVM.cpp, and
the TypeScriptExceptionPass files (beyond the 10 findings already closed
in PRs #230/#232/#233/#234/#235) found and fixes 7 more confirmed bugs:

- SwitchStateOpLowering: generator yield/resume state labels were
  collected into a SmallPtrSet<Operation*,16>, whose iteration order is
  only insertion order below 16 entries; past that it silently falls back
  to pointer-hash order, scrambling the positional switch dispatch used
  by generator resume. Replaced with an order-preserving SmallVector.
- StringConcatOpLowering: the stack-allocation path sized its Alloca
  using a pointer type instead of i8, over-allocating by sizeof(ptr) for
  every multi-argument string concatenation (e.g. console.log with 2+
  args).
- ArraySpliceOpLowering: mixed genuine MLIR index-dialect ops with
  already-LLVM-converted operands, a type mismatch that made every
  Array.prototype.splice() call fail to lower correctly (the only
  .splice() call in the test suite was commented out because of this).
  Reworked to stay consistently in the LLVM-converted domain, matching
  every sibling array-mutation lowering.
- GlobalOpLowering: the side-effect enumeration deciding whether a global
  initializer can be inlined as a constant vs. must run as a real
  constructor was missing several ops with real side effects (array
  push/unshift/splice/pop/shift, delete, set-length, string concat/
  char-to-string).
- LandingPadFixPass: MadeChange was set unconditionally for every
  landingpad visited, even when nothing was rewritten, causing needless
  analysis invalidation on every EH-containing function on every compile.
- Win32ExceptionPass: the PHI-user removal loop erased only the first PHI
  user found then unconditionally erased the underlying value, violating
  LLVM's no-remaining-uses invariant when more than one PHI referenced the
  same to-be-removed value (own TODO comment flagged this as incomplete).
- Win32ExceptionPass: getThrowFn's fallback path created a stray
  _CxxThrowException Function without attaching it to the module.

Added 00array_splice.ts (JIT + AOT) covering shrink/grow/equal-size
splice cases, since this path had zero prior test coverage. Full ctest
suite green: 692/692.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant