TSLANG_ISSUES 6: a string returned by C is copied

TSLANG_ISSUES.md

@@ -14,7 +14,7 @@ https://github.com/ASDAlexander77/native_ts_graphics
 | 3 | `u64` `<<` shifted by the count modulo 32 | bug | **fixed** |
 | 4 | `Ref<string>` field with a `null` initializer rejected | question | **answered**: declare it `Ref<string> \| null` |
 | 5 | `@cppvtable` overload naming (C++ overloads need distinct TypeScript names) | open | **addressed** on branch `cppvtable-slot`: `@cpp` + `@slot(N)` (docs/cpp-classes.md) |
-| 6 | C `string` return values keep the raw `char*` (no copy) | open | open, not re-checked |
+| 6 | C `string` return values keep the raw `char*` (no copy) | open | **fixed**: copied at the call (this PR; `Opaque` is the raw pointer) |
 | 7 | `Opaque` compared with `Opaque \| null` crashed the compiler | - | **fixed** by #536 (re-checked with the original drawScene shape) |
 | 8 | A ternary of number literals in arithmetic truncated to the other operand's integer type | - | **fixed** by #560 (still wrong on pre-alpha97) |
 | 9-12 | Bool const arrays, `Math.max`, `f32` array literal, `let x = 0` | - | fixed (#537, DefaultLib#31, works, #544 warning) |
@@ -120,6 +120,11 @@ to TypeScript later; the project now returns pointers to storage that lives as l
 Decide whether this is by design; if so, the C interop docs should say the string must outlive its use,
 otherwise copying on return (or an explicit borrowed annotation) would be safer. Not re-checked.
 
+**Fixed**: worse than stale text - under `-mm=rc` the count was kept in the 8 bytes before the
+pointer, so storing `strchr(base, 'k')` rewrote two characters of `base`. A `string` returned by a
+top-level `declare function` (defined nowhere in the program) or a `@cpp` method is now copied at the
+call; declare the result `Opaque` for the pointer itself (docs/c-cpp-header-import.md).
+
 ### 13. `T` not inferred from a generic tuple alias argument
 
 Found on pre-alpha100, writing `stdVectorSize<T>(items: vector<T>)` for native_ts_graphics' NVRHI

docs/c-cpp-header-import.md

@@ -21,13 +21,20 @@ into a C library exists — only the declaration authoring is manual:
 | C varargs | `func.varargs` attribute (`MLIRGen.cpp`) | `declare function printf(format: string, ...args)`-style declarations. |
 | Linking | `tslang.cpp` driver: `-lib=` (static libs, `--emit=exe`), `-obj=` (object files), `-shared-libs=` (JIT), plus `*-lib-path` options | Both AOT (lld) and JIT (ORC symbol resolution) paths exist. |
 
-Values crossing the boundary are passed as they are, with no conversion or copy. These cost
-time in real ports because nothing reports them:
-
-- **A `string` returned by a declared C function is the C `char*` itself.** tslang does not
-  copy it and does not track its lifetime. The C side must return storage that outlives every
-  use of the TypeScript string: a C++ helper that reused one `std::string` buffer for several
-  calls handed TypeScript strings that changed under it.
+Values crossing the boundary are passed as they are, with no conversion or copy, with one
+exception. These cost time in real ports because nothing reports them:
+
+- **A `string` returned by C is copied.** The result of a function declared at the top level
+  with `declare function` and defined nowhere in the program, or of a `@cpp` class's method, is
+  copied into a string of the program's own (null stays null). Kept as C's `char*`, it was a
+  buffer C may reuse (`strerror`, a C++ helper's one `std::string`) or the middle of another
+  string (`strstr`), and under `-mm=rc` the reference count was kept in the 8 bytes before the
+  pointer: memory that isn't the program's. Declare the result `Opaque` to get the pointer
+  itself - for pointer arithmetic, or for a string the caller has to free (`strdup`), which a
+  copy would leak. A function in a `declare namespace` or a `declare class` is TypeScript
+  compiled elsewhere (the default library's `lib.d.ts`), and its result is not copied.
+- **A `string` going to C is the program's own `char*`**, valid for the call. A C function that
+  keeps it must copy it.
 - **`number` is a C `double`.** A C function declared with a `number` parameter must take a
   `double`. A `float` parameter reads garbage with no error, so declare it `f32` instead (see the
   table in section 2).

docs/cpp-classes.md

@@ -190,7 +190,8 @@ const log = new MessageLog();          // pass <Opaque>log to C++
   parameters the way the ABI passes them (see "Writing a binding").
 - **Lifetime.** C++ keeps a raw pointer that the collector can't see. Hold the object in tslang,
   for example in a global, for as long as C++ may call it. A `string` argument is C++'s buffer:
-  copy it to keep it past the call.
+  copy it to keep it past the call. (The other way, a `string` a C++ method returns to
+  TypeScript is copied by the call; declare the method to return `Opaque` for the pointer.)
 - **Not yet supported:**
   - returning a struct (an error);
   - exporting the class (an error: an importer would get the methods but not the vtable);

tslang/lib/TypeScript/MLIRGenFunctions.cpp

@@ -799,6 +799,10 @@ namespace mlirgen
             if (declared == declaredFunctions.end() || declared->second.second != theModule.getOperation())
             {
                 declaredFunctions[fullName] = {functionDeclarationAST, theModule.getOperation()};
+                if (fullName == name)
+                {
+                    rootDeclaredFunctions.insert(fullName);
+                }
             }
             else if (declared->second.first != functionDeclarationAST)
             {

tslang/lib/TypeScript/MLIRGenImpl.h

@@ -8518,6 +8518,17 @@ class MLIRGenImpl
                 auto hasReturn = callValue.getType() != getVoidType();
                 if (hasReturn)
                 {
+                    // A string from C is C's memory: a buffer it reuses (`strerror`), or the middle
+                    // of another string (`strstr`). Kept as it was, it changed under the program,
+                    // and rc counted it in the 8 bytes before it - someone else's memory. It is
+                    // copied into a string of the program's own; `Opaque` is the raw pointer.
+                    if (isa<mlir_ts::StringType>(callValue.getType()) && isForeignCall(funcRefValue, callAttrs))
+                    {
+                        auto copy = builder.create<mlir_ts::StringCopyOp>(location, callValue.getType(), callValue);
+                        markFreshBlockOwned(location, copy);
+                        return V(copy);
+                    }
+
                     return callValue;
                 }
             }
@@ -8526,6 +8537,34 @@ class MLIRGenImpl
         return mlir::success();
     }
 
+    // A call into C or C++: a method of a @cpp class, or a function declared at the top level with
+    // `declare function` and defined nowhere in the program. A `declare namespace` or a `declare
+    // class` (the default library's lib.d.ts) is TypeScript compiled elsewhere.
+    bool isForeignCall(mlir::Value funcRefValue, mlir::ArrayRef<mlir::NamedAttribute> callAttrs)
+    {
+        if (llvm::any_of(callAttrs, [](auto attr) { return attr.getName() == CPP_CALL_ATTR_NAME; }))
+        {
+            return true;
+        }
+
+        auto symbolRef = funcRefValue.getDefiningOp<mlir_ts::SymbolRefOp>();
+        if (!symbolRef)
+        {
+            return false;
+        }
+
+        auto name = symbolRef.getIdentifier().str();
+        auto declared = declaredFunctions.find(name);
+        if (declared == declaredFunctions.end() || declared->second.second != theModule.getOperation()
+            || !rootDeclaredFunctions.contains(name))
+        {
+            return false;
+        }
+
+        auto funcOp = theModule.lookupSymbol<mlir_ts::FuncOp>(name);
+        return funcOp && funcOp.getBody().empty();
+    }
+
     // A call with fewer arguments than the callee needs is TypeScript's TS2554. Only a parameter
     // whose type admits `undefined` itself may be left out - an optional (`b?: T`) or a
     // default-valued (`b = 5`) one, both typed !ts.optional, and an `undefined` or `void` one.
@@ -14193,6 +14232,8 @@ class MLIRGenImpl
     // generated into - discovery's is thrown away, and a library's __decls is parsed again for the
     // next pass; see mlirGen(FunctionDeclaration)
     std::map<std::string, std::pair<FunctionDeclaration, mlir::Operation *>> declaredFunctions;
+    // those of them declared outside any namespace: C functions, unless a definition follows (isForeignCall)
+    llvm::StringSet<> rootDeclaredFunctions;
 
     // Set while an imported source file is generated (mlirGenInclude): the errors of the file go
     // here instead of being printed, and the import hands them to the importer, whose

tslang/test/tester/CMakeLists.txt

@@ -371,6 +371,7 @@ tslang_add_test(NAME test-compile-00-narrowed-assign-other-member COMMAND test-r
 tslang_add_test(NAME test-compile-00-const-record-owned-fields COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00const_record_owned_fields.ts")
 tslang_add_test(NAME test-compile-00-array-length-valid COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00array_length_valid.ts")
 tslang_add_test(NAME test-compile-00-string-view-copy COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00string_view_copy.ts")
+tslang_add_test(NAME test-compile-00-c-string-copy COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00c_string_copy.ts")
 tslang_add_test(NAME test-compile-00-array-data-string-alias COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00array_data_string_alias.ts")
 tslang_add_test(NAME test-compile-00-arrays7 COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00array7.ts")
 tslang_add_test(NAME test-compile-00-arrays8-tuple-spread COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00array8_tuple_spread.ts")
@@ -910,6 +911,7 @@ tslang_add_test(NAME test-jit-00-narrowed-assign-other-member COMMAND test-runne
 tslang_add_test(NAME test-jit-00-const-record-owned-fields COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00const_record_owned_fields.ts")
 tslang_add_test(NAME test-jit-00-array-length-valid COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00array_length_valid.ts")
 tslang_add_test(NAME test-jit-00-string-view-copy COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00string_view_copy.ts")
+tslang_add_test(NAME test-jit-00-c-string-copy COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00c_string_copy.ts")
 tslang_add_test(NAME test-jit-00-array-data-string-alias COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00array_data_string_alias.ts")
 tslang_add_test(NAME test-jit-00-arrays7 COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00array7.ts")
 tslang_add_test(NAME test-jit-00-arrays8-tuple-spread COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00array8_tuple_spread.ts")
@@ -1794,6 +1796,7 @@ set(TSLANG_CORPUS
     00const_record_owned_fields.ts
     00array_length_valid.ts
     00string_view_copy.ts
+    00c_string_copy.ts
     00array_data_string_alias.ts
     00array7.ts
     00array8_tuple_spread.ts

tslang/test/tester/tests/00c_string_copy.ts

@@ -0,0 +1,44 @@
+// regression test: a string a C function returns is copied into a string of the program's own.
+//
+// bug found and fixed: the string was C's memory, kept as it was. A buffer C reuses changed under
+// the program (`strerror(2)` read the text of the next call), and under -mm=rc the count was kept
+// in the 8 bytes before the pointer - someone else's memory: storing `strchr(base, 'k')` rewrote
+// two characters of `base`. Null stays null.
+
+declare function strchr(s: string, c: s32): string;
+declare function strerror(errnum: s32): string;
+declare function getenv(name: string): string;
+
+let kept = "";
+
+function tail(s: string): string {
+    return strchr(s, 107);   // 'k'
+}
+
+function main() {
+    let n = 1;
+    const base = "abcdefghijklmnop" + n;
+
+    // a pointer into base, stored and copied around: base is untouched
+    kept = strchr(base, 107);
+    let other = kept;
+    for (let i = 0; i < 3; i++) {
+        let t = tail(base);
+        assert(t == "klmnop1");
+    }
+
+    assert(base == "abcdefghijklmnop1");
+    assert(kept == "klmnop1");
+    assert(other == "klmnop1");
+
+    // two results of one C function are two strings
+    const a = strerror(2);
+    const b = strerror(13);
+    assert(a != b);
+
+    // a copy of nothing is nothing
+    const missing = getenv("TSLANG_C_STRING_COPY_TEST_UNSET_VARIABLE");
+    assert(missing == null);
+
+    print("done.");
+}