diff --git a/TSLANG_ISSUES.md b/TSLANG_ISSUES.md index a67bbe141..f3b6229fc 100644 --- a/TSLANG_ISSUES.md +++ b/TSLANG_ISSUES.md @@ -435,6 +435,50 @@ array spec's ruling T4-D accepts for a parameter. In the sweep, 4 errors less (8 keep others. Regression tests `tslang/test/tester/tests/00tslang_issue_62.ts` (every model) and `own/own_err_borrowed_array_pushed_returned.ts`. +63 (a lazy getter that fills an empty field with a C call in between: `if (!this.cachedVertexBuffer) +this.cachedVertexBuffer = new NriBuffer(Nri_GetGltfMeshVertexBuffer(this.handle));` in `core/nri.ts`, which +made every call of such a getter one that "may release or overwrite" whatever the caller borrowed) is fixed. +A fill of an empty field (TSLANG 47) may not run a call that can reach the field in between, and an imported +C function could reach anything. One declared `@nodrops`, with no export, import or dllimport, not variadic, +and given and returning only numbers, strings, `null`/`undefined` or `@cpp` handles (or unions of them), runs +no function of this module and gets nothing that leads to one: the fill now holds across it. Regression test +`tslang/test/tester/tests/00tslang_issue_63.ts` (every model; it also holds 64 and 65). + +64 (a value moved into a field of an object already pushed into an array, then used: `const queuedFrame = +new QueuedFrame(); this.queuedFrames.push(queuedFrame); queuedFrame.pbrView = pbrView; if (!pbrView) ...` in +`examples/nri/simple_pbr_nri.ts`, refused "is used here after its value was moved") is fixed. TSLANG 50's +borrow of the field the value went into is now also taken after the push, and for a value built by `new` +in one arm of a `?:` (its construction's own uses are not uses of the merge). Popping or splicing the +object out before the use, or overwriting the field, is still refused. Regression test +`own/own_err_pushed_object_field_popped.ts`. + +65 (a value moved into an element, or a local's value moved into a field or an element, then used: `let +pipeline = null; if (loaded) pipeline = create(); this.baselinePipelines[logicOp] = pipeline; return +pipeline;` in `examples/nri/logic_op_dynamic_state_nri.ts`) is fixed. The use borrows the element (or field) +from the store on, as TSLANG 50 does for a fresh value, when the local's slot is not assigned again after the +store and the store does not run again (a loop). The element reset or popped before the use, or the store in +a loop, is still refused. Regression test `own/own_err_local_stored_element_reset.ts`. + +With 63-65 in, errors in the sweep went from 807 to 767 (21 sources have fewer; `simple_pbr_nri.ts` 6 to 1), +and no source has more; 119 sources still fail, each for other errors as well. + +66 (a `let` borrowing a field, then given a fresh value on some paths: `let dir = this.dir; ... if (yaw != 0.0 +|| pitch != 0.0) { dir = rotateAround(...); } ... this.dir = normalize(dir);` in +`core/donut_first_person_camera.ts`'s `animate`, refused "borrows a field and cannot be assigned; -mm=own +cannot prove that yet") is fixed. The `let` would own its value on one path and borrow it on the other, and +-mm=own has no flag to release it on one only. Where the dry run finds such a `let` (initialized from a +place, only read, assigned and released), each fresh value it is given goes into a slot of its own, released +where the `let` is (an assignment of another fresh value, the scope's end, an unwind), and the `let` is given +a borrow of that slot's value; it then holds only borrows, as TSLANG 56's do. Each borrow of the slot is +checked like any other: it may not be stored, returned or captured, nor used after the slot's value is +released. The `let` given its field on the other paths stays bounded by the field, so the field overwritten +before a use is still refused. In the sweep, the camera module compiles, and with its facts the examples that +call it do too: errors went from 767 to 664, and failing sources from 119 to 118 (`async_compute_bloom_nri.ts`, +`multi_draw_indirect_nri.ts` and `terrain_tessellation_nri.ts` 8 errors to 1, both `ray_tracing_*_nri.ts` +ports 25 to 6), and no source has more. Regression tests `tslang/test/tester/tests/00tslang_issue_66.ts` +(every model; loops, a class-typed `let`, a throw), `own/own_err_split_let_stored.ts` and +`own/own_err_split_let_field_overwritten.ts`. + For native_ts_graphics, from the same sweep (its code, not the compiler): - `core/ray_tracing_extended_scene.ts`: the exported classes' constructors keep their array parameters (`this.origin = origin`), which an exported constructor has to say: `@keeps("origin", "direction")` @@ -443,9 +487,9 @@ For native_ts_graphics, from the same sweep (its code, not the compiler): - `examples/dynamic_primitive_clipping.ts:223`, `desc.setDepthClip(clip)` with `clip` the loop's `int`, fails under every model since #846 (a number is not a `boolean` argument, TS2345). alpha127 still takes it; the next release won't. `desc.setDepthClip(clip != 0)` fixes it. -- Under `-mm=own`, 4 `core/` modules do not compile yet (`donut_first_person_camera.ts`, - `nri_donut_textures.ts`, `ray_tracing_extended_scene.ts`, `rtxpt_game.ts`), and nor do many examples, - which #310 did not port: with every facts object built and 50-62 in, 119 of the 276 sources fail. The - largest group left is "borrows a field but is used here after it may be released or overwritten" (about - 200 sites), then "used here after its value was moved" (37: mostly a value a pushed object or an +- Under `-mm=own`, 3 `core/` modules do not compile yet (`nri_donut_textures.ts`, + `ray_tracing_extended_scene.ts`, `rtxpt_game.ts`), and nor do many examples, + which #310 did not port: with every facts object built and 50-66 in, 118 of the 276 sources fail (664 errors). The + largest group left is "borrows a field but is used here after it may be released or overwritten" (396, + 93 of them `this.sample.run` callbacks), then "used here after its value was moved" (58: mostly a value a pushed object or an earlier call keeps, then returned or used). diff --git a/tslang/include/TypeScript/Defines.h b/tslang/include/TypeScript/Defines.h index 6fc2adf85..83d9a0dd7 100644 --- a/tslang/include/TypeScript/Defines.h +++ b/tslang/include/TypeScript/Defines.h @@ -56,6 +56,8 @@ // `ts.RetainSlot` to pair the release with. The ownership verifier reads this attribute as the // retain it stands in for. #define OWNED_LOCAL_CONSUMED_ATTR_NAME "__owned_consumed" +// the owning slot that holds the fresh values a `let` borrowing a field is given (TSLANG 66) +#define OWN_LET_HOLDER_ATTR_NAME "__own_let_holder" // Marks an OWNED_RESULT_ATTR_NAME operation whose reference some receiver has taken over, so the // +1 it produced is now somebody's to give back. Set at each of the receiving sites (ยง9.25) and diff --git a/tslang/lib/TypeScript/OwnershipFacts.h b/tslang/lib/TypeScript/OwnershipFacts.h index 572f5ba31..d346397ea 100644 --- a/tslang/lib/TypeScript/OwnershipFacts.h +++ b/tslang/lib/TypeScript/OwnershipFacts.h @@ -647,10 +647,32 @@ inline bool isFreshMerge(mlir::BlockArgument argument, unsigned depth = 0) continue; } - // the fresh value and the views it reaches the branch through, used by nothing else + // the fresh value and the views it reaches the branch through, used by nothing else - but + // what makes an object `ts.New` gives (TSLANG 64: `c ? new V(i) : null`): its vtable's store + // into field 0 and the call of its constructor + auto constructs = [](mlir::Value value, mlir::Operation *user) { + if (!value.getDefiningOp()) + { + return false; + } + + if (auto propertyRefOp = mlir::dyn_cast(user)) + { + return propertyRefOp.getPosition() == 0 && llvm::all_of(propertyRefOp->getUsers(), [&](mlir::Operation *fieldUser) { + auto storeOp = mlir::dyn_cast(fieldUser); + return storeOp && storeOp.getReference() == propertyRefOp.getResult(); + }); + } + + auto callOp = mlir::dyn_cast(user); + return callOp && callOp->getNumOperands() > 0 && callOp->getOperand(0) == value && + callOp.getCallee().ends_with(".constructor") && + llvm::count(callOp->getOperands(), value) == 1; + }; for (auto value = incoming;;) { - if (!value.hasOneUse()) + auto uses = llvm::count_if(value.getUsers(), [&](mlir::Operation *user) { return !constructs(value, user); }); + if (uses != 1) { return false; } diff --git a/tslang/lib/TypeScript/OwnershipInferenceBorrowers.cpp b/tslang/lib/TypeScript/OwnershipInferenceBorrowers.cpp index 7e48140fe..fb5d640a6 100644 --- a/tslang/lib/TypeScript/OwnershipInferenceBorrowers.cpp +++ b/tslang/lib/TypeScript/OwnershipInferenceBorrowers.cpp @@ -249,17 +249,21 @@ namespace own_inference return true; } + bool OwnershipInferencePass::letBorrows(mlir::Value value) + { + // a read of a place or of a parameter - or of a split `let`'s holder (TSLANG 66) + auto root = rootOf(value); + auto rootLoad = root.getDefiningOp(); + auto slot = rootLoad ? rootLoad.getReference().getDefiningOp() : mlir_ts::VariableOp(); + int index = -1; + return placeReadOf(root) || (slot && (isParameterSlot(slot, index) || slot->hasAttr(OWN_LET_HOLDER_ATTR_NAME))); + } + bool OwnershipInferencePass::borrowsOnly(mlir_ts::VariableOp varOp) { // `let layout = this.computeLayout; if (fp16) layout = this.computeLayoutFP16;` (TSLANG 56): // an owning `let` given only reads of places and parameters, each a borrow of its own - auto borrowed = [&](mlir::Value value) { - auto root = rootOf(value); - auto rootLoad = root.getDefiningOp(); - auto slot = rootLoad ? rootLoad.getReference().getDefiningOp() : mlir_ts::VariableOp(); - int index = -1; - return placeReadOf(root) || (slot && isParameterSlot(slot, index)); - }; + auto borrowed = [&](mlir::Value value) { return letBorrows(value); }; if (!varOp.getInitializer() || !isOwningVariable(varOp) || varOp.getCaptured().value_or(false) || !borrowed(varOp.getInitializer())) @@ -390,6 +394,155 @@ namespace own_inference return nullptr; } + void OwnershipInferencePass::splitBorrowingLets() + { + // `let dir = this.dir; if (turn) dir = rotate(dir); this.dir = normalize(dir);` (TSLANG 66): each + // fresh value the `let` is given goes into a holder, an owning slot of its own released where the + // `let` is, and the `let` borrows it from the holder - as it borrows the field + for (auto *candidate : splitLetCandidates) + { + auto varOp = mlir::cast(candidate); + auto slot = varOp.getResult(); + if (!varOp.getInitializer() || !letBorrows(varOp.getInitializer()) || varOp.getCaptured().value_or(false)) + { + continue; + } + + llvm::SmallVector fresh; + llvm::SmallVector exits; + auto splits = true; + for (auto *user : slot.getUsers()) + { + if (mlir::isa(user)) + { + continue; + } + + // the release of its old value before an assignment, or of its last at a way out + if (mlir::isa(user)) + { + auto next = mlir::dyn_cast_or_null(user->getNextNode()); + if (!next || next.getReference() != slot) + { + exits.push_back(user); + } + + continue; + } + + auto storeOp = mlir::dyn_cast(user); + if (!storeOp || storeOp.getReference() != slot) + { + splits = false; + break; + } + + if (letBorrows(storeOp.getValue())) + { + continue; + } + + auto release = mlir::dyn_cast_or_null(storeOp->getPrevNode()); + if (!release || release->getOperand(0) != slot) + { + splits = false; + break; + } + + fresh.push_back(storeOp); + } + + if (!splits || fresh.empty()) + { + continue; + } + + auto type = mlir::cast(slot.getType()).getElementType(); + mlir::OpBuilder builder(varOp); + builder.setInsertionPointAfter(varOp); + auto empty = builder.create(varOp.getLoc(), type); + auto holder = builder.create(varOp.getLoc(), slot.getType(), empty, varOp.getCapturedAttr(), + mlir::IntegerAttr()); + holder->setAttr(OWNED_LOCAL_ATTR_NAME, builder.getUnitAttr()); + holder->setAttr(OWN_LET_HOLDER_ATTR_NAME, builder.getUnitAttr()); + for (auto storeOp : fresh) + { + storeOp->getPrevNode()->setOperand(0, holder.getResult()); + storeOp.getReferenceMutable().assign(holder.getResult()); + builder.setInsertionPointAfter(storeOp); + auto read = builder.create(storeOp.getLoc(), type, holder.getResult()); + builder.create(storeOp.getLoc(), read, slot); + } + + for (auto *exit : exits) + { + builder.setInsertionPointAfter(exit); + builder.create(exit->getLoc(), holder.getResult()); + } + } + } + + void OwnershipInferencePass::checkLetHolders(llvm::SetVector &toErase) + { + getFunction()->walk([&](mlir_ts::VariableOp holder) { + if (!holder->hasAttr(OWN_LET_HOLDER_ATTR_NAME)) + { + return; + } + + auto ends = slotEnds(holder.getResult()); + for (auto *user : holder.getResult().getUsers()) + { + auto readOp = mlir::dyn_cast(user); + if (!readOp || !readOp->hasOneUse()) + { + continue; + } + + // the `let` given the holder's value: its reads up to its next assignment borrow it + auto storeOp = mlir::dyn_cast(*readOp->getUsers().begin()); + auto borrower = storeOp ? storeOp.getReference().getDefiningOp() : mlir_ts::VariableOp(); + if (!borrower || storeOp.getValue() != readOp.getResult()) + { + continue; + } + + auto name = varName(borrower); + llvm::SmallVector uses; + llvm::SmallVector bookkeeping; + // (a `let` declared in a loop: its declaration starts it afresh) + mlir::Operation *declaration = borrower; + if (auto *kept = collectBorrowerUses(borrower, declaration, uses, bookkeeping, storeOp)) + { + if (isAssignmentOf(kept, borrower)) + { + reportBorrowerAssigned(kept, name, name); + } + else + { + reportBorrowEscapes(kept, name, name); + } + + return; + } + + for (auto *end : ends) + { + for (auto &use : uses) + { + if (reachableAfter(end, use.op, use.kills)) + { + reportBorrowOutlives(use.op, end, name, name); + return; + } + } + } + + toErase.insert(bookkeeping.begin(), bookkeeping.end()); + } + }); + } + bool OwnershipInferencePass::isAssignmentOf(mlir::Operation *op, mlir_ts::VariableOp varOp) { auto storeOp = mlir::dyn_cast(op); diff --git a/tslang/lib/TypeScript/OwnershipInferencePass.cpp b/tslang/lib/TypeScript/OwnershipInferencePass.cpp index cea770bc2..a3edc5be9 100644 --- a/tslang/lib/TypeScript/OwnershipInferencePass.cpp +++ b/tslang/lib/TypeScript/OwnershipInferencePass.cpp @@ -38,8 +38,11 @@ namespace own_inference if (!passed) { makeStringCopies(toErase); + splitBorrowingLets(); } + splitLetCandidates.clear(); + for (auto &[op, name] : dryRunAttrs) { op->removeAttr(name); @@ -352,6 +355,7 @@ namespace own_inference checkCannotShare(); checkBorrowingRecords(); checkRecordResults(); + checkLetHolders(toErase); toErase.insert(fieldBorrowReleases.begin(), fieldBorrowReleases.end()); if (dryRun) diff --git a/tslang/lib/TypeScript/OwnershipInferencePass.h b/tslang/lib/TypeScript/OwnershipInferencePass.h index 5abb515a8..1292f29f8 100644 --- a/tslang/lib/TypeScript/OwnershipInferencePass.h +++ b/tslang/lib/TypeScript/OwnershipInferencePass.h @@ -174,6 +174,15 @@ class OwnershipInferencePass // Strings as values (spec 22.3): each string retain the dry run did not erase is rewritten, where // its one use can be found, into a copy for that use. void makeStringCopies(const llvm::SetVector &erased); + // `let dir = this.dir; if (turn) dir = rotate(dir); this.dir = normalize(dir);` (TSLANG 66): a `let` + // borrowing a field, then given a fresh value, the dry run found it could not be assigned. The fresh + // values go into an owning slot of their own (its holder), released where the `let` is, and the + // `let` borrows the holder's value: it then holds only borrows (borrowsOnly). + llvm::SetVector splitLetCandidates; + void splitBorrowingLets(); + // The checks of a split `let`'s borrows of its holder's values: none outlives the holder's value + // nor escapes. + void checkLetHolders(llvm::SetVector &toErase); // The value a parameter of this function is, when its callers were told this function keeps it // or returns a borrow of it (spec 15): copying it would leak what the caller gave up. @@ -295,6 +304,8 @@ class OwnershipInferencePass // An owning `let` given only reads of places and parameters, and assigned at least once // (TSLANG 56): each value it is given is a borrow until the next assignment. bool borrowsOnly(mlir_ts::VariableOp varOp); + // Is `value` a read a `let` given it only borrows (borrowsOnly)? + bool letBorrows(mlir::Value value); // The `let` (borrowsOnly) that `user` assigns `value` to - the store itself, or rc's retain right // before it - or null. mlir_ts::VariableOp borrowingAssignmentOf(mlir::Operation *user, mlir::Value value); diff --git a/tslang/lib/TypeScript/OwnershipInferencePlaceReads.cpp b/tslang/lib/TypeScript/OwnershipInferencePlaceReads.cpp index 0f98cc3b9..cb9f1c104 100644 --- a/tslang/lib/TypeScript/OwnershipInferencePlaceReads.cpp +++ b/tslang/lib/TypeScript/OwnershipInferencePlaceReads.cpp @@ -973,6 +973,12 @@ namespace own_inference if (!kept && isAssignmentOf(letKept, letKeeps)) { + // a candidate for a holder of its fresh values (TSLANG 66) + if (dryRun) + { + splitLetCandidates.insert(letKeeps); + } + reportPlaceBorrowerAssigned(letKept, varName(letKeeps), place); } else diff --git a/tslang/lib/TypeScript/OwnershipSignatureFieldLocals.cpp b/tslang/lib/TypeScript/OwnershipSignatureFieldLocals.cpp index 1eb6bbe60..029a93e42 100644 --- a/tslang/lib/TypeScript/OwnershipSignatureFieldLocals.cpp +++ b/tslang/lib/TypeScript/OwnershipSignatureFieldLocals.cpp @@ -618,10 +618,78 @@ namespace own_signature // where each value moved by the steps below lives after the move: the move, and the read // of its new place made right after it llvm::DenseMap>> movedTo; + // Run again after the pushes below (TSLANG 64): `const q = new Frame(); this.frames.push(q); + // q.view = view; if (!view) ...` - once the push makes `q` a read of the array's last + // element, `q.view` is a field of an object that can be read again. Each store once. + llvm::DenseSet followedStores; + // The uses after `store` of the value it moves: those of `root` itself, the store + // dominates; or, when `root` is a read of an owning local every other assignment of which + // comes before the store (TSLANG 65: `let pipeline = null; if (ok) pipeline = make(); this. + // pipelines[i] = pipeline; return pipeline;`), every later read of the local the store + // dominates. False when the value is neither. + auto usesAfterMove = [&](mlir::Operation *store, mlir::Value root, llvm::SmallVectorImpl &uses) { + auto loadOp = root.getDefiningOp(); + auto slot = loadOp ? loadOp.getReference().getDefiningOp() : mlir_ts::VariableOp(); + if (!slot) + { + if (!ownValue(root, root.getDefiningOp())) + { + return false; + } + + for (auto &use : root.getUses()) + { + auto *user = use.getOwner(); + if (user != store && !mlir::isa(user) && dominance.properlyDominates(store, user)) + { + uses.push_back(&use); + } + } + + return true; + } + + // run once (not in a loop), so an assignment the store does not dominate runs before it + // or on a way that does not reach the reads after it + if (!own_facts::isOwningVariable(slot) || slot.getCaptured().value_or(false) || slot->getParentRegion() != body || + mayRunAgain(store)) + { + return false; + } + + for (auto *user : slot.getResult().getUsers()) + { + if (auto assignment = mlir::dyn_cast(user); + assignment && assignment.getReference() == slot.getResult() && dominance.dominates(store, assignment)) + { + return false; + } + + if (!mlir::isa(user)) + { + return false; // its address taken: anything may change it + } + } + + for (auto *user : slot.getResult().getUsers()) + { + if (user != loadOp.getOperation() && mlir::isa(user) && dominance.properlyDominates(store, user)) + { + for (auto &use : user->getResult(0).getUses()) + { + uses.push_back(&use); + } + } + } + + return true; + }; + auto followFieldStores = [&]() { llvm::SmallVector movedStores; body->walk([&](mlir_ts::StoreOp storeOp) { auto field = storeOp.getReference().getDefiningOp(); - if (field && objectReadableRightAfter(field)) + if (field && objectReadableRightAfter(field) && !followedStores.contains(storeOp)) { movedStores.push_back(storeOp); } @@ -629,6 +697,7 @@ namespace own_signature for (auto fieldStore : movedStores) { + followedStores.insert(fieldStore); auto root = stripViews(fieldStore.getValue()); auto *def = root.getDefiningOp(); auto field = fieldStore.getReference().getDefiningOp(); @@ -640,22 +709,14 @@ namespace own_signature mlir::isa(fieldType); // a string and the like are copied into the field, and a Shared handle counted: // the value stays the function's own, not moved - if (!ownValue(root, def) || !own_facts::ownsHeap(root) || - own_facts::isCopyableValueAt(fieldStore, root.getType()) || - MLIRTypeHelper::isSharedHandleType(root.getType()) || (!narrowed && !fieldReadsAs(field, root.getType()))) - { - continue; - } - + // (a read of a local: TSLANG 65, usesAfterMove) llvm::SmallVector uses; - for (auto &use : root.getUses()) + (void)def; + if (!own_facts::ownsHeap(root) || own_facts::isCopyableValueAt(fieldStore, root.getType()) || + MLIRTypeHelper::isSharedHandleType(root.getType()) || (!narrowed && !fieldReadsAs(field, root.getType())) || + !usesAfterMove(fieldStore, root, uses)) { - auto *user = use.getOwner(); - if (user != fieldStore.getOperation() && !mlir::isa(user) && - dominance.properlyDominates(fieldStore.getOperation(), user)) - { - uses.push_back(&use); - } + continue; } // made even with no use left: an argument kept by this object may read through it @@ -676,6 +737,53 @@ namespace own_signature movedTo[root].push_back({fieldStore.getOperation(), read}); } + // The same for a value stored into an element of an array that is a field or a local nothing + // assigns (TSLANG 65): `this.baselinePipelines[logicOp] = pipeline; return pipeline;`. The + // element's reference is there, and nothing runs between: a load of it right after the + // store reads what was stored - a borrow of the element. + llvm::SmallVector elementStores; + body->walk([&](mlir_ts::StoreOp storeOp) { + auto elementRefOp = storeOp.getReference().getDefiningOp(); + if (!elementRefOp || followedStores.contains(storeOp)) + { + return; + } + + auto arrayLoad = elementRefOp->getOperand(0).getDefiningOp(); + auto arrayField = arrayLoad ? arrayLoad.getReference().getDefiningOp() : mlir_ts::PropertyRefOp(); + if (arrayLoad && ((arrayField && objectReadableRightAfter(arrayField)) || isUnassignedSlot(arrayLoad.getReference()))) + { + elementStores.push_back(storeOp); + } + }); + + for (auto elementStore : elementStores) + { + followedStores.insert(elementStore); + auto root = stripViews(elementStore.getValue()); + auto elementType = mlir::cast(elementStore.getReference().getType()).getElementType(); + llvm::SmallVector uses; + if (elementType != root.getType() || !own_facts::ownsHeap(root) || + own_facts::isCopyableValueAt(elementStore, root.getType()) || MLIRTypeHelper::isSharedHandleType(root.getType()) || + !usesAfterMove(elementStore, root, uses) || uses.empty()) + { + continue; + } + + mlir::OpBuilder builder(elementStore->getContext()); + builder.setInsertionPointAfter(elementStore); + mlir::Value read = builder.create(elementStore.getLoc(), elementType, elementStore.getReference()); + for (auto *use : uses) + { + use->set(read); + } + + movedTo[root].push_back({elementStore.getOperation(), read}); + } + }; + + followFieldStores(); + // The same for a value pushed into an array that is a field (TSLANG 52): `this.textures.push( // texture); this.views.push(texture.createView(...))`. After the push the value is the // array's last element, so each use the push dominates reads `array[newLength - 1]`, @@ -742,6 +850,8 @@ namespace own_signature movedTo[root].push_back({pushOp.getOperation(), read}); } + followFieldStores(); + // A value of the function's own given to a constructor that keeps it in a field // (TSLANG 53): `const app = new App(sample); sample.use();`, or the object pushed on // (`this.frames.push(new Frame(viewSet)); viewSet.update(...)`). After the call the diff --git a/tslang/lib/TypeScript/OwnershipSignaturePass.cpp b/tslang/lib/TypeScript/OwnershipSignaturePass.cpp index 0e40f2edd..393da39c8 100644 --- a/tslang/lib/TypeScript/OwnershipSignaturePass.cpp +++ b/tslang/lib/TypeScript/OwnershipSignaturePass.cpp @@ -1695,12 +1695,22 @@ namespace own_signature auto field = releaseSlotOp.getSlot().getDefiningOp(); return field && fillsEmptyField(releaseSlotOp, [&](mlir::Operation *op) { auto *call = callOf.lookup(op); - // another module's facts count its fills as no drops: they cannot clear a call - if (!call || call->callees.imported) + if (!call) { return true; } + // another module's facts count its fills as no drops: they cannot clear a call. + // A C function declared `@nodrops` runs no function of this module, and given only + // numbers, strings and `@cpp` handles it reaches no field of an object here at all: + // `if (!this.cached) this.cached = new B(C_Get(this.handle))`, a lazy getter in + // native_ts_graphics' core/nri.ts (TSLANG 63) + if (call->callees.imported) + { + auto funcOp = functions.lookup(call->callees.importedName); + return !funcOp || !foreignOnlyDeclaration(funcOp); + } + if (!callMayDrop(call->callees)) { return false; diff --git a/tslang/lib/TypeScript/OwnershipSignaturePass.h b/tslang/lib/TypeScript/OwnershipSignaturePass.h index 7d67ff8d0..958d93547 100644 --- a/tslang/lib/TypeScript/OwnershipSignaturePass.h +++ b/tslang/lib/TypeScript/OwnershipSignaturePass.h @@ -696,6 +696,9 @@ class OwnershipSignaturePass : public mlir::PassWrapper(type) || scalarType(type); }); } + bool OwnershipSignaturePass::foreignOnlyDeclaration(mlir_ts::FuncOp funcOp) + { + // a number, a string, a `@cpp` handle, or one of them or null: nothing that reaches an object + // of this module + std::function foreign = [&](mlir::Type type) { + if (scalarType(type) || mlir::isa(type)) + { + return true; + } + + if (auto classType = mlir::dyn_cast(type)) + { + auto storage = mlir::dyn_cast(classType.getStorageType()); + return storage && storage.isCpp(); + } + + if (auto optionalType = mlir::dyn_cast(type)) + { + return foreign(optionalType.getElementType()); + } + + if (auto unionType = mlir::dyn_cast(type)) + { + return llvm::all_of(unionType.getTypes(), foreign); + } + + return false; + }; + + auto functionType = funcOp.getFunctionType(); + return funcOp.isDeclaration() && funcOp->hasAttr(OWN_DECLARED_NO_DROPS_ATTR_NAME) && !funcOp->hasAttr("export") && + !funcOp->hasAttr("import") && !funcOp->hasAttr(DLL_IMPORT) && !functionType.isVarArg() && + llvm::all_of(functionType.getInputs(), foreign) && llvm::all_of(functionType.getResults(), foreign); + } + bool OwnershipSignaturePass::scalarDeclaration(mlir_ts::FuncOp funcOp) { return funcOp.isDeclaration() && !funcOp->hasAttr("export") && !funcOp->hasAttr("import") && diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index e2e98a39c..b824c8243 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -1918,11 +1918,13 @@ endforeach() # TSLANG_ISSUES 60: a call that reaches an overridable method names it where a borrow ends # TSLANG_ISSUES 61: an array global nothing assigns is immortal # TSLANG_ISSUES 62: an array changed through a local that borrows its field +# TSLANG_ISSUES 63-65: a lazy getter with a @nodrops C call is a fill; a field of a pushed object; a local or element store then used +# TSLANG_ISSUES 66: a `let` borrowing a field, given fresh values on some paths, borrows them from a slot of their own foreach(issues_mm gc rc none own) foreach(issues_test 00tslang_issue_44 00tslang_issue_45 00tslang_issue_46 00tslang_issue_47 00tslang_issue_48 00tslang_issue_49 00tslang_issue_50 00tslang_issue_52 00tslang_issue_53 00tslang_issue_54 00tslang_issue_55 00tslang_issue_56 00tslang_issue_57 00tslang_issue_58 00tslang_issue_59 00tslang_issue_60 - 00tslang_issue_61 00tslang_issue_62) + 00tslang_issue_61 00tslang_issue_62 00tslang_issue_63 00tslang_issue_66) tslang_add_test(NAME test-jit-${issues_test}-${issues_mm} COMMAND test-runner -jit -mm=${issues_mm} "${PROJECT_SOURCE_DIR}/test/tester/tests/${issues_test}.ts") tslang_add_test(NAME test-compile-${issues_test}-${issues_mm} COMMAND test-runner -mm=${issues_mm} "${PROJECT_SOURCE_DIR}/test/tester/tests/${issues_test}.ts") endforeach() @@ -5194,7 +5196,7 @@ set(own_error_cases "own_err_merge_root_assigned|:36:12: error: '[^']*' borrows a field but is used here after it may be released or overwritten" "own_err_container_read_stored|:27:5: error: '[^']*' borrows a field and cannot be stored, returned or captured" "own_err_field_let_overwritten|:28:11: error: '[^']*' borrows a field but is used here after it may be released or overwritten" - "own_err_field_let_assigned|:26:5: error: '[^']*' borrows a field and cannot be assigned" + "own_err_field_let_assigned|:12:5: error: '[^']*' borrows a field and cannot be assigned" "own_err_call_drops_indirectly|:24:11: error: '[^']*' borrows a field but is used here after it may be released or overwritten" "own_err_call_given_borrow|:21:5: error: '[^']*' borrows 'g[.]c' but is used here after it may be released or overwritten" "own_err_call_this_borrow|:19:5: error: '[^']*' borrows 'g[.]c' but is used here after it may be released or overwritten" @@ -5479,7 +5481,11 @@ set(own_error_cases "own_err_element_field_moved_replaced|:11:12: error: '[^']*' borrows an element but is used here after it may be released or overwritten" "own_err_overridable_reached|:34:13: note: 'Prop.tick' calls 'setNodeTransform', which calls 'World.setTransform'" "own_err_immortal_array_element_overwritten|:15:12: error: '[^']*' borrows 'G\\[\\]' but is used here after it may be released or overwritten" - "own_err_borrowed_array_pushed_returned|:14:5: error: '[^']*' borrows a field and cannot be stored, returned or captured") + "own_err_borrowed_array_pushed_returned|:14:5: error: '[^']*' borrows a field and cannot be stored, returned or captured" + "own_err_local_stored_element_reset|:22:12: error: '[^']*' borrows an element but is used here after it may be released or overwritten" + "own_err_pushed_object_field_popped|:24:16: error: '[^']*' borrows a field but is used here after it may be released or overwritten" + "own_err_split_let_stored|:14:9: error: '[^']*' borrows a field and cannot be stored, returned or captured" + "own_err_split_let_field_overwritten|:14:16: error: '[^']*' borrows a field but is used here after it may be released or overwritten") foreach(own_error_case ${own_error_cases}) string(FIND "${own_error_case}" "|" own_error_sep) string(SUBSTRING "${own_error_case}" 0 ${own_error_sep} own_error_name) diff --git a/tslang/test/tester/own/own_err_field_let_assigned.ts b/tslang/test/tester/own/own_err_field_let_assigned.ts index 36755120d..1b93d6358 100644 --- a/tslang/test/tester/own/own_err_field_let_assigned.ts +++ b/tslang/test/tester/own/own_err_field_let_assigned.ts @@ -1,28 +1,15 @@ -// -mm=own, phase 3 rejects: `c` borrows `h.c`, so it cannot be assigned. -class C { - v: number[] = []; - constructor(public x: number) {} -} - -function churn() { - const junk: C[] = []; - for (let i = 0; i < 64; i++) { - const c = new C(999); - c.v.push(99); - junk.push(c); - } -} - +// -mm=own, phase 3 rejects: `a` borrows `h.a`, and is changed in place after it is given an array of its +// own (`a.push`, through its slot). A `let` that is only read after a fresh value (TSLANG 66) borrows it +// from a slot of its own instead; one changed through its slot is not split, so it cannot be assigned. class H { - c: C = new C(0); - n: number = 0; - s: string = "a"; + a: number[] = [1.0]; } function main() { const h = new H(); - let c = h.c; - print(c.x); - c = new C(5); - print(c.x); + let a = h.a; + print(a.length); + a = [2.0, 3.0]; + a.push(4.0); + print(a.length); } diff --git a/tslang/test/tester/own/own_err_local_stored_element_reset.ts b/tslang/test/tester/own/own_err_local_stored_element_reset.ts new file mode 100644 index 000000000..8a50c2080 --- /dev/null +++ b/tslang/test/tester/own/own_err_local_stored_element_reset.ts @@ -0,0 +1,25 @@ +// TSLANG_ISSUES 65: a local's value moved into an element, then used, borrows the element, so the +// element given another value before the use frees it. +class V { + n: int; + constructor(n: int) { + this.n = n; + } +} + +class H { + items: (V | null)[] = [null, null]; +} + +export function f(h: H, c: boolean): int { + let m: V | null = null; + if (c) { + m = new V(1); + } + + h.items[0] = m; + h.items[0] = null; + return m ? m.n : 0; +} + +function main() { print(f(new H(), true)); } diff --git a/tslang/test/tester/own/own_err_pushed_object_field_popped.ts b/tslang/test/tester/own/own_err_pushed_object_field_popped.ts new file mode 100644 index 000000000..434f245df --- /dev/null +++ b/tslang/test/tester/own/own_err_pushed_object_field_popped.ts @@ -0,0 +1,27 @@ +// TSLANG_ISSUES 64: a value moved into a field of an object already pushed into an array borrows that +// field, so the object popped before the use frees it. +class V { + n: int; + constructor(n: int) { + this.n = n; + } +} + +class F { + v: V | null = null; +} + +class H { + frames: F[] = []; +} + +export function f(h: H, c: boolean): int { + const fr = new F(); + h.frames.push(fr); + const v = c ? new V(2) : null; + fr.v = v; + h.frames.pop(); + return v ? v.n : 0; +} + +function main() { print(f(new H(), true)); } diff --git a/tslang/test/tester/own/own_err_split_let_field_overwritten.ts b/tslang/test/tester/own/own_err_split_let_field_overwritten.ts new file mode 100644 index 000000000..95f8732e2 --- /dev/null +++ b/tslang/test/tester/own/own_err_split_let_field_overwritten.ts @@ -0,0 +1,21 @@ +// TSLANG_ISSUES 66: a `let` borrowing a field, given a fresh value on some paths only, is still the +// field's on the others: the field overwritten frees it. +function rot(v: number[], a: number) { + return [v[0] * a, v[1], v[2]]; +} +class Cam { + dir: number[] = [1.0, 0.0, 1.0]; + turn(c: boolean) { + let dir = this.dir; + if (c) { + dir = rot(dir, 2.0); + } + this.dir = [5.0, 0.0, 0.0]; + return dir[0]; + } +} +function main() { + const c = new Cam(); + print(c.turn(false)); + print("done."); +} diff --git a/tslang/test/tester/own/own_err_split_let_stored.ts b/tslang/test/tester/own/own_err_split_let_stored.ts new file mode 100644 index 000000000..7b2bab955 --- /dev/null +++ b/tslang/test/tester/own/own_err_split_let_stored.ts @@ -0,0 +1,21 @@ +// TSLANG_ISSUES 66: a `let` borrowing a field, or the fresh value it is given on some paths, cannot be +// stored: on the other paths it is the field's. +function rot(v: number[], a: number) { + return [v[0] * a, v[1], v[2]]; +} +class Cam { + dir: number[] = [1.0, 0.0, 1.0]; + other: number[] = []; + turn(c: boolean) { + let dir = this.dir; + if (c) { + dir = rot(dir, 2.0); + } + this.other = dir; + } +} +function main() { + const c = new Cam(); + c.turn(true); + print("done."); +} diff --git a/tslang/test/tester/tests/00tslang_issue_63.ts b/tslang/test/tester/tests/00tslang_issue_63.ts new file mode 100644 index 000000000..8e98d9d48 --- /dev/null +++ b/tslang/test/tester/tests/00tslang_issue_63.ts @@ -0,0 +1,119 @@ +// TSLANG_ISSUES 63-65 (-mm=own), from native_ts_graphics: +// 63: core/nri.ts's lazy getters, `if (!this.cachedVertexBuffer) this.cachedVertexBuffer = new +// NriBuffer(Nri_GetGltfMeshVertexBuffer(this.handle));` - a fill of an empty field (TSLANG 47) with a C +// call in between, which cannot reach the field. The getter destroys nothing, so a borrow held across +// it (`commandBuffer` below) holds. +// 64: examples/nri/simple_pbr_nri.ts, `const queuedFrame = new QueuedFrame(); this.queuedFrames.push( +// queuedFrame); queuedFrame.pbrView = pbrView; if (!pbrView) ...` - a value moved into a field of an +// object that was pushed first. +// 65: examples/nri/logic_op_dynamic_state_nri.ts, `let pipeline = null; if (loaded) pipeline = create(); +// this.baselinePipelines[logicOp] = pipeline; return pipeline;` - a value moved into an element, and +// a local's value moved into a field or an element, then used. + +@nodrops declare function abs(n: int): int; + +class Buf { + constructor(public readonly handle: int) {} +} + +class Mesh { + cached: Buf | null = null; + constructor(public readonly handle: int) {} + + @lends("this") + getBuffer(): Buf { + if (!this.cached) { + this.cached = new Buf(abs(this.handle)); + } + + return this.cached; + } +} + +class Recorder { + count = 0; + record(b: Buf) { + this.count += b.handle; + } +} + +class Ring { + recorders: Recorder[] = [new Recorder(), new Recorder()]; + + @lends("this") + recorder(i: int): Recorder { + return this.recorders[i]; + } +} + +class View { + n: int; + constructor(n: int) { + this.n = n; + } +} + +class Frame { + view: View | null = null; + other: View | null = null; +} + +class App { + ring = new Ring(); + mesh = new Mesh(3); + frames: Frame[] = []; + pipelines: (View | null)[] = [null, null, null]; + current: View | null = null; + + draw(): int { + const recorder = this.ring.recorder(1); + const mesh = this.mesh; + recorder.record(mesh.getBuffer()); + recorder.record(mesh.getBuffer()); + return recorder.count; + } + + addFrame(i: int): int { + const frame = new Frame(); + this.frames.push(frame); + const view = i > 0 ? new View(i) : null; + const other = new View(i + 10); + frame.view = view; + frame.other = other; + if (!view) { + return -1; + } + + return view.n + other.n; + } + + pipeline(index: int, make: boolean): View | null { + let made: View | null = null; + if (make) { + made = new View(index * 100); + } + + this.pipelines[index] = made; + return made; + } + + keep(n: int): int { + let v = new View(n); + if (n > 5) { + v = new View(n * 2); + } + + this.current = v; + return v.n; + } +} + +function main() { + const app = new App(); + assert(app.draw() == 6); + assert(app.addFrame(0) == -1 && app.addFrame(2) == 14 && app.frames.length == 2); + const p = app.pipeline(2, true); + assert(p != null && p.n == 200 && app.pipeline(1, false) == null); + assert(app.keep(3) == 3 && app.keep(7) == 14); + print("done."); +} diff --git a/tslang/test/tester/tests/00tslang_issue_66.ts b/tslang/test/tester/tests/00tslang_issue_66.ts new file mode 100644 index 000000000..0d4f67047 --- /dev/null +++ b/tslang/test/tester/tests/00tslang_issue_66.ts @@ -0,0 +1,68 @@ +// TSLANG_ISSUES 66 (-mm=own), from native_ts_graphics' core/donut_first_person_camera.ts: +// `let dir = this.dir; if (dragging) dir = rotateAround(dir, ...); this.dir = normalize(dir);` - a `let` +// borrowing a field, then given a fresh value on some paths. The fresh values go into a slot of their own, +// released where the `let` is; the `let` borrows them as it borrows the field. + +function rot(v: number[], a: number) { + return [v[0] * a, v[1], v[2]]; +} + +class Box { + constructor(public n: int) {} +} + +function grow(b: Box) { + return new Box(b.n + 1); +} + +class Cam { + dir: number[] = [1.0, 0.0, 1.0]; + box = new Box(1); + loop(n: int) { + let dir = this.dir; + for (let i = 0; i < n; i++) { + dir = rot(dir, 2.0); + } + this.dir = rot(dir, 1.0); + } + inner(n: int): number { + let total = 0.0; + for (let i = 0; i < n; i++) { + let d = this.dir; + if (i % 2 == 0) { + d = rot(d, 3.0); + } + total += d[0]; + } + return total; + } + boxed(c: boolean): int { + let b = this.box; + if (c) { + b = grow(b); + } + return b.n; + } + thrown(c: boolean): int { + let b = this.box; + try { + if (c) { + b = grow(b); + throw "x"; + } + } catch (e) { + return b.n * 10; + } + return b.n; + } +} + +function main() { + const c = new Cam(); + c.loop(3); + assert(c.dir[0] == 8.0); + assert(c.inner(3) == 8.0 * 3 + 8.0 + 8.0 * 3); + assert(c.boxed(true) == 2 && c.boxed(false) == 1); + assert(c.thrown(true) == 20 && c.thrown(false) == 1); + print("done."); +}