Commit 04480f72d8993196052375c0f0aca9d33f475fe7 | zig

author	Andrew Kelley <andrew@ziglang.org> 2023-12-03 16:15:27-07:00
committer	Andrew Kelley <andrew@ziglang.org> 2024-01-01 17:51:18-07:00
log	04480f72d8993196052375c0f0aca9d33f475fe7
tree	b742c23c5c60b57676d3d56bc09db49497324a9d
parent	0ee8fbb15dd32a2bb91696e4765926e9170d2a23

fix linker test regressions

Caused by problems with transitive dependencies

4 files changed, 81 insertions(+), 33 deletions(-)

lib
- std
  - Build
    Module.zig
    Step
    Compile.zig
    Run.zig
test
- link
  - elf.zig

lib/std/Build/Module.zig+7-2

...	@@ -229,7 +229,7 @@ pub fn init(m: Module, owner: std.Build, options: CreateOptions, compile: ?*St	...	@@ -229,7 +229,7 @@ pub fn init(m: Module, owner: std.Build, options: CreateOptions, compile: ?*St
229	}	229	}
230		230
231	// This logic accesses `depending_steps` which was just modified above.	231	// This logic accesses `depending_steps` which was just modified above.
232	var it = m.iterateDependencies(null);	232	var it = m.iterateDependencies(null, false);
233	while (it.next()) \|item\| addShallowDependencies(m, item.module);	233	while (it.next()) \|item\| addShallowDependencies(m, item.module);
234	}	234	}
235		235
...	@@ -244,7 +244,7 @@ pub fn addImport(m: Module, name: []const u8, module: Module) void {	...	@@ -244,7 +244,7 @@ pub fn addImport(m: Module, name: []const u8, module: Module) void {
244	const b = m.owner;	244	const b = m.owner;
245	m.import_table.put(b.allocator, b.dupe(name), module) catch @panic("OOM");	245	m.import_table.put(b.allocator, b.dupe(name), module) catch @panic("OOM");
246		246
247	var it = module.iterateDependencies(null);	247	var it = module.iterateDependencies(null, false);
248	while (it.next()) \|item\| addShallowDependencies(m, item.module);	248	while (it.next()) \|item\| addShallowDependencies(m, item.module);
249	}	249	}
250		250
...	@@ -319,6 +319,7 @@ pub const DependencyIterator = struct {	...	@@ -319,6 +319,7 @@ pub const DependencyIterator = struct {
319	allocator: std.mem.Allocator,	319	allocator: std.mem.Allocator,
320	index: usize,	320	index: usize,
321	set: std.AutoArrayHashMapUnmanaged(Key, []const u8),	321	set: std.AutoArrayHashMapUnmanaged(Key, []const u8),
		322	chase_dyn_libs: bool,
322		323
323	pub const Key = struct {	324	pub const Key = struct {
324	/// The compilation that contains the `Module`. Note that a `Module` might be	325	/// The compilation that contains the `Module`. Note that a `Module` might be
...	@@ -361,6 +362,8 @@ pub const DependencyIterator = struct {	...	@@ -361,6 +362,8 @@ pub const DependencyIterator = struct {
361	if (key.compile != null) {	362	if (key.compile != null) {
362	for (module.link_objects.items) \|link_object\| switch (link_object) {	363	for (module.link_objects.items) \|link_object\| switch (link_object) {
363	.other_step => \|compile\| {	364	.other_step => \|compile\| {
		365	if (!it.chase_dyn_libs and compile.isDynamicLibrary()) continue;
		366
364	it.set.put(it.allocator, .{	367	it.set.put(it.allocator, .{
365	.module = &compile.root_module,	368	.module = &compile.root_module,
366	.compile = compile,	369	.compile = compile,
...	@@ -381,11 +384,13 @@ pub const DependencyIterator = struct {	...	@@ -381,11 +384,13 @@ pub const DependencyIterator = struct {
381	pub fn iterateDependencies(	384	pub fn iterateDependencies(
382	m: *Module,	385	m: *Module,
383	chase_steps: ?*Step.Compile,	386	chase_steps: ?*Step.Compile,
		387	chase_dyn_libs: bool,
384	) DependencyIterator {	388	) DependencyIterator {
385	var it: DependencyIterator = .{	389	var it: DependencyIterator = .{
386	.allocator = m.owner.allocator,	390	.allocator = m.owner.allocator,
387	.index = 0,	391	.index = 0,
388	.set = .{},	392	.set = .{},
		393	.chase_dyn_libs = chase_dyn_libs,
389	};	394	};
390	it.set.ensureUnusedCapacity(m.owner.allocator, m.import_table.count() + 1) catch @panic("OOM");	395	it.set.ensureUnusedCapacity(m.owner.allocator, m.import_table.count() + 1) catch @panic("OOM");
391	it.set.putAssumeCapacity(.{	396	it.set.putAssumeCapacity(.{

lib/std/Build/Step/Compile.zig+63-24

 @@ -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++.
+            // While walking transitive dependencies, if a given link object is
-            if (module.link_libc == true) self.is_linking_libc = true;
+            // already included in a library, it should not redundantly be
-            if (module.link_libcpp == true) self.is_linking_libcpp = true;
+            // 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
+                                // For DLLs, we must link against the implib.
-                                // everything else, we directly link against the library file.
+                                // For everything else, we directly link
-                                const full_path_lib = if (other.producesImplib())
+                                // 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;
                     },
                 }
             }

lib/std/Build/Step/Run.zig+1-1

...	@@ -1291,7 +1291,7 @@ fn evalGeneric(self: Run, child: std.process.Child) !StdIoResult {	...	@@ -1291,7 +1291,7 @@ fn evalGeneric(self: Run, child: std.process.Child) !StdIoResult {
1291		1291
1292	fn addPathForDynLibs(self: Run, artifact: Step.Compile) void {	1292	fn addPathForDynLibs(self: Run, artifact: Step.Compile) void {
1293	const b = self.step.owner;	1293	const b = self.step.owner;
1294	var it = artifact.root_module.iterateDependencies(artifact);	1294	var it = artifact.root_module.iterateDependencies(artifact, true);
1295	while (it.next()) \|item\| {	1295	while (it.next()) \|item\| {
1296	const other = item.compile.?;	1296	const other = item.compile.?;
1297	if (item.module == &other.root_module) {	1297	if (item.module == &other.root_module) {

test/link/elf.zig+10-6

 @@ -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" });
+    const dso = addSharedLibrary(b, opts, .{
-    addCSourceBytes(dso,
+        .name = "a",
+        .c_source_bytes =
         \\#include<stdio.h>
         \\__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" });
+    const exe = addExecutable(b, opts, .{
-    addCSourceBytes(exe,
+        .name = "main",
+        .c_source_bytes =
         \\void foobar(void);
         \\int main() {
         \\  foobar();
         \\}
-    , &.{});
+        ,
+    });
     exe.linkLibrary(dso);
     const run = addRunArtifact(exe);