threads: make mapped arguments severing atomic

Author: chrisbbreuer

src/builtins.zig

@@ -1715,7 +1715,7 @@ pub fn defineOneResult(self: *Interpreter, target: *value.Object, key: []const u
                 try target.setAttr(self.arena, key, attr);
                 target.has_indexed_property.store(true, .monotonic);
                 // Redefining a mapped index as non-writable severs the parameter link.
-                if (am_mapped and !attr.writable) target.arg_map_names[i] = "";
+                if (am_mapped and !attr.writable) interpreter.argMapSever(target, i);
                 target.extendArrayLengthFloor(i + 1);
                 return true;
             }
@@ -1802,7 +1802,7 @@ pub fn defineOneResult(self: *Interpreter, target: *value.Object, key: []const u
             // `length`; 2^32 - 1 and above are ordinary properties.
             if (i < 4294967295 and i + 1 > target.arrayLength()) target.extendArrayLengthFloor(i + 1);
             // Defining a mapped arguments index as an accessor severs its link.
-            if (target.is_arguments and (get != null or set != null) and i < target.arg_map_names.len) target.arg_map_names[i] = "";
+            if (target.is_arguments and (get != null or set != null)) interpreter.argMapSever(target, i);
         }
     }
     return true;

src/context.zig

@@ -9662,6 +9662,63 @@ test "parallel_js: TypedArray set snapshots array-like source length under no-GI
     try std.testing.expect(result.asBool());
 }
 
