diff --git a/tslang/include/TypeScript/Defines.h b/tslang/include/TypeScript/Defines.h index 4699a7730..15bb64628 100644 --- a/tslang/include/TypeScript/Defines.h +++ b/tslang/include/TypeScript/Defines.h @@ -61,6 +61,15 @@ // MLIROwnedReference.h. #define OWNED_RESULT_NAMED_ATTR_NAME "__owned_result_named" +// Marks a release MLIRGen put at the end of a hand-written `finally` for the exceptional leg only: +// what the scopes around the `try` owe once the exception carries on past it. The finally region +// is one region in MLIRGen but several copies after TryOpLowering - the copy the landing pad runs +// before resuming, and the copies normal completion, a `return` and a `break` run - and only the +// first leaves the function's scopes behind; on every other copy they are still live, and their +// own exits release them. TryOpLowering keeps these in the unwind copy and erases them from the +// rest. +#define UNWIND_ONLY_ATTR_NAME "__unwind_only" + // Marks a `ts.CreateBoundFunction` whose `this` is a capture box built for it a moment earlier, // rather than a receiver that belongs to somebody else. Only such a closure owns its `this`, and // only it gets the type tag at CLOSURE_TYPE_INDEX - a bound method must not take ownership of diff --git a/tslang/lib/TypeScript/LowerToAffineLoops.cpp b/tslang/lib/TypeScript/LowerToAffineLoops.cpp index b685d2df3..64392924d 100644 --- a/tslang/lib/TypeScript/LowerToAffineLoops.cpp +++ b/tslang/lib/TypeScript/LowerToAffineLoops.cpp @@ -1618,6 +1618,36 @@ struct TryOpLowering : public TsPattern tsContext->finallyBlockOf[tryOp.getOperation()] = finallyBlock; finallyBlockLast = continuation->getPrevNode(); + // That copy is the one the landing pad runs before the exception carries on, and the + // only one whose way out leaves the scopes around the try: the releases MLIRGen put at + // the end of the finally for them stay in it and go from every later copy. Not from a + // try nested in the finally - its own lowering sorts its own. + // + // Erased from each later copy through its mapping, and from the region itself only once + // the last copy is made: the rewriter defers an erasure, so an op erased from the + // region still goes into every copy cloned from it afterwards. + SmallVector unwindOnlyOps; + tryOp.getFinally().walk([&](Operation *op) { + if (isa(op)) + { + return WalkResult::skip(); + } + + if (op->hasAttr(UNWIND_ONLY_ATTR_NAME)) + { + unwindOnlyOps.push_back(op); + } + + return WalkResult::advance(); + }); + + auto eraseUnwindOnlyOps = [&](const mlir::IRMapping &mapping) { + for (auto *op : unwindOnlyOps) + { + rewriter.eraseOp(mapping.lookup(op)); + } + }; + // add clone for 'return' if (returns.size() > 0) { @@ -1625,6 +1655,7 @@ struct TryOpLowering : public TsPattern rewriter.cloneRegionBefore(tryOp.getFinally(), *continuation->getParent(), continuation->getIterator(), returnFinallyMapping); propagateTsContextEntries(returnFinallyMapping); + eraseUnwindOnlyOps(returnFinallyMapping); auto returnFinallyBlockLast = continuation->getPrevNode(); rewriter.setInsertionPoint(returnFinallyBlockLast->getTerminator()); auto resultOpOfReturnFinallyBlock = cast(returnFinallyBlockLast->getTerminator()); @@ -1655,6 +1686,7 @@ struct TryOpLowering : public TsPattern rewriter.cloneRegionBefore(tryOp.getFinally(), *continuation->getParent(), continuation->getIterator(), jumpFinallyMapping); propagateTsContextEntries(jumpFinallyMapping); + eraseUnwindOnlyOps(jumpFinallyMapping); auto jumpFinallyBlockLast = continuation->getPrevNode(); rewriter.setInsertionPoint(jumpFinallyBlockLast->getTerminator()); auto resultOpOfJumpFinallyBlock = cast(jumpFinallyBlockLast->getTerminator()); @@ -1679,6 +1711,11 @@ struct TryOpLowering : public TsPattern } } + for (auto *op : unwindOnlyOps) + { + rewriter.eraseOp(op); + } + rewriter.inlineRegionBefore(tryOp.getFinally(), continuation); exitBlockLast = continuation->getPrevNode(); } diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index 14e6d6033..f8e0cb0ca 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -502,6 +502,29 @@ class MLIRGenImpl return false; } + // Whether this block declares a `let` or `const` of its own - a local that may own a + // reference, and that the TryOp around it would not release if an exception left the block + // (see mlirGen(Block)). Over-approximates: the type that decides it is not known yet. + bool blockDeclaresLocals(ts::Block blockAST, int skipStatements = 0) + { + auto index = 0; + for (auto statement : blockAST->statements) + { + if (index++ < skipStatements || (SyntaxKind)statement != SyntaxKind::VariableStatement) + { + continue; + } + + auto flags = statement.as()->declarationList->flags; + if ((flags & NodeFlags::Let) == NodeFlags::Let || (flags & NodeFlags::Const) == NodeFlags::Const) + { + return true; + } + } + + return false; + } + // Whether every top-level `using` this block declares initializes directly from // `new SomeClass(...)`. // @@ -836,7 +859,8 @@ class MLIRGenImpl return mlir::success(); } - void releaseScopeOwnedVars(mlir::Location location, const SmallVector &ownedVars) + void releaseScopeOwnedVars(mlir::Location location, const SmallVector &ownedVars, + bool unwindOnly = false) { // reverse declaration order, the order a scope is unwound in: a later local may // hold the only other reference to what an earlier one points at @@ -846,22 +870,33 @@ class MLIRGenImpl // released as a cell whatever its contents are owned by - the block has to go // back regardless. A slot that is neither is here only because it might have // turned into a cell and did not; see trackPossibleCell. + mlir::Operation *releaseOp = nullptr; if (isCapturedVariableCell(storage)) { - builder.create(location, storage); + releaseOp = builder.create(location, storage); } else if (isOwnedLocalSlot(storage)) { - builder.create(location, storage); + releaseOp = builder.create(location, storage); + } + + if (releaseOp && unwindOnly) + { + releaseOp->setAttr(UNWIND_ONLY_ATTR_NAME, builder.getUnitAttr()); } } } // The rest of the unwind leg of a catch-less TryOp's cleanup region - the one wrapped around - // a block or a switch for its `using`. Once the region has given back its own scope, the - // exception keeps unwinding, and the scopes around it that have no cleanup region of their - // own get no other chance: a function's body holding `let s = a + b` and, further in, - // `{ using r = new R(); ... }` leaked `s` on every exception through the inner block. + // a block or a switch for its `using`, or around a block with locals inside a try body. Once + // the region has given back its own scope, the exception keeps unwinding, and the scopes + // around it that have no cleanup region of their own get no other chance: a function's body + // holding `let s = a + b` and, further in, `{ using r = new R(); ... }` leaked `s` on every + // exception through the inner block. + // + // The same walk serves the two other places an exception leaves a scope: a `throw` statement, + // which raises it there, and the copy of a hand-written `finally` that runs on the unwind leg + // and then resumes it (`unwindOnly`, see UNWIND_ONLY_ATTR_NAME). // // Walks outwards from the scope around the wrapped one, releasing as it goes, and stops at // the first scope that is itself a TryOp's body (isUnwindCleanupScope) without touching its @@ -876,17 +911,32 @@ class MLIRGenImpl // locals declared ahead of the wrapped scope are in the lists yet, so every slot released // here has had its store - or, hoisted in front of a switch whose case did not run, is // null, which a release does nothing with (VariableOpLowering). - void mlirGenReleaseOwnedOnUnwind(mlir::Location location, const GenContext *genContext) + void mlirGenReleaseOwnedOnUnwind(mlir::Location location, const GenContext *genContext, bool unwindOnly = false) { for (; genContext != nullptr && !genContext->isUnwindCleanupScope; genContext = genContext->parentBlockContext) { if (genContext->ownedVars != nullptr) { - releaseScopeOwnedVars(location, *genContext->ownedVars); + releaseScopeOwnedVars(location, *genContext->ownedVars, unwindOnly); } } } + // Whether an exception out of a scope opened in this context lands on a TryOp's landing pad + // in this function: some scope between here and the function is a TryOp's body. + static bool isInsideUnwindCleanupScope(const GenContext &genContext) + { + for (auto *context = &genContext; context != nullptr; context = context->parentBlockContext) + { + if (context->isUnwindCleanupScope) + { + return true; + } + } + + return false; + } + // Does this reference address a captured variable's cell - a heap block the frame shares // with the closures that captured it - rather than ordinary frame storage? // diff --git a/tslang/lib/TypeScript/MLIRGenStatements.cpp b/tslang/lib/TypeScript/MLIRGenStatements.cpp index 317b90015..09263f89d 100644 --- a/tslang/lib/TypeScript/MLIRGenStatements.cpp +++ b/tslang/lib/TypeScript/MLIRGenStatements.cpp @@ -157,6 +157,12 @@ namespace mlirgen // that is an exit the scope's owned locals have to be released before - see // blockHasTypedCatch. Same wrapping, same cleanup region, different reason to want it. // + // So are a block's own locals, once the block sits inside a TryOp's body: the TryOp's + // cleanup releases its body's scope and no other, so an exception out of a nested block + // - `try { { let s = a + b; h(); } } catch {}` - left `s` behind. The wrap's cleanup + // gives back the block's scope and the scopes between it and that body + // (mlirGenReleaseOwnedOnUnwind). Outside any TryOp nothing would run it. + // // The two remaining conditions each guard against a real, still-open bug the wrapping // would otherwise hit (see their comments). blockUsingInitializersAreAllNewExpr is // about disposing a `using`, so it only has a say when there is one - but it keeps its @@ -167,7 +173,8 @@ namespace mlirgen auto declaresUsing = blockDeclaresUsing(blockAST, skipStatements); auto usingIsDisposable = !declaresUsing || blockUsingInitializersAreAllNewExpr(blockAST, skipStatements); auto rethrowNeedsCleanup = !compileOptions.isWindows && blockHasTypedCatch(blockAST, skipStatements); - if (usingIsDisposable && (declaresUsing || rethrowNeedsCleanup) && !blockIsInsideCatchOrFinally()) + auto localsNeedCleanup = isInsideUnwindCleanupScope(genContext) && blockDeclaresLocals(blockAST, skipStatements); + if (usingIsDisposable && (declaresUsing || rethrowNeedsCleanup || localsNeedCleanup) && !blockIsInsideCatchOrFinally()) { return mlirGenBlockWithUnwindCleanup(blockAST, genContext, skipStatements); } @@ -1395,6 +1402,16 @@ namespace mlirgen EXIT_IF_FAILED_OR_NO_VALUE(result) auto exception = V(result); + // The exception leaves every scope from here out to the nearest TryOp body, and those + // scopes have no landing pad of their own to release their locals from - the function's + // body, a `catch` clause's enclosing scopes. So the throw gives them back itself, as a + // `return` would, and first takes a reference for the exception, as a `return` does for + // its caller: `throw s` must not be freed by the release of `s`. Nothing gives that + // reference back - a caught value is not owned (takeOwnershipOfLocal) - so a thrown heap + // value leaks rather than dangles. + mlirGenRetainCaptured(location, mlir::ValueRange{exception}, /*atLastUse=*/true); + mlirGenReleaseOwnedOnUnwind(location, &genContext); + auto throwOp = builder.create(location, exception); if (!genContext.allowPartialResolve) @@ -1527,7 +1544,16 @@ namespace mlirgen } } - auto result = mlirGen(tryStatementAST->catchClause->block, tryGenContext); + // With a finally, an exception out of the catch clause unwinds into the finally's + // unwind copy, which releases the scopes around the `try` (below) - so a `throw` in + // the clause must stop short of them, as it does at a TryOp's body + GenContext catchGenContext(tryGenContext); + if (tryStatementAST->finallyBlock) + { + catchGenContext.isUnwindCleanupScope = true; + } + + auto result = mlirGen(tryStatementAST->catchClause->block, catchGenContext); EXIT_IF_FAILED(result) } @@ -1540,6 +1566,14 @@ namespace mlirgen { auto result = mlirGen(tryStatementAST->finallyBlock, tryGenContext); EXIT_IF_FAILED(result) + + // The copy of the finally that runs on the unwind leg resumes the exception past the + // `try`, so it is the last chance the scopes around it get, up to the nearest TryOp + // body, as in a catch-less TryOp's cleanup. It cannot be the cleanup region itself: + // that runs before the finally, which may still read them. Marked unwind-only, so + // TryOpLowering drops them from the copies normal completion, `return` and `break` + // run, where those scopes are still live. + mlirGenReleaseOwnedOnUnwind(location, &tryGenContext, /*unwindOnly=*/true); } // terminator diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 1a9513139..2fa85d6f4 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -337,6 +337,7 @@ tslang_add_test(NAME test-compile-00-labelled-scope-exit COMMAND test-runner "${ tslang_add_test(NAME test-compile-00-switch-case-scope-exit COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00switch_case_scope_exit.ts") tslang_add_test(NAME test-compile-00-using-dispose-order COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00using_dispose_order.ts") tslang_add_test(NAME test-compile-00-using-unwind-outer-locals COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00using_unwind_outer_locals.ts") +tslang_add_test(NAME test-compile-00-try-unwind-outer-locals COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_unwind_outer_locals.ts") tslang_add_test(NAME test-compile-00-vars COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00vars.ts") tslang_add_test(NAME test-compile-00-var-bindings COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00var_bindings.ts") tslang_add_test(NAME test-compile-00-globals COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00globals.ts") @@ -880,6 +881,7 @@ tslang_add_test(NAME test-jit-00-labelled-scope-exit COMMAND test-runner -jit "$ tslang_add_test(NAME test-jit-00-switch-case-scope-exit COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00switch_case_scope_exit.ts") tslang_add_test(NAME test-jit-00-using-dispose-order COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00using_dispose_order.ts") tslang_add_test(NAME test-jit-00-using-unwind-outer-locals COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00using_unwind_outer_locals.ts") +tslang_add_test(NAME test-jit-00-try-unwind-outer-locals COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00try_unwind_outer_locals.ts") tslang_add_test(NAME test-jit-00-vars COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00vars.ts") tslang_add_test(NAME test-jit-00-var-bindings COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00var_bindings.ts") tslang_add_test(NAME test-jit-00-globals COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00globals.ts") @@ -1834,6 +1836,7 @@ set(TSLANG_CORPUS 00switch_case_scope_exit.ts 00using_dispose_order.ts 00using_unwind_outer_locals.ts + 00try_unwind_outer_locals.ts 00capture_in_new_arguments.ts 00cast_to_f32.ts 00catch_untyped_value.ts diff --git a/tslang/test/tester/tests/00try_unwind_outer_locals.ts b/tslang/test/tester/tests/00try_unwind_outer_locals.ts new file mode 100644 index 000000000..cd706da00 --- /dev/null +++ b/tslang/test/tester/tests/00try_unwind_outer_locals.ts @@ -0,0 +1,314 @@ +// An exception that leaves a scope gives back what that scope and the scopes around it took, on +// the paths 00using_unwind_outer_locals.ts does not cover. +// +// - out of a `try` with a `finally`: the copy of the finally that runs on the unwind leg releases +// the scopes around the `try` once it is done, and only that copy - the finally still reads them, +// and normal completion, a `return` and a `break` through it leave them live; +// - out of a block nested in a `try` body: such a block is wrapped in a catch-less TryOp of its own, +// since the `try`'s cleanup releases its body's scope and no other; +// - from a `throw` statement, in a `catch` clause or anywhere else: the throw releases the scopes it +// leaves, up to the nearest `try` body, after taking a reference for the thrown value. +// +// Under -mm=rc each of these leaked; the ownership verifier (--verify-ownership, swept over this +// corpus in ctest) reported them, and is quiet on this file now. The values are read again after +// churn() wherever a release one too many would free them early. +// +// Not covered, and still leaking: a call that throws out of a `catch` clause of a `try` with no +// `finally`, a typed catch that does not match in a `try` with no `finally`, and a call that throws +// where no `try` encloses it - none of those has a landing pad to release from. + +let trail = ""; + +function h() { + throw 1; +} + +// allocates over whatever was freed last, so that a string released one time too many no longer +// reads as itself +function churn() { + let junk = ""; + for (let i = 0; i < 64; i++) { + junk = junk + "#"; + } + + return junk.length; +} + +function expectThrow(f: () => void) { + try { + f(); + } + catch (e: TypeOf<1>) { + return true; + } + + return false; +} + +// the finally reads `s` after the exception: the release has to come after it +function throwThroughFinally(pre: string) { + let s = pre + "1"; + try { + h(); + } + finally { + churn(); + trail += s + ";"; + } + + return s; +} + +function throwThroughEmptyFinally(pre: string) { + let s = pre + "2"; + try { + h(); + } + finally { + } + + return s; +} + +// the same finally, completed normally: `s` is still the function's +function completeThroughFinally(pre: string) { + let s = pre + "3"; + try { + trail += "body;"; + } + finally { + trail += s + ";"; + } + + churn(); + return s; +} + +// no unwind-only release in the copy a `return` runs. The finally does not read `s`: the return +// releases the frame before its finally runs, a separate bug. +function returnThroughFinally(pre: string) { + let s = pre + "4"; + try { + return s + "r"; + } + finally { + churn(); + trail += "f;"; + } +} + +function breakThroughFinally(pre: string) { + let s = pre + "5"; + for (let i = 0; i < 3; i++) { + try { + if (i == 1) { + break; + } + } + finally { + trail += s + i + ";"; + } + } + + churn(); + return s; +} + +function throwFromCatch(pre: string) { + let s = pre + "6"; + try { + h(); + } + catch { + trail += s + ";"; + throw 2; + } + + return s; +} + +function throwFromCatchWithFinally(pre: string) { + let s = pre + "7"; + try { + h(); + } + catch { + throw 2; + } + finally { + churn(); + trail += s + ";"; + } + + return s; +} + +// a try in a try body: the outer one's cleanup releases its own body, so the inner finally leaves it +function finallyInTryBody(pre: string) { + let s = pre + "8"; + try { + let t = s + "t"; + try { + h(); + } + finally { + churn(); + trail += t + ";"; + } + } + catch { + trail += "caught;"; + } + + churn(); + return s; +} + +function throwThroughNestedScope(pre: string) { + let s = pre + "9"; + try { + { + let t = s + "t"; + { + let u = t + "u"; + trail += u + ";"; + h(); + } + } + } + catch { + trail += "caught;"; + } + + churn(); + return s; +} + +// the nested scope completes and releases before the exception: nothing may release it again +function throwAfterNestedScope(pre: string) { + let s = pre + "a"; + try { + { + let t = s + "t"; + trail += t + ";"; + } + churn(); + h(); + } + catch { + trail += "caught;"; + } + + churn(); + return s; +} + +function throwFromNestedLoopBody(pre: string) { + let s = pre + "b"; + try { + for (let i = 0; i < 3; i++) { + let t = s + i; + churn(); + trail += t + ";"; + if (i == 1) { + h(); + } + } + } + catch { + trail += "caught;"; + } + + churn(); + return s; +} + +function throwFromFunctionBody(pre: string) { + let s = pre + "c"; + { + let t = s + "t"; + trail += t + ";"; + if (t.length > 0) { + throw 1; + } + } + + return s; +} + +// the throw releases `s`, and must not take the thrown value with it +function throwLocal(pre: string) { + let s = pre + "d"; + trail += s + ";"; + throw s; +} + +function check(f: () => void, expected: string, what: string) { + trail = ""; + f(); + churn(); + print(what + ": " + trail); + assert(trail == expected, what); +} + +function main() { + let pre = "p"; + + check(() => assert(expectThrow(() => { throwThroughFinally(pre); }), "must throw"), "p1;", "throw through a finally"); + check(() => assert(expectThrow(() => { throwThroughEmptyFinally(pre); }), "must throw"), "", "throw through an empty finally"); + + trail = ""; + let c = completeThroughFinally(pre); + print("complete through a finally: " + c + " " + trail); + assert(c == "p3" && trail == "body;p3;", "complete through a finally"); + + trail = ""; + let r = returnThroughFinally(pre); + churn(); + print("return through a finally: " + r + " " + trail); + assert(r == "p4r" && trail == "f;", "return through a finally"); + + trail = ""; + let b = breakThroughFinally(pre); + print("break through a finally: " + b + " " + trail); + assert(b == "p5" && trail == "p50;p51;", "break through a finally"); + + check(() => assert(expectThrow(() => { throwFromCatch(pre); }), "must throw"), "p6;", "throw from a catch"); + check(() => assert(expectThrow(() => { throwFromCatchWithFinally(pre); }), "must throw"), "p7;", "throw from a catch with a finally"); + + trail = ""; + let f = finallyInTryBody(pre); + print("finally in a try body: " + f + " " + trail); + assert(f == "p8" && trail == "p8t;caught;", "finally in a try body"); + + trail = ""; + let n = throwThroughNestedScope(pre); + print("throw through a nested scope: " + n + " " + trail); + assert(n == "p9" && trail == "p9tu;caught;", "throw through a nested scope"); + + trail = ""; + let a = throwAfterNestedScope(pre); + print("throw after a nested scope: " + a + " " + trail); + assert(a == "pa" && trail == "pat;caught;", "throw after a nested scope"); + + trail = ""; + let l = throwFromNestedLoopBody(pre); + print("throw from a nested loop body: " + l + " " + trail); + assert(l == "pb" && trail == "pb0;pb1;caught;", "throw from a nested loop body"); + + check(() => assert(expectThrow(() => { throwFromFunctionBody(pre); }), "must throw"), "pct;", "throw from a function body"); + + trail = ""; + let thrown = false; + try { + throwLocal(pre); + } + catch { + thrown = true; + } + churn(); + print("throw a local: " + trail); + assert(thrown && trail == "pd;", "throw a local"); + + print("done."); +}