@value T | null of a one-pointer T; rc conditional with nested @value literals

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")

tslang/lib/TypeScript/OwnedReturnConsumptionPass.cpp

@@ -17,6 +17,7 @@
 #include "llvm/ADT/TypeSwitch.h"
 #include "llvm/Support/Debug.h"
 
+#include <functional>
 #include <set>
 #include <tuple>
 
@@ -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<mlir_ts::LoadOp>();
+            // 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<bool(mlir::Value, mlir::Region *)> isLiteralOfBranchTemporary = [&](mlir::Value value,
+                                                                                               mlir::Region *region) {
+                auto loadOp = producerOf(value).getDefiningOp<mlir_ts::LoadOp>();
                 auto varOp = loadOp ? loadOp.getReference().getDefiningOp<mlir_ts::VariableOp>() : 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<mlir_ts::PropertyRefOp>(user);
                     return propertyRefOp && llvm::any_of(propertyRefOp->getUsers(), [&](mlir::Operation *fieldUser) {
                                auto storeOp = mlir::dyn_cast<mlir_ts::StoreOp>(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))

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)

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.");
+}

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.");
+}