authorgravatar for kcbanner@gmail.comCasey Banner <kcbanner@gmail.com> 2023-01-04 14:51:43-05:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2023-01-09 15:15:27-07:00
logdc18b8174ae6dacb3bc32fc842363159916bd657
treed81a4d9313c698fb101ae14c02d093e33a197467
parent7d538d6e536aedc3d71c806b8831c73bb5dd0c62

Fix another LockViolation case on Windows (#14162)

- Add an assert that an exclusive lock is help to writeManifest - Only call writeManifest in updateCObject if an exclusive lock is held - cache: fixup test to verify hits don't take an exclusive lock, instead of writing the manifest

2 files changed, 23 insertions(+), 17 deletions(-)

src/Cache.zig+5-3
...@@ -735,6 +735,8 @@ pub const Manifest = struct {...@@ -735,6 +735,8 @@ pub const Manifest = struct {
735 /// If `want_shared_lock` is true, this function automatically downgrades the735 /// If `want_shared_lock` is true, this function automatically downgrades the
736 /// lock from exclusive to shared.736 /// lock from exclusive to shared.
737 pub fn writeManifest(self: *Manifest) !void {737 pub fn writeManifest(self: *Manifest) !void {
738 assert(self.have_exclusive_lock);
739
738 const manifest_file = self.manifest_file.?;740 const manifest_file = self.manifest_file.?;
739 if (self.manifest_dirty) {741 if (self.manifest_dirty) {
740 self.manifest_dirty = false;742 self.manifest_dirty = false;
...@@ -936,7 +938,7 @@ test "cache file and then recall it" {...@@ -936,7 +938,7 @@ test "cache file and then recall it" {
936 try testing.expect(try ch.hit());938 try testing.expect(try ch.hit());
937 digest2 = ch.final();939 digest2 = ch.final();
938940
939 try ch.writeManifest();941 try testing.expectEqual(false, ch.have_exclusive_lock);
940 }942 }
941943
942 try testing.expectEqual(digest1, digest2);944 try testing.expectEqual(digest1, digest2);
...@@ -1062,7 +1064,7 @@ test "no file inputs" {...@@ -1062,7 +1064,7 @@ test "no file inputs" {
10621064
1063 try testing.expect(try man.hit());1065 try testing.expect(try man.hit());
1064 digest2 = man.final();1066 digest2 = man.final();
1065 try man.writeManifest();1067 try testing.expectEqual(false, man.have_exclusive_lock);
1066 }1068 }
10671069
1068 try testing.expectEqual(digest1, digest2);1070 try testing.expectEqual(digest1, digest2);
...@@ -1124,7 +1126,7 @@ test "Manifest with files added after initial hash work" {...@@ -1124,7 +1126,7 @@ test "Manifest with files added after initial hash work" {
1124 try testing.expect(try ch.hit());1126 try testing.expect(try ch.hit());
1125 digest2 = ch.final();1127 digest2 = ch.final();
11261128
1127 try ch.writeManifest();1129 try testing.expectEqual(false, ch.have_exclusive_lock);
1128 }1130 }
1129 try testing.expect(mem.eql(u8, &digest1, &digest2));1131 try testing.expect(mem.eql(u8, &digest1, &digest2));
11301132
src/Compilation.zig+18-14
...@@ -3693,13 +3693,15 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {...@@ -3693,13 +3693,15 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
3693 break :digest digest;3693 break :digest digest;
3694 } else man.final();3694 } else man.final();
36953695
3696 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is3696 if (man.have_exclusive_lock) {
3697 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but3697 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
3698 // the contents were the same, we hit the cache but the manifest is dirty and we need to update3698 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
3699 // it to prevent doing a full file content comparison the next time around.3699 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
3700 man.writeManifest() catch |err| {3700 // it to prevent doing a full file content comparison the next time around.
3701 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});3701 man.writeManifest() catch |err| {
3702 };3702 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
3703 };
3704 }
37033705
3704 const out_zig_path = try comp.local_cache_directory.join(comp.gpa, &[_][]const u8{3706 const out_zig_path = try comp.local_cache_directory.join(comp.gpa, &[_][]const u8{
3705 "o", &digest, cimport_zig_basename,3707 "o", &digest, cimport_zig_basename,
...@@ -4160,13 +4162,15 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P...@@ -4160,13 +4162,15 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
4160 break :blk digest;4162 break :blk digest;
4161 };4163 };
41624164
4163 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is4165 if (man.have_exclusive_lock) {
4164 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but4166 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
4165 // the contents were the same, we hit the cache but the manifest is dirty and we need to update4167 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
4166 // it to prevent doing a full file content comparison the next time around.4168 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
4167 man.writeManifest() catch |err| {4169 // it to prevent doing a full file content comparison the next time around.
4168 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });4170 man.writeManifest() catch |err| {
4169 };4171 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
4172 };
4173 }
41704174
4171 const o_basename = try std.fmt.allocPrint(arena, "{s}{s}", .{ o_basename_noext, o_ext });4175 const o_basename = try std.fmt.allocPrint(arena, "{s}{s}", .{ o_basename_noext, o_ext });
41724176