From 7faf6be3535195b96b69fb9b8d12f16015d7ec36 Mon Sep 17 00:00:00 2001 From: Matthew Lugg Date: Sun, 19 Jul 2026 12:41:03 +0100 Subject: [PATCH] link.Lld: handle errors properly in ZigLLVMWriteArchive So... it turns out https://codeberg.org/ziglang/zig/issues/31520 is probably our fault! This commit does not close that issue, but it at least makes some progress. LLVM's `Error` type is basically `anyerror!void`, and when an error occurs, it can detect that the caller didn't look at the error code, in which case it will abort with an error message. Our implementation of `ZigLLVMWriteArchive` was checking *that* there was an error, but not what that error *was*, so was triggering this code path. (I think that LLVM's approach to error handling here is unnecessarily dangerous, but whatcha gonna do :shrug:) It is possible for the "no such file or directory" error to occur in this function due to a TOCTOU bug. If we are producing a static library, and some object file is deleted after the Zig frontend checks that it exists but *before* `ZigLLVMWriteArchive` has a chance to read the file, LLVM will report that the input file does not exist. The reason I found this bug is that I was able to somewhat consistently trigger the same condition by running a faulty build script which had duplicated steps. Consider a case where build step A and B generate the exact same object file, using the same options etc; and build step C depends on A, and puts that object file into a static library. At some point, step A and B both start running. Step A finishes first, placing an object file into the cache directory. The build system then starts the dependent step C. The compiler frontend confirms that the object file (in the cache directory) exists, as expected, and passes it off to `ZigLLVMWriteArchive`---but around this time, step B finishes, and begins writing the *new* file. Depending on how LLVM writes that output file, there may be a period of time where the file does not exist or is truncated, either of which could cause `ZigLLVMWriteArchive` to fail. The reproduction I described above does not explain #31520, because it relies on a faulty build script (where the same object file is being generated by multiple identical build steps), but that is not happening in #31520. It also relies on building a static library, which again, is not happening in #31520. Instead, if #31520 is indeed coming from this function (which I believe is the most likely explanation), it must be that #31520 is caused by a compiler bug which ultimately has the same effect (an object file in the cache being overwritten while being used to build a static library) This makes sense, because in many compilations we *do* build several static libraries: our vendored implementations of libraries, including musl libc, libc++, libunwind, etc. However, this still doesn't fully explain the bug. When we build these libraries, we create a `Compilation` with a bunch of C source file inputs. For each of those, the Zig compiler intentionally keeps hold of a shared advisory lock on the output file (well, technically on its cache manifest), and does not release it until the `Compilation` is destroyed, which only happens after we've completely finished emitting our archive. The advisory lock should be preventing any other Zig compiler process from trying to write the object file until we've finished building the archive. I did briefly audit the C object compilation logic for bugs, but barring a bug in `std.Build.Cache` itself, I couldn't spot any problems. Nonetheless, I still consider it likely that `ZigLLVMWriteArchive` is where the errors in #31520 are coming from. This patch---which fixes the unclean termination and actually reports LLVM's error properly---will help to test that hypothesis, and if it's correct, the improved error message will hopefully help to track down the underlying bug. --- src/codegen/llvm/bindings.zig | 2 ++ src/link/Lld.zig | 23 ++++++++++++++++++----- src/zig_llvm.cpp | 19 ++++++++++++++++--- src/zig_llvm.h | 7 ++++++- 4 files changed, 42 insertions(+), 9 deletions(-) diff --git a/src/codegen/llvm/bindings.zig b/src/codegen/llvm/bindings.zig index e4f4d6eafefcca250babb49e4a907eba22866d22..a312285db3310ba0fc69dffe55d56e484c293e50 100644 --- a/src/codegen/llvm/bindings.zig +++ b/src/codegen/llvm/bindings.zig @@ -331,6 +331,8 @@ extern fn ZigLLVMWriteArchive( file_names_ptr: [*]const [*:0]const u8, file_names_len: usize, archive_kind: ArchiveKind, + err_file_index_out: *usize, + err_msg_out: *[*:0]u8, ) bool; pub const ParseCommandLineOptions = ZigLLVMParseCommandLineOptions; diff --git a/src/link/Lld.zig b/src/link/Lld.zig index 04ffde350e146aa96a338a62aa953c51f501686f..cf64d3fd5bc51817cc86a7554ac65ee7aa53e373 100644 --- a/src/link/Lld.zig +++ b/src/link/Lld.zig @@ -269,12 +269,12 @@ pub fn flush( .wasm => wasmLink(lld, arena), }; result catch |err| switch (err) { - error.OutOfMemory, error.AlreadyReported => |e| return e, + error.OutOfMemory, error.AlreadyReported, error.Canceled => |e| return e, else => |e| return lld.base.comp.link_diags.fail("failed to link with LLD: {t}", .{e}), }; } -fn linkAsArchive(lld: *Lld, arena: Allocator) !void { +fn linkAsArchive(lld: *Lld, arena: Allocator) link.Error!void { const base = &lld.base; const comp = base.comp; const directory = base.emit.root_dir; // Just an alias to make it shorter to type. @@ -338,7 +338,9 @@ fn linkAsArchive(lld: *Lld, arena: Allocator) !void { const llvm = @import("../codegen/llvm.zig"); const target = &comp.root_mod.resolved_target.result; llvm.initializeLLVMTarget(target.cpu.arch); - const bad = llvm_bindings.WriteArchive( + var err_file_index: usize = undefined; + var err_msg: [*:0]u8 = undefined; + if (llvm_bindings.WriteArchive( full_out_path_z, object_files.items.ptr, object_files.items.len, @@ -346,8 +348,19 @@ fn linkAsArchive(lld: *Lld, arena: Allocator) !void { .windows => .COFF, else => if (target.os.tag.isDarwin()) .DARWIN else .GNU, }, - ); - if (bad) return error.UnableToWriteArchive; + &err_file_index, + &err_msg, + )) { + defer std.c.free(err_msg); + if (err_file_index < object_files.items.len) { + return comp.link_diags.fail("LLD failed to open input file '{s}': {s}", .{ + object_files.items[err_file_index], + err_msg, + }); + } else { + return comp.link_diags.fail("LLD failed to write archive: {s}", .{err_msg}); + } + } } fn addCommonArgs(argv: *std.array_list.Managed([]const u8), coff: bool) !void { diff --git a/src/zig_llvm.cpp b/src/zig_llvm.cpp index 9bba8e96d5107c6bf20ea2fd7f15e3df243d5bac..6b3ff4a5fbe626b37c0a49f7b734564c6563201e 100644 --- a/src/zig_llvm.cpp +++ b/src/zig_llvm.cpp @@ -473,19 +473,32 @@ void ZigLLVMParseCommandLineOptions(size_t argc, const char *const *argv) { } bool ZigLLVMWriteArchive(const char *archive_name, const char **file_names, size_t file_name_count, - ZigLLVMArchiveKind archive_kind) + ZigLLVMArchiveKind archive_kind, size_t *err_file_index_out, char **err_msg_out) { SmallVector new_members; for (size_t i = 0; i < file_name_count; i += 1) { Expected new_member = NewArchiveMember::getFile(file_names[i], true); Error err = new_member.takeError(); - if (err) return true; + if (err) { + *err_file_index_out = i; + const std::string msg = toString(std::move(err)); + *err_msg_out = (char *)malloc(msg.length() + 1); + strcpy(*err_msg_out, msg.c_str()); + return true; + } new_members.push_back(std::move(*new_member)); } Error err = writeArchive(archive_name, new_members, SymtabWritingMode::NormalSymtab, static_cast(archive_kind), true, false, nullptr); - if (err) return true; + if (err) { + *err_file_index_out = file_name_count; + const std::string msg = toString(std::move(err)); + *err_msg_out = (char *)malloc(msg.length() + 1); + strcpy(*err_msg_out, msg.c_str()); + return true; + } + return false; } diff --git a/src/zig_llvm.h b/src/zig_llvm.h index 64da388d477bfed46e0c4ae2e7da1cd16c73ce57..79ba0272df2f73743705ac832f5d508b5099542d 100644 --- a/src/zig_llvm.h +++ b/src/zig_llvm.h @@ -121,7 +121,12 @@ ZIG_EXTERN_C bool ZigLLDLinkCOFF(int argc, const char **argv, bool can_exit_earl ZIG_EXTERN_C bool ZigLLDLinkELF(int argc, const char **argv, bool can_exit_early, bool disable_output); ZIG_EXTERN_C bool ZigLLDLinkWasm(int argc, const char **argv, bool can_exit_early, bool disable_output); +// On error, populates `*err_file_index_out` and `*err_msg_out` and returns `true`. The caller is +// responsible for freeing `*err_msg_out` using `free`. +// +// If an error occurs reading an input file, `*err_file_index_out` is set to the index of that input +// file in `file_names`. Otherwise, `*err_file_index_out` is set to `file_name_count`. ZIG_EXTERN_C bool ZigLLVMWriteArchive(const char *archive_name, const char **file_names, size_t file_name_count, - ZigLLVMArchiveKind archive_kind); + ZigLLVMArchiveKind archive_kind, size_t *err_file_index_out, char **err_msg_out); #endif -- 2.54.0