AOT: follow-up to #982: 48 fixes

Source/JavaScriptCore/aot/AOT.cmake

@@ -594,6 +594,18 @@ function(AOT_GENERATE_RUNTIME_FACTS)
     set(JavaScriptCore_SOURCES ${JavaScriptCore_SOURCES} "${JavaScriptCore_AOT_GENERATED_DIR}/AOTRuntimeFactsOfLayoutsIncludes.h" PARENT_SCOPE)
 endfunction()
 
+# It reads the text of the compiler's sources too. What it writes says only that they have passed.
+function(AOT_CHECK_READS_THAT_ASSUME)
+    file(GLOB_RECURSE sources CONFIGURE_DEPENDS "${JAVASCRIPTCORE_DIR}/aot/*.cpp" "${JAVASCRIPTCORE_DIR}/aot/*.h")
+    add_custom_command(
+        OUTPUT "${JavaScriptCore_AOT_GENERATED_DIR}/AOTReadsThatAssumeChecked.h"
+        DEPENDS ${sources} "${JAVASCRIPTCORE_DIR}/aot/codegen/generator/reads-that-assume.ts" "${JAVASCRIPTCORE_DIR}/aot/codegen/AOTReadsThatAssume.txt"
+        COMMAND "${NODE_EXECUTABLE}" "${JAVASCRIPTCORE_DIR}/aot/codegen/generator/reads-that-assume.ts" --javascriptcore "${JAVASCRIPTCORE_DIR}" --out "${JavaScriptCore_AOT_GENERATED_DIR}"
+        VERBATIM
+    )
+    set(JavaScriptCore_SOURCES ${JavaScriptCore_SOURCES} "${JavaScriptCore_AOT_GENERATED_DIR}/AOTReadsThatAssumeChecked.h" PARENT_SCOPE)
+endfunction()
+
 function(AOT_READ_LIBRARY_SOURCES result problemsResult)
     set(problems)
     set(sources)
@@ -696,6 +708,7 @@ AOT_GENERATE_FROM_SCHEMA()
 AOT_GENERATE_BUILTIN_TABLE()
 AOT_GENERATE_VECTOR()
 AOT_GENERATE_RUNTIME_FACTS()
+AOT_CHECK_READS_THAT_ASSUME()
 AOT_COMPUTE_SOURCES()
 AOT_CHECK_SOURCE_LISTS()
 AOT_CHECK_EXPORTED_HEADERS()

Source/JavaScriptCore/aot/builtins/AOTBuiltinTable.h

@@ -666,6 +666,9 @@ struct BuiltinHostObject {
     bool keepsStringsOfStoredValues { false };
     // For each of these names the object has an accessor whose getter and setter are native code.
     std::span<const BuiltinName> nativeAccessors { };
+    // The build reads what lies under these names, so native code locks them in a compiled program. The holder is the member that hands out the object.
+    std::span<const BuiltinName> lockedNames { };
+    BuiltinID holder { };
     // The systems on which native code makes the object.
     OptionSet<BuiltinPlatform> platforms { everyBuiltinPlatform };
 };

Source/JavaScriptCore/aot/builtins/AOTBuiltinTableDescription.cpp

@@ -347,7 +347,7 @@ class TableWriter {
     void write(const BuiltinHostObject& hostObject)
     {
         object("cell"_s, PlainName { hostObject.nativeCell }, "function"_s, hostObject.function, "prototype"_s, hostObject.prototype, "fields"_s, hostObject.fields,
-            "keepsStringsOfStoredValues"_s, hostObject.keepsStringsOfStoredValues, "nativeAccessors"_s, hostObject.nativeAccessors, "platforms"_s, hostObject.platforms);
+            "keepsStringsOfStoredValues"_s, hostObject.keepsStringsOfStoredValues, "nativeAccessors"_s, hostObject.nativeAccessors, "lockedNames"_s, hostObject.lockedNames, "holder"_s, hostObject.holder, "platforms"_s, hostObject.platforms);
     }
 
     void write(const FeatureUseSummary& summary)

Source/JavaScriptCore/aot/builtins/generator/build-attachments.ts

@@ -976,8 +976,10 @@ function buildHostObjects(context: AttachmentContext): ModelHostObject[] {
     }));
     const described = declared.function === null ? null : builder.resolve(declared.function, source);
     const prototype = declared.prototype === null ? null : resolveObject(builder, declared.prototype, source);
