From d02d9a73afd2f0c6c0c92a22a39c37752322c63f Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Mon, 19 Oct 2026 22:59:22 +0100 Subject: [PATCH 1/2] A generator's yielded value and its temporaries before a yield are given back Two leaks left after #619: - A temporary made in front of a statement with a yield inside it (`const n = ("b" + i).length; if (n > 0) { yield n; }`) was never released, under rc and own: 165 MB over 500000 runs, now 4.4 MB. The release now goes in front of the first yield, resume point or statement holding one, where the definition still dominates. - Under rc a yielded value (`yield "s" + i`, `yield new C(i)`) kept the reference it was born with: 50 MB over 500000 runs, now 4.4 MB. A yield now retains its `{ value, done }` record the way a return does, a generator's `next` is classified as returning owned, and its caller takes the record over: `next` is resolved through the state object's field that only ever holds it (own_facts::FieldFunctions, moved out of OwnershipSignaturePass). own already moved the value into the record. A temporary used after a yield cannot occur: MLIRGen keeps a generator's locals in its state object. Co-Authored-By: Claude Opus 5.5 --- .../TypeScript/OwnedReturnConsumptionPass.cpp | 186 ++++++++++++------ tslang/lib/TypeScript/OwnershipFacts.h | 138 +++++++++++++ .../lib/TypeScript/OwnershipSignaturePass.cpp | 132 +------------ .../test/tester/generator-temporaries.cmake | 114 +++++++---- .../tester/tests/00generator_temporaries.ts | 39 +++- 5 files changed, 372 insertions(+), 237 deletions(-) diff --git a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp index 243e4f020..74e74fcf6 100644 --- a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp +++ b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp @@ -64,6 +64,16 @@ class OwnedReturnConsumptionPass copyEscapingStringViews(module); + retainYieldedRecords(mth, module); + + own_facts::FieldFunctions fieldFunctionsOfModule; + const own_facts::FieldFunctions *fieldFunctions = nullptr; + if (compileOptions.isRefCounted()) + { + fieldFunctionsOfModule.collect(module); + fieldFunctions = &fieldFunctionsOfModule; + } + llvm::DenseSet returnsOwned; // Every method in the module, grouped by the name after the last dot: the candidate set // for a virtual call, see virtualCallReturnsOwned. @@ -73,7 +83,7 @@ class OwnedReturnConsumptionPass auto anyUnclassifiedOwningReturn = false; module.walk([&](mlir_ts::FuncOp funcOp) { auto name = funcOp.getName(); - auto classified = functionReturnsOwned(mth, funcOp); + auto classified = functionReturnsOwned(mth, funcOp, compileOptions.isRefCounted()); if (classified) { returnsOwned.insert(name); @@ -145,7 +155,7 @@ class OwnedReturnConsumptionPass return; } - auto callee = calleeNameOf(callOp); + auto callee = calleeNameOf(callOp, fieldFunctions); auto calleeReturnsOwned = !callee.empty() && returnsOwned.contains(callee); // A call through a plain value - a callback handed to `reduce`, a function-typed @@ -246,6 +256,36 @@ class OwnedReturnConsumptionPass releaseDiscardedTemporaries(mth, module); } + // A `yield` hands its `{ value, done }` record to the caller the way a `return` does, with a + // reference of its own (§9.24) - under rc, where a return statement retains what it returns + // and a yield, built by MLIRGen without going through it, did not. The record was filled by + // stores that take no count, so what it held lived only on the +1 it was born with: a fresh + // `"s" + i` was never given back (the caller retains what it reads and releases that), and a + // value read out of the state object was the state's own reference. With the retain the + // record owns its fields, a fresh value made for it is a temporary released before the yield + // like any other, and `next` returns owned like a plain function, so its caller can take the + // record over (functionReturnsOwned, fieldFunctionNameOf). + // + // Under own the yielded value moves into the record instead, and a retain is an error. + void retainYieldedRecords(MLIRTypeHelper &mth, mlir::ModuleOp module) + { + if (!compileOptions.isRefCounted()) + { + return; + } + + module.walk([&](mlir_ts::YieldReturnValOp yieldOp) { + auto record = yieldOp.getOperand(); + if (!mth.ownsHeapMemory(yieldOp.getLoc(), record.getType()) || retainPrecedes(yieldOp, record)) + { + return; + } + + mlir::OpBuilder builder(yieldOp); + builder.create(yieldOp.getLoc(), record); + }); + } + // A global is born holding the reference its initializer carries. // // The initializer region ends in `ts.GlobalResult`, a terminator, so a value given to it @@ -814,13 +854,14 @@ class OwnedReturnConsumptionPass // (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 - // machine re-enters the block THERE, so the end of the block is reachable on a path that - // never ran the op that produced the value. Nothing about that is visible while the - // generator is still one block - it only becomes a use before definition once the state - // machine is expanded, and it surfaces as a dominance failure in the affine lowering - // rather than as anything this pass could notice. A `yield` in front of the resume point - // is different: the release goes before it, inside the stretch the definition dominates. + // - a user at or after a `ts.StateLabel` that follows the definition, which is a generator's + // resume point: the state machine re-enters the block THERE, so the end of the block is + // reachable on a path that never ran the op that produced the value. Nothing about that is + // visible while the generator is still one block - it only becomes a use before definition + // once the state machine is expanded, and it surfaces as a dominance failure in the affine + // lowering rather than as anything this pass could notice. So in a generator the release + // does not go at the end of the block but in front of the first yield, resume point or + // statement holding one (releaseRangeEnd), inside the stretch the definition dominates. // // That bias is the same one the rest of this arc takes: an unreleased reference is invisible, // a released one that was still owned is a use-after-free. @@ -858,7 +899,8 @@ class OwnedReturnConsumptionPass if (auto funcOp = op->getParentOfType()) { - if (generators.contains(funcOp.getOperation()) && escapesThroughStore(op->getResult(0))) + if (!compileOptions.isRefCounted() && generators.contains(funcOp.getOperation()) && + escapesThroughStore(op->getResult(0))) { return; } @@ -931,10 +973,15 @@ class OwnedReturnConsumptionPass }); } - if (auto *exiting = firstExitingOpAfter(op)) + auto *exiting = firstExitingOpAfter(op); + if (exiting && (rangeEnd == block->end() || exiting->isBeforeInBlock(&*rangeEnd))) { builder.setInsertionPoint(exiting); } + else if (rangeEnd != block->end()) + { + builder.setInsertionPoint(&*rangeEnd); + } else if (auto *terminator = block->getTerminator()) { builder.setInsertionPoint(terminator); @@ -1031,13 +1078,6 @@ class OwnedReturnConsumptionPass auto *block = op->getBlock(); auto rangeEnd = releaseRangeEnd(op); - // a resume point between the definition and the end of the block with no yield in front - // of it to release before - see above - if (rangeEnd != block->end() && mlir::isa(*rangeEnd)) - { - return false; - } - for (auto *user : op->getResult(0).getUsers()) { if (user->hasTrait() || !runsWithinBlock(user, block)) @@ -1045,7 +1085,10 @@ class OwnedReturnConsumptionPass return false; } - // a use after the yield runs on the resume path, which the release has already left + // A use at or after the end of the range runs on a path the release has already left: + // after the yield, or inside the statement that holds one (the resume point there + // re-enters past the definition). Such a value leaks. MLIRGen keeps a generator's + // locals in its state object, so a temporary used across a yield does not occur. auto *inBlock = block->findAncestorOpInBlock(*user); if (rangeEnd != block->end() && !inBlock->isBeforeInBlock(&*rangeEnd)) { @@ -1053,42 +1096,45 @@ class OwnedReturnConsumptionPass } } - // A resume point inside a statement written before the yield - `if (c) { yield 1; }` - - // re-enters the block in the middle of that statement and falls through to the release - // without having run the definition. - for (auto it = std::next(mlir::Block::iterator(op)); it != rangeEnd; ++it) + return true; + } + + // Where the stretch of `op`'s block that a release of it may cover ends: at the first op after + // it that is a yield or a resume point or has one inside it, or at the end of the block. The + // yield ends the call (the affine lowering turns it into a store and a jump to the exit + // block), and the next call re-enters at the resume point after it, past the definition - so + // a release has to come in front of it, and nothing after is on the path that ran the + // definition. A statement with a yield inside - `if (c) { yield 1; }` - is the same: resuming + // in the middle of it falls through to the rest of the block. A plain function has neither and + // gets the whole block. + static mlir::Block::iterator releaseRangeEnd(mlir::Operation *op) + { + auto *block = op->getBlock(); + for (auto it = std::next(mlir::Block::iterator(op)); it != block->end(); ++it) { + if (mlir::isa(*it)) + { + return it; + } + + if (it->getNumRegions() == 0) + { + continue; + } + auto resumes = it->walk([&](mlir::Operation *nested) { + // a nested function's yields are its own if (mlir::isa(nested)) { return mlir::WalkResult::skip(); } - return mlir::isa(nested) ? mlir::WalkResult::interrupt() - : mlir::WalkResult::advance(); + return mlir::isa(nested) + ? mlir::WalkResult::interrupt() + : mlir::WalkResult::advance(); }); if (resumes.wasInterrupted()) - { - return false; - } - } - - return true; - } - - // Where the stretch of `op`'s block that a release of it may cover ends: at the first yield - // or resume point after it, or at the end of the block. The yield ends the call (the affine - // lowering turns it into a store and a jump to the exit block), and the next call re-enters - // at the resume point after it, past the definition - so a release has to come before the - // yield, and nothing after it is on the path that ran the definition. A plain function has - // neither and gets the whole block. - static mlir::Block::iterator releaseRangeEnd(mlir::Operation *op) - { - auto *block = op->getBlock(); - for (auto it = std::next(mlir::Block::iterator(op)); it != block->end(); ++it) - { - if (mlir::isa(*it)) { return it; } @@ -1097,12 +1143,12 @@ class OwnedReturnConsumptionPass return block->end(); } - // Does this generator temporary, or a view of it, get stored somewhere? The `{ value, done }` - // record a `yield` hands back is filled by a store that takes no count of its own - the - // caller retains on receipt - so `yield "a" + i` keeps the concatenation alive only through - // the +1 it was born with, and a release before the yield would free what the caller reads. - // Any store is treated that way: it leaks, as before, rather than telling the record apart - // from a place that does count. + // Does this generator temporary, or a view of it, get stored somewhere? Asked under own only. + // There the `{ value, done }` record a `yield` hands back is filled by stores that move the + // value in, so `yield "a" + i` keeps the concatenation alive only through the reference it + // was born with, and a release before the yield would free what the caller reads. Any store + // is treated that way, rather than telling the record apart from a place that does count. + // Under rc the record takes a reference of its own (retainYieldedRecords). static bool escapesThroughStore(mlir::Value value) { for (auto *user : value.getUsers()) @@ -1173,7 +1219,7 @@ class OwnedReturnConsumptionPass // canonicalizer (SimplifyIndirectCallWithKnownCallee) already rewrites into direct calls, // which is the argument that they name one callee: it is the same judgement, made here // before canonicalization has run. - static mlir::StringRef calleeNameOf(mlir_ts::CallIndirectOp callOp) + static mlir::StringRef calleeNameOf(mlir_ts::CallIndirectOp callOp, const own_facts::FieldFunctions *fieldFunctions) { if (callOp.getNumOperands() == 0) { @@ -1217,6 +1263,17 @@ class OwnedReturnConsumptionPass return {}; } + // A generator's `next`, read out of its state object: a field every state object of that + // type holds the same function in, and nothing writes (own_facts::FieldFunctions). Asked + // under rc only, where a yield hands its record over retained (retainYieldedRecords). + if (fieldFunctions) + { + if (auto symbol = fieldFunctions->of(boundFunc.getDefiningOp())) + { + return symbol.getValue(); + } + } + // `ts.ThisVirtualSymbolRef` is deliberately absent here. It carries an identifier, but // that names the declaration the call was written against, not what the runtime class put // in the slot - so reading it as *the* callee would consume a reference an override may @@ -1518,29 +1575,34 @@ class OwnedReturnConsumptionPass // body, a return whose retain is not in the same block, a return with no retain at all. A // false "no" costs a leak; a false "yes" frees a value the callee never retained. - static bool functionReturnsOwned(MLIRTypeHelper &mth, mlir_ts::FuncOp funcOp) + static bool functionReturnsOwned(MLIRTypeHelper &mth, mlir_ts::FuncOp funcOp, bool refCounted) { if (funcOp.isExternal() || funcOp.getBody().empty()) { return importedFunctionReturnsOwned(mth, funcOp); } - // A generator's returns are not the value its caller receives - the caller gets the - // generator object, built by the transformation rather than by these returns. Rather - // than reason about what that transformation leaves behind, leave any function with a - // yield in it alone; its callers keep retaining, and leak. + // A generator's `next` - the function the yields are in - hands its caller a `{ value, + // done }` record from every yield and every return. Under rc each of them retains it + // (retainYieldedRecords for the yields), so it is classified like any other function. Under + // the other models the yields do not, and it is left alone; its callers keep retaining. auto isGenerator = false; funcOp.walk([&](mlir_ts::YieldReturnValOp) { isGenerator = true; }); - if (isGenerator) + if (isGenerator && !refCounted) { return false; } auto sawOwningReturn = false; auto everyReturnRetains = true; - funcOp.walk([&](mlir_ts::ReturnValOp returnOp) { - auto value = returnOp.getOperand(); - if (!mth.ownsHeapMemory(returnOp.getLoc(), value.getType())) + funcOp.walk([&](mlir::Operation *op) { + if (!mlir::isa(op)) + { + return; + } + + auto value = op->getOperand(0); + if (!mth.ownsHeapMemory(op->getLoc(), value.getType())) { return; } @@ -1553,7 +1615,7 @@ class OwnedReturnConsumptionPass // rather than adding a second (§9.25). Reading only the first shape as "returns // owned" left every `static times(...) { return new Vector(...) }` unclassified, // which is most of what expression-shaped code is built from. - if (!retainPrecedes(returnOp, value) && !producesOwnedResult(value)) + if (!retainPrecedes(op, value) && !producesOwnedResult(value)) { everyReturnRetains = false; } diff --git a/tslang/lib/TypeScript/OwnershipFacts.h b/tslang/lib/TypeScript/OwnershipFacts.h index 67c917cea..a7dddfb4f 100644 --- a/tslang/lib/TypeScript/OwnershipFacts.h +++ b/tslang/lib/TypeScript/OwnershipFacts.h @@ -7,6 +7,8 @@ #include "mlir/Interfaces/ControlFlowInterfaces.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/STLFunctionalExtras.h" @@ -807,6 +809,142 @@ inline int borrowedParam(mlir::Value value, int depth = 0) return -1; } +// The tuple an object, a reference or a tuple value of this type stores its fields in; null for +// any other type. A method sees its object as `!ts.object>`, whose +// fields are the tuple's. +inline mlir_ts::TupleType storageTupleOf(mlir::MLIRContext *context, mlir::Type type) +{ + if (auto objectType = mlir::dyn_cast(type)) + { + type = objectType.getStorageType(); + if (auto storageType = mlir::dyn_cast(type)) + { + return mlir_ts::TupleType::get(context, storageType.getFields()); + } + } + else if (auto refType = mlir::dyn_cast(type)) + { + type = refType.getElementType(); + } + else if (auto valueRefType = mlir::dyn_cast(type)) + { + type = valueRefType.getElementType(); + } + + return mlir::dyn_cast(type); +} + +// (tuple type, field) -> the one function an object of that type holds in that field: every +// `ts.Constant` of the type's shape puts it there, nothing stores into the field, and nothing but +// a constant or a read makes a value of the type. A generator's state object holds its `next` so. +// Used by OwnershipSignaturePass (own) and OwnedReturnConsumptionPass (rc). +class FieldFunctions +{ + llvm::DenseMap, mlir::StringAttr> fieldFunctions; + mlir::MLIRContext *context = nullptr; + + public: + void collect(mlir::ModuleOp module) + { + context = module->getContext(); + fieldFunctions.clear(); + + llvm::DenseSet> unknownFields; + llvm::DenseSet unknownTuples; + module.walk([&](mlir::Operation *op) { + if (auto constantOp = mlir::dyn_cast(op)) + { + auto constTupleType = mlir::dyn_cast(constantOp.getType()); + auto values = mlir::dyn_cast(constantOp.getValue()); + if (!constTupleType || !values) + { + return; + } + + mlir::Type tupleType = mlir_ts::TupleType::get(context, constTupleType.getFields()); + for (auto [index, value] : llvm::enumerate(values)) + { + auto symbol = mlir::dyn_cast(value); + if (!symbol) + { + continue; + } + + std::pair key{tupleType, static_cast(index)}; + auto [found, inserted] = fieldFunctions.try_emplace(key, symbol.getAttr()); + if (!inserted && found->second != symbol.getAttr()) + { + unknownFields.insert(key); + } + } + + return; + } + + // a field reference that is not only read: the field may be given anything + if (auto propertyRefOp = mlir::dyn_cast(op)) + { + auto tupleType = storageTupleOf(context, propertyRefOp->getOperand(0).getType()); + auto onlyRead = llvm::all_of(propertyRefOp->getUsers(), + [](mlir::Operation *user) { return mlir::isa(user); }); + if (tupleType && !onlyRead) + { + unknownFields.insert({tupleType, propertyRefOp.getPosition()}); + } + + return; + } + + // a value of the type made any other way may hold anything in any field + if (mlir::isa(op)) + { + return; + } + + for (auto result : op->getResults()) + { + if (auto tupleType = mlir::dyn_cast(result.getType())) + { + unknownTuples.insert(tupleType); + } + } + }); + + for (auto &key : unknownFields) + { + fieldFunctions.erase(key); + } + + llvm::SmallVector> dropped; + for (auto &entry : fieldFunctions) + { + if (unknownTuples.contains(entry.first.first)) + { + dropped.push_back(entry.first); + } + } + + for (auto &key : dropped) + { + fieldFunctions.erase(key); + } + } + + // `ts.Load(ts.PropertyRef(object, p))` of a field that only ever holds one function. + mlir::StringAttr of(mlir::Operation *def) const + { + auto loadOp = mlir::dyn_cast_or_null(def); + auto propertyRefOp = loadOp ? loadOp.getReference().getDefiningOp() : mlir_ts::PropertyRefOp(); + auto tupleType = propertyRefOp ? storageTupleOf(context, propertyRefOp->getOperand(0).getType()) : mlir_ts::TupleType(); + if (!tupleType) + { + return {}; + } + + return fieldFunctions.lookup({tupleType, propertyRefOp.getPosition()}); + } +}; + } // namespace own_facts #endif // MLIR_TYPESCRIPT_OWNERSHIPFACTS_H diff --git a/tslang/lib/TypeScript/OwnershipSignaturePass.cpp b/tslang/lib/TypeScript/OwnershipSignaturePass.cpp index ac6e9fc92..22df904aa 100644 --- a/tslang/lib/TypeScript/OwnershipSignaturePass.cpp +++ b/tslang/lib/TypeScript/OwnershipSignaturePass.cpp @@ -176,7 +176,7 @@ class OwnershipSignaturePass : public mlir::PassWrapper importedNoDrops; - // (tuple type, field) -> the one function an object of that type holds in that field: every - // `ts.Constant` of the type's shape puts it there, nothing stores into the field, and nothing - // but a constant or a read makes a value of the type. A generator's state object holds its - // `next` so. - llvm::DenseMap, mlir::StringAttr> fieldFunctions; + // (tuple type, field) -> the one function an object of that type holds in that field. A + // generator's state object holds its `next` so. + own_facts::FieldFunctions fieldFunctions; // Class names, from their vtables, to split a method's symbol into class and method name. llvm::StringSet<> classNames; @@ -458,127 +456,9 @@ class OwnershipSignaturePass : public mlir::PassWrapper>`, whose - // fields are the tuple's. mlir_ts::TupleType storageTupleOf(mlir::Type type) { - if (auto objectType = mlir::dyn_cast(type)) - { - type = objectType.getStorageType(); - if (auto storageType = mlir::dyn_cast(type)) - { - return mlir_ts::TupleType::get(&getContext(), storageType.getFields()); - } - } - else if (auto refType = mlir::dyn_cast(type)) - { - type = refType.getElementType(); - } - else if (auto valueRefType = mlir::dyn_cast(type)) - { - type = valueRefType.getElementType(); - } - - return mlir::dyn_cast(type); - } - - void collectFieldFunctions(mlir::ModuleOp module) - { - auto *context = &getContext(); - llvm::DenseSet> unknownFields; - llvm::DenseSet unknownTuples; - module.walk([&](mlir::Operation *op) { - if (auto constantOp = mlir::dyn_cast(op)) - { - auto constTupleType = mlir::dyn_cast(constantOp.getType()); - auto values = mlir::dyn_cast(constantOp.getValue()); - if (!constTupleType || !values) - { - return; - } - - mlir::Type tupleType = mlir_ts::TupleType::get(context, constTupleType.getFields()); - for (auto [index, value] : llvm::enumerate(values)) - { - auto symbol = mlir::dyn_cast(value); - if (!symbol) - { - continue; - } - - std::pair key{tupleType, static_cast(index)}; - auto [found, inserted] = fieldFunctions.try_emplace(key, symbol.getAttr()); - if (!inserted && found->second != symbol.getAttr()) - { - unknownFields.insert(key); - } - } - - return; - } - - // a field reference that is not only read: the field may be given anything - if (auto propertyRefOp = mlir::dyn_cast(op)) - { - auto tupleType = storageTupleOf(propertyRefOp->getOperand(0).getType()); - auto onlyRead = llvm::all_of(propertyRefOp->getUsers(), - [](mlir::Operation *user) { return mlir::isa(user); }); - if (tupleType && !onlyRead) - { - unknownFields.insert({tupleType, propertyRefOp.getPosition()}); - } - - return; - } - - // a value of the type made any other way may hold anything in any field - if (mlir::isa(op)) - { - return; - } - - for (auto result : op->getResults()) - { - if (auto tupleType = mlir::dyn_cast(result.getType())) - { - unknownTuples.insert(tupleType); - } - } - }); - - for (auto &key : unknownFields) - { - fieldFunctions.erase(key); - } - - llvm::SmallVector> dropped; - for (auto &entry : fieldFunctions) - { - if (unknownTuples.contains(entry.first.first)) - { - dropped.push_back(entry.first); - } - } - - for (auto &key : dropped) - { - fieldFunctions.erase(key); - } - } - - // `ts.Load(ts.PropertyRef(object, p))` of a field that only ever holds one function. - mlir::StringAttr fieldFunctionOf(mlir::Operation *def) - { - auto loadOp = mlir::dyn_cast_or_null(def); - auto propertyRefOp = loadOp ? loadOp.getReference().getDefiningOp() : mlir_ts::PropertyRefOp(); - auto tupleType = propertyRefOp ? storageTupleOf(propertyRefOp->getOperand(0).getType()) : mlir_ts::TupleType(); - if (!tupleType) - { - return {}; - } - - return fieldFunctions.lookup({tupleType, propertyRefOp.getPosition()}); + return own_facts::storageTupleOf(&getContext(), type); } // The functions a class vtable holds, by position; empty for any other global. @@ -791,7 +671,7 @@ class OwnershipSignaturePass : public mlir::PassWrapper [(][)][\r\n]+ *") -set(releasesBeforeYield "${release}${release}\"ts[.]YieldReturnVal\"") +set(release "\"ts[.]Release\"[(]%[0-9]+[)] : [(]!ts[.](string|class<@Item[^)]*)[)] -> [(][)][\r\n]+ *") +set(recordRetain "\"ts[.]Retain\"[(]%[0-9]+[)] : [(]!ts[.]tuple<[^\r\n]*[\r\n]+ *") +set(yield "\"ts[.]YieldReturnVal\"") +set(stringRelease "\"ts[.]Release\"[(]%[0-9]+[)] : [(]!ts[.]string[)]") + +emit(rc out) -next_body(lengths body) -if(NOT body MATCHES "${releasesBeforeYield}") +func_body("${out}" "\".f_lengths.." body) +if(NOT body MATCHES "${release}${release}${yield}") message(FATAL_ERROR "`lengths` does not release its two temporaries before the yield:\n${body}") endif() -next_body(strings body) -count_releases("${body}" count) -string(REGEX MATCH "%([0-9]+) = \"ts[.]ArithmeticBinary\"[^\r\n]*-> !ts[.]string" concat "${body}") -if(NOT count EQUAL 1 OR NOT concat OR body MATCHES "\"ts[.]Release\"[(]%${CMAKE_MATCH_1}[)]") - message(FATAL_ERROR "`strings` must release the number's string and keep the yielded one:\n${body}") +func_body("${out}" "\".f_strings.." body) +if(NOT body MATCHES "${recordRetain}${release}${release}${yield}") + message(FATAL_ERROR "`strings` does not retain its record and release both strings before the yield:\n${body}") endif() -next_body(branchYield body) -count_releases("${body}" count) -if(NOT count EQUAL 0) - message(FATAL_ERROR "`branchYield` releases a temporary a resume point cuts off:\n${body}") +func_body("${out}" "\".f_items.." body) +if(NOT body MATCHES "${recordRetain}${release}${yield}") + message(FATAL_ERROR "`items` does not retain its record and release the instance before the yield:\n${body}") endif() -next_body(twoSegments body) -string(REGEX MATCHALL "${releasesBeforeYield}" segments "${body}") -list(LENGTH segments count) -if(NOT count EQUAL 2) +func_body("${out}" "\".f_branchYield.." body) +if(NOT body MATCHES "${release}${release}\"ts[.]If\"") + message(FATAL_ERROR "`branchYield` does not release its temporaries before the `if` holding the yield:\n${body}") +endif() + +func_body("${out}" "\".f_twoSegments.." body) +count("${release}${release}${yield}" "${body}" n) +if(NOT n EQUAL 2) message(FATAL_ERROR "`twoSegments` does not release two temporaries before each yield:\n${body}") endif() +# `next` is called with the state object as its only argument: `ts.CallIndirect(%next, %state)`. +# Only a record that holds a string or an instance has anything to take over; a `const` holding +# the record names it (`__owned_result_named`) and releases it at scope exit. +set(heapRecord "[^\r\n]*-> !ts[.]tuple<{\"value\",!ts[.](string|class)") +func_body("${out}" "main " body) +count("\"ts[.]CallIndirect\"[(]%[0-9]+, %[0-9]+[)] {__owned_result, __owned_result_(consumed|named)}${heapRecord}" "${body}" taken) +count("\"ts[.]CallIndirect\"[(]%[0-9]+, %[0-9]+[)]${heapRecord}" "${body}" calls) +if(calls EQUAL 0 OR NOT taken EQUAL calls) + message(FATAL_ERROR "`main` does not take over every record `next` returns (${taken} of ${calls}):\n${body}") +endif() + +emit(own out) +func_body("${out}" "\".f_strings.." body) +count("${stringRelease}" "${body}" n) +string(REGEX MATCH "%([0-9]+) = \"ts[.]ArithmeticBinary\"[^\r\n]*-> !ts[.]string" concat "${body}") +if(NOT n EQUAL 1 OR NOT concat OR body MATCHES "\"ts[.]Release\"[(]%${CMAKE_MATCH_1}[)]") + message(FATAL_ERROR "under own `strings` must release the number's string and keep the yielded one:\n${body}") +endif() + execute_process(COMMAND "${TSLANG}" --emit=mlir-affine --verify-ownership --no-default-lib -mm=own "${FILE}" OUTPUT_QUIET ERROR_VARIABLE err RESULT_VARIABLE result) if(NOT result EQUAL 0) diff --git a/tslang/test/tester/tests/00generator_temporaries.ts b/tslang/test/tester/tests/00generator_temporaries.ts index c7efa8293..c7ecdddcb 100644 --- a/tslang/test/tester/tests/00generator_temporaries.ts +++ b/tslang/test/tester/tests/00generator_temporaries.ts @@ -1,20 +1,27 @@ -// A temporary made inside a generator body is given back before the yield that ends the call. +// What a generator body makes and what it yields is given back. // // OwnedReturnConsumptionPass used to skip whole generators: the state machine cuts their blocks // at every resume point later, so a release at the end of a block could stop being dominated by // the definition. Under rc and own `yield ("a" + i).length` leaked the number's string and the // concatenation on every call: 500000 runs of `lengths` below grew by about 165 MB. // -// Now a temporary is released when no resume point lies between it and the release, which goes -// in front of the first yield after it. Three cases stay leaking, as before: -// - a value that goes out with the yield (`yield "a" + i`): the `{ value, done }` record holds -// it without a count of its own, and the caller reads it after the call returns; -// - a value made in front of a statement that has a yield inside it (`branchYield`): resuming -// there reaches the end of the block without having made it; -// - a value used after a yield. +// A temporary is released in front of the first yield, resume point or statement holding one +// after it (`branchYield`: the `if` with a yield inside), where the definition still dominates. +// A temporary used after that point would leak, but none is: MLIRGen keeps a generator's locals +// in its state object. +// +// A yielded value: under rc a yield retains its `{ value, done }` record the way a return does, +// so the record owns what it holds, and the caller takes the record over - `next` is read out of +// a field of the state object that only ever holds it. Before, `yield "s" + i` and +// `yield new Item(i)` kept the reference they were born with and nobody gave it back (50 MB over +// 500000 runs). Under own the value moves into the record and the caller owns it. // // test-rc-generator-temporaries reads the IR for which is which. +class Item { + constructor(public n: number) {} +} + function* lengths() { let i = 0; while (true) { @@ -33,6 +40,12 @@ function* strings() { } } +function* items() { + for (let i = 1; i <= 3; i++) { + yield new Item(i); + } +} + function* branchYield() { let i = 0; while (i < 3) { @@ -62,6 +75,16 @@ function main() { for (const s of strings()) joined += s; assert(joined == "s1s2s3", "strings"); + // kept past the next call: the caller's own reference + const it = strings(); + const first = it.next(); + const second = it.next(); + assert(first.value == "s1" && second.value == "s2", "strings by hand"); + + let itemTotal = 0; + for (const item of items()) itemTotal += item.n; + assert(itemTotal == 6, "items"); + let branchTotal = 0; for (const v of branchYield()) branchTotal += v; assert(branchTotal == 6, "branchYield"); From 80b59dfe034fd65174f59faf9ff6efca108539ed Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Mon, 19 Oct 2026 23:03:51 +0100 Subject: [PATCH 2/2] Generator temporaries test: the caller's takeover is checked through the field, not the closed world A declared C function in the file keeps a call through a plain value from being taken as owned by the whole-module rule, so the main check fails when the field resolution is turned off (checked). Comments and the own spec's found-not-fixed list updated. Co-Authored-By: Claude Opus 5.5 --- .../specs/2026-09-24-own-memory-model-design.md | 8 ++++++-- tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp | 11 ++++++----- tslang/test/tester/tests/00generator_temporaries.ts | 7 +++++++ 3 files changed, 19 insertions(+), 7 deletions(-) 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 d44693279..8e03e90eb 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 @@ -1307,9 +1307,13 @@ Found, not fixed: - **A temporary made in front of a `yield`** in the generator's own body is never released, under rc as well. `OwnedReturnConsumptionPass` does not release past a resume point, so - `yield "s" + i` leaks the number's string (50 MB over 200000 runs). + `yield "s" + i` leaks the number's string (50 MB over 200000 runs). *Fixed (#619 and its + follow-up):* a temporary is released in front of the first yield, resume point or statement + holding one after it. - **rc leaks yielded objects.** The call's reference and the record's retain are two, and the - caller releases one. + caller releases one. *Fixed (follow-up to #619):* under rc a yield retains its record the way a + return does, `next` is classified as returning owned, and its caller takes the record over + through the state object's field that only ever holds `next` (`own_facts::FieldFunctions`). - **Reading a record out of a call or a folded `const`** (`print(f().value.x)`, `const r = f(); r.value.x`) is an error. A `let` works. - **A method of an object literal held in a `let`**, returning a block, called and kept, is a diff --git a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp index 74e74fcf6..c750f26b6 100644 --- a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp +++ b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp @@ -36,9 +36,10 @@ namespace // // All three dissolve once every function is present, which is why this is a pass rather than // another marking site. It does not predict which callees retain - it looks. A function counts -// as returning owned only when every `ts.ReturnVal` of a heap-owning value in it is preceded by -// a `ts.Retain` of that same value. A callee with no body, a generator whose yields return -// without retaining, and anything else that does not match are simply not marked, and their +// as returning owned only when every `ts.ReturnVal` (and, in a generator under rc, every +// `ts.YieldReturnVal`) of a heap-owning value in it is preceded by a `ts.Retain` of that same +// value. A callee with no body, a generator under any other model, and anything else that does +// not match are simply not marked, and their // callers go on retaining - which leaks rather than frees something live. The whole design puts // the uncertain case on the leaking side. // @@ -1143,8 +1144,8 @@ class OwnedReturnConsumptionPass return block->end(); } - // Does this generator temporary, or a view of it, get stored somewhere? Asked under own only. - // There the `{ value, done }` record a `yield` hands back is filled by stores that move the + // Does this generator temporary, or a view of it, get stored somewhere? Asked under every model but + // rc (only own has releases left by the time it matters). Under own the `{ value, done }` record a `yield` hands back is filled by stores that move the // value in, so `yield "a" + i` keeps the concatenation alive only through the reference it // was born with, and a release before the yield would free what the caller reads. Any store // is treated that way, rather than telling the record apart from a place that does count. diff --git a/tslang/test/tester/tests/00generator_temporaries.ts b/tslang/test/tester/tests/00generator_temporaries.ts index c7ecdddcb..29653eec3 100644 --- a/tslang/test/tester/tests/00generator_temporaries.ts +++ b/tslang/test/tester/tests/00generator_temporaries.ts @@ -18,6 +18,11 @@ // // test-rc-generator-temporaries reads the IR for which is which. +// A function whose returned string the pass cannot see retained. With it in the module, a call +// through a plain value is not taken to return owned for that reason alone, so `main`'s calls of +// `next` are taken over only because the field they are read from names the generator's `next`. +declare function strerror(errnum: s32): string; + class Item { constructor(public n: number) {} } @@ -93,5 +98,7 @@ function main() { for (const v of twoSegments()) segments += v; assert(segments == 6, "twoSegments"); + assert(strerror(2).length > 0, "strerror"); + print("done."); }