Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions tslang/docs/reference-counting-evaluation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
9 changes: 9 additions & 0 deletions tslang/include/TypeScript/Defines.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 7 additions & 4 deletions tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
}

Expand Down
58 changes: 58 additions & 0 deletions tslang/include/TypeScript/MLIRLogic/MLIROwnedReference.h
Original file line number Diff line number Diff line change
@@ -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_
2 changes: 1 addition & 1 deletion tslang/lib/TypeScript/MLIRGenExpressions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
24 changes: 16 additions & 8 deletions tslang/lib/TypeScript/MLIRGenImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
{
Expand All @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions tslang/lib/TypeScript/MLIRGenStatements.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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));

Expand Down
13 changes: 13 additions & 0 deletions tslang/lib/TypeScript/MLIRGenVariables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
Loading
Loading