diff --git a/docs/bugs/native-ts-graphics-findings.md b/docs/bugs/native-ts-graphics-findings.md index 6f143ac2e..95931a00e 100644 --- a/docs/bugs/native-ts-graphics-findings.md +++ b/docs/bugs/native-ts-graphics-findings.md @@ -29,7 +29,7 @@ keeps failing by design. The repros stay here as the record of what was reported | 4 | Escapes not processed in templates with substitutions | Wrong value, silent | `ntg_template_literal_escape.ts` | [#539](https://github.com/ASDAlexander77/TypeScriptCompiler/pull/539) | | 5 | `Math.max` of negative numbers returns 2.2e-308 | Wrong value, silent (default lib) | `ntg_math_max_negative.ts` | [DefaultLib#31](https://github.com/ASDAlexander77/TypeScriptCompilerDefaultLib/pull/31), and [#535](https://github.com/ASDAlexander77/TypeScriptCompiler/pull/535) for why its test passed | | 6 | `boolean && (Class \| null)` as a condition fails to compile | Compile error | `ntg_and_nullable_class.ts` | [#540](https://github.com/ASDAlexander77/TypeScriptCompiler/pull/540), and [#541](https://github.com/ASDAlexander77/TypeScriptCompiler/pull/541) for its misplaced first error | -| 7 | The JIT cache crashes a run that compiles two or more modules (pre-alpha93) | Compiler crash, no message | `ntg_jit_cache_import.ts` | open | +| 7 | The JIT cache crashes a run that compiles two or more modules (pre-alpha93) | Compiler crash, no message | `ntg_jit_cache_import.ts` | [#549](https://github.com/ASDAlexander77/TypeScriptCompiler/pull/549) | The silent ones (2 to 5) cost the most time: the programs compile and run, and only the output is wrong. @@ -226,6 +226,14 @@ per run) every run works. imports `core/donut.ts`, so every example crashed under the JIT twice before running. `run_jit.bat` passes `--jit-cache=false`. +**Fixed by #549:** the cache compiles each module in an MLIRContext of its own, and the built-in +types (`i32`, `f32`, `Opaque`, ...) were kept in a `static` map, made once, in the first module's +context. Every later module in the run got types from a context destroyed by then. The map is +now per module. The default library's declarations name such types, which is why every program +importing a module crashed; without it (the suite runs with `--no-default-lib`) a module only +crashed if it named one itself. Regression test: `test-jit-cache-embedded-types`, which runs from +an empty cache every time. + --- ## Seen earlier but not reproduced in minimal form diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index aa3f0efbe..2d242308b 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -14055,6 +14055,11 @@ class MLIRGenImpl // read of it loads the field again and casts it, so no value outlives the region it is in llvm::ScopedHashTable safeTypesMap; + // the built-in types by name (getEmbeddedType), made on first use; not static: they belong to + // this module's MLIRContext, and one run can compile several modules, each in a context of its + // own (the JIT cache) - a type kept from an earlier one points into freed memory + llvm::StringMap embeddedTypeMap; + // helper to get line number Parser parser; ts::SourceFile sourceFile; diff --git a/tslang/lib/TypeScript/MLIRGenTypes.cpp b/tslang/lib/TypeScript/MLIRGenTypes.cpp index c062f40d0..83fdcf282 100644 --- a/tslang/lib/TypeScript/MLIRGenTypes.cpp +++ b/tslang/lib/TypeScript/MLIRGenTypes.cpp @@ -1025,7 +1025,7 @@ namespace mlirgen mlir::Type MLIRGenImpl::getEmbeddedTypeBuiltins(mlir::StringRef name) { - static llvm::StringMap embeddedTypes { + if (embeddedTypeMap.empty()) embeddedTypeMap = { {"TemplateStringsArray", getArrayType(getStringType()) }, {"const",getConstType() }, #ifdef ENABLE_JS_BUILTIN_TYPES @@ -1082,13 +1082,12 @@ namespace mlirgen {"Opaque", getOpaqueType()}, }; - auto type = embeddedTypes[name]; - return type; + return embeddedTypeMap.lookup(name); } mlir::Type MLIRGenImpl::getEmbeddedTypeNoBuiltins(mlir::StringRef name) { - static llvm::StringMap embeddedTypes { + if (embeddedTypeMap.empty()) embeddedTypeMap = { {"TemplateStringsArray", getArrayType(getStringType()) }, {"const",getConstType() }, #ifdef ENABLE_JS_BUILTIN_TYPES @@ -1146,8 +1145,7 @@ namespace mlirgen {"Opaque", getOpaqueType()}, }; - auto type = embeddedTypes[name]; - return type; + return embeddedTypeMap.lookup(name); } mlir::Type MLIRGenImpl::getEmbeddedTypeWithParam(mlir::StringRef name, NodeArray &typeArguments, diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index a0392616d..ea2c210bf 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -3252,6 +3252,15 @@ add_test(NAME test-jit-cache-reuse-and-invalidate "-DSOURCE_DIR=${jit_cache_dir}" "-DWORK_DIR=${CMAKE_CURRENT_BINARY_DIR}/jit-cache-reuse" -P "${CMAKE_CURRENT_SOURCE_DIR}/jit-cache.cmake") +# Two modules compiled in one run, both naming built-in types (i32, f32): the second one used to +# get the types made in the first one's MLIRContext, destroyed by then, and crash the compiler. +add_test(NAME test-jit-cache-embedded-types + COMMAND ${CMAKE_COMMAND} + "-DTSLANG=$" + "-DSOURCE=${jit_cache_dir}/embedded_types.ts" + "-DWORK_DIR=${CMAKE_CURRENT_BINARY_DIR}/jit-cache-embedded-types" + "-DEXPECTED=done." + -P "${CMAKE_CURRENT_SOURCE_DIR}/jit-cache-empty.cmake") # `import { a as b }` and `import * as M` - each used to be "can't resolve name". set(import_bindings_dir "${PROJECT_SOURCE_DIR}/test/tester/import-bindings") diff --git a/tslang/test/tester/jit-cache-empty.cmake b/tslang/test/tester/jit-cache-empty.cmake new file mode 100644 index 000000000..f70dc0908 --- /dev/null +++ b/tslang/test/tester/jit-cache-empty.cmake @@ -0,0 +1,33 @@ +# Runs a program under the JIT cache from an empty cache, so the run compiles the program and every +# module it imports - several modules in one process, each in an MLIRContext of its own. (A test +# run by test-runner keeps its cache in __jit beside the source: from the second run on, it +# compiles nothing.) +# +# TSLANG, SOURCE (the program), WORK_DIR (the cache, emptied first), EXPECTED (its output). +# -mm=none and --no-default-lib keep the run to the compiler alone. + +cmake_minimum_required(VERSION 3.17.3) + +foreach(var TSLANG SOURCE WORK_DIR EXPECTED) + if(NOT DEFINED ${var}) + message(FATAL_ERROR "${var} is required") + endif() +endforeach() + +file(REMOVE_RECURSE "${WORK_DIR}") +file(MAKE_DIRECTORY "${WORK_DIR}") + +execute_process(COMMAND "${TSLANG}" --emit=jit -mm=none --no-default-lib "--jit-cache-dir=${WORK_DIR}" "${SOURCE}" + OUTPUT_VARIABLE out + ERROR_VARIABLE err + RESULT_VARIABLE status) +string(STRIP "${out}" out) +if(NOT status EQUAL 0 OR NOT out STREQUAL EXPECTED) + message(FATAL_ERROR "expected '${EXPECTED}': exit ${status}\n${out}\n${err}") +endif() + +file(GLOB objects "${WORK_DIR}/*.o" "${WORK_DIR}/*.obj") +list(LENGTH objects count) +if(count LESS 2) + message(FATAL_ERROR "expected an object for the program and one for each module it imports, found: ${objects}") +endif() diff --git a/tslang/test/tester/jit-cache/embedded_types.ts b/tslang/test/tester/jit-cache/embedded_types.ts new file mode 100644 index 000000000..cacf022f6 --- /dev/null +++ b/tslang/test/tester/jit-cache/embedded_types.ts @@ -0,0 +1,13 @@ +// Each module the JIT cache compiles has an MLIRContext of its own. The built-in types (i32, +// f32, Opaque, ...) were kept in a static map, built in the first module's context: the next +// module got types from a context that no longer existed, and the compiler crashed (0xC0000005). +import { widen, half } from './embedded_types_module' + +const a: i32 = 21; +const b: i64 = widen(a) * 2; +assert(b == 42); + +const c: f32 = 5; +assert(half(c) == 2.5); + +print("done."); diff --git a/tslang/test/tester/jit-cache/embedded_types_module.ts b/tslang/test/tester/jit-cache/embedded_types_module.ts new file mode 100644 index 000000000..61e0eb871 --- /dev/null +++ b/tslang/test/tester/jit-cache/embedded_types_module.ts @@ -0,0 +1,8 @@ +// Imported by embedded_types.ts (see there). +export function widen(x: i32): i64 { + return x; +} + +export function half(x: f32): f32 { + return x / 2; +}