Merge pull request #24171 from mlugg/atomic-order-derp

compiler: fix races in link queue

Author: mlugg

src/Zcu/PerThread.zig

@@ -4373,34 +4373,29 @@ pub fn addDependency(pt: Zcu.PerThread, unit: AnalUnit, dependee: InternPool.Dep
 /// codegen thread, depending on whether the backend supports `Zcu.Feature.separate_thread`.
 pub fn runCodegen(pt: Zcu.PerThread, func_index: InternPool.Index, air: *Air, out: *@import("../link.zig").ZcuTask.LinkFunc.SharedMir) void {
     const zcu = pt.zcu;
-    if (runCodegenInner(pt, func_index, air)) |mir| {
+    const success: bool = if (runCodegenInner(pt, func_index, air)) |mir| success: {
         out.value = mir;
-        out.status.store(.ready, .release);
-    } else |err| switch (err) {
-        error.OutOfMemory => {
-            zcu.comp.setAllocFailure();
-            out.status.store(.failed, .monotonic);
-        },
-        error.CodegenFail => {
-            zcu.assertCodegenFailed(zcu.funcInfo(func_index).owner_nav);
-            out.status.store(.failed, .monotonic);
-        },
-        error.NoLinkFile => {
-            assert(zcu.comp.bin_file == null);
-            out.status.store(.failed, .monotonic);
-        },
-        error.BackendDoesNotProduceMir => {
-            const backend = target_util.zigBackend(zcu.root_mod.resolved_target.result, zcu.comp.config.use_llvm);
-            switch (backend) {
+        break :success true;
+    } else |err| success: {
+        switch (err) {
+            error.OutOfMemory => zcu.comp.setAllocFailure(),
+            error.CodegenFail => zcu.assertCodegenFailed(zcu.funcInfo(func_index).owner_nav),
+            error.NoLinkFile => assert(zcu.comp.bin_file == null),
+            error.BackendDoesNotProduceMir => switch (target_util.zigBackend(
+                zcu.root_mod.resolved_target.result,
+                zcu.comp.config.use_llvm,
+            )) {
                 else => unreachable, // assertion failure
                 .stage2_spirv64,
                 .stage2_llvm,
                 => {},
-            }
-            out.status.store(.failed, .monotonic);
-        },
-    }
-    zcu.comp.link_task_queue.mirReady(zcu.comp, out);
+            },
+        }
+        break :success false;
+    };
+    // release `out.value` with this store; synchronizes with acquire loads in `link`
+    out.status.store(if (success) .ready else .failed, .release);
+    zcu.comp.link_task_queue.mirReady(zcu.comp, func_index, out);
     if (zcu.pending_codegen_jobs.rmw(.Sub, 1, .monotonic) == 1) {
         // Decremented to 0, so all done.
         zcu.codegen_prog_node.end();

src/link.zig

@@ -1249,7 +1249,7 @@ pub const ZcuTask = union(enum) {
             .update_line_number,
             => {},
             .link_func => |link_func| {
-                switch (link_func.mir.status.load(.monotonic)) {
+                switch (link_func.mir.status.load(.acquire)) {
                     .pending => unreachable, // cannot deinit until MIR done
                     .failed => {}, // MIR not populated so doesn't need freeing
                     .ready => link_func.mir.value.deinit(zcu),
@@ -1453,7 +1453,7 @@ pub fn doZcuTask(comp: *Compilation, tid: usize, task: ZcuTask) void {
             const fqn_slice = ip.getNav(nav).fqn.toSlice(ip);
             const nav_prog_node = comp.link_prog_node.start(fqn_slice, 0);
             defer nav_prog_node.end();
-            switch (func.mir.status.load(.monotonic)) {
+            switch (func.mir.status.load(.acquire)) {
                 .pending => unreachable,
                 .ready => {},
                 .failed => return,

src/link/Queue.zig

@@ -64,7 +64,7 @@ state: union(enum) {
     finished,
     /// The link thread is not running or queued, because it is waiting for this MIR to be populated.
     /// Once codegen completes, it must call `mirReady` which will restart the link thread.
-    wait_for_mir: *ZcuTask.LinkFunc.SharedMir,
+    wait_for_mir: InternPool.Index,
 },
 
 /// In the worst observed case, MIR is around 50 times as large as AIR. More typically, the ratio is
@@ -113,20 +113,20 @@ pub fn start(q: *Queue, comp: *Compilation) void {
 
 /// Called by codegen workers after they have populated a `ZcuTask.LinkFunc.SharedMir`. If the link
 /// thread was waiting for this MIR, it can resume.
-pub fn mirReady(q: *Queue, comp: *Compilation, mir: *ZcuTask.LinkFunc.SharedMir) void {
+pub fn mirReady(q: *Queue, comp: *Compilation, func_index: InternPool.Index, mir: *ZcuTask.LinkFunc.SharedMir) void {
     // We would like to assert that `mir` is not pending, but that would race with a worker thread
     // potentially freeing it.
     {
         q.mutex.lock();
         defer q.mutex.unlock();
         switch (q.state) {
             .finished, .running => return,
-            .wait_for_mir => |wait_for| if (wait_for != mir) return,
+            .wait_for_mir => |wait_for| if (wait_for != func_index) return,
         }
         // We were waiting for `mir`, so we will restart the linker thread.
         q.state = .running;
     }
-    assert(mir.status.load(.monotonic) != .pending);
+    assert(mir.status.load(.acquire) != .pending);
     comp.thread_pool.spawnWgId(&comp.link_task_wait_group, flushTaskQueue, .{ q, comp });
 }
 
@@ -170,8 +170,8 @@ pub fn enqueueZcu(q: *Queue, comp: *Compilation, task: ZcuTask) Allocator.Error!
             .finished => if (q.pending_prelink_tasks != 0) return,
         }
         // Restart the linker thread, unless it would immediately be blocked
-        if (task == .link_func and task.link_func.mir.status.load(.monotonic) == .pending) {
-            q.state = .{ .wait_for_mir = task.link_func.mir };
+        if (task == .link_func and task.link_func.mir.status.load(.acquire) == .pending) {
+            q.state = .{ .wait_for_mir = task.link_func.func };
             return;
         }
         q.state = .running;
@@ -243,12 +243,12 @@ fn flushTaskQueue(tid: usize, q: *Queue, comp: *Compilation) void {
             if (task != .link_func) break :pending;
             const status_ptr = &task.link_func.mir.status;
             // First check without the mutex to optimize for the common case where MIR is ready.
-            if (status_ptr.load(.monotonic) != .pending) break :pending;
+            if (status_ptr.load(.acquire) != .pending) break :pending;
             q.mutex.lock();
             defer q.mutex.unlock();
-            if (status_ptr.load(.monotonic) != .pending) break :pending;
+            if (status_ptr.load(.acquire) != .pending) break :pending;
             // We will stop for now, and get restarted once this MIR is ready.
-            q.state = .{ .wait_for_mir = task.link_func.mir };
+            q.state = .{ .wait_for_mir = task.link_func.func };
             q.flush_safety.unlock();
             return;
         }
@@ -273,6 +273,7 @@ const std = @import("std");
 const assert = std.debug.assert;
 const Allocator = std.mem.Allocator;
 const Compilation = @import("../Compilation.zig");
+const InternPool = @import("../InternPool.zig");
 const link = @import("../link.zig");
 const PrelinkTask = link.PrelinkTask;
 const ZcuTask = link.ZcuTask;