diff --git a/tslang/docs/reference-counting-evaluation.md b/tslang/docs/reference-counting-evaluation.md index 7ca8b6922..c9351df21 100644 --- a/tslang/docs/reference-counting-evaluation.md +++ b/tslang/docs/reference-counting-evaluation.md @@ -5903,3 +5903,58 @@ its characters; a C-owned `char *` has none. Under `-mm=gc` that is harmless - n header. Under `-mm=rc` a release of such a value reads a count that is not there, which is undefined. Not fixed: tsbindgen v1 targets `gc`. Revisit if rc becomes a supported model for bindings - the likely shape is a distinct C-string type in the compiler that is never released. + +### 9.79 A folded `const` was taken over by every receiver that met it + +Found by the review of `-mm=own` phase 0, which crashed on these programs and so asked why `rc` +did not. `rc` did: the same programs double-free or read freed memory under `rc` today, and +only luck in what the allocator reused next had hidden it. `00owned_named_consts.ts` collects +eight of them. Under `rc` each failed on its own - four read a block `churn()` had since +claimed (999, 1006 where 5 and 7 belong, a string compare gone false), two corrupted the heap +(JIT exit 127, AOT `0xC0000374`). + +**The cause.** `const a = new C()` gets no storage: the name is folded into the `new`'s own +result. Every later mention of `a` is that one SSA value, marked `__owned_result`, and every +receiver of §9.25 asked only that question. So each one took the reference over: + +- `{ let b = a; } ... a.v` - the inner `let` took it and gave it back at its scope exit, and `a` + read freed memory. +- `let b = a; let c = a;`, and `arr.push(a); let d = a;` - two holders, one count. +- `for (...) { arr.push(a) }`, `for (...) { h.s = s }` - one reference taken on every iteration; + the field store's overwrite released the very string it was storing. +- `const r = make(); { let b = r; }` - the same through §9.27's pass, which took the first + receiver's retain for the call's +1. + +And one without a name: `let b = h.c = new C()` offers one `new` to the field and then the local. +Only the array push (§9.74) asked whether the reference had been taken already; no other +receiver did. + +**The fix.** One question for every receiver, `mayTakeOverReference` in `MLIROwnedReference.h`: +the producer carries a reference, no receiver took it yet, and it is not a folded `const`. +MLIRGen marks a folded `const`'s producer `__owned_result_named` where the name is bound. A named +value is retained by each receiver instead, and its own reference is given back at the end of +its block like any unclaimed temporary (§9.30). A `return` and a `delete` still take it over - +nothing reads the name after either - through `mayTakeOverReferenceAtLastUse`, on every path: +`if (x) return c; return c;` has two, and whichever runs is the last use. (The first version of +this fix let only the first `return` take it over and made the second retain, which leaked on +that path: 1M calls went from 4.7 MB to 20.1 MB.) §9.27's pass likewise keeps a named result's +retains except those a `return` took, and now takes every one of those rather than the first. + +Cost: a retain and a release for `const a = new C(); arr.push(a)`, which used to be free. + +Under `-mm=own` the extra pair is a move, not a second owner: one taker plus the temporary's +release later in the same block is one owner, and the inference pass erases both. A million +`const p = make(); let q = p;` iterations peak at 4.2 MB under both `rc` and `own` (`none` 50.3). + +**And a leak the fix exposed.** §9.30 gave a temporary back only if every use was in its own +block, and the body of an `if` or a loop is a region of an op in that block, not the block. Once +the `let` in `const a = new C(); if (x) { let b = a; }` retained instead of taking over, nothing +gave `a`'s own reference back: 1M calls of such a function reached 89.2 MB at `-O1` (`none` 89.2, +and `-O3` hid it by removing the allocation). A use nested in an `if`, loop, `switch`, label or +`try` body now counts as in the block (`runsWithinBlock`), since those bodies finish before the +next op runs; a nested function or an `async.execute` body still does not. After: 4.7 MB. It +also releases a `const` that is only *read* in a branch, which leaked before this arc. + +**Open, and older than this:** `const c = new C(); if (x) return c; print(c.x);` leaks `c` on the +path that does not return. The `return` marks the producer consumed, which is what stops §9.30 +releasing it, and that mark is per operation, not per path. diff --git a/tslang/docs/superpowers/specs/2026-09-24-own-memory-model-design.md b/tslang/docs/superpowers/specs/2026-09-24-own-memory-model-design.md index e3038b216..3f836d3eb 100644 --- a/tslang/docs/superpowers/specs/2026-09-24-own-memory-model-design.md +++ b/tslang/docs/superpowers/specs/2026-09-24-own-memory-model-design.md @@ -410,6 +410,14 @@ The rule as built: non-owning declaration, `--di`'s `ts.DebugVariable`, and the retain/release bookkeeping. - **Owners** of V are its taking uses, plus one if V is released as a temporary. There must be at most one owner. Zero is a leak, as under rc. +- **A move out of a temporary.** One taking use and one temporary release count as one owner + when the release comes after the taker in the same block and rc gave the taker a reference of + its own (a `ts.Retain` of V in front of it, or the `ts.RetainSlot` of an owning `let`). The + value moves into the taker, and the release is erased with the retains. This is the shape of + a folded `const` handed on, `const a = new C(); let b = a;`, since rc stopped letting a + receiver take a named `const`'s reference over (that let one reference have several holders + under rc: `00owned_named_consts.ts`). A + taker rc gives no reference, such as `ts.NewInterface`, is still a second owner. - **Locality**: the taking use is in V's defining block, and nothing uses V after it. That one check rejects a move inside a loop (§2.6), a move on one branch, and use after move, with no liveness analysis. Phase 1 relaxes it. @@ -477,3 +485,5 @@ Tested both ways: value the callee does not own; spec §2.4's owned-by-callee parameter is phase 4), and a move out of the value's own block are all rejected. - Results of runtime helpers and `declare`d functions are rejected. +- A field initialized from an array literal of eight numbers is rejected ("takes a second + reference" at the literal); four are accepted. Not yet diagnosed. diff --git a/tslang/include/TypeScript/Defines.h b/tslang/include/TypeScript/Defines.h index 3d4047278..137578aa7 100644 --- a/tslang/include/TypeScript/Defines.h +++ b/tslang/include/TypeScript/Defines.h @@ -47,6 +47,15 @@ // and then dropped, which is what expression-shaped code is made of. See §9.30. #define OWNED_RESULT_CONSUMED_ATTR_NAME "__owned_result_consumed" +// Marks the producer of a `const` that was folded into its value rather than given storage - +// `const a = new C()`. The name and the value are one SSA value, so every later mention of `a` +// is the operation's own result, and a receiver cannot tell whether this mention is the last. +// None that stores it may take the reference over; each retains, and the reference the producer +// carried is given back where §9.30 gives back any other unclaimed one. A `return` or a `delete` +// still takes it over, on every path it appears on: nothing after either reads the name. See +// MLIROwnedReference.h. +#define OWNED_RESULT_NAMED_ATTR_NAME "__owned_result_named" + // 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/include/TypeScript/MLIRLogic/MLIRCodeLogic.h b/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h index 4e68b5d88..da48336c3 100644 --- a/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h +++ b/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h @@ -7,6 +7,7 @@ #include "TypeScript/Defines.h" #include "TypeScript/MLIRLogic/MLIRDefines.h" #include "TypeScript/MLIRLogic/MLIRGenContext.h" +#include "TypeScript/MLIRLogic/MLIROwnedReference.h" #include "TypeScript/MLIRLogic/MLIRTypeHelper.h" #include "llvm/ADT/APSInt.h" @@ -761,11 +762,13 @@ class MLIRCustomMethods // first freed an element the other still held. Nothing to do with `splice` - it // reproduces with no splice anywhere - but §9.74's release is what made it visible, // because until then an array that outlived its elements never gave them back. - auto *definingOp = value.getDefiningOp(); - if (definingOp && definingOp->hasAttr(OWNED_RESULT_ATTR_NAME) && - !definingOp->hasAttr(OWNED_RESULT_CONSUMED_ATTR_NAME)) + // + // And the same one array receiving it again in a loop, from a `const` made once + // before the loop, is the same mistake repeated: mayTakeOverReference turns away a + // folded `const` for that reason (see OWNED_RESULT_NAMED_ATTR_NAME). + if (mayTakeOverReference(value)) { - definingOp->setAttr(OWNED_RESULT_CONSUMED_ATTR_NAME, builder.getUnitAttr()); + value.getDefiningOp()->setAttr(OWNED_RESULT_CONSUMED_ATTR_NAME, builder.getUnitAttr()); continue; } diff --git a/tslang/include/TypeScript/MLIRLogic/MLIROwnedReference.h b/tslang/include/TypeScript/MLIRLogic/MLIROwnedReference.h new file mode 100644 index 000000000..d07e61f0c --- /dev/null +++ b/tslang/include/TypeScript/MLIRLogic/MLIROwnedReference.h @@ -0,0 +1,58 @@ +#ifndef MLIR_TYPESCRIPT_COMMONGENLOGIC_MLIROWNEDREFERENCE_H_ +#define MLIR_TYPESCRIPT_COMMONGENLOGIC_MLIROWNEDREFERENCE_H_ + +#include "TypeScript/Defines.h" + +#include "mlir/IR/Operation.h" +#include "mlir/IR/Value.h" + +namespace typescript +{ + +// Does this value still carry a reference nobody has taken over? +// +// The producer has to carry one at all (OWNED_RESULT_ATTR_NAME), and no receiver can have taken +// it already (OWNED_RESULT_CONSUMED_ATTR_NAME). There is only ever one to take, and one value can +// reach two receivers: `let b = h.c = new C()` offers the same `new` to the field and then to the +// local. +// +// A receiver that stores the value asks mayTakeOverReference, and one after which nothing reads +// it - a `return`, a `delete` - asks mayTakeOverReferenceAtLastUse. +inline bool carriesUnclaimedReference(mlir::Value value) +{ + auto *definingOp = value ? value.getDefiningOp() : nullptr; + return definingOp && definingOp->hasAttr(OWNED_RESULT_ATTR_NAME) && + !definingOp->hasAttr(OWNED_RESULT_CONSUMED_ATTR_NAME); +} + +// May a receiver that stores this value - a local, a field or element, an array, a literal - take +// its reference over instead of retaining? +// +// Not when the value is a folded `const` (OWNED_RESULT_NAMED_ATTR_NAME). Every mention of the +// name is this very value, so the receiver cannot tell the last mention from the first: one in an +// inner scope releases the reference while the name still reads it, and one in a loop takes the +// same reference on every iteration. +// +// Answering no costs a retain and a release. Answering yes wrongly frees live memory. +inline bool mayTakeOverReference(mlir::Value value) +{ + return carriesUnclaimedReference(value) && !value.getDefiningOp()->hasAttr(OWNED_RESULT_NAMED_ATTR_NAME); +} + +// May a `return` or a `delete` take the reference over? Nothing reads the value after either. +// +// A folded `const` may be taken over even when already marked consumed: no storing receiver +// takes a named value, so what consumed it can only be another `return` or `delete`, on a path +// that excludes this one - `if (x) return c; return c;`. Retaining here instead would hand the +// caller two references on the second path. +inline bool mayTakeOverReferenceAtLastUse(mlir::Value value) +{ + auto *definingOp = value ? value.getDefiningOp() : nullptr; + return definingOp && definingOp->hasAttr(OWNED_RESULT_ATTR_NAME) && + (definingOp->hasAttr(OWNED_RESULT_NAMED_ATTR_NAME) || + !definingOp->hasAttr(OWNED_RESULT_CONSUMED_ATTR_NAME)); +} + +} // namespace typescript + +#endif // MLIR_TYPESCRIPT_COMMONGENLOGIC_MLIROWNEDREFERENCE_H_ diff --git a/tslang/lib/TypeScript/MLIRGenExpressions.cpp b/tslang/lib/TypeScript/MLIRGenExpressions.cpp index b96cfd679..42bc2c6a8 100644 --- a/tslang/lib/TypeScript/MLIRGenExpressions.cpp +++ b/tslang/lib/TypeScript/MLIRGenExpressions.cpp @@ -1196,7 +1196,7 @@ namespace mlirgen // `delete new C()` and `delete c`, where `c` is a `const` the compiler kept as a // value rather than storage: the reference is one nobody has claimed, and §9.30 // releases those at the end of the block. This is the claim. - if (producesOwnedReference(expr)) + if (producesOwnedReferenceAtLastUse(expr)) { consumeOwnedReference(expr); } diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index 0b50f9439..30124a2d1 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -986,15 +986,20 @@ class MLIRGenImpl // runtime helper, or a function from a module built before returns retained their result, // hands back a heap value with no retain behind it, and consuming one of those would skip a // retain nobody performed. Answering "no" for something that was in fact owned only leaks. + // + // Asked by a receiver that stores the value, so a reference some other receiver already + // took, or one a folded `const` still needs for its later mentions, is not there to take - + // see mayTakeOverReference. bool producesOwnedReference(mlir::Value value) { - if (!value) - { - return false; - } + return mayTakeOverReference(value); + } - auto *definingOp = value.getDefiningOp(); - return definingOp && definingOp->hasAttr(OWNED_RESULT_ATTR_NAME); + // The same question from a receiver after which nothing reads the value - a `return`, a + // `delete` - which may take over a folded `const`'s reference too, on every path. + bool producesOwnedReferenceAtLastUse(mlir::Value value) + { + return mayTakeOverReferenceAtLastUse(value); } // Records that a receiver has taken over the reference this value carried, so nothing later @@ -1023,7 +1028,10 @@ class MLIRGenImpl // // A record-shaped value retains through its own routine, which walks its owning fields, so // the boxed-literal case needs one of these on the whole tuple rather than one per field. - void mlirGenRetainCaptured(mlir::Location location, mlir::ValueRange values) + // + // `atLastUse` is for a `return`: nothing after it reads the value, so a folded `const`'s + // reference can go out with it rather than being retained here and given back as a temporary. + void mlirGenRetainCaptured(mlir::Location location, mlir::ValueRange values, bool atLastUse = false) { for (auto value : values) { @@ -1034,7 +1042,7 @@ class MLIRGenImpl // a value that already carries a reference for its receiver is taken over rather // than retained again (§9.25) - `[new C()]`, and `return new C()` alike - if (producesOwnedReference(value)) + if (atLastUse ? producesOwnedReferenceAtLastUse(value) : producesOwnedReference(value)) { consumeOwnedReference(value); } diff --git a/tslang/lib/TypeScript/MLIRGenStatements.cpp b/tslang/lib/TypeScript/MLIRGenStatements.cpp index cb181ef26..105b419ee 100644 --- a/tslang/lib/TypeScript/MLIRGenStatements.cpp +++ b/tslang/lib/TypeScript/MLIRGenStatements.cpp @@ -50,7 +50,7 @@ namespace mlirgen // Cast first, for the reason spelled out at the return statement below. EXIT_IF_FAILED(castToDeclaredReturnType(loc(body), resultValue, genContext)) - mlirGenRetainCaptured(loc(body), mlir::ValueRange{resultValue}); + mlirGenRetainCaptured(loc(body), mlir::ValueRange{resultValue}, /*atLastUse=*/true); return mlirGenReturnValue(loc(body), resultValue, false, genContext); } @@ -479,7 +479,7 @@ namespace mlirgen // interface builds a heap block the tuple's own reference says nothing about. EXIT_IF_FAILED(castToDeclaredReturnType(location, expressionValue, genContext)) - mlirGenRetainCaptured(location, mlir::ValueRange{expressionValue}); + mlirGenRetainCaptured(location, mlir::ValueRange{expressionValue}, /*atLastUse=*/true); EXIT_IF_FAILED(mlirGenScopeExit(location, DisposeDepth::FullStack, {}, &genContext)); diff --git a/tslang/lib/TypeScript/MLIRGenVariables.cpp b/tslang/lib/TypeScript/MLIRGenVariables.cpp index 8a780bcf0..ac71bc435 100644 --- a/tslang/lib/TypeScript/MLIRGenVariables.cpp +++ b/tslang/lib/TypeScript/MLIRGenVariables.cpp @@ -211,6 +211,19 @@ namespace mlirgen if (variableDeclarationInfo.needsIdentityStorage && mlir::failed(createLocalVariable(location, variableDeclarationInfo, genContext))) return mlir::Type(); + + // Folded: from here the name *is* this value, so a receiver meeting it later + // cannot know whether it is the last mention - see OWNED_RESULT_NAMED_ATTR_NAME. + // A ConstRef's initial is a reference to storage and owns nothing of its own. + if (!variableDeclarationInfo.needsIdentityStorage && compileOptions.tracksOwnership() + && variableDeclarationInfo.initial + && mth.ownsHeapMemory(location, variableDeclarationInfo.initial.getType())) + { + if (auto *definingOp = variableDeclarationInfo.initial.getDefiningOp()) + { + definingOp->setAttr(OWNED_RESULT_NAMED_ATTR_NAME, builder.getUnitAttr()); + } + } } else if (mlir::failed(createLocalVariable(location, variableDeclarationInfo, genContext))) return mlir::Type(); } diff --git a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp index ed5760913..db9c161cd 100644 --- a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp +++ b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp @@ -5,6 +5,7 @@ #include "TypeScript/Passes.h" #include "TypeScript/Defines.h" #include "TypeScript/MLIRLogic/MLIRTypeHelper.h" +#include "TypeScript/MLIRLogic/MLIROwnedReference.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/DenseSet.h" @@ -168,11 +169,12 @@ class OwnedReturnConsumptionPass return; } - if (auto *retain = findReceiverRetain(result)) + auto retains = findReceiverRetains(result); + if (!retains.empty()) { callOp->setAttr(OWNED_RESULT_ATTR_NAME, mlir::UnitAttr::get(&getContext())); callOp->setAttr(OWNED_RESULT_CONSUMED_ATTR_NAME, mlir::UnitAttr::get(&getContext())); - toErase.push_back(retain); + toErase.append(retains.begin(), retains.end()); return; } @@ -263,8 +265,7 @@ class OwnedReturnConsumptionPass // branch borrows, the receiver's retain is already the right and only answer. auto carriesOwned = [&](mlir_ts::ResultOp resultOp) { auto *definingOp = resultOp->getOperand(0).getDefiningOp(); - return definingOp && definingOp->hasAttr(OWNED_RESULT_ATTR_NAME) && - !definingOp->hasAttr(OWNED_RESULT_CONSUMED_ATTR_NAME) && + return ::typescript::mayTakeOverReference(resultOp->getOperand(0)) && definingOp->getParentRegion() == resultOp->getParentRegion(); }; @@ -290,10 +291,11 @@ class OwnedReturnConsumptionPass ifOp->setAttr(OWNED_RESULT_ATTR_NAME, mlir::UnitAttr::get(&getContext())); - if (auto *retain = findReceiverRetain(result)) + auto retains = findReceiverRetains(result); + if (!retains.empty()) { ifOp->setAttr(OWNED_RESULT_CONSUMED_ATTR_NAME, mlir::UnitAttr::get(&getContext())); - toErase.push_back(retain); + toErase.append(retains.begin(), retains.end()); } }); @@ -325,7 +327,8 @@ class OwnedReturnConsumptionPass // Two things disqualify a value, and both simply leave it leaking as before: // // - a user outside the producer's block, so the value outlives the block or is used on a - // path this cannot see; + // path this cannot see - the body of a statement written in the block is not outside it + // (runsWithinBlock); // - a user that is a terminator, since a value handed to a successor as a block argument is // still live after the point this would release it; // - a `ts.StateLabel` after the definition, which is a generator's resume point: the state @@ -397,9 +400,10 @@ class OwnedReturnConsumptionPass // the path that falls through (section 9.60). // // Only exits that come after the definition in this block are covered, because only - // those are dominated by it. Nothing else has to be checked: a temporary whose value - // is used from another block is excluded by allUsesReleasableInOwnBlock above, so a - // nested exit cannot be returning this value or reading it. + // those are dominated by it. A nested exit may read the value - a branch that reads a + // `const` and returns - but it cannot return it: a `return` of the value takes it over + // and marks it consumed, which keeps it out of this set. The release goes in front of + // the exit, after everything the branch read. // // A `return` leaves for good wherever it is written. A `break` or `continue` only // sometimes does, and which it is decides between a leak and a double release: @@ -510,14 +514,38 @@ class OwnedReturnConsumptionPass return false; } + // Does `user` run before control reaches the op that follows its ancestor in `block`? True + // for `block`'s own ops, and for ops nested in structured statements only. + static bool runsWithinBlock(mlir::Operation *user, mlir::Block *block) + { + for (auto *op = user; op->getBlock() != block; op = op->getParentOp()) + { + auto *parent = op->getParentOp(); + if (!parent || !mlir::isa(parent)) + { + return false; + } + } + + return true; + } + // Can a release at the end of this value's own block give its reference back safely? See // releaseDiscardedTemporaries for what disqualifies a use and why. + // + // A user inside the body of an `if`, a loop, a `switch`, a label or a `try` written in this + // block is not outside it: those regions run to completion before the op after them does, so + // the end of the block still follows every use. A folded `const` read inside a branch - + // `const a = new C(); if (x) { let b = a; }` - is exactly that, and leaked before this. Any + // other region is: a nested function or an `async.execute` body runs later, from somewhere + // else. static bool allUsesReleasableInOwnBlock(mlir::Operation *op) { auto *block = op->getBlock(); for (auto *user : op->getResult(0).getUsers()) { - if (user->getBlock() != block || user->hasTrait()) + if (user->hasTrait() || !runsWithinBlock(user, block)) { return false; } @@ -891,23 +919,66 @@ class OwnedReturnConsumptionPass return false; } + // Is this retain the one a `return` of `result` took? Scope exit may release other locals + // between the two, but nothing else may use the value before it is returned. + static bool retainFeedsReturn(mlir_ts::RetainOp retainOp, mlir::Value result) + { + for (auto *op = retainOp->getNextNode(); op; op = op->getNextNode()) + { + if (mlir::isa(op)) + { + return op->getNumOperands() > 0 && op->getOperand(0) == result; + } + + if (llvm::is_contained(op->getOperands(), result)) + { + return false; + } + } + + return false; + } + // The retain a receiver put on this call's result, if it took one. Two shapes, matching the // two ways §9.25's receivers acquire: a `ts.Retain` on the value itself (a field or element // store, a literal capturing it, a return passing it on), and a `ts.RetainSlot` on the // storage of a local declared from it. // - // Returning null is the ordinary answer for a result nobody took - `f();` on its own, or + // Returning none is the ordinary answer for a result nobody took - `f();` on its own, or // `f().n` - and it is left exactly as it is. Consuming a reference no receiver balances // would free the value while the expression is still using it. - static mlir::Operation *findReceiverRetain(mlir::Value result) + // + // A folded `const` (OWNED_RESULT_NAMED_ATTR_NAME) is the exception: its name is this very + // value, so the one receiver that may take its reference over is a `return`, after which + // nothing reads the name. Any other receiver keeps its retain, and the call's +1 is given + // back as a temporary like any other nobody took. Every `return` of it takes it over, since + // each is on a path of its own - `if (x) return r; return r;` - and one left retaining would + // hand its caller two references. + static llvm::SmallVector findReceiverRetains(mlir::Value result) { + auto *definingOp = result.getDefiningOp(); + if (definingOp && definingOp->hasAttr(OWNED_RESULT_NAMED_ATTR_NAME)) + { + llvm::SmallVector returned; + for (auto *user : result.getUsers()) + { + auto retainOp = mlir::dyn_cast(user); + if (retainOp && retainOp.getReference() == result && retainFeedsReturn(retainOp, result)) + { + returned.push_back(user); + } + } + + return returned; + } + for (auto *user : result.getUsers()) { if (auto retainOp = mlir::dyn_cast(user)) { if (retainOp.getReference() == result) { - return retainOp.getOperation(); + return {retainOp.getOperation()}; } } } @@ -928,12 +999,12 @@ class OwnedReturnConsumptionPass // the declaration becomes the acquisition, which is what keeps the // ownership verifier able to pair the release still to come varOp->setAttr(OWNED_LOCAL_CONSUMED_ATTR_NAME, mlir::UnitAttr::get(varOp.getContext())); - return retainSlotOp.getOperation(); + return {retainSlotOp.getOperation()}; } } } - return nullptr; + return {}; } }; diff --git a/tslang/lib/TypeScript/OwnershipInferencePass.cpp b/tslang/lib/TypeScript/OwnershipInferencePass.cpp index d78c00770..3625a2858 100644 --- a/tslang/lib/TypeScript/OwnershipInferencePass.cpp +++ b/tslang/lib/TypeScript/OwnershipInferencePass.cpp @@ -90,18 +90,23 @@ class OwnershipInferencePass : public mlir::PassWrapper proven; + llvm::SmallVector toErase; for (auto value : candidates) { - if (hasOneOwner(value) && isFresh(value)) + mlir::Operation *movedRelease = nullptr; + if (hasOneOwner(value, movedRelease) && isFresh(value)) { proven.insert(value); + if (movedRelease) + { + toErase.push_back(movedRelease); + } } } // What is proven loses its retains; any other retain is a second reference nobody // proved safe - of a loaded value, a parameter, an unknown producer, or of a fresh value // whose own check was reported above. - llvm::SmallVector toErase; for (auto *op : retains) { auto value = retainedValue(op); @@ -236,12 +241,19 @@ class OwnershipInferencePass : public mlir::PassWrapper releases; for (auto *user : value.getUsers()) { if (mlir::isa(user)) @@ -251,7 +263,7 @@ class OwnershipInferencePass : public mlir::PassWrapper(user)) { - released = true; + releases.push_back(user); continue; } @@ -282,17 +294,23 @@ class OwnershipInferencePass : public mlir::PassWrappergetBlock() != home || + !taker->isBeforeInBlock(releases.front()) || !takerAcquires(taker, value)) + { + reportSecondReference(taker); + return false; + } + + movedRelease = releases.front(); } // Every other use must come first. A use in another block runs after the whole of this // one, and so after the move. for (auto *user : value.getUsers()) { - if (user == taker || mlir::isa(user)) + if (user == taker || user == movedRelease || mlir::isa(user)) { continue; } @@ -308,6 +326,25 @@ class OwnershipInferencePass : public mlir::PassWrapper(taker)) + { + return llvm::any_of(varOp.getResult().getUsers(), + [](mlir::Operation *user) { return mlir::isa(user); }); + } + + return llvm::any_of(value.getUsers(), [&](mlir::Operation *user) { + return mlir::isa(user) && user->getBlock() == taker->getBlock() && + user->isBeforeInBlock(taker); + }); + } + // The declaration's name survives only as debug metadata (`--di`), the way LowerToLLVM's // VariableOp lowering reads it. Without it the error still points at the right place. static llvm::StringRef varName(mlir_ts::VariableOp varOp) diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index f9625e479..6d4ceb2cb 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -423,6 +423,7 @@ tslang_add_test(NAME test-compile-00-owned-array-ops COMMAND test-runner "${PROJ tslang_add_test(NAME test-compile-00-owned-inline-records COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_inline_records.ts") tslang_add_test(NAME test-compile-00-owned-transfer COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_transfer.ts") tslang_add_test(NAME test-compile-00-owned-call-results COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_call_results.ts") +tslang_add_test(NAME test-compile-00-owned-named-consts COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_named_consts.ts") tslang_add_test(NAME test-compile-00-owned-temporaries COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_temporaries.ts") tslang_add_test(NAME test-compile-00-owned-interfaces COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_interfaces.ts") tslang_add_test(NAME test-compile-00-owned-closures COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_closures.ts") @@ -876,6 +877,7 @@ tslang_add_test(NAME test-jit-00-owned-array-ops COMMAND test-runner -jit "${PRO tslang_add_test(NAME test-jit-00-owned-inline-records COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_inline_records.ts") tslang_add_test(NAME test-jit-00-owned-transfer COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_transfer.ts") tslang_add_test(NAME test-jit-00-owned-call-results COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_call_results.ts") +tslang_add_test(NAME test-jit-00-owned-named-consts COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_named_consts.ts") tslang_add_test(NAME test-jit-00-owned-temporaries COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_temporaries.ts") tslang_add_test(NAME test-jit-00-owned-interfaces COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_interfaces.ts") tslang_add_test(NAME test-jit-00-owned-closures COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_closures.ts") @@ -1468,6 +1470,8 @@ tslang_add_test(NAME test-jit-rc-owned-transfer COMMAND test-runner -jit -mm=rc tslang_add_test(NAME test-jit-none-owned-transfer COMMAND test-runner -jit -mm=none "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_transfer.ts") tslang_add_test(NAME test-jit-rc-owned-call-results COMMAND test-runner -jit -mm=rc "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_call_results.ts") tslang_add_test(NAME test-jit-none-owned-call-results COMMAND test-runner -jit -mm=none "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_call_results.ts") +tslang_add_test(NAME test-jit-rc-owned-named-consts COMMAND test-runner -jit -mm=rc "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_named_consts.ts") +tslang_add_test(NAME test-jit-none-owned-named-consts COMMAND test-runner -jit -mm=none "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_named_consts.ts") tslang_add_test(NAME test-jit-rc-owned-temporaries COMMAND test-runner -jit -mm=rc "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_temporaries.ts") tslang_add_test(NAME test-jit-none-owned-temporaries COMMAND test-runner -jit -mm=none "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_temporaries.ts") tslang_add_test(NAME test-jit-rc-owned-interfaces COMMAND test-runner -jit -mm=rc "${PROJECT_SOURCE_DIR}/test/tester/tests/00owned_interfaces.ts") @@ -1774,6 +1778,7 @@ set(TSLANG_CORPUS 00owned_iteration.ts 00owned_literals.ts 00owned_locals.ts + 00owned_named_consts.ts 00owned_nested_captures.ts 00owned_strings.ts 00owned_temporaries.ts @@ -2032,6 +2037,7 @@ set(TSLANG_CORPUS_RC_NAMED 00owned_iteration.ts 00owned_literals.ts 00owned_locals.ts + 00owned_named_consts.ts 00owned_nested_captures.ts 00owned_strings.ts 00owned_temporaries.ts @@ -2074,6 +2080,7 @@ set(TSLANG_CORPUS_NONE_NAMED 00owned_interfaces.ts 00owned_iteration.ts 00owned_literals.ts + 00owned_named_consts.ts 00owned_nested_captures.ts 00owned_strings.ts 00owned_temporaries.ts diff --git a/tslang/test/tester/tests/00owned_named_consts.ts b/tslang/test/tester/tests/00owned_named_consts.ts new file mode 100644 index 000000000..077895fed --- /dev/null +++ b/tslang/test/tester/tests/00owned_named_consts.ts @@ -0,0 +1,225 @@ +// A `const` whose initializer carries a reference - `const a = new C()`, `const s = "a" + k`, +// `const r = make()` - has no storage of its own: the name is folded into the value itself. So +// every later mention of the name is the very value that carried the reference, and a receiver +// that asks "does this carry a reference I can take over?" gets yes at every mention, not just +// the first. Taking it over is only right for a value that is used once, right there. +// +// Each case below had a receiver take over a reference the name still needed: +// - a `let` in an inner scope took it, released it at its scope exit, and the name went on +// reading freed memory; +// - two receivers both took the one reference, so the value had two holders and one count; +// - a receiver inside a loop took it on every iteration, from a value made once. +// And one without a name: `let b = h.c = new C()` hands the same `new` to the field and then to +// the local, and both took it. +// +// Every case calls `churn()` between the suspect release and the read, so that a freed block is +// claimed by something else and a use-after-free reads a wrong answer instead of a lucky one. +// See 00owned_transfer.ts for why that matters. + +class C { + x: number; + v: number[] = []; + + constructor(x: number) { + this.x = x; + } +} + +class H { + c: C = new C(0); + s: string = ""; +} + +function churn() { + const keep: C[] = []; + for (let i = 0; i < 64; i++) { + const c = new C(999); + c.v.push(99); + c.v.push(99); + c.v.push(99); + c.v.push(99); + c.v.push(99); + c.v.push(99); + c.v.push(99); + keep.push(c); + } +} + +function churnStrings() { + const keep: string[] = []; + for (let i = 0; i < 64; i++) { + keep.push("b" + (i % 10)); + } +} + +function filled(n: number) { + const c = new C(n); + for (let i = 0; i < n; i++) { + c.v.push(i); + } + + return c; +} + +// a `let` in an inner scope, then the name again after that scope has ended +function innerLetThenName() { + const a = new C(3); + a.v.push(1); + a.v.push(2); + a.v.push(3); + { + let b = a; + b.x = 4; + } + + churn(); + return a.v.length + a.x; +} + +// the same `let` inside a loop: taken over on every iteration +function innerLetInLoop() { + const a = new C(0); + for (let i = 0; i < 5; i++) { + a.v.push(i); + } + + for (let i = 0; i < 3; i++) { + let b = a; + b.x += 1; + } + + churn(); + return a.v.length + a.x; +} + +// two `let`s of one name +function twoLets() { + const a = new C(3); + a.v.push(1); + a.v.push(2); + a.v.push(3); + let r = 0; + { + let b = a; + let c = a; + r = b.v.length + c.v.length; + } + + churn(); + return r + a.v.length; +} + +// a push, then a `let` of the same name +function pushThenLet() { + const arr: C[] = []; + const c = new C(3); + c.v.push(1); + c.v.push(2); + c.v.push(3); + arr.push(c); + { + let d = c; + d.x = 4; + } + + churn(); + return arr[0].v.length + arr[0].x; +} + +// a push inside a loop: one value, three elements +function pushInLoop() { + let keep = new C(0); + { + const arr: C[] = []; + const c = new C(5); + for (let i = 0; i < 3; i++) { + arr.push(c); + } + + keep = arr[0]; + } + + churn(); + return keep.x; +} + +// a fresh string stored into a field inside a loop: each overwrite released the one before it, +// and the one before it was the same string +function fieldStoreInLoop() { + let k: number = 3; + const h = new H(); + const s = "a" + k; + for (let i = 0; i < 3; i++) { + h.s = s; + } + + churnStrings(); + return h.s == "a3" && s == "a3" ? 1 : 0; +} + +// a call's result, named, then a `let` in an inner scope +function callResultThenName() { + const a = filled(3); + { + let b = a; + b.x = 4; + } + + churn(); + return a.v.length + a.x; +} + +// no name at all: an assignment's value handed on to a second receiver +function chainedAssignment() { + const h = new H(); + { + let b = h.c = new C(3); + b.v.push(1); + b.v.push(2); + b.v.push(3); + } + + churn(); + return h.c.v.length + h.c.x; +} + +// A name returned on two paths. Each `return` is the last use on its own path, so each hands the +// reference over; one that retained instead would give its caller a second reference, a leak no +// assertion here can see - `-mm=own` and the ownership verifier are what notice that. This pins +// that both paths still hand back a live value. +function returnedOnTwoPaths(x: boolean) { + const c = new C(3); + c.v.push(1); + if (x) return c; + return c; +} + +// the same for a named call result, which the pass after MLIRGen settles +function callResultReturnedOnTwoPaths(x: boolean) { + const r = filled(2); + if (x) return r; + return r; +} + +function returns() { + const a = returnedOnTwoPaths(true); + const b = returnedOnTwoPaths(false); + const c = callResultReturnedOnTwoPaths(true); + const d = callResultReturnedOnTwoPaths(false); + churn(); + return a.x + b.x + c.v.length + d.v.length; +} + +function main() { + assert(innerLetThenName() == 7, "a name outlives an inner `let` of it"); + assert(innerLetInLoop() == 8, "a name outlives a `let` of it in a loop"); + assert(twoLets() == 9, "two `let`s of one name are two holders"); + assert(pushThenLet() == 7, "a pushed name outlives a `let` of it"); + assert(pushInLoop() == 5, "a name pushed in a loop is three references"); + assert(fieldStoreInLoop() == 1, "a string stored into a field in a loop survives the overwrites"); + assert(callResultThenName() == 7, "a named call result outlives an inner `let` of it"); + assert(chainedAssignment() == 6, "a field and a local assigned one `new` are two holders"); + assert(returns() == 10, "a name returned on either of two paths is handed back alive"); + + print("done."); +}