Fix loop-carried results, Array.shift GEP stride, and mismatched-catch rethrow#230
Merged
Conversation
…h rethrow Three correctness bugs found in a static audit of LowerToAffineLoops.cpp and LowerToLLVM.cpp: - ForOpLowering dropped loop-carried results for infinite for(;;) loops because the NoConditionOp branch never assigned the replacement values. - ArrayShiftOpLowering computed the post-shift memmove offset with a hardcoded i32 GEP stride instead of the array's real element type, corrupting Array<number>.shift() and other wider-than-i32 element arrays. - TryOpLowering's typed-catch RTTI mismatch (non-Windows/Itanium path) silently fell through to normal control flow instead of rethrowing, per the code's own TODO comment. Adds regression tests for all three and a review doc covering these plus 7 additional lower-confidence findings tracked for follow-up.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ForOpLoweringdropped loop-carried results for infinitefor(;;)loops — theNoConditionOpbranch never assigned the replacementValueRange, soreplaceOpran with an empty list whenever the loop carried results.ArrayShiftOpLoweringcomputed the post-shift memmove source offset using a hardcoded i32 GEP stride instead of the array's real element type, corruptingArray<number>.shift()(and any element type wider than i32).TryOpLowering's typed-catch RTTI mismatch (non-Windows/Itanium path) silently fell through to normal post-try control flow instead of rethrowing — the code carried its own// TODO: when catch not matching - should go into result (rethrow). Now rethrows via the existingNullOp+ThrowOpidiom, routing to the parent try's landing pad when one exists.Found via a static audit of
LowerToAffineLoops.cpp/LowerToLLVM.cpp(neither file had been touched since PR #198). Full findings, including 7 additional lower-confidence issues tracked for follow-up, are written up indocs/LowerToAffineLoops-LowerToLLVM-review.md.Test plan
00for_infinite_result.ts(AOT + JIT) — infinitefor(;;)with a loop-carried sum, asserts correct result.00array_shift.ts(AOT + JIT) —Array<number>.shift()asserts correct return value and remaining array contents.00try_catch_mismatch_rethrow.ts(AOT only — see note below) — nested try/catch where a mismatched typed catch must rethrow past it to an outer handler.--emit=mlir-affine --mtriple=x86_64-pc-linux-gnu(correctThrowCall/ThrowUnwindrouting, including re-entry into an outer try's landing pad for the nested case), since this Windows JIT host only exercises the SEH path natively.Note: the JIT variant of the mismatch-rethrow test is commented out in
CMakeLists.txt— it hits a separate, pre-existing JIT limitation (Windows SEH landing-pad plumbing exits with code 3, no output) that reproduces identically on unmodifiedmain, unrelated to this fix (which only touches the non-Windows/ItaniumcmpValuepath). The AOT variant passes and exercises the fix correctly.🤖 Generated with Claude Code