+test "parallel_js: mapped arguments severing is atomic under no-GIL readers" {
+    if (builtin.single_threaded) return error.SkipZigTest;
+    const ctx = try Context.createWithTestingOptions(std.testing.allocator, .{
+        .enable_threads = true,
+        .enable_gc = true,
+        .parallel_gc = true,
+        .parallel_js = true,
+    });
+    defer ctx.destroy();
+
+    const result = try ctx.evaluate(
+        \\(() => {
+        \\  if ($vm.useThreadGIL() !== false)
+        \\    throw new Error("main still holds the thread GIL");
+        \\  function mint(a, b, c) { return arguments; }
+        \\  const shared = { slot: null, stop: 0, started: 0, go: 0 };
+        \\  const sentinel = "value-one";
+        \\  const readers = [];
+        \\  for (let r = 0; r < 2; ++r) {
+        \\    readers.push(new Thread((shared, sentinel) => {
+        \\      if ($vm.useThreadGIL() !== false)
+        \\        throw new Error("worker still holds the thread GIL");
+        \\      Atomics.add(shared, "started", 1);
+        \\      while (Atomics.load(shared, "go") === 0)
+        \\        Atomics.wait(shared, "go", 0, 100);
+        \\      let failures = 0;
+        \\      while (Atomics.load(shared, "stop") === 0) {
+        \\        const args = shared.slot;
+        \\        if (args === null) continue;
+        \\        if (args[1] !== sentinel) ++failures;
+        \\        const v0 = args[0];
+        \\        if (!(v0 === 7 || v0 === undefined)) ++failures;
+        \\        const len = args.length;
+        \\        if (!(len === 3 || len === 99)) ++failures;
+        \\      }
+        \\      return failures;
+        \\    }, shared, sentinel));
+        \\  }
+        \\  while (Atomics.load(shared, "started") !== readers.length)
+        \\    Atomics.wait(shared, "started", Atomics.load(shared, "started"), 100);
+        \\  Atomics.store(shared, "go", 1);
+        \\  Atomics.notify(shared, "go", readers.length);
+        \\  for (let i = 0; i < 750; ++i) {
+        \\    const args = mint(7, sentinel, true);
+        \\    shared.slot = args;
+        \\    delete args[0];
+        \\    args.length = 99;
+        \\    if ((i & 31) === 0) Atomics.wait(shared, "go", 1, 1);
+        \\  }
+        \\  Atomics.store(shared, "stop", 1);
+        \\  Atomics.notify(shared, "stop", readers.length);
+        \\  return readers.every(t => t.join() === 0);
+        \\})()
+    );
+    try std.testing.expect(result.asBool());
+}
+
 test "TypedArray set coerces offset before detached buffer checks" {
     try std.testing.expect((try evalIn(
         \\class ExpectedError extends Error {}

src/gc.zig

@@ -355,6 +355,11 @@ fn finalizeObjectBacking(o: *Object, a: std.mem.Allocator) usize {
         o.arg_map_names = &.{};
         released += 1;
     }
+    if (flags.arg_map_severed) {
+        a.free(o.arg_map_severed);
+        o.arg_map_severed = &.{};
+        released += 1;
+    }
 
     o.backing_flags = .{};
     o.backing_allocator = null;

src/interpreter.zig

@@ -5956,7 +5956,9 @@ pub const Interpreter = struct {
         if (!func.is_strict and !non_simple_params and func.params.len > 0) {
             const n = @min(args.len, func.params.len);
             const names = try args_obj.asObj().argMapNamesAllocator(self.arena).alloc([]const u8, n);
+            const severed = try args_obj.asObj().argMapSeveredAllocator(self.arena).alloc(std.atomic.Value(bool), n);
             for (names, 0..) |*nm, i| nm.* = func.params[i].name;
+            for (severed) |*flag| flag.* = .init(false);
             // A duplicated parameter name maps only its last index.
             for (names, 0..) |nm, i| {
                 var j = i + 1;
@@ -5967,6 +5969,7 @@ pub const Interpreter = struct {
             }
             args_obj.asObj().arg_map_env = @ptrCast(call_env);
             args_obj.asObj().arg_map_names = names;
+            args_obj.asObj().arg_map_severed = severed;
         }
         return args_obj;
     }
@@ -10704,7 +10707,7 @@ pub const Interpreter = struct {
             if (o.getOwn(key) == null and o.denseElementInBounds(i)) {
                 if (o.attrsMap() != null and !o.getAttr(key).configurable) return false;
                 // Deleting a mapped arguments index severs its parameter link.
-                if (o.is_arguments and i < o.arg_map_names.len) o.arg_map_names[i] = "";
+                if (o.is_arguments) argMapSever(o, i);
                 _ = try o.deleteDenseElement(self.arena, i);
                 return true;
             }
@@ -39028,10 +39031,17 @@ pub fn hasProperty(o: *value.Object, name: []const u8) bool {
 /// mapped arguments object or index `i` is unmapped (out of range / severed).
 pub fn argMapName(o: *value.Object, i: usize) ?[]const u8 {
     if (o.arg_map_env == null or i >= o.arg_map_names.len) return null;
+    if (i < o.arg_map_severed.len and o.arg_map_severed[i].load(.acquire)) return null;
     const nm = o.arg_map_names[i];
     return if (nm.len == 0) null else nm;
 }
 
+/// Atomically sever a mapped-arguments index from its parameter binding.
+pub fn argMapSever(o: *value.Object, i: usize) void {
+    if (i >= o.arg_map_severed.len) return;
+    o.arg_map_severed[i].store(true, .release);
+}
+
 /// Read a mapped index's parameter binding (null if unmapped).
 pub fn argMapGet(o: *value.Object, i: usize) ?Value {
     const nm = argMapName(o, i) orelse return null;

src/value.zig

@@ -614,6 +614,7 @@ pub const ObjectBackingFlags = packed struct {
     data_view: bool = false,
     temporal: bool = false,
     arg_map_names: bool = false,
+    arg_map_severed: bool = false,
 };
 
 pub const ObjectPrivateDataTag = enum(u8) {
@@ -815,10 +816,14 @@ pub const Object = struct {
     /// A mapped (sloppy-mode, simple-parameter) arguments object's
     /// `[[ParameterMap]]`: the call's environment record (type-erased
     /// `*Environment`) and the parameter name each index maps to (`""` = not
-    /// mapped). A mapped index reads/writes the parameter binding; defining it as
-    /// an accessor or non-writable, or deleting it, severs the mapping.
+    /// initially mapped). A mapped index reads/writes the parameter binding;
+    /// defining it as an accessor or non-writable, or deleting it, atomically
+    /// severs the mapping in `arg_map_severed`. The names slice is immutable once
+    /// the arguments object is published so no-GIL readers cannot tear a slice
+    /// pair while another thread severs an index.
     arg_map_env: ?*anyopaque = null,
     arg_map_names: [][]const u8 = &.{},
+    arg_map_severed: []std.atomic.Value(bool) = &.{},
     /// `Map`/`Set` instances. A Map keeps `[key,value]` pair-arrays in
     /// `elements`; a Set keeps values directly. `size` is a maintained property.
     is_map: bool = false,
@@ -1026,6 +1031,10 @@ pub const Object = struct {
         return self.backingFor(fallback, "arg_map_names");
     }
 
+    pub fn argMapSeveredAllocator(self: *Object, fallback: std.mem.Allocator) std.mem.Allocator {
+        return self.backingFor(fallback, "arg_map_severed");
+    }
+
     pub fn lockProperties(self: *const Object) void {
         var spins: usize = 0;
         const mutex = &@constCast(self).property_lock;