authorgravatar for kcbanner@gmail.comCasey Banner <kcbanner@gmail.com> 2023-01-04 14:51:43-05:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2023-01-04 14:51:43-05:00
loga3e2ee091139740d076da1176ac8c487ca20a9a6
tree9b8c8aaaa8166a7b0f338b0b07755e936c66ec1e
parent9ed4a93ae7244a02071196e8913f43bcc9144c25
signaturebadge-question-mark Signed by PGP key 4AEE18F83AFDEB23

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
...@@ -830,6 +830,8 @@ pub const Manifest = struct {...@@ -830,6 +830,8 @@ pub const Manifest = struct {
830 /// If `want_shared_lock` is true, this function automatically downgrades the830 /// If `want_shared_lock` is true, this function automatically downgrades the
831 /// lock from exclusive to shared.831 /// lock from exclusive to shared.
832 pub fn writeManifest(self: *Manifest) !void {832 pub fn writeManifest(self: *Manifest) !void {
833 assert(self.have_exclusive_lock);
834
833 const manifest_file = self.manifest_file.?;835 const manifest_file = self.manifest_file.?;
834 if (self.manifest_dirty) {836 if (self.manifest_dirty) {
835 self.manifest_dirty = false;837 self.manifest_dirty = false;
...@@ -1033,7 +1035,7 @@ test "cache file and then recall it" {...@@ -1033,7 +1035,7 @@ test "cache file and then recall it" {
1033 try testing.expect(try ch.hit());1035 try testing.expect(try ch.hit());
1034 digest2 = ch.final();1036 digest2 = ch.final();
10351037
1036 try ch.writeManifest();1038 try testing.expectEqual(false, ch.have_exclusive_lock);
1037 }1039 }
10381040
1039 try testing.expectEqual(digest1, digest2);1041 try testing.expectEqual(digest1, digest2);
...@@ -1161,7 +1163,7 @@ test "no file inputs" {...@@ -1161,7 +1163,7 @@ test "no file inputs" {
11611163
1162 try testing.expect(try man.hit());1164 try testing.expect(try man.hit());
1163 digest2 = man.final();1165 digest2 = man.final();
1164 try man.writeManifest();1166 try testing.expectEqual(false, man.have_exclusive_lock);
1165 }1167 }
11661168
1167 try testing.expectEqual(digest1, digest2);1169 try testing.expectEqual(digest1, digest2);
...@@ -1224,7 +1226,7 @@ test "Manifest with files added after initial hash work" {...@@ -1224,7 +1226,7 @@ test "Manifest with files added after initial hash work" {
1224 try testing.expect(try ch.hit());1226 try testing.expect(try ch.hit());
1225 digest2 = ch.final();1227 digest2 = ch.final();
12261228
1227 try ch.writeManifest();1229 try testing.expectEqual(false, ch.have_exclusive_lock);
1228 }1230 }
1229 try testing.expect(mem.eql(u8, &digest1, &digest2));1231 try testing.expect(mem.eql(u8, &digest1, &digest2));
12301232
src/Compilation.zig+18-14
...@@ -3681,13 +3681,15 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {...@@ -3681,13 +3681,15 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
3681 break :digest digest;3681 break :digest digest;
3682 } else man.final();3682 } else man.final();
36833683
3684 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is3684 if (man.have_exclusive_lock) {
3685 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but3685 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
3686 // the contents were the same, we hit the cache but the manifest is dirty and we need to update3686 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
3687 // it to prevent doing a full file content comparison the next time around.3687 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
3688 man.writeManifest() catch |err| {3688 // it to prevent doing a full file content comparison the next time around.
3689 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});3689 man.writeManifest() catch |err| {
3690 };3690 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
3691 };
3692 }
36913693
3692 const out_zig_path = try comp.local_cache_directory.join(comp.gpa, &[_][]const u8{3694 const out_zig_path = try comp.local_cache_directory.join(comp.gpa, &[_][]const u8{
3693 "o", &digest, cimport_zig_basename,3695 "o", &digest, cimport_zig_basename,
...@@ -4086,13 +4088,15 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P...@@ -4086,13 +4088,15 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
4086 break :blk digest;4088 break :blk digest;
4087 };4089 };
40884090
4089 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is4091 if (man.have_exclusive_lock) {
4090 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but4092 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
4091 // the contents were the same, we hit the cache but the manifest is dirty and we need to update4093 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
4092 // it to prevent doing a full file content comparison the next time around.4094 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
4093 man.writeManifest() catch |err| {4095 // it to prevent doing a full file content comparison the next time around.
4094 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });4096 man.writeManifest() catch |err| {
4095 };4097 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
4098 };
4099 }
40964100
4097 const o_basename = try std.fmt.allocPrint(arena, "{s}{s}", .{ o_basename_noext, o_ext });4101 const o_basename = try std.fmt.allocPrint(arena, "{s}{s}", .{ o_basename_noext, o_ext });
40984102