Commit c8e44d82bd

Andrew Kelley <andrew@ziglang.org>
2021-01-04 04:34:17
stage2: remove the Cache deadlock detection code
It's more trouble than it's worth; it didn't even catch the most recent incident because it was across process boundaries anyway.
1 parent 404dc96
Changed files (3)
src/Cache.zig
@@ -12,15 +12,6 @@ const mem = std.mem;
 const fmt = std.fmt;
 const Allocator = std.mem.Allocator;
 
-/// Process-scoped map keeping track of all locked Cache hashes, to detect deadlocks.
-/// This protection is conditionally compiled depending on `want_debug_deadlock`.
-var all_cache_digest_set: std.AutoHashMapUnmanaged(BinDigest, void) = .{};
-var all_cache_digest_lock: std.Mutex = .{};
-var all_cache_digest_allocator: ?*Allocator = null;
-const want_debug_deadlock = std.debug.runtime_safety;
-const DebugBinDigest = if (want_debug_deadlock) BinDigest else void;
-const null_debug_bin_digest = if (want_debug_deadlock) ([1]u8{0} ** bin_digest_len) else {};
-
 /// Be sure to call `Manifest.deinit` after successful initialization.
 pub fn obtain(cache: *const Cache) Manifest {
     return Manifest{
@@ -169,15 +160,8 @@ pub const HashHelper = struct {
 
 pub const Lock = struct {
     manifest_file: fs.File,
-    debug_bin_digest: DebugBinDigest,
 
     pub fn release(lock: *Lock) void {
-        if (want_debug_deadlock) {
-            const held = all_cache_digest_lock.acquire();
-            defer held.release();
-
-            all_cache_digest_set.removeAssertDiscard(lock.debug_bin_digest);
-        }
         lock.manifest_file.close();
         lock.* = undefined;
     }
@@ -194,7 +178,6 @@ pub const Manifest = struct {
     manifest_dirty: bool,
     files: std.ArrayListUnmanaged(File) = .{},
     hex_digest: [hex_digest_len]u8,
-    debug_bin_digest: DebugBinDigest = null_debug_bin_digest,
     /// Populated when hit() returns an error because of one
     /// of the files listed in the manifest.
     failed_file_index: ?usize = null,
@@ -267,31 +250,6 @@ pub const Manifest = struct {
         var bin_digest: BinDigest = undefined;
         self.hash.hasher.final(&bin_digest);
 
-        if (want_debug_deadlock) {
-            self.debug_bin_digest = bin_digest;
-
-            const held = all_cache_digest_lock.acquire();
-            defer held.release();
-
-            if (all_cache_digest_allocator) |prev_gpa| {
-                if (prev_gpa != self.cache.gpa) {
-                    @panic("The deadlock debug code in Cache depends on using the same allocator for everything");
-                }
-            } else {
-                all_cache_digest_allocator = self.cache.gpa;
-            }
-
-            const gop = try all_cache_digest_set.getOrPut(self.cache.gpa, bin_digest);
-            if (gop.found_existing) {
-                std.debug.print("Cache deadlock detected in Cache.hit. Manifest has {d} files:\n", .{self.files.items.len});
-                for (self.files.items) |file| {
-                    const p: []const u8 = file.path orelse "(null)";
-                    std.debug.print("  file: {s}\n", .{p});
-                }
-                @panic("Cache deadlock detected");
-            }
-        }
-
         _ = std.fmt.bufPrint(&self.hex_digest, "{x}", .{bin_digest}) catch unreachable;
 
         self.hash.hasher = hasher_init;
@@ -628,10 +586,8 @@ pub const Manifest = struct {
     pub fn toOwnedLock(self: *Manifest) Lock {
         const lock: Lock = .{
             .manifest_file = self.manifest_file.?,
-            .debug_bin_digest = self.debug_bin_digest,
         };
         self.manifest_file = null;
-        self.debug_bin_digest = null_debug_bin_digest;
         return lock;
     }
 
@@ -639,14 +595,6 @@ pub const Manifest = struct {
     /// `Manifest.hit` must be called first.
     /// Don't forget to call `writeManifest` before this!
     pub fn deinit(self: *Manifest) void {
-        if (want_debug_deadlock) {
-            if (!mem.eql(u8, &self.debug_bin_digest, &null_debug_bin_digest)) {
-                const held = all_cache_digest_lock.acquire();
-                defer held.release();
-
-                all_cache_digest_set.removeAssertDiscard(self.debug_bin_digest);
-            }
-        }
         if (self.manifest_file) |file| {
             file.close();
         }
@@ -725,22 +673,11 @@ fn isProblematicTimestamp(fs_clock: i128) bool {
     return wall_nsec == fs_nsec and wall_sec == fs_sec;
 }
 
-pub fn deinitDebugMap() void {
-    if (!want_debug_deadlock) return;
-
-    if (all_cache_digest_set.count() != 0) {
-        @panic("there's a Cache not deinitialized somewhere");
-    }
-    const gpa = all_cache_digest_allocator orelse return;
-    all_cache_digest_set.clearAndFree(gpa);
-}
-
 test "cache file and then recall it" {
     if (std.Target.current.os.tag == .wasi) {
         // https://github.com/ziglang/zig/issues/5437
         return error.SkipZigTest;
     }
-    defer deinitDebugMap();
 
     const cwd = fs.cwd();
 
@@ -819,7 +756,6 @@ test "check that changing a file makes cache fail" {
         // https://github.com/ziglang/zig/issues/5437
         return error.SkipZigTest;
     }
-    defer deinitDebugMap();
     const cwd = fs.cwd();
 
     const temp_file = "cache_hash_change_file_test.txt";
@@ -896,7 +832,6 @@ test "no file inputs" {
         // https://github.com/ziglang/zig/issues/5437
         return error.SkipZigTest;
     }
-    defer deinitDebugMap();
     const cwd = fs.cwd();
     const temp_manifest_dir = "no_file_inputs_manifest_dir";
     defer cwd.deleteTree(temp_manifest_dir) catch {};
@@ -942,7 +877,6 @@ test "Manifest with files added after initial hash work" {
         // https://github.com/ziglang/zig/issues/5437
         return error.SkipZigTest;
     }
-    defer deinitDebugMap();
     const cwd = fs.cwd();
 
     const temp_file1 = "cache_hash_post_file_test1.txt";
src/main.zig
@@ -3292,7 +3292,6 @@ fn detectNativeTargetInfo(gpa: *Allocator, cross_target: std.zig.CrossTarget) !s
 /// calls exit(0), and does not return.
 pub fn cleanExit() void {
     if (std.builtin.mode == .Debug) {
-        Cache.deinitDebugMap();
         return;
     } else {
         process.exit(0);
src/test.zig
@@ -518,7 +518,6 @@ pub const TestContext = struct {
             case.updates.deinit();
         }
         self.cases.deinit();
-        @import("Cache.zig").deinitDebugMap();
         self.* = undefined;
     }