Commit 404dc9692e33099cc59925d5cf03805224fcb36e | zig

author	Andrew Kelley <andrew@ziglang.org> 2021-01-03 20:25:04-07:00
committer	Andrew Kelley <andrew@ziglang.org> 2021-01-03 20:34:51-07:00
log	404dc9692e33099cc59925d5cf03805224fcb36e
tree	0701332eab1d990a655ae9a6dcb3975d0b2c1737
parent	5c92e24a29ca403cc66515044a25d341a724f093

stage2: fix Cache debug deadlock code memory leak

3 files changed, 30 insertions(+), 19 deletions(-)

src/Cache.zig+24-18

 @@ -16,8 +16,8 @@ const Allocator = std.mem.Allocator;
 /// 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 = .{};
-// TODO: Figure out how to make sure that `all_cache_digest_set` does not leak memory!
+var all_cache_digest_allocator: ?*Allocator = null;
-pub const want_debug_deadlock = false;
+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 {};
 @@ -273,6 +273,14 @@ pub const Manifest = struct {
             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});
 @@ -717,15 +725,22 @@ 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 if (want_debug_deadlock) {
+    defer deinitDebugMap();
-        testing.expect(all_cache_digest_set.count() == 0);
-        all_cache_digest_set.clearAndFree(testing.allocator);
-    };
     const cwd = fs.cwd();
 @@ -804,10 +819,7 @@ test "check that changing a file makes cache fail" {
         // https://github.com/ziglang/zig/issues/5437
         return error.SkipZigTest;
     }
-    defer if (want_debug_deadlock) {
+    defer deinitDebugMap();
-        testing.expect(all_cache_digest_set.count() == 0);
-        all_cache_digest_set.clearAndFree(testing.allocator);
-    };
     const cwd = fs.cwd();
     const temp_file = "cache_hash_change_file_test.txt";
 @@ -884,10 +896,7 @@ test "no file inputs" {
         // https://github.com/ziglang/zig/issues/5437
         return error.SkipZigTest;
     }
-    defer if (want_debug_deadlock) {
+    defer deinitDebugMap();
-        testing.expect(all_cache_digest_set.count() == 0);
-        all_cache_digest_set.clearAndFree(testing.allocator);
-    };
     const cwd = fs.cwd();
     const temp_manifest_dir = "no_file_inputs_manifest_dir";
     defer cwd.deleteTree(temp_manifest_dir) catch {};
 @@ -933,10 +942,7 @@ test "Manifest with files added after initial hash work" {
         // https://github.com/ziglang/zig/issues/5437
         return error.SkipZigTest;
     }
-    defer if (want_debug_deadlock) {
+    defer deinitDebugMap();
-        testing.expect(all_cache_digest_set.count() == 0);
-        all_cache_digest_set.clearAndFree(testing.allocator);
-    };
     const cwd = fs.cwd();
     const temp_file1 = "cache_hash_post_file_test1.txt";

src/main.zig+5-1

...	@@ -104,7 +104,10 @@ pub fn log(	...	@@ -104,7 +104,10 @@ pub fn log(
104	var general_purpose_allocator = std.heap.GeneralPurposeAllocator(.{}){};	104	var general_purpose_allocator = std.heap.GeneralPurposeAllocator(.{}){};
105		105
106	pub fn main() anyerror!void {	106	pub fn main() anyerror!void {
107	const gpa = if (std.builtin.link_libc) std.heap.raw_c_allocator else &general_purpose_allocator.allocator;	107	const gpa = if (std.builtin.link_libc)
		108	std.heap.raw_c_allocator
		109	else
		110	&general_purpose_allocator.allocator;
108	defer if (!std.builtin.link_libc) {	111	defer if (!std.builtin.link_libc) {
109	_ = general_purpose_allocator.deinit();	112	_ = general_purpose_allocator.deinit();
110	};	113	};
...	@@ -3289,6 +3292,7 @@ fn detectNativeTargetInfo(gpa: *Allocator, cross_target: std.zig.CrossTarget) !s	...	@@ -3289,6 +3292,7 @@ fn detectNativeTargetInfo(gpa: *Allocator, cross_target: std.zig.CrossTarget) !s
3289	/// calls exit(0), and does not return.	3292	/// calls exit(0), and does not return.
3290	pub fn cleanExit() void {	3293	pub fn cleanExit() void {
3291	if (std.builtin.mode == .Debug) {	3294	if (std.builtin.mode == .Debug) {
		3295	Cache.deinitDebugMap();
3292	return;	3296	return;
3293	} else {	3297	} else {
3294	process.exit(0);	3298	process.exit(0);

src/test.zig+1

...	@@ -518,6 +518,7 @@ pub const TestContext = struct {	...	@@ -518,6 +518,7 @@ pub const TestContext = struct {
518	case.updates.deinit();	518	case.updates.deinit();
519	}	519	}
520	self.cases.deinit();	520	self.cases.deinit();
		521	@import("Cache.zig").deinitDebugMap();
521	self.* = undefined;	522	self.* = undefined;
522	}	523	}
523		524