diff --git a/docs/LowerToAffineLoops-LowerToLLVM-review.md b/docs/LowerToAffineLoops-LowerToLLVM-review.md index 853d4ccf5..eb4370716 100644 --- a/docs/LowerToAffineLoops-LowerToLLVM-review.md +++ b/docs/LowerToAffineLoops-LowerToLLVM-review.md @@ -267,17 +267,48 @@ Fixed on branch `fix/lowering-passes-bugs-3`, verified via full ctest suite not rerouted — finally runs exactly once). All added to `00try_finally_break_continue.ts`. -## Remaining known issues (not yet fixed, tracked here for follow-up) +## Fifth pass (2026-07-15) — item 8 investigated: NOT A DEFECT, comments fixed 8. **`LowerToLLVM.cpp:4159`** (`LandingPadOpLowering`, Windows path) — - cleanup-only branch reuses the typed-catch filter value instead of an - empty/undef cleanup clause; the code has its own `// BUG: in LLVM landing - pad is not fully implemented` comment. PLAUSIBLE, not yet fixed — - deliberately left alone in the second pass: this is deep Windows SEH - landing-pad construction with the original author's own admission it's - incomplete, and a wrong guess here risks a worse regression than the - status quo. Needs someone with direct SEH/landingpad-clause expertise, or - a way to single-step actual Windows exception dispatch through it. + RESOLVED as not-a-bug after tracing the full cross-pass flow. The + suspicious clause is not "the typed-catch filter value" (the original + review misread it): for cleanup pads, `transformed.getCatches().front()` + is always the undef-array marker value built in `TryOpLowering` + (`LowerToAffineLoops.cpp` ~1554), never an RTTI pointer. Being + array-typed, it becomes a *filter* clause on the LLVM landingpad — a + deliberate **cleanup marker** consumed by the custom LLVM passes + (`tslang/lib/TypeScriptExceptionPass/`): + + - `LandingPadFixPass` (registered unconditionally FIRST in the LLVM + pipeline, `tslang/tslang/transform.cpp:323`, before any optimization) + rewrites every filter-bearing landingpad to a canonical clause-less + `landingpad { cleanup }`. + - `Win32ExceptionPass` (Windows, second, `transform.cpp:326`) converts + pads to SEH funclets (`catchswitch`/`catchpad`/`cleanuppad`) and erases + every landingpad; its own filter-detection branch in + `CatchRegion::isCleanup()` is a defensive fallback. + - The optimizer/inliner (`buildPerModuleDefaultPipeline`) runs only + after both — the filter never reaches EH-table emission on either + platform. + + Verified empirically via `--emit=llvm` final-IR dumps of + `00try_finally.ts`: Windows triple → zero `landingpad`/`filter` + instructions (24 catchpad / 12 cleanuppad / 13 catchswitch); Linux + triple → zero `filter`, cleanup pads canonical (`landingpad { cleanup }`, + no clauses), catch pads carrying proper `catch ptr @_ZTI...` clauses. + The whole EH test corpus (try/catch/finally/disposable, JIT + AOT) + passes. + + The misleading self-flagged `// BUG` comments at all three sites + (marker construction in `LowerToAffineLoops.cpp`, filter emission in + `windows::LandingPadOpLowering`, filter detection in + `Win32ExceptionPass::isCleanup()`) were rewritten into accurate + contract documentation cross-referencing each other. No functional + change. + +**All 10 findings from this review are now closed** (9 fixed, 1 resolved +as not-a-bug), across PRs #230, #232, #233, #234, and the comment-contract +PR for this pass. ## Other observations (not correctness bugs, lower priority) diff --git a/tslang/lib/TypeScript/LowerToAffineLoops.cpp b/tslang/lib/TypeScript/LowerToAffineLoops.cpp index 733002ecf..e8a7fffa6 100644 --- a/tslang/lib/TypeScript/LowerToAffineLoops.cpp +++ b/tslang/lib/TypeScript/LowerToAffineLoops.cpp @@ -1554,7 +1554,11 @@ struct TryOpLowering : public TsPattern mlir::Value undefArrayValue; if (finallyHasOps || cleanupHasOps) { - // BUG: HACK, i need to add marker type to treat it as cleanup landing pad later + // cleanup marker: an array-typed landing-pad operand becomes a *filter* clause at + // the LLVM level, which (together with the cleanup flag) marks the pad as a cleanup + // pad for the later LLVM passes; LandingPadFixPass strips it into a canonical + // clause-less cleanup landingpad before any optimization runs. See + // windows::LandingPadOpLowering in LowerToLLVM.cpp. auto arrTy = mth.getConstArrayValueType(mth.getOpaqueType(), 1); undefArrayValue = rewriter.create(loc, arrTy); auto nullVal = rewriter.create(loc, mth.getNullType()); diff --git a/tslang/lib/TypeScript/LowerToLLVM.cpp b/tslang/lib/TypeScript/LowerToLLVM.cpp index 4748ea224..4c230a37a 100644 --- a/tslang/lib/TypeScript/LowerToLLVM.cpp +++ b/tslang/lib/TypeScript/LowerToLLVM.cpp @@ -4173,13 +4173,18 @@ struct LandingPadOpLowering : public TsLlvmPattern } else { - // BUG: in LLVM landing pad is not fully implemented - // so lets create filter with undef value to mark cleanup landing - auto catch1Fake = transformed.getCatches().front(); + // cleanup marker contract: the operand here is the undef-array value built by + // TryOpLowering, and being array-typed it becomes a *filter* clause on the LLVM + // landingpad -- a deliberate marker (alongside the cleanup flag) identifying + // cleanup pads across the LLVM-level passes. LandingPadFixPass rewrites any + // filter-bearing landingpad to a canonical clause-less `landingpad { cleanup }` + // and Win32ExceptionPass then replaces pads with SEH funclets -- both run before + // any optimization, so the filter never reaches EH-table emission. + auto cleanupMarker = transformed.getCatches().front(); mlir::Type llvmLandingPadTy = getTypeConverter()->convertType(landingPadOp.getType()); rewriter.replaceOpWithNewOp(landingPadOp, llvmLandingPadTy, true, - ValueRange{catch1Fake}); + ValueRange{cleanupMarker}); } return success(); diff --git a/tslang/lib/TypeScriptExceptionPass/Win32ExceptionPass.cpp b/tslang/lib/TypeScriptExceptionPass/Win32ExceptionPass.cpp index a84ea0640..2a3de7579 100644 --- a/tslang/lib/TypeScriptExceptionPass/Win32ExceptionPass.cpp +++ b/tslang/lib/TypeScriptExceptionPass/Win32ExceptionPass.cpp @@ -48,7 +48,11 @@ struct CatchRegion return true; } - // BUG: HACK. using filter[] as cleanup landingpad + // a filter clause is the compiler's cleanup marker (see windows::LandingPadOpLowering + // in LowerToLLVM.cpp): normally LandingPadFixPass has already canonicalized such pads + // to clause-less cleanup landingpads before this pass runs, so this branch is a + // defensive fallback for pads that still carry the marker (e.g. if pass ordering ever + // changes or the inliner merges landing pads before canonicalization). if ((landingPad->getNumClauses() > 0 && !landingPad->isCatch(0))) { // this is filter[], treat it as cleanup