authorgravatar for kcbanner@gmail.comCasey Banner <kcbanner@gmail.com> 2022-12-06 23:15:54-05:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2022-12-06 23:15:54-05:00
log8ccb9a6ad327a4d7fbc321b33d4aa66a27a1f5ee
tree13556a64c4a9af587f12605085b4c880334772e3
parent14416b522e697997f21dd634bfdc70fbff59471b
signaturebadge-question-mark Signed by PGP key 4AEE18F83AFDEB23

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
...@@ -232,6 +232,12 @@ pub const Lock = struct {...@@ -232,6 +232,12 @@ pub const Lock = struct {
232 manifest_file: fs.File,232 manifest_file: fs.File,
233233
234 pub fn release(lock: *Lock) void {234 pub fn release(lock: *Lock) void {
235 if (builtin.os.tag == .windows) {
236 // Windows does not guarantee that locks are immediately unlocked when
237 // the file handle is closed. See LockFileEx documentation.
238 lock.manifest_file.unlock();
239 }
240
235 lock.manifest_file.close();241 lock.manifest_file.close();
236 lock.* = undefined;242 lock.* = undefined;
237 }243 }
...@@ -554,7 +560,10 @@ pub const Manifest = struct {...@@ -554,7 +560,10 @@ pub const Manifest = struct {
554 return false;560 return false;
555 }561 }
556562
557 try self.downgradeToSharedLock();563 if (self.want_shared_lock) {
564 try self.downgradeToSharedLock();
565 }
566
558 return true;567 return true;
559 }568 }
560569
...@@ -866,11 +875,13 @@ pub const Manifest = struct {...@@ -866,11 +875,13 @@ pub const Manifest = struct {
866 const manifest_file = self.manifest_file.?;875 const manifest_file = self.manifest_file.?;
867 try manifest_file.downgradeLock();876 try manifest_file.downgradeLock();
868 }877 }
878
869 self.have_exclusive_lock = false;879 self.have_exclusive_lock = false;
870 }880 }
871881
872 fn upgradeToExclusiveLock(self: *Manifest) !void {882 fn upgradeToExclusiveLock(self: *Manifest) !void {
873 if (self.have_exclusive_lock) return;883 if (self.have_exclusive_lock) return;
884 assert(self.manifest_file != null);
874885
875 // WASI does not currently support flock, so we bypass it here.886 // WASI does not currently support flock, so we bypass it here.
876 // TODO: If/when flock is supported on WASI, this check should be removed.887 // TODO: If/when flock is supported on WASI, this check should be removed.
...@@ -892,6 +903,7 @@ pub const Manifest = struct {...@@ -892,6 +903,7 @@ pub const Manifest = struct {
892 const lock: Lock = .{903 const lock: Lock = .{
893 .manifest_file = self.manifest_file.?,904 .manifest_file = self.manifest_file.?,
894 };905 };
906
895 self.manifest_file = null;907 self.manifest_file = null;
896 return lock;908 return lock;
897 }909 }
...@@ -901,6 +913,11 @@ pub const Manifest = struct {...@@ -901,6 +913,11 @@ pub const Manifest = struct {
901 /// Don't forget to call `writeManifest` before this!913 /// Don't forget to call `writeManifest` before this!
902 pub fn deinit(self: *Manifest) void {914 pub fn deinit(self: *Manifest) void {
903 if (self.manifest_file) |file| {915 if (self.manifest_file) |file| {
916 if (builtin.os.tag == .windows) {
917 // See Lock.release for why this is required on Windows
918 file.unlock();
919 }
920
904 file.close();921 file.close();
905 }922 }
906 for (self.files.items) |*file| {923 for (self.files.items) |*file| {
src/Compilation.zig+4
...@@ -3565,6 +3565,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {...@@ -3565,6 +3565,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
3565 const cimport_zig_basename = "cimport.zig";3565 const cimport_zig_basename = "cimport.zig";
35663566
3567 var man = comp.obtainCObjectCacheManifest();3567 var man = comp.obtainCObjectCacheManifest();
3568 man.want_shared_lock = false;
3568 defer man.deinit();3569 defer man.deinit();
35693570
3570 man.hash.add(@as(u16, 0xb945)); // Random number to distinguish translate-c from compiling C objects3571 man.hash.add(@as(u16, 0xb945)); // Random number to distinguish translate-c from compiling C objects
...@@ -3678,6 +3679,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {...@@ -3678,6 +3679,7 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
3678 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but3679 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
3679 // the contents were the same, we hit the cache but the manifest is dirty and we need to update3680 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
3680 // it to prevent doing a full file content comparison the next time around.3681 // it to prevent doing a full file content comparison the next time around.
3682 man.want_shared_lock = true;
3681 man.writeManifest() catch |err| {3683 man.writeManifest() catch |err| {
3682 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});3684 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
3683 };3685 };
...@@ -3852,6 +3854,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P...@@ -3852,6 +3854,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
3852 }3854 }
38533855
3854 var man = comp.obtainCObjectCacheManifest();3856 var man = comp.obtainCObjectCacheManifest();
3857 man.want_shared_lock = false;
3855 defer man.deinit();3858 defer man.deinit();
38563859
3857 man.hash.add(comp.clang_preprocessor_mode);3860 man.hash.add(comp.clang_preprocessor_mode);
...@@ -4147,6 +4150,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P...@@ -4147,6 +4150,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
4147 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but4150 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
4148 // the contents were the same, we hit the cache but the manifest is dirty and we need to update4151 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
4149 // it to prevent doing a full file content comparison the next time around.4152 // it to prevent doing a full file content comparison the next time around.
4153 man.want_shared_lock = true;
4150 man.writeManifest() catch |err| {4154 man.writeManifest() catch |err| {
4151 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });4155 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
4152 };4156 };
src/main.zig+1
...@@ -3505,6 +3505,7 @@ fn cmdTranslateC(comp: *Compilation, arena: Allocator, enable_cache: bool) !void...@@ -3505,6 +3505,7 @@ fn cmdTranslateC(comp: *Compilation, arena: Allocator, enable_cache: bool) !void
3505 const translated_zig_basename = try std.fmt.allocPrint(arena, "{s}.zig", .{comp.bin_file.options.root_name});3505 const translated_zig_basename = try std.fmt.allocPrint(arena, "{s}.zig", .{comp.bin_file.options.root_name});
35063506
3507 var man: Cache.Manifest = comp.obtainCObjectCacheManifest();3507 var man: Cache.Manifest = comp.obtainCObjectCacheManifest();
3508 man.want_shared_lock = false;
3508 defer if (enable_cache) man.deinit();3509 defer if (enable_cache) man.deinit();
35093510
3510 man.hash.add(@as(u16, 0xb945)); // Random number to distinguish translate-c from compiling C objects3511 man.hash.add(@as(u16, 0xb945)); // Random number to distinguish translate-c from compiling C objects