diff --git a/tslang/lib/TypeScript/LowerToLLVM.cpp b/tslang/lib/TypeScript/LowerToLLVM.cpp index 755ad4e68..5d2f52930 100644 --- a/tslang/lib/TypeScript/LowerToLLVM.cpp +++ b/tslang/lib/TypeScript/LowerToLLVM.cpp @@ -8877,8 +8877,22 @@ static void populateTypeScriptConversionPatterns(LLVMTypeConverter &converter, m // can sit in that padding: at 32-bit a member's f64 lands in the storage member's padding // after a 4-byte pointer, and it read back as garbage once the union was copied. Without // a tag there is no real union - the members share one base type - and that type is kept - // as it is. - mlir::Type selectedType = needTag ? ltch.getUnionStorageType(type) : ltch.findMaxSizeType(type); + // as it is. `T | null` of a @value T kept without a tag (MLIRTypeHelper::getValueNullNiche) + // is T: with one pointer field T's `{ptr}` is no larger than null's `ptr`, and the largest + // member was null's - a bare pointer the niche code could not extract T's field from. + mlir::Type selectedType; + if (needTag) + { + selectedType = ltch.getUnionStorageType(type); + } + else if (auto valueType = MLIRTypeHelper::getMissingValueElement(type)) + { + selectedType = converter.convertType(valueType); + } + else + { + selectedType = ltch.findMaxSizeType(type); + } LLVM_DEBUG(llvm::dbgs() << "\n!! storage type in union: " << selectedType << "\n size: " << ltch.getTypeAllocSizeInBytes(selectedType) << "\n Tag: " << (needTag ? "yes" : "no") diff --git a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp index 88cb0f42a..356d42089 100644 --- a/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp +++ b/tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp @@ -17,6 +17,7 @@ #include "llvm/ADT/TypeSwitch.h" #include "llvm/Support/Debug.h" +#include #include #include @@ -1663,13 +1664,15 @@ class OwnedReturnConsumptionPass // ...or some branch yields a record literal it built whose field holds a temporary it made // (`c ? { start: this.pos, dir: new V(1) } : ...`): the branch gives the temporary back before // it ends, so the receiver's retain after the `ts.If` - which retains the fields - comes too - // late, and the field is freed under it. The branch retains it first. Not under -mm=own, - // which settles a literal's fields itself. - auto yieldsLiteralOfBranchTemporary = [&](mlir_ts::ResultOp resultOp) { - auto loadOp = producerOf(resultOp->getOperand(0)).getDefiningOp(); + // late, and the field is freed under it. The branch retains it first. A field may hold + // another literal the branch built (`{ one: { h: new H() } }` of a @value type holding a + // @value type), whose fields are looked into the same way. Not under -mm=own, which + // settles a literal's fields itself. + std::function isLiteralOfBranchTemporary = [&](mlir::Value value, + mlir::Region *region) { + auto loadOp = producerOf(value).getDefiningOp(); auto varOp = loadOp ? loadOp.getReference().getDefiningOp() : mlir_ts::VariableOp(); - if (compileOptions.memoryModel == MemoryModelOwn || !varOp || - varOp->getParentRegion() != resultOp->getParentRegion()) + if (!varOp || varOp->getParentRegion() != region) { return false; } @@ -1678,14 +1681,22 @@ class OwnedReturnConsumptionPass auto propertyRefOp = mlir::dyn_cast(user); return propertyRefOp && llvm::any_of(propertyRefOp->getUsers(), [&](mlir::Operation *fieldUser) { auto storeOp = mlir::dyn_cast(fieldUser); - auto stored = storeOp && storeOp.getReference() == propertyRefOp.getResult() - ? producerOf(storeOp.getValue()) - : mlir::Value(); - return stored && stored.getDefiningOp() && ::typescript::mayTakeOverReference(stored) && - stored.getDefiningOp()->getParentRegion() == resultOp->getParentRegion(); + if (!storeOp || storeOp.getReference() != propertyRefOp.getResult()) + { + return false; + } + + auto stored = producerOf(storeOp.getValue()); + return (stored.getDefiningOp() && ::typescript::mayTakeOverReference(stored) && + stored.getDefiningOp()->getParentRegion() == region) || + isLiteralOfBranchTemporary(storeOp.getValue(), region); }); }); }; + auto yieldsLiteralOfBranchTemporary = [&](mlir_ts::ResultOp resultOp) { + return compileOptions.memoryModel != MemoryModelOwn && + isLiteralOfBranchTemporary(resultOp->getOperand(0), resultOp->getParentRegion()); + }; if (llvm::none_of(yields, carriesOwned) && llvm::none_of(yields, borrowsBranchTemporary) && llvm::none_of(yields, readsNamedObjectField) && llvm::none_of(yields, yieldsLiteralOfBranchTemporary)) diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 3b5ddd0e7..cf2b15355 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -615,6 +615,7 @@ tslang_add_test(NAME test-compile-00-object-literal-contextual-lambda COMMAND te tslang_add_test(NAME test-compile-own-00-object-literal-contextual-lambda COMMAND test-runner -mm=own "${PROJECT_SOURCE_DIR}/test/tester/tests/00object_literal_contextual_lambda.ts") tslang_add_test(NAME test-compile-00-interface-field-getter-owned COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_field_getter_owned.ts") tslang_add_test(NAME test-compile-00-conditional-record-literal-temporary COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00conditional_record_literal_temporary.ts") +tslang_add_test(NAME test-compile-00-conditional-nested-record-literal-temporary COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00conditional_nested_record_literal_temporary.ts") tslang_add_test(NAME test-compile-00-interface-print COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_print.ts") tslang_add_test(NAME test-compile-00-class-method-devirtualized COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_method_devirtualized.ts") tslang_add_test(NAME test-compile-00-call-arg-param-field-global COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00call_arg_param_field_global.ts") @@ -1258,6 +1259,7 @@ tslang_add_test(NAME test-jit-00-object-literal-contextual-lambda COMMAND test-r tslang_add_test(NAME test-jit-own-00-object-literal-contextual-lambda COMMAND test-runner -jit -mm=own "${PROJECT_SOURCE_DIR}/test/tester/tests/00object_literal_contextual_lambda.ts") tslang_add_test(NAME test-jit-00-interface-field-getter-owned COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_field_getter_owned.ts") tslang_add_test(NAME test-jit-00-conditional-record-literal-temporary COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00conditional_record_literal_temporary.ts") +tslang_add_test(NAME test-jit-00-conditional-nested-record-literal-temporary COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00conditional_nested_record_literal_temporary.ts") tslang_add_test(NAME test-jit-00-interface-print COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_print.ts") tslang_add_test(NAME test-jit-00-class-method-devirtualized COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_method_devirtualized.ts") tslang_add_test(NAME test-jit-00-call-arg-param-field-global COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00call_arg_param_field_global.ts") @@ -2282,6 +2284,7 @@ set(TSLANG_CORPUS 00object_literal_contextual_lambda.ts 00interface_field_getter_owned.ts 00conditional_record_literal_temporary.ts + 00conditional_nested_record_literal_temporary.ts 00interface_print.ts 00class_method_devirtualized.ts 00call_arg_param_field_global.ts @@ -3401,6 +3404,11 @@ foreach(value_mm gc rc none) tslang_add_test(NAME test-compile-value-missing-niche-${value_mm} COMMAND test-runner -mm=${value_mm} "${PROJECT_SOURCE_DIR}/test/tester/value/value_missing_niche.ts") tslang_add_test(NAME test-jit-value-missing-niche-${value_mm} COMMAND test-runner -jit -mm=${value_mm} "${PROJECT_SOURCE_DIR}/test/tester/value/value_missing_niche.ts") endforeach() +# ...and with that reference as T's only field, where T is no larger than null's pointer +foreach(value_mm gc rc none) + tslang_add_test(NAME test-compile-value-missing-niche-one-field-${value_mm} COMMAND test-runner -mm=${value_mm} "${PROJECT_SOURCE_DIR}/test/tester/value/value_missing_niche_one_field.ts") + tslang_add_test(NAME test-jit-value-missing-niche-one-field-${value_mm} COMMAND test-runner -jit -mm=${value_mm} "${PROJECT_SOURCE_DIR}/test/tester/value/value_missing_niche_one_field.ts") +endforeach() # ...made by `?:` from null and a literal holding a new object, given to `??`, and printed. Not under own, # which cannot yet tell which branch of a `?:` owns a literal it merges. foreach(value_mm gc rc none) diff --git a/tslang/test/tester/tests/00conditional_nested_record_literal_temporary.ts b/tslang/test/tester/tests/00conditional_nested_record_literal_temporary.ts new file mode 100644 index 000000000..60fcdfa60 --- /dev/null +++ b/tslang/test/tester/tests/00conditional_nested_record_literal_temporary.ts @@ -0,0 +1,52 @@ +// 00conditional_record_literal_temporary.ts one level down: each branch of the conditional builds a +// @value literal whose field is another @value literal holding an object the branch makes +// (`{ one: { h: new H(d) } }`). Under rc the branch gave its new object back before the receiver +// after the conditional retained the outer literal, so the object was freed under it (heap +// corruption). The branch now retains the literal it yields when a literal inside it holds a branch +// temporary too. Each value is read after churn() has reused anything freed too early. +class H { + constructor(public x: number) {} +} + +@value +interface One { + readonly h: H; + readonly n: number; +} + +@value +interface Outer { + readonly one: One; + readonly m: number; +} + +function churn() { + const keep: H[] = []; + for (let i = 0; i < 200; i++) keep.push(new H(77777)); + return keep.length; +} + +function pick(d: number): Outer { + return d < 0 ? { one: { h: new H(-d), n: 1 }, m: 2 } : { one: { h: new H(d), n: 3 }, m: 4 }; +} + +function maybe(d: number): Outer | null { + return d < 0 ? null : { one: { h: new H(d), n: 5 }, m: 6 }; +} + +function main() { + let s: number = 0; + for (let i = -2; i < 3; i++) { + const o = pick(i); + churn(); + s += o.one.h.x + o.one.n + o.m; + + const p = maybe(i); + churn(); + if (p) s += p.one.h.x + p.one.n + p.m; + } + + // pick: h.x 2 1 0 1 2 = 6, n+m 3 3 7 7 7 = 27; maybe (i >= 0): h.x 0 1 2 = 3, n+m 11 * 3 = 33 + assert(s == 6 + 27 + 3 + 33, "sum"); + print("done."); +} diff --git a/tslang/test/tester/value/value_missing_niche_one_field.ts b/tslang/test/tester/value/value_missing_niche_one_field.ts new file mode 100644 index 000000000..caab30f66 --- /dev/null +++ b/tslang/test/tester/value/value_missing_niche_one_field.ts @@ -0,0 +1,69 @@ +// value_missing_niche.ts with the reference as the @value type's only field: T's storage `{ptr}` is +// no larger than null's `ptr`, and `T | null` (no tag) was laid out as null's bare pointer, so a +// compare with null crashed tslang and passing T to a `T | null` parameter left an +// unrealized_conversion_cast (TSLANG_ISSUES 22). +class H { + constructor(public x: number) {} + + toString() { + return `H${this.x}`; + } +} + +@value +interface One { + readonly h: H; +} + +@value +class Wrap { + readonly h: H; + constructor(h: H) { + this.h = h; + } +} + +// a @value type holding a one-field @value type +@value +interface Outer { + readonly one: One; +} + +function f(d: number): One | null { + return d < 0 ? null : { h: new H(d) }; +} + +function take(w: Wrap | null): number { + return w ? (w as Wrap).h.x : 0; +} + +function outer(d: number): Outer | null { + if (d < 0) return null; + return { one: { h: new H(d) } }; +} + +let global: One | null = null; + +function main() { + assert(f(-1) == null && !(f(1) == null), "== null"); + assert(f(-1) === null && f(1) !== null, "=== null"); + assert(global == null, "a global null"); + global = f(3); + assert(global != null && (global as One).h.x == 3, "a global set"); + + const set = f(2); + if (set) assert(set.h.x == 2); else assert(false, "set read as null"); + const none = f(-2); + if (none) assert(false, "null read as set"); + + assert(take(new Wrap(new H(5))) == 5 && take(null) == 0, "passed to a T | null parameter"); + const o: One = { h: new H(6) }; + const held: One | null = o; + assert(held != null && held.h.x == 6); + + assert(outer(-1) == null && (outer(4) as Outer).one.h.x == 4, "nested one-field value"); + + assert(`${f(-1)}` == "null" && `${f(7)}` == "{ h: H7 }", "printed"); + + print("done."); +}