Commit e5aef96293

Andrew Kelley <andrew@ziglang.org>
2020-09-14 07:54:16
stage2: CRT files retain locks on the build artifacts
1 parent 97ea5da
Changed files (4)
src-self-hosted/link/Elf.zig
@@ -123,9 +123,6 @@ dbg_info_decl_free_list: std.AutoHashMapUnmanaged(*TextBlock, void) = .{},
 dbg_info_decl_first: ?*TextBlock = null,
 dbg_info_decl_last: ?*TextBlock = null,
 
-/// Prevents other processes from clobbering the output file this is linking.
-lock: ?std.cache_hash.Lock = null,
-
 /// `alloc_num / alloc_den` is the factor of padding when allocating.
 const alloc_num = 4;
 const alloc_den = 3;
@@ -290,21 +287,7 @@ pub fn createEmpty(gpa: *Allocator, options: link.Options) !*Elf {
     return self;
 }
 
-pub fn releaseLock(self: *Elf) void {
-    if (self.lock) |*lock| {
-        lock.release();
-        self.lock = null;
-    }
-}
-
-pub fn toOwnedLock(self: *Elf) std.cache_hash.Lock {
-    const lock = self.lock.?;
-    self.lock = null;
-    return lock;
-}
-
 pub fn deinit(self: *Elf) void {
-    self.releaseLock();
     self.sections.deinit(self.base.allocator);
     self.program_headers.deinit(self.base.allocator);
     self.shstrtab.deinit(self.base.allocator);
@@ -1253,7 +1236,7 @@ fn linkWithLLD(self: *Elf, comp: *Compilation) !void {
     const id_symlink_basename = "id.txt";
 
     // We are about to obtain this lock, so here we give other processes a chance first.
-    self.releaseLock();
+    self.base.releaseLock();
 
     var ch = comp.cache_parent.obtain();
     defer ch.deinit();
@@ -1302,14 +1285,16 @@ fn linkWithLLD(self: *Elf, comp: *Compilation) !void {
     const digest = ch.final();
 
     var prev_digest_buf: [digest.len]u8 = undefined;
-    const prev_digest: []u8 = directory.handle.readLink(id_symlink_basename, &prev_digest_buf) catch blk: {
+    const prev_digest: []u8 = directory.handle.readLink(id_symlink_basename, &prev_digest_buf) catch |err| blk: {
+        log.debug("ELF LLD new_digest={} readlink error: {}", .{digest, @errorName(err)});
         // Handle this as a cache miss.
         mem.set(u8, &prev_digest_buf, 0);
         break :blk &prev_digest_buf;
     };
+    log.debug("ELF LLD prev_digest={} new_digest={}", .{prev_digest, digest});
     if (mem.eql(u8, prev_digest, &digest)) {
         // Hot diggity dog! The output binary is already there.
-        self.lock = ch.toOwnedLock();
+        self.base.lock = ch.toOwnedLock();
         return;
     }
 
@@ -1611,7 +1596,7 @@ fn linkWithLLD(self: *Elf, comp: *Compilation) !void {
     };
     // We hang on to this lock so that the output file path can be used without
     // other processes clobbering it.
-    self.lock = ch.toOwnedLock();
+    self.base.lock = ch.toOwnedLock();
 }
 
 fn append_diagnostic(context: usize, ptr: [*]const u8, len: usize) callconv(.C) void {
src-self-hosted/Compilation.zig
@@ -70,13 +70,24 @@ libc_static_lib: ?[]const u8 = null,
 /// For example `Scrt1.o` and `libc.so.6`. These are populated after building libc from source,
 /// The set of needed CRT (C runtime) files differs depending on the target and compilation settings.
 /// The key is the basename, and the value is the absolute path to the completed build artifact.
-crt_files: std.StringHashMapUnmanaged([]const u8) = .{},
+crt_files: std.StringHashMapUnmanaged(CRTFile) = .{},
 
 /// Keeping track of this possibly open resource so we can close it later.
 owned_link_dir: ?std.fs.Dir,
 
 pub const InnerError = Module.InnerError;
 
+pub const CRTFile = struct {
+    lock: std.cache_hash.Lock,
+    full_object_path: []const u8,
+
+    fn deinit(self: *CRTFile, gpa: *Allocator) void {
+        self.lock.release();
+        gpa.free(self.full_object_path);
+        self.* = undefined;
+    }
+};
+
 /// For passing to a C compiler.
 pub const CSourceFile = struct {
     src_path: []const u8,
@@ -625,7 +636,7 @@ pub fn destroy(self: *Compilation) void {
         var it = self.crt_files.iterator();
         while (it.next()) |entry| {
             gpa.free(entry.key);
-            gpa.free(entry.value);
+            entry.value.deinit(gpa);
         }
         self.crt_files.deinit(gpa);
     }
@@ -1512,7 +1523,7 @@ fn detectLibCFromLibCInstallation(arena: *Allocator, target: Target, lci: *const
 
 pub fn get_libc_crt_file(comp: *Compilation, arena: *Allocator, basename: []const u8) ![]const u8 {
     if (comp.wantBuildGLibCFromSource()) {
-        return comp.crt_files.get(basename).?;
+        return comp.crt_files.get(basename).?.full_object_path;
     }
     const lci = comp.bin_file.options.libc_installation orelse return error.LibCInstallationNotAvailable;
     const crt_dir_path = lci.crt_dir orelse return error.LibCInstallationMissingCRTDir;
src-self-hosted/glibc.zig
@@ -648,5 +648,8 @@ fn build_libc_object(comp: *Compilation, basename: []const u8, c_source_file: Co
         try comp.gpa.dupe(u8, basename);
 
     // TODO obtain a lock on the artifact and put that in crt_files as well.
-    comp.crt_files.putAssumeCapacityNoClobber(basename, artifact_path);
+    comp.crt_files.putAssumeCapacityNoClobber(basename, .{
+        .full_object_path = artifact_path,
+        .lock = sub_compilation.bin_file.toOwnedLock(),
+    });
 }
src-self-hosted/link.zig
@@ -90,6 +90,10 @@ pub const File = struct {
     /// this location, and then this path can be placed on the LLD linker line.
     intermediary_basename: ?[]const u8 = null,
 
+    /// Prevents other processes from clobbering files in the output directory
+    /// of this linking operation.
+    lock: ?std.cache_hash.Lock = null,
+
     pub const LinkBlock = union {
         elf: Elf.TextBlock,
         coff: Coff.TextBlock,
@@ -228,7 +232,21 @@ pub const File = struct {
         }
     }
 
+    pub fn releaseLock(self: *File) void {
+        if (self.lock) |*lock| {
+            lock.release();
+            self.lock = null;
+        }
+    }
+
+    pub fn toOwnedLock(self: *File) std.cache_hash.Lock {
+        const lock = self.lock.?;
+        self.lock = null;
+        return lock;
+    }
+
     pub fn destroy(base: *File) void {
+        base.releaseLock();
         if (base.file) |f| f.close();
         if (base.intermediary_basename) |sub_path| base.allocator.free(sub_path);
         switch (base.tag) {