From f746e118795af8f8eb16deefc8f6fed26f3be4dd Mon Sep 17 00:00:00 2001 From: Casey Banner Date: Sat, 19 Nov 2022 08:57:08 -0500 Subject: [PATCH] linker: fail the compilation if there were linker errors There was no check for linker errors after flushing, which meant that if the link failed the build would continue and try to copy the non-existant exe, and also write the manifest as if it had succeeded. Also adds parsing of lld output, which is surfaced at the end of the compilation with the other errors instead of via stderr --- src/Compilation.zig | 92 +++++++++++++++++++++++++++++++++++++++++-- src/link/Coff/lld.zig | 7 ++-- src/link/Elf.zig | 7 ++-- src/link/Wasm.zig | 7 ++-- 4 files changed, 98 insertions(+), 15 deletions(-) diff --git a/src/Compilation.zig b/src/Compilation.zig index 50afe2c7b6e370c53f9e5b60c84200ac41cab09e..60064fefd10c0f43681952ad1dd3181a29a74062 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -51,6 +51,7 @@ whole_cache_manifest: ?*Cache.Manifest = null, whole_cache_manifest_mutex: std.Thread.Mutex = .{}, link_error_flags: link.File.ErrorFlags = .{}, +lld_errors: std.ArrayListUnmanaged(LldError) = .{}, work_queue: std.fifo.LinearFifo(Job, .Dynamic), anon_work_queue: std.fifo.LinearFifo(Job, .Dynamic), @@ -335,6 +336,21 @@ pub const MiscError = struct { } }; +pub const LldError = struct { + /// Allocated with gpa. + msg: []const u8, + context_lines: []const []const u8 = &.{}, + + pub fn deinit(self: *LldError, gpa: Allocator) void { + for (self.context_lines) |line| { + gpa.free(line); + } + + gpa.free(self.context_lines); + gpa.free(self.msg); + } +}; + /// To support incremental compilation, errors are stored in various places /// so that they can be created and destroyed appropriately. This structure /// is used to collect all the errors from the various places into one @@ -498,7 +514,7 @@ pub const AllErrors = struct { } ttyconf.setColor(stderr, .Reset); for (plain.notes) |note| { - try note.renderToWriter(ttyconf, stderr, "error", .Red, indent + 4); + try note.renderToWriter(ttyconf, stderr, "note", .Cyan, indent + 4); } }, } @@ -2166,6 +2182,11 @@ pub fn destroy(self: *Compilation) void { } self.failed_c_objects.deinit(gpa); + for (self.lld_errors.items) |*lld_error| { + lld_error.deinit(gpa); + } + self.lld_errors.deinit(gpa); + self.clearMiscFailures(); self.cache_parent.manifest_dir.close(); @@ -2465,6 +2486,10 @@ pub fn update(comp: *Compilation) !void { try comp.flush(main_progress_node); } + if (comp.totalErrorCount() != 0) { + return; + } + // Failure here only means an unnecessary cache miss. man.writeManifest() catch |err| { log.warn("failed to write cache manifest: {s}", .{@errorName(err)}); @@ -2494,7 +2519,7 @@ fn flush(comp: *Compilation, prog_node: *std.Progress.Node) !void { // This is needed before reading the error flags. comp.bin_file.flush(comp, prog_node) catch |err| switch (err) { error.FlushFailure => {}, // error reported through link_error_flags - error.LLDReportedFailure => {}, // error reported through log.err + error.LLDReportedFailure => {}, // error reported via lockAndParseLldStderr else => |e| return e, }; comp.link_error_flags = comp.bin_file.errorFlags(); @@ -2727,7 +2752,7 @@ pub fn makeBinFileWritable(self: *Compilation) !void { /// This function is temporally single-threaded. pub fn totalErrorCount(self: *Compilation) usize { var total: usize = self.failed_c_objects.count() + self.misc_failures.count() + - @boolToInt(self.alloc_failure_occurred); + @boolToInt(self.alloc_failure_occurred) + self.lld_errors.items.len; if (self.bin_file.options.module) |module| { total += module.failed_exports.count(); @@ -2815,6 +2840,21 @@ pub fn getAllErrorsAlloc(self: *Compilation) !AllErrors { }); } } + for (self.lld_errors.items) |lld_error| { + const notes = try arena_allocator.alloc(AllErrors.Message, lld_error.context_lines.len); + for (lld_error.context_lines) |context_line, i| { + notes[i] = .{ .plain = .{ + .msg = try arena_allocator.dupe(u8, context_line), + } }; + } + + try errors.append(.{ + .plain = .{ + .msg = try arena_allocator.dupe(u8, lld_error.msg), + .notes = notes, + }, + }); + } for (self.misc_failures.values()) |*value| { try AllErrors.addPlainWithChildren(&arena, &errors, value.msg, value.children); } @@ -4989,6 +5029,52 @@ pub fn lockAndSetMiscFailure( return setMiscFailure(comp, tag, format, args); } +fn parseLldStderr(comp: *Compilation, comptime prefix: []const u8, stderr: []const u8) Allocator.Error!void { + var context_lines = std.ArrayList([]const u8).init(comp.gpa); + defer context_lines.deinit(); + + var current_err: ?*LldError = null; + var lines = mem.split(u8, stderr, std.cstr.line_sep); + while (lines.next()) |line| { + if (mem.startsWith(u8, line, prefix ++ ":")) { + if (current_err) |err| { + err.context_lines = context_lines.toOwnedSlice(); + } + + var split = std.mem.split(u8, line, "error: "); + _ = split.first(); + + const duped_msg = try std.fmt.allocPrint(comp.gpa, "{s}: {s}", .{ prefix, split.rest() }); + errdefer comp.gpa.free(duped_msg); + + current_err = try comp.lld_errors.addOne(comp.gpa); + current_err.?.* = .{ .msg = duped_msg }; + } else if (current_err != null) { + const context_prefix = ">>> "; + var trimmed = mem.trimRight(u8, line, &std.ascii.whitespace); + if (mem.startsWith(u8, trimmed, context_prefix)) { + trimmed = trimmed[context_prefix.len..]; + } + + if (trimmed.len > 0) { + const duped_line = try comp.gpa.dupe(u8, trimmed); + try context_lines.append(duped_line); + } + } + } + + if (current_err) |err| { + err.context_lines = context_lines.toOwnedSlice(); + } +} + +pub fn lockAndParseLldStderr(comp: *Compilation, comptime prefix: []const u8, stderr: []const u8) void { + comp.mutex.lock(); + defer comp.mutex.unlock(); + + comp.parseLldStderr(prefix, stderr) catch comp.setAllocFailure(); +} + pub fn dump_argv(argv: []const []const u8) void { for (argv[0 .. argv.len - 1]) |arg| { std.debug.print("{s} ", .{arg}); diff --git a/src/link/Coff/lld.zig b/src/link/Coff/lld.zig index 00e6c4d8d1e64733a7b6a42402509674faa9c136..ebc2208606c009207b53de71706edcce93fff2e7 100644 --- a/src/link/Coff/lld.zig +++ b/src/link/Coff/lld.zig @@ -175,7 +175,8 @@ pub fn linkWithLLD(self: *Coff, comp: *Compilation, prog_node: *std.Progress.Nod // We will invoke ourselves as a child process to gain access to LLD. // This is necessary because LLD does not behave properly as a library - // it calls exit() and does not reset all global data between invocations. - try argv.appendSlice(&[_][]const u8{ comp.self_exe_path.?, "lld-link" }); + const linker_command = "lld-link"; + try argv.appendSlice(&[_][]const u8{ comp.self_exe_path.?, linker_command }); try argv.append("-ERRORLIMIT:0"); try argv.append("-NOLOGO"); @@ -556,9 +557,7 @@ pub fn linkWithLLD(self: *Coff, comp: *Compilation, prog_node: *std.Progress.Nod switch (term) { .Exited => |code| { if (code != 0) { - // TODO parse this output and surface with the Compilation API rather than - // directly outputting to stderr here. - std.debug.print("{s}", .{stderr}); + comp.lockAndParseLldStderr(linker_command, stderr); return error.LLDReportedFailure; } }, diff --git a/src/link/Elf.zig b/src/link/Elf.zig index b6391b31c6c58ca7d192186cabbf5eaa1b61f8e3..59f620f6380588bc90cf0336dca77934f28a0e31 100644 --- a/src/link/Elf.zig +++ b/src/link/Elf.zig @@ -1422,7 +1422,8 @@ fn linkWithLLD(self: *Elf, comp: *Compilation, prog_node: *std.Progress.Node) !v // We will invoke ourselves as a child process to gain access to LLD. // This is necessary because LLD does not behave properly as a library - // it calls exit() and does not reset all global data between invocations. - try argv.appendSlice(&[_][]const u8{ comp.self_exe_path.?, "ld.lld" }); + const linker_command = "ld.lld"; + try argv.appendSlice(&[_][]const u8{ comp.self_exe_path.?, linker_command }); if (is_obj) { try argv.append("-r"); } @@ -1841,9 +1842,7 @@ fn linkWithLLD(self: *Elf, comp: *Compilation, prog_node: *std.Progress.Node) !v switch (term) { .Exited => |code| { if (code != 0) { - // TODO parse this output and surface with the Compilation API rather than - // directly outputting to stderr here. - std.debug.print("{s}", .{stderr}); + comp.lockAndParseLldStderr(linker_command, stderr); return error.LLDReportedFailure; } }, diff --git a/src/link/Wasm.zig b/src/link/Wasm.zig index b9f2d74bd85eef58f4f25747e01a04478ec9897c..39f8a6bbe62b4dcbd9e1a0da70559dcdd01f639e 100644 --- a/src/link/Wasm.zig +++ b/src/link/Wasm.zig @@ -3125,7 +3125,8 @@ fn linkWithLLD(wasm: *Wasm, comp: *Compilation, prog_node: *std.Progress.Node) ! // We will invoke ourselves as a child process to gain access to LLD. // This is necessary because LLD does not behave properly as a library - // it calls exit() and does not reset all global data between invocations. - try argv.appendSlice(&[_][]const u8{ comp.self_exe_path.?, "wasm-ld" }); + const linker_command = "wasm-ld"; + try argv.appendSlice(&[_][]const u8{ comp.self_exe_path.?, linker_command }); try argv.append("--error-limit=0"); if (wasm.base.options.lto) { @@ -3357,9 +3358,7 @@ fn linkWithLLD(wasm: *Wasm, comp: *Compilation, prog_node: *std.Progress.Node) ! switch (term) { .Exited => |code| { if (code != 0) { - // TODO parse this output and surface with the Compilation API rather than - // directly outputting to stderr here. - std.debug.print("{s}", .{stderr}); + comp.lockAndParseLldStderr(linker_command, stderr); return error.LLDReportedFailure; } }, -- 2.54.0