From 01d33855c736b04160d9616f138442aa4e41a738 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 15 Dec 2020 16:55:56 -0700 Subject: stage2: protect mutable state from data races in updateCObject --- src/Compilation.zig | 81 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 28 deletions(-) (limited to 'src') diff --git a/src/Compilation.zig b/src/Compilation.zig index 5e95575642..a7fd75fe56 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -41,7 +41,12 @@ link_error_flags: link.File.ErrorFlags = .{}, work_queue: std.fifo.LinearFifo(Job, .Dynamic), +/// These jobs are to invoke the Clang compiler to create an object file, which +/// gets linked with the Compilation. +c_object_work_queue: std.fifo.LinearFifo(*CObject, .Dynamic), + /// The ErrorMsg memory is owned by the `CObject`, using Compilation's general purpose allocator. +/// This data is accessed by multiple threads and is protected by `mutex`. failed_c_objects: std.AutoArrayHashMapUnmanaged(*CObject, *ErrorMsg) = .{}, keep_source_files_loaded: bool, @@ -111,6 +116,9 @@ owned_link_dir: ?std.fs.Dir, /// Don't use this for anything other than stage1 compatibility. color: @import("main.zig").Color = .auto, +/// This mutex guards all `Compilation` mutable state. +mutex: std.Mutex = .{}, + test_filter: ?[]const u8, test_name_prefix: ?[]const u8, test_evented_io: bool, @@ -150,9 +158,6 @@ const Job = union(enum) { /// The source file containing the Decl has been updated, and so the /// Decl may need its line number information updated in the debug info. update_line_number: *Module.Decl, - /// Invoke the Clang compiler to create an object file, which gets linked - /// with the Compilation. - c_object: *CObject, /// one of the glibc static objects glibc_crt_file: glibc.CRTFile, @@ -971,6 +976,7 @@ pub fn create(gpa: *Allocator, options: InitOptions) !*Compilation { .emit_analysis = options.emit_analysis, .emit_docs = options.emit_docs, .work_queue = std.fifo.LinearFifo(Job, .Dynamic).init(gpa), + .c_object_work_queue = std.fifo.LinearFifo(*CObject, .Dynamic).init(gpa), .keep_source_files_loaded = options.keep_source_files_loaded, .use_clang = use_clang, .clang_argv = options.clang_argv, @@ -1190,11 +1196,13 @@ pub fn update(self: *Compilation) !void { const tracy = trace(@src()); defer tracy.end(); + self.c_object_cache_digest_set.clearRetainingCapacity(); + // For compiling C objects, we rely on the cache hash system to avoid duplicating work. // Add a Job for each C object. - try self.work_queue.ensureUnusedCapacity(self.c_object_table.items().len); + try self.c_object_work_queue.ensureUnusedCapacity(self.c_object_table.items().len); for (self.c_object_table.items()) |entry| { - self.work_queue.writeItemAssumeCapacity(.{ .c_object = entry.key }); + self.c_object_work_queue.writeItemAssumeCapacity(entry.key); } const use_stage1 = build_options.is_stage1 and self.bin_file.options.use_llvm; @@ -1372,6 +1380,26 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor var c_comp_progress_node = main_progress_node.start("Compile C Objects", self.c_source_files.len); defer c_comp_progress_node.end(); + while (self.c_object_work_queue.readItem()) |c_object| { + self.updateCObject(c_object, &c_comp_progress_node) catch |err| switch (err) { + error.AnalysisFail => continue, + else => { + { + var lock = self.mutex.acquire(); + defer lock.release(); + try self.failed_c_objects.ensureCapacity(self.gpa, self.failed_c_objects.items().len + 1); + self.failed_c_objects.putAssumeCapacityNoClobber(c_object, try ErrorMsg.create( + self.gpa, + 0, + "unable to build C object: {s}", + .{@errorName(err)}, + )); + } + c_object.status = .{ .failure = {} }; + }, + }; + } + while (self.work_queue.readItem()) |work_item| switch (work_item) { .codegen_decl => |decl| switch (decl.analysis) { .unreferenced => unreachable, @@ -1447,21 +1475,6 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor decl.analysis = .codegen_failure_retryable; }; }, - .c_object => |c_object| { - self.updateCObject(c_object, &c_comp_progress_node) catch |err| switch (err) { - error.AnalysisFail => continue, - else => { - try self.failed_c_objects.ensureCapacity(self.gpa, self.failed_c_objects.items().len + 1); - self.failed_c_objects.putAssumeCapacityNoClobber(c_object, try ErrorMsg.create( - self.gpa, - 0, - "unable to build C object: {s}", - .{@errorName(err)}, - )); - c_object.status = .{ .failure = {} }; - }, - }; - }, .glibc_crt_file => |crt_file| { glibc.buildCRTFile(self, crt_file) catch |err| { // TODO Expose this as a normal compile error rather than crashing here. @@ -1553,7 +1566,7 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor }; } -pub fn obtainCObjectCacheManifest(comp: *Compilation) Cache.Manifest { +pub fn obtainCObjectCacheManifest(comp: *const Compilation) Cache.Manifest { var man = comp.cache_parent.obtain(); // Only things that need to be added on top of the base hash, and only things @@ -1720,6 +1733,8 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_comp_progress_node: * if (c_object.clearStatus(comp.gpa)) { // There was previous failure. + var lock = comp.mutex.acquire(); + defer lock.release(); comp.failed_c_objects.removeAssertDiscard(c_object); } @@ -1747,8 +1762,14 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_comp_progress_node: * } { - const gop = try comp.c_object_cache_digest_set.getOrPut(comp.gpa, man.hash.peekBin()); - if (gop.found_existing) { + const is_collision = blk: { + var lock = comp.mutex.acquire(); + defer lock.release(); + + const gop = try comp.c_object_cache_digest_set.getOrPut(comp.gpa, man.hash.peekBin()); + break :blk gop.found_existing; + }; + if (is_collision) { return comp.failCObj( c_object, "the same source file was already added to the same compilation with the same flags", @@ -1929,7 +1950,7 @@ pub fn addTranslateCCArgs( /// Add common C compiler args between translate-c and C object compilation. pub fn addCCArgs( - comp: *Compilation, + comp: *const Compilation, arena: *Allocator, argv: *std.ArrayList([]const u8), ext: FileExt, @@ -2164,10 +2185,14 @@ fn failCObj(comp: *Compilation, c_object: *CObject, comptime format: []const u8, fn failCObjWithOwnedErrorMsg(comp: *Compilation, c_object: *CObject, err_msg: *ErrorMsg) InnerError { { - errdefer err_msg.destroy(comp.gpa); - try comp.failed_c_objects.ensureCapacity(comp.gpa, comp.failed_c_objects.items().len + 1); + var lock = comp.mutex.acquire(); + defer lock.release(); + { + errdefer err_msg.destroy(comp.gpa); + try comp.failed_c_objects.ensureCapacity(comp.gpa, comp.failed_c_objects.items().len + 1); + } + comp.failed_c_objects.putAssumeCapacityNoClobber(c_object, err_msg); } - comp.failed_c_objects.putAssumeCapacityNoClobber(c_object, err_msg); c_object.status = .failure; return error.AnalysisFail; } @@ -2324,7 +2349,7 @@ test "classifyFileExt" { std.testing.expectEqual(FileExt.zir, classifyFileExt("foo.zir")); } -fn haveFramePointer(comp: *Compilation) bool { +fn haveFramePointer(comp: *const Compilation) bool { // If you complicate this logic make sure you update the parent cache hash. // Right now it's not in the cache hash because the value depends on optimize_mode // and strip which are both already part of the hash. -- cgit v1.2.3