+    const holder = declared.holder === null ? null : builder.resolve(declared.holder, source);
     if ((declared.function !== null && described === null) || (declared.prototype !== null && prototype === null))
       continue;
+    if (declared.holder !== null && holder === null) continue;
     objects.push({
       cell: builder.internName(declared.cell, "Text"),
       function: described,
@@ -991,6 +993,8 @@ function buildHostObjects(context: AttachmentContext): ModelHostObject[] {
       internalFields: internalFields.flatMap(({ index, uses }) => (uses === null ? [] : [{ index, uses }])),
       keepsStringsOfStoredValues: declared.keepsStringsOfStoredValues,
       nativeAccessors: declared.nativeAccessors.map(name => builder.internComponent(name)),
+      lockedNames: declared.lockedNames.map(name => builder.internComponent(name)),
+      holder,
       platforms: declared.platforms,
       source,
     });

Source/JavaScriptCore/aot/builtins/generator/declarations.ts

@@ -344,6 +344,9 @@ export interface HostObjectDeclaration {
   internalFields: readonly HostObjectInternalFieldSpec[];
   keepsStringsOfStoredValues: boolean;
   nativeAccessors: readonly string[];
+  lockedNames: readonly string[];
+  /** The member that hands out the object. null: no name of it is locked. */
+  holder: BuiltinPath | null;
   /** The systems on which native code makes it. */
   platforms: readonly TargetPlatform[];
 }

Source/JavaScriptCore/aot/builtins/generator/dump.ts

@@ -294,6 +294,8 @@ export interface DumpedHostObject {
   fields: { name: string | null; value: DumpedPlace }[];
   keepsStringsOfStoredValues: boolean;
   nativeAccessors: string[];
+  lockedNames: string[];
+  holder: string | null;
   platforms: TargetPlatform[];
 }
 

Source/JavaScriptCore/aot/builtins/generator/emit-cpp-attachments.ts

@@ -243,6 +243,8 @@ function formatHostObject(context: AttachmentContext, object: ModelHostObject):
     ],
     ["keepsStringsOfStoredValues", object.keepsStringsOfStoredValues ? "true" : null],
     ["nativeAccessors", formatNames(context, object.nativeAccessors)],
+    ["lockedNames", formatNames(context, object.lockedNames)],
+    ["holder", formatId("BuiltinID", object.holder)],
     [
       "platforms",
       formatOptionSet(

Source/JavaScriptCore/aot/builtins/generator/emit-dump.ts

@@ -401,6 +401,8 @@ function dumpHostObject(object: ModelHostObject): DumpedHostObject {
     })),
     keepsStringsOfStoredValues: object.keepsStringsOfStoredValues,
     nativeAccessors: object.nativeAccessors.map(spellingOf),
+    lockedNames: object.lockedNames.map(spellingOf),
+    holder: object.holder?.token ?? null,
     platforms: inListOrder(targetPlatforms, object.platforms),
   };
 }

Source/JavaScriptCore/aot/builtins/generator/emit-rust-table.ts

@@ -301,7 +301,7 @@ function findNamesStoredByNativeCode(model: TableModel): Set<ModelName> {
   add(model.builtins.filter(builtin => builtin.holder !== null).map(builtin => builtin.name));
   for (const builtinClass of model.classes) add(builtinClass.namesDefinedOnNewInstances ?? []);
   for (const layout of model.layouts) add(layout.fields.map(field => field.name));
-  for (const hostObject of model.hostObjects) add([...hostObject.fields.map(field => field.name), ...hostObject.nativeAccessors]);
+  for (const hostObject of model.hostObjects) add([...hostObject.fields.map(field => field.name), ...hostObject.nativeAccessors, ...hostObject.lockedNames]);
   for (const { result, flow } of model.overloads) {
     add(result.properties.map(property => property.name));
     for (const { action, key } of flow.propertyActions) {

Source/JavaScriptCore/aot/builtins/generator/lib.ts

@@ -1507,6 +1507,8 @@ export interface HostObjectOptions {
   keepsStringsOfStoredValues?: boolean;
   /** The names under which native code lays an accessor of its own. Whoever asks for the description of one gets two functions of native code. */
   nativeAccessors?: readonly string[];
+  /** The build reads what lies under these names, so native code locks them in a compiled program. `holder`: the member that hands out the object. */
+  locked?: { holder: BuiltinPath; names: readonly string[] };
   /** By the index of an internal field: what native code does with what code of the library stores there. */
   internalFields?: Readonly<Record<number, readonly HostUseSpec[]>>;
   /** The systems on which native code makes the object. Without it: all of them. */
@@ -1533,6 +1535,8 @@ export const hostObject = (cell: string, options: HostObjectOptions): Declaratio
       })),
       keepsStringsOfStoredValues: options.keepsStringsOfStoredValues ?? false,
       nativeAccessors: options.nativeAccessors ?? [],
+      lockedNames: options.locked?.names ?? [],
+      holder: options.locked?.holder ?? null,
       platforms: options.platforms ?? targetPlatforms,
     },
   ];
