From 3bcaa8fd62b38f9f231b520d1d375be92c50c924 Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Tue, 13 Oct 2026 19:12:33 +0100 Subject: [PATCH] A string returned by C is copied into a string of the program's own A `string` result of a C function (a top-level `declare function` with no definition in the program) or of a @cpp method was C's `char*` itself: a buffer C may reuse (`strerror(2)` read the next call's text) or the middle of another string (`strstr`). Under -mm=rc the reference count lives in the 8 bytes before a string, so retaining it wrote into memory that isn't the program's: storing `strchr(base, 'k')` rewrote two characters of `base`, and a release reaching zero would free C's pointer. The call's result now goes through ts.StringCopy (null stays null), owned like any fresh string. `Opaque` is the way to get the pointer itself. A function in a `declare namespace` or `declare class` (the default library's lib.d.ts) is TypeScript and isn't copied. TSLANG_ISSUES.md 6. Needs TypeScriptCompilerDefaultLib#33, which declares strstr, setlocale, regexp_* and the C time functions `Opaque` (or `int`). Co-Authored-By: Claude Opus 5.5 --- TSLANG_ISSUES.md | 7 +++- docs/c-cpp-header-import.md | 21 ++++++---- docs/cpp-classes.md | 3 +- tslang/lib/TypeScript/MLIRGenFunctions.cpp | 4 ++ tslang/lib/TypeScript/MLIRGenImpl.h | 41 +++++++++++++++++++ tslang/test/tester/CMakeLists.txt | 3 ++ tslang/test/tester/tests/00c_string_copy.ts | 44 +++++++++++++++++++++ 7 files changed, 114 insertions(+), 9 deletions(-) create mode 100644 tslang/test/tester/tests/00c_string_copy.ts diff --git a/TSLANG_ISSUES.md b/TSLANG_ISSUES.md index a8a2fdd53..8fbafc5d8 100644 --- a/TSLANG_ISSUES.md +++ b/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` field with a `null` initializer rejected | question | **answered**: declare it `Ref \| 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(items: vector)` for native_ts_graphics' NVRHI diff --git a/docs/c-cpp-header-import.md b/docs/c-cpp-header-import.md index a73f0edd8..8e6cfde76 100644 --- a/docs/c-cpp-header-import.md +++ b/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). diff --git a/docs/cpp-classes.md b/docs/cpp-classes.md index 7f3e5d36a..0609edbfe 100644 --- a/docs/cpp-classes.md +++ b/docs/cpp-classes.md @@ -190,7 +190,8 @@ const log = new MessageLog(); // pass 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); diff --git a/tslang/lib/TypeScript/MLIRGenFunctions.cpp b/tslang/lib/TypeScript/MLIRGenFunctions.cpp index 5e4171a74..ed55967de 100644 --- a/tslang/lib/TypeScript/MLIRGenFunctions.cpp +++ b/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) { diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index 338d4d4c6..5d7995bcf 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/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(callValue.getType()) && isForeignCall(funcRefValue, callAttrs)) + { + auto copy = builder.create(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 callAttrs) + { + if (llvm::any_of(callAttrs, [](auto attr) { return attr.getName() == CPP_CALL_ATTR_NAME; })) + { + return true; + } + + auto symbolRef = funcRefValue.getDefiningOp(); + 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(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> 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 diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 2a542c03d..1af824fa3 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/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 diff --git a/tslang/test/tester/tests/00c_string_copy.ts b/tslang/test/tester/tests/00c_string_copy.ts new file mode 100644 index 000000000..04850a08f --- /dev/null +++ b/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."); +}