From 04480f72d8993196052375c0f0aca9d33f475fe7 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Sun, 3 Dec 2023 16:15:27 -0700 Subject: [PATCH] fix linker test regressions Caused by problems with transitive dependencies --- lib/std/Build/Module.zig | 9 +++- lib/std/Build/Step/Compile.zig | 87 ++++++++++++++++++++++++---------- lib/std/Build/Step/Run.zig | 2 +- test/link/elf.zig | 16 ++++--- 4 files changed, 81 insertions(+), 33 deletions(-) diff --git a/lib/std/Build/Module.zig b/lib/std/Build/Module.zig index 3428581333..f17f296e64 100644 --- a/lib/std/Build/Module.zig +++ b/lib/std/Build/Module.zig @@ -229,7 +229,7 @@ pub fn init(m: *Module, owner: *std.Build, options: CreateOptions, compile: ?*St } // This logic accesses `depending_steps` which was just modified above. - var it = m.iterateDependencies(null); + var it = m.iterateDependencies(null, false); while (it.next()) |item| addShallowDependencies(m, item.module); } @@ -244,7 +244,7 @@ pub fn addImport(m: *Module, name: []const u8, module: *Module) void { const b = m.owner; m.import_table.put(b.allocator, b.dupe(name), module) catch @panic("OOM"); - var it = module.iterateDependencies(null); + var it = module.iterateDependencies(null, false); while (it.next()) |item| addShallowDependencies(m, item.module); } @@ -319,6 +319,7 @@ pub const DependencyIterator = struct { allocator: std.mem.Allocator, index: usize, set: std.AutoArrayHashMapUnmanaged(Key, []const u8), + chase_dyn_libs: bool, pub const Key = struct { /// The compilation that contains the `Module`. Note that a `Module` might be @@ -361,6 +362,8 @@ pub const DependencyIterator = struct { if (key.compile != null) { for (module.link_objects.items) |link_object| switch (link_object) { .other_step => |compile| { + if (!it.chase_dyn_libs and compile.isDynamicLibrary()) continue; + it.set.put(it.allocator, .{ .module = &compile.root_module, .compile = compile, @@ -381,11 +384,13 @@ pub const DependencyIterator = struct { pub fn iterateDependencies( m: *Module, chase_steps: ?*Step.Compile, + chase_dyn_libs: bool, ) DependencyIterator { var it: DependencyIterator = .{ .allocator = m.owner.allocator, .index = 0, .set = .{}, + .chase_dyn_libs = chase_dyn_libs, }; it.set.ensureUnusedCapacity(m.owner.allocator, m.import_table.count() + 1) catch @panic("OOM"); it.set.putAssumeCapacity(.{ diff --git a/lib/std/Build/Step/Compile.zig b/lib/std/Build/Step/Compile.zig index 52c9703db0..93027003f3 100644 --- a/lib/std/Build/Step/Compile.zig +++ b/lib/std/Build/Step/Compile.zig @@ -488,11 +488,12 @@ pub fn forceUndefinedSymbol(self: *Compile, symbol_name: []const u8) void { } /// Returns whether the library, executable, or object depends on a particular system library. +/// Includes transitive dependencies. pub fn dependsOnSystemLibrary(self: *const Compile, name: []const u8) bool { var is_linking_libc = false; var is_linking_libcpp = false; - var it = self.root_module.iterateDependencies(self); + var it = self.root_module.iterateDependencies(self, true); while (it.next()) |module| { for (module.link_objects.items) |link_object| { switch (link_object) { @@ -841,7 +842,7 @@ fn appendModuleArgs(cs: *Compile, zig_args: *ArrayList([]const u8)) !void { var names = std.StringHashMap(void).init(b.allocator); { - var it = cs.root_module.iterateDependencies(null); + var it = cs.root_module.iterateDependencies(null, false); _ = it.next(); // Skip over the root module. while (it.next()) |item| { // While we're traversing the root dependencies, let's make sure that no module names @@ -1000,33 +1001,51 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { try self.root_module.appendZigProcessFlags(&zig_args, step); - var it = self.root_module.iterateDependencies(self); + { + // Fully recursive iteration including dynamic libraries to detect + // libc and libc++ linkage. + var it = self.root_module.iterateDependencies(self, true); + while (it.next()) |key| { + if (key.module.link_libc == true) self.is_linking_libc = true; + if (key.module.link_libcpp == true) self.is_linking_libcpp = true; + } + } + + // For this loop, don't chase dynamic libraries because their link + // objects are already linked. + var it = self.root_module.iterateDependencies(self, false); + while (it.next()) |key| { const module = key.module; const compile = key.compile.?; - const dyn = compile.isDynamicLibrary(); - // Inherit dependency on libc and libc++. - if (module.link_libc == true) self.is_linking_libc = true; - if (module.link_libcpp == true) self.is_linking_libcpp = true; + // While walking transitive dependencies, if a given link object is + // already included in a library, it should not redundantly be + // placed on the linker line of the dependee. + const my_responsibility = compile == self; + const already_linked = !my_responsibility and compile.isDynamicLibrary(); // Inherit dependencies on darwin frameworks. - if (!dyn) { + if (!already_linked) { for (module.frameworks.keys(), module.frameworks.values()) |name, info| { try frameworks.put(b.allocator, name, info); } } // Inherit dependencies on system libraries and static libraries. - total_linker_objects += module.link_objects.items.len; for (module.link_objects.items) |link_object| { switch (link_object) { - .static_path => |static_path| try zig_args.append(static_path.getPath(b)), + .static_path => |static_path| { + if (my_responsibility) { + try zig_args.append(static_path.getPath(b)); + total_linker_objects += 1; + } + }, .system_lib => |system_lib| { if ((try seen_system_libs.fetchPut(b.allocator, system_lib.name, {})) != null) continue; - if (dyn) + if (already_linked) continue; if ((system_lib.search_strategy != prev_search_strategy or @@ -1088,29 +1107,36 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { } }, .other_step => |other| { - const included_in_lib = (compile.kind == .lib and other.kind == .obj); - if (dyn or included_in_lib) - continue; - switch (other.kind) { .exe => return step.fail("cannot link with an executable build artifact", .{}), .@"test" => return step.fail("cannot link with a test", .{}), .obj => { - try zig_args.append(other.getEmittedBin().getPath(b)); + const included_in_lib = !my_responsibility and + compile.kind == .lib and other.kind == .obj; + if (!already_linked and !included_in_lib) { + try zig_args.append(other.getEmittedBin().getPath(b)); + total_linker_objects += 1; + } }, .lib => l: { - if (self.isStaticLibrary() and other.isStaticLibrary()) { + const other_produces_implib = other.producesImplib(); + const other_is_static = other_produces_implib or other.isStaticLibrary(); + + if (self.isStaticLibrary() and other_is_static) { // Avoid putting a static library inside a static library. break :l; } - // For DLLs, we gotta link against the implib. For - // everything else, we directly link against the library file. - const full_path_lib = if (other.producesImplib()) + // For DLLs, we must link against the implib. + // For everything else, we directly link + // against the library file. + const full_path_lib = if (other_produces_implib) other.getGeneratedFilePath("generated_implib", &self.step) else other.getGeneratedFilePath("generated_bin", &self.step); + try zig_args.append(full_path_lib); + total_linker_objects += 1; if (other.linkage == Linkage.dynamic and self.rootModuleTarget().os.tag != .windows) @@ -1123,16 +1149,21 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { }, } }, - .assembly_file => |asm_file| { + .assembly_file => |asm_file| l: { + if (!my_responsibility) break :l; + if (prev_has_cflags) { try zig_args.append("-cflags"); try zig_args.append("--"); prev_has_cflags = false; } try zig_args.append(asm_file.getPath(b)); + total_linker_objects += 1; }, - .c_source_file => |c_source_file| { + .c_source_file => |c_source_file| l: { + if (!my_responsibility) break :l; + if (c_source_file.flags.len == 0) { if (prev_has_cflags) { try zig_args.append("-cflags"); @@ -1148,9 +1179,12 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { prev_has_cflags = true; } try zig_args.append(c_source_file.file.getPath(b)); + total_linker_objects += 1; }, - .c_source_files => |c_source_files| { + .c_source_files => |c_source_files| l: { + if (!my_responsibility) break :l; + if (c_source_files.flags.len == 0) { if (prev_has_cflags) { try zig_args.append("-cflags"); @@ -1165,6 +1199,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { try zig_args.append("--"); prev_has_cflags = true; } + if (c_source_files.dependency) |dep| { for (c_source_files.files) |file| { try zig_args.append(dep.builder.pathFromRoot(file)); @@ -1174,9 +1209,12 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { try zig_args.append(b.pathFromRoot(file)); } } + total_linker_objects += c_source_files.files.len; }, - .win32_resource_file => |rc_source_file| { + .win32_resource_file => |rc_source_file| l: { + if (!my_responsibility) break :l; + if (rc_source_file.flags.len == 0) { if (prev_has_rcflags) { try zig_args.append("-rcflags"); @@ -1192,6 +1230,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { prev_has_rcflags = true; } try zig_args.append(rc_source_file.file.getPath(b)); + total_linker_objects += 1; }, } } diff --git a/lib/std/Build/Step/Run.zig b/lib/std/Build/Step/Run.zig index 40b72c7bca..92a40dcd1f 100644 --- a/lib/std/Build/Step/Run.zig +++ b/lib/std/Build/Step/Run.zig @@ -1291,7 +1291,7 @@ fn evalGeneric(self: *Run, child: *std.process.Child) !StdIoResult { fn addPathForDynLibs(self: *Run, artifact: *Step.Compile) void { const b = self.step.owner; - var it = artifact.root_module.iterateDependencies(artifact); + var it = artifact.root_module.iterateDependencies(artifact, true); while (it.next()) |item| { const other = item.compile.?; if (item.module == &other.root_module) { diff --git a/test/link/elf.zig b/test/link/elf.zig index d395344edd..95bb929dc0 100644 --- a/test/link/elf.zig +++ b/test/link/elf.zig @@ -1317,8 +1317,9 @@ fn testIFuncDlopen(b: *Build, opts: Options) *Step { fn testIFuncDso(b: *Build, opts: Options) *Step { const test_step = addTestStep(b, "ifunc-dso", opts); - const dso = addSharedLibrary(b, opts, .{ .name = "a" }); - addCSourceBytes(dso, + const dso = addSharedLibrary(b, opts, .{ + .name = "a", + .c_source_bytes = \\#include \\__attribute__((ifunc("resolve_foobar"))) \\void foobar(void); @@ -1329,16 +1330,19 @@ fn testIFuncDso(b: *Build, opts: Options) *Step { \\static Func *resolve_foobar(void) { \\ return real_foobar; \\} - , &.{}); + , + }); dso.linkLibC(); - const exe = addExecutable(b, opts, .{ .name = "main" }); - addCSourceBytes(exe, + const exe = addExecutable(b, opts, .{ + .name = "main", + .c_source_bytes = \\void foobar(void); \\int main() { \\ foobar(); \\} - , &.{}); + , + }); exe.linkLibrary(dso); const run = addRunArtifact(exe);