Fix loop-carried results, Array.shift GEP stride, and mismatched-catch rethrow#230
Open
ASDAlexander77 wants to merge 1 commit into
Open
Fix loop-carried results, Array.shift GEP stride, and mismatched-catch rethrow#230ASDAlexander77 wants to merge 1 commit into
ASDAlexander77 wants to merge 1 commit into
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.
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
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