Skip to content

Commit 404dc96

Browse files
committed
stage2: fix Cache debug deadlock code memory leak
1 parent 5c92e24 commit 404dc96

File tree

3 files changed

+30
-19
lines changed

3 files changed

+30
-19
lines changed

src/Cache.zig

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ const Allocator = std.mem.Allocator;
1616
/// This protection is conditionally compiled depending on `want_debug_deadlock`.
1717
var all_cache_digest_set: std.AutoHashMapUnmanaged(BinDigest, void) = .{};
1818
var all_cache_digest_lock: std.Mutex = .{};
19-
// TODO: Figure out how to make sure that `all_cache_digest_set` does not leak memory!
20-
pub const want_debug_deadlock = false;
19+
var all_cache_digest_allocator: ?*Allocator = null;
20+
const want_debug_deadlock = std.debug.runtime_safety;
2121
const DebugBinDigest = if (want_debug_deadlock) BinDigest else void;
2222
const null_debug_bin_digest = if (want_debug_deadlock) ([1]u8{0} ** bin_digest_len) else {};
2323

@@ -273,6 +273,14 @@ pub const Manifest = struct {
273273
const held = all_cache_digest_lock.acquire();
274274
defer held.release();
275275

276+
if (all_cache_digest_allocator) |prev_gpa| {
277+
if (prev_gpa != self.cache.gpa) {
278+
@panic("The deadlock debug code in Cache depends on using the same allocator for everything");
279+
}
280+
} else {
281+
all_cache_digest_allocator = self.cache.gpa;
282+
}
283+
276284
const gop = try all_cache_digest_set.getOrPut(self.cache.gpa, bin_digest);
277285
if (gop.found_existing) {
278286
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 {
717725
return wall_nsec == fs_nsec and wall_sec == fs_sec;
718726
}
719727

728+
pub fn deinitDebugMap() void {
729+
if (!want_debug_deadlock) return;
730+
731+
if (all_cache_digest_set.count() != 0) {
732+
@panic("there's a Cache not deinitialized somewhere");
733+
}
734+
const gpa = all_cache_digest_allocator orelse return;
735+
all_cache_digest_set.clearAndFree(gpa);
736+
}
737+
720738
test "cache file and then recall it" {
721739
if (std.Target.current.os.tag == .wasi) {
722740
// https://github.com/ziglang/zig/issues/5437
723741
return error.SkipZigTest;
724742
}
725-
defer if (want_debug_deadlock) {
726-
testing.expect(all_cache_digest_set.count() == 0);
727-
all_cache_digest_set.clearAndFree(testing.allocator);
728-
};
743+
defer deinitDebugMap();
729744

730745
const cwd = fs.cwd();
731746

@@ -804,10 +819,7 @@ test "check that changing a file makes cache fail" {
804819
// https://github.com/ziglang/zig/issues/5437
805820
return error.SkipZigTest;
806821
}
807-
defer if (want_debug_deadlock) {
808-
testing.expect(all_cache_digest_set.count() == 0);
809-
all_cache_digest_set.clearAndFree(testing.allocator);
810-
};
822+
defer deinitDebugMap();
811823
const cwd = fs.cwd();
812824

813825
const temp_file = "cache_hash_change_file_test.txt";
@@ -884,10 +896,7 @@ test "no file inputs" {
884896
// https://github.com/ziglang/zig/issues/5437
885897
return error.SkipZigTest;
886898
}
887-
defer if (want_debug_deadlock) {
888-
testing.expect(all_cache_digest_set.count() == 0);
889-
all_cache_digest_set.clearAndFree(testing.allocator);
890-
};
899+
defer deinitDebugMap();
891900
const cwd = fs.cwd();
892901
const temp_manifest_dir = "no_file_inputs_manifest_dir";
893902
defer cwd.deleteTree(temp_manifest_dir) catch {};
@@ -933,10 +942,7 @@ test "Manifest with files added after initial hash work" {
933942
// https://github.com/ziglang/zig/issues/5437
934943
return error.SkipZigTest;
935944
}
936-
defer if (want_debug_deadlock) {
937-
testing.expect(all_cache_digest_set.count() == 0);
938-
all_cache_digest_set.clearAndFree(testing.allocator);
939-
};
945+
defer deinitDebugMap();
940946
const cwd = fs.cwd();
941947

942948
const temp_file1 = "cache_hash_post_file_test1.txt";

src/main.zig

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,10 @@ pub fn log(
104104
var general_purpose_allocator = std.heap.GeneralPurposeAllocator(.{}){};
105105

106106
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;
108111
defer if (!std.builtin.link_libc) {
109112
_ = general_purpose_allocator.deinit();
110113
};
@@ -3289,6 +3292,7 @@ fn detectNativeTargetInfo(gpa: *Allocator, cross_target: std.zig.CrossTarget) !s
32893292
/// calls exit(0), and does not return.
32903293
pub fn cleanExit() void {
32913294
if (std.builtin.mode == .Debug) {
3295+
Cache.deinitDebugMap();
32923296
return;
32933297
} else {
32943298
process.exit(0);

src/test.zig

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,6 +518,7 @@ pub const TestContext = struct {
518518
case.updates.deinit();
519519
}
520520
self.cases.deinit();
521+
@import("Cache.zig").deinitDebugMap();
521522
self.* = undefined;
522523
}
523524

0 commit comments

Comments
 (0)