@@ -1565,6 +1569,8 @@ export function hostFunction(
       internalFields: [],
       keepsStringsOfStoredValues: false,
       nativeAccessors: [],
+      lockedNames: [],
+      holder: null,
       platforms: targetPlatforms,
     },
   ];

Source/JavaScriptCore/aot/builtins/generator/model.ts

@@ -484,6 +484,8 @@ export interface ModelHostObject {
   internalFields: readonly ModelHostObjectInternalField[];
   keepsStringsOfStoredValues: boolean;
   nativeAccessors: readonly ModelName[];
+  lockedNames: readonly ModelName[];
+  holder: ModelBuiltin | null;
   platforms: readonly TargetPlatform[];
   source: Source;
 }

Source/JavaScriptCore/aot/builtins/math.builtins.ts

@@ -68,8 +68,8 @@ export default b.declarations(
       random: {
         returns: b.f64,
         capability: "none",
-        reads: ["RuntimeState"],
-        writes: ["RuntimeState"],
+        reads: ["RuntimeState", "ExternalState"],
+        writes: ["RuntimeState", "ExternalState"],
         implementation: b.inline("MathRandom"),
       },
       round: rounding("MathRound"),

Source/JavaScriptCore/aot/codegen/AOTReadsThatAssume.txt

@@ -0,0 +1,24 @@
+# Who names the heap of a field whose reading takes more for granted than a type says, and who calls a function that trusts its caller: <name> | <file> | <how often> | <why each is right>.
+# aot/codegen/generator/reads-that-assume.ts holds the sources to these counts in every build. A declaration and a definition count as well.
+JSString_value | builtins/AOTHashTableBuiltinEmitter.cpp | 5 | two test the rope bit next to the load, in front of any read through the word; three only compare the word with another one, which is right for a rope too
+JSString_value | codegen/AOTInnerPointerValidation.cpp | 1 | a list of heaps, no load
+JSString_value | codegen/AOTPrimitiveEmitter.h | 1 | isRopeString(): it is the test
+JSString_value | codegen/AOTRuntimeValidationEmitter.cpp | 1 | tests the rope bit next to the load
+JSString_value | codegen/AOTStringBuilderEmitter.cpp | 1 | tests the rope bit, reads nothing through the word
+JSString_value | codegen/AOTStringBuiltinEmitter.cpp | 1 | tests the rope bit next to the load
+JSString_value | codegen/AOTStringEmitter.cpp | 7 | implOfFlatString() takes a FlatStringCell; viewOfFlatString() is listed below; three test the rope bit next to the load; one reads behind the stub that has resolved the rope in place, or off the base of a substring, which is flat; one reads what FlattenString has just made
+JSString_value | codegen/AOTStringEmitterComparisons.cpp | 4 | areBoth8Bit() is listed below; one only compares the word with the address of an atom; two test the rope bit next to the load
+JSString_value | codegen/AOTStringSwitch.cpp | 1 | tests the rope bit next to the load
+flatStringWrittenHere | builtins/AOTHashTableBuiltinEmitter.cpp | 1 | the cell is the key in its boxed form with the box taken off: a key without a number in its type is handed on as it came
+flatStringWrittenHere | codegen/AOTStringEmitter.cpp | 2 | the definition; flatStringAssumedBy() hands it the cell that it has just made of the same value
+flatStringWrittenHere | codegen/AOTStringEmitter.h | 1 | the declaration
+flatStringAssumedBy | codegen/AOTStringEmitter.cpp | 2 | the definition; under a sabotage alone, which shows that it stops the build
+flatStringAssumedBy | codegen/AOTStringEmitter.h | 1 | the declaration
+viewOfFlatString | builtins/AOTMathBuiltinEmitterNumbers.cpp | 1 | behind isRopeString() of the same cell
+viewOfFlatString | codegen/AOTStringBuiltinEmitter.cpp | 5 | each behind isRopeString() of the same cell
+viewOfFlatString | codegen/AOTStringEmitter.cpp | 1 | the definition
+viewOfFlatString | codegen/AOTStringEmitter.h | 1 | the declaration
+viewOfFlatString | codegen/AOTStringEmitterComparisons.cpp | 1 | behind isRopeString() of the same cell
+areBoth8Bit | codegen/AOTOperatorEmitterEquality.cpp | 1 | both operands are one character read off a string, which is flat; such a read inherits nothing, because the prototypes of built-ins are read-only in compiled code
+areBoth8Bit | codegen/AOTStringEmitter.h | 1 | the declaration
+areBoth8Bit | codegen/AOTStringEmitterComparisons.cpp | 1 | the definition

Source/JavaScriptCore/aot/codegen/generator/reads-that-assume.ts

@@ -0,0 +1,125 @@
+// To read some fields is to take more for granted than the type of a value says: the word in the cell of a string is a StringImpl only while the string is flat,
+// and a type says what the characters are, not which object holds them. Such a field is read by few functions, which test first or take a type that only a proof can make.
+// This step of the build counts, file by file, who names the heap of such a field and who calls a function that trusts its caller, and holds the counts to
+// aot/codegen/AOTReadsThatAssume.txt. A place that the list lacks stops the build: go through one of the functions named in the message, or add the place with the reason why it is right.
+import { mkdirSync, readdirSync, readFileSync, writeFileSync } from "node:fs";
+import { join, resolve } from "node:path";
+import { parseArgs } from "node:util";
+
+const compilerDirectories = ["builtins", "codegen", "image", "object", "passes", "place", "represent", "select", "stubs"];
+const listFile = "aot/codegen/AOTReadsThatAssume.txt";
+const stampFile = "AOTReadsThatAssumeChecked.h";
+
+const watched: { name: string; pattern: RegExp; instead: string }[] = [
+  {
+    name: "JSString_value",
+    pattern: /\b(?:m_)?heaps\.JSString_value\b/g,
+    instead: "StringEmitter::viewOf(), lengthOf(), viewOfString(), isRopeString(), or implOfFlatString() with a FlatStringCell",
+  },
+  {
+    name: "flatStringWrittenHere",
+    pattern: /\bflatStringWrittenHere\(/g,
+    instead: "StringEmitter::flatStringByType(): this one trusts its caller that the cell is the value's",
+  },
+  {
+    name: "flatStringAssumedBy",
+    pattern: /\bflatStringAssumedBy\(/g,
+    instead: "StringEmitter::flatStringByType(), and another road for what is not flat: this one stops the build of a user's program",
+  },
+  {
+    name: "viewOfFlatString",
+    pattern: /\bviewOfFlatString\(/g,
+    instead: "StringEmitter::viewOf(): this one trusts its caller that isRopeString() of the same cell has just answered no",
+  },
+  {
+    name: "areBoth8Bit",
+    pattern: /\bareBoth8Bit\(/g,
+    instead: "StringEmitter::areEqual(): this one trusts its caller that both strings are flat",
+  },
+];
+
+type Counts = Map<string, number>;
+
+function keyOf(name: string, file: string): string {
+  return `${name} | ${file}`;
+}
+
+function countPlaces(javascriptcore: string): Counts {
+  const counts: Counts = new Map();
+  for (const directory of compilerDirectories) {
+    for (const entry of readdirSync(join(javascriptcore, "aot", directory), { withFileTypes: true }).sort((a, b) => (a.name < b.name ? -1 : 1))) {
+      if (!entry.isFile() || !/\.(?:cpp|h)$/.test(entry.name)) continue;
+      const text = readFileSync(join(javascriptcore, "aot", directory, entry.name), "utf8");
+      for (const { name, pattern } of watched) {
+        const found = text.match(pattern)?.length ?? 0;
+        if (found) counts.set(keyOf(name, `${directory}/${entry.name}`), found);
+      }
+    }
+  }
+  return counts;
+}
+
+function readList(text: string): { listed: Counts; problems: string[] } {
+  const listed: Counts = new Map();
+  const problems: string[] = [];
+  for (const line of text.split("\n")) {
+    if (!line || line.startsWith("#")) continue;
+    const [name, file, count, reason] = line.split(" | ");
+    if (!name || !file || !/^\d+$/.test(count ?? "") || !reason?.trim()) problems.push(`${listFile}: not "<name> | <file> | <count> | <reason>": ${line}`);
+    else listed.set(keyOf(name, file), Number(count));
+  }
+  return { listed, problems };
+}
+
+function compare(found: Counts, listed: Counts): string[] {
+  const problems: string[] = [];
+  for (const [key, count] of found) {
+    const allowed = listed.get(key) ?? 0;
+    if (count === allowed) continue;
+    const [name, file] = key.split(" | ");
+    if (count > allowed)
+      problems.push(
+        `aot/${file} has ${count} of ${name}, the list has ${allowed}. It takes for granted what no type says. Use ${watched.find(entry => entry.name === name)!.instead}; or say in ${listFile} why the new place is right.`,
+      );
+    else problems.push(`aot/${file} has ${count} of ${name}, the list has ${allowed}: lower the count in ${listFile}, or the next place passes unseen.`);
+  }
+  for (const [key, allowed] of listed) {
+    if (!found.has(key)) problems.push(`aot/${key.split(" | ")[1]} has none of ${key.split(" | ")[0]}, the list has ${allowed}: take the line out of ${listFile}.`);
+  }
+  return problems;
+}
+
+// A check that was never seen to fail proves nothing: every name must be found somewhere, and one place too many, one too few and one in a file of its own must each be noticed.
+function hasTeeth(found: Counts): boolean {
+  if (!watched.every(({ name }) => [...found.keys()].some(key => key.startsWith(`${name} | `)))) return false;
+  const [key, count] = [...found][0] ?? [];
+  if (key === undefined || count === undefined) return false;
+  const more = new Map(found).set(key, count + 1);
+  const fewer = new Map(found).set(key, count - 1);
+  const elsewhere = new Map(found).set(keyOf(watched[0]!.name, "codegen/AOTNoSuchFile.cpp"), 1);
+  return !compare(found, found).length && [more, fewer, elsewhere].every(changed => compare(changed, found).length === 1 && compare(found, changed).length === 1);
+}
+
+const { values } = parseArgs({
+  strict: true,
+  options: { "javascriptcore": { type: "string" }, "out": { type: "string" }, "print-counts": { type: "boolean" } },
+});
+if (values.javascriptcore === undefined || (values.out === undefined && !values["print-counts"])) {
+  console.error("usage: <bun or node> aot/codegen/generator/reads-that-assume.ts --javascriptcore <Source/JavaScriptCore> (--out <dir> | --print-counts)");
+  process.exit(1);
+}
+const javascriptcore = resolve(values.javascriptcore);
+const found = countPlaces(javascriptcore);
+if (values["print-counts"]) {
+  for (const [key, count] of found) console.log(`${key} | ${count}`);
+  process.exit(0);
+}
+const { listed, problems } = readList(readFileSync(join(javascriptcore, listFile), "utf8"));
+if (!hasTeeth(found)) problems.push("aot/codegen/generator/reads-that-assume.ts does not notice a place too many or too few: it checks nothing.");
+problems.push(...compare(found, listed));
+if (problems.length) {
+  console.error(problems.join("\n"));
+  process.exit(1);
+}
+mkdirSync(values.out!, { recursive: true });
+writeFileSync(join(values.out!, stampFile), "// Written by aot/codegen/generator/reads-that-assume.ts when the sources of the compiler have passed it. Do not edit.\n");

Source/JavaScriptCore/aot/image/AOTFaultHandler.h

@@ -14,6 +14,9 @@ namespace JSC { namespace AOT {
 
 void installFaultHandler();
 
+// A thread that has run out of stack has none left for whoever reports that.
+void takeFaultsOfThisThreadOnAnotherStack();
+
 // Whether compiled code raised this access fault on purpose. If so, the registers continue where that code handles it.
 JS_EXPORT_PRIVATE bool handleAccessFault(void* faultingAddress, PlatformRegisters&);