Document the landing-pad cleanup-marker contract; close out review item 8

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)
 

tslang/lib/TypeScript/LowerToAffineLoops.cpp

@@ -1554,7 +1554,11 @@ struct TryOpLowering : public TsPattern<mlir_ts::TryOp>
         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<mlir_ts::UndefOp>(loc, arrTy);
             auto nullVal = rewriter.create<mlir_ts::NullOp>(loc, mth.getNullType());

tslang/lib/TypeScript/LowerToLLVM.cpp

@@ -4173,13 +4173,18 @@ struct LandingPadOpLowering : public TsLlvmPattern<mlir_ts::LandingPadOp>
         }
         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<LLVM::LandingpadOp>(landingPadOp, llvmLandingPadTy, true,
-                                                            ValueRange{catch1Fake});
+                                                            ValueRange{cleanupMarker});
         }
 
         return success();

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