authorgravatar for kcbanner@gmail.comCasey Banner <kcbanner@gmail.com> 2022-12-06 23:15:54-05:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2023-01-09 15:00:03-07:00
loga4418a8fd6663f532a021c3ad2ae807a6ccd62cc
tree4215b70f84b4dceb2d78bf2014a6fdd334e7d158
parentbb957a976d214915c51d72a8112ef905a3777602

cache: Fix LockViolation during C compilation paths (#13591)

- C compilation flows didn't hold an exclusive lock on the cache manifest file when writing to it in all cases - On windows, explicitly unlock the file lock before closing it

3 files changed, 23 insertions(+), 1 deletions(-)

src/Cache.zig+18-1
...@@ -170,6 +170,12 @@ pub const Lock = struct {...@@ -170,6 +170,12 @@ pub const Lock = struct {
170 manifest_file: fs.File,170 manifest_file: fs.File,
171171
172 pub fn release(lock: *Lock) void {172 pub fn release(lock: *Lock) void {
173 if (builtin.os.tag == .windows) {
174 // Windows does not guarantee that locks are immediately unlocked when
175 // the file handle is closed. See LockFileEx documentation.
176 lock.manifest_file.unlock();
177 }
178
173 lock.manifest_file.close();179 lock.manifest_file.close();
174 lock.* = undefined;180 lock.* = undefined;
175 }181 }
...@@ -480,7 +486,10 @@ pub const Manifest = struct {...@@ -480,7 +486,10 @@ pub const Manifest = struct {
480 return false;486 return false;
481 }487 }
482488
483 try self.downgradeToSharedLock();489 if (self.want_shared_lock) {
490 try self.downgradeToSharedLock();
491 }
492
484 return true;493 return true;
485 }494 }
486495
...@@ -770,11 +779,13 @@ pub const Manifest = struct {...@@ -770,11 +779,13 @@ pub const Manifest = struct {
770 const manifest_file = self.manifest_file.?;779 const manifest_file = self.manifest_file.?;
771 try manifest_file.downgradeLock();780 try manifest_file.downgradeLock();
772 }781 }
782
773 self.have_exclusive_lock = false;783 self.have_exclusive_lock = false;
774 }784 }
775785
776 fn upgradeToExclusiveLock(self: *Manifest) !void {786 fn upgradeToExclusiveLock(self: *Manifest) !void {
777 if (self.have_exclusive_lock) return;787 if (self.have_exclusive_lock) return;
788 assert(self.manifest_file != null);
778789
779 // WASI does not currently support flock, so we bypass it here.790 // WASI does not currently support flock, so we bypass it here.
780 // TODO: If/when flock is supported on WASI, this check should be removed.791 // TODO: If/when flock is supported on WASI, this check should be removed.
...@@ -796,6 +807,7 @@ pub const Manifest = struct {...@@ -796,6 +807,7 @@ pub const Manifest = struct {
796 const lock: Lock = .{807 const lock: Lock = .{
797 .manifest_file = self.manifest_file.?,808 .manifest_file = self.manifest_file.?,
798 };809 };
810
799 self.manifest_file = null;811 self.manifest_file = null;
800 return lock;812 return lock;
801 }813 }
...@@ -805,6 +817,11 @@ pub const Manifest = struct {...@@ -805,6 +817,11 @@ pub const Manifest = struct {
805 /// Don't forget to call `writeManifest` before this!817 /// Don't forget to call `writeManifest` before this!
806 pub fn deinit(self: *Manifest) void {818 pub fn deinit(self: *Manifest) void {
807 if (self.manifest_file) |file| {819 if (self.manifest_file) |file| {
820 if (builtin.os.tag == .windows) {
821 // See Lock.release for why this is required on Windows
822 file.unlock();
823 }
824
808 file.close();825 file.close();
809 }826 }
810 for (self.files.items) |*file| {827 for (self.files.items) |*file| {
src/Compilation.zig+4
...@@ -3580,6 +3580,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {...@@ -3580,6 +3580,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
3580 const cimport_zig_basename = "cimport.zig";3580 const cimport_zig_basename = "cimport.zig";
35813581
3582 var man = comp.obtainCObjectCacheManifest();3582 var man = comp.obtainCObjectCacheManifest();
3583 man.want_shared_lock = false;
3583 defer man.deinit();3584 defer man.deinit();
35843585
3585 const use_stage1 = build_options.have_stage1 and comp.bin_file.options.use_stage1;3586 const use_stage1 = build_options.have_stage1 and comp.bin_file.options.use_stage1;
...@@ -3697,6 +3698,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {...@@ -3697,6 +3698,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
3697 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but3698 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
3698 // the contents were the same, we hit the cache but the manifest is dirty and we need to update3699 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
3699 // it to prevent doing a full file content comparison the next time around.3700 // it to prevent doing a full file content comparison the next time around.
3701 man.want_shared_lock = true;
3700 man.writeManifest() catch |err| {3702 man.writeManifest() catch |err| {
3701 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});3703 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
3702 };3704 };
...@@ -3871,6 +3873,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P...@@ -3871,6 +3873,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
3871 }3873 }
38723874
3873 var man = comp.obtainCObjectCacheManifest();3875 var man = comp.obtainCObjectCacheManifest();
3876 man.want_shared_lock = false;
3874 defer man.deinit();3877 defer man.deinit();
38753878
3876 man.hash.add(comp.clang_preprocessor_mode);3879 man.hash.add(comp.clang_preprocessor_mode);
...@@ -4164,6 +4167,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P...@@ -4164,6 +4167,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
4164 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but4167 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
4165 // the contents were the same, we hit the cache but the manifest is dirty and we need to update4168 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
4166 // it to prevent doing a full file content comparison the next time around.4169 // it to prevent doing a full file content comparison the next time around.
4170 man.want_shared_lock = true;
4167 man.writeManifest() catch |err| {4171 man.writeManifest() catch |err| {
4168 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });4172 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
4169 };4173 };
src/main.zig+1
...@@ -3429,6 +3429,7 @@ fn cmdTranslateC(comp: *Compilation, arena: Allocator, enable_cache: bool, stage...@@ -3429,6 +3429,7 @@ fn cmdTranslateC(comp: *Compilation, arena: Allocator, enable_cache: bool, stage
3429 const translated_zig_basename = try std.fmt.allocPrint(arena, "{s}.zig", .{comp.bin_file.options.root_name});3429 const translated_zig_basename = try std.fmt.allocPrint(arena, "{s}.zig", .{comp.bin_file.options.root_name});
34303430
3431 var man: Cache.Manifest = comp.obtainCObjectCacheManifest();3431 var man: Cache.Manifest = comp.obtainCObjectCacheManifest();
3432 man.want_shared_lock = false;
3432 defer if (enable_cache) man.deinit();3433 defer if (enable_cache) man.deinit();
34333434
3434 man.hash.add(@as(u16, 0xb945)); // Random number to distinguish translate-c from compiling C objects3435 man.hash.add(@as(u16, 0xb945)); // Random number to distinguish translate-c from compiling C objects