Commit 8fc0c7dce1

Jakub Konka <kubkon@jakubkonka.com>
2024-05-22 07:47:08
link/macho: apply fixes to deduping logic
* test non-ObjC literal deduping logic
1 parent 434e694
src/link/MachO/Atom.zig
@@ -113,12 +113,18 @@ pub fn getThunk(self: Atom, macho_file: *MachO) *Thunk {
     return macho_file.getThunk(extra.thunk);
 }
 
+pub fn getLiteralPoolIndex(self: Atom, macho_file: *MachO) ?MachO.LiteralPool.Index {
+    if (!self.flags.literal_pool) return null;
+    return self.getExtra(macho_file).?.literal_index;
+}
+
 const AddExtraOpts = struct {
     thunk: ?u32 = null,
     rel_index: ?u32 = null,
     rel_count: ?u32 = null,
     unwind_index: ?u32 = null,
     unwind_count: ?u32 = null,
+    literal_index: ?u32 = null,
 };
 
 pub fn addExtra(atom: *Atom, opts: AddExtraOpts, macho_file: *MachO) !void {
@@ -1177,7 +1183,7 @@ pub const Flags = packed struct {
     /// Specifies whether this atom is alive or has been garbage collected.
     alive: bool = true,
 
-    /// Specifies if the atom has been visited during garbage collection.
+    /// Specifies if this atom has been visited during garbage collection.
     visited: bool = false,
 
     /// Whether this atom has a range extension thunk.
@@ -1188,6 +1194,9 @@ pub const Flags = packed struct {
 
     /// Whether this atom has any unwind records.
     unwind: bool = false,
+
+    /// Whether this atom has LiteralPool entry.
+    literal_pool: bool = false,
 };
 
 pub const Extra = struct {
@@ -1205,6 +1214,9 @@ pub const Extra = struct {
 
     /// Count of relocations belonging to this atom.
     unwind_count: u32 = 0,
+
+    /// Index into LiteralPool entry for this atom.
+    literal_index: u32 = 0,
 };
 
 pub const Alignment = @import("../../InternPool.zig").Alignment;
src/link/MachO/InternalObject.zig
@@ -109,12 +109,9 @@ fn addObjcSelrefsSection(self: *InternalObject, methname_atom_index: Atom.Index,
     return atom_index;
 }
 
-pub fn dedupLiterals(self: InternalObject, lp: *MachO.LiteralPool, macho_file: *MachO) !void {
+pub fn resolveLiterals(self: InternalObject, lp: *MachO.LiteralPool, macho_file: *MachO) !void {
     const gpa = macho_file.base.comp.gpa;
 
-    var killed_atoms = std.AutoHashMap(Atom.Index, Atom.Index).init(gpa);
-    defer killed_atoms.deinit();
-
     var buffer = std.ArrayList(u8).init(gpa);
     defer buffer.deinit();
 
@@ -126,10 +123,9 @@ pub fn dedupLiterals(self: InternalObject, lp: *MachO.LiteralPool, macho_file: *
             const res = try lp.insert(gpa, header.type(), data);
             if (!res.found_existing) {
                 res.atom.* = atom_index;
-                continue;
             }
-            atom.flags.alive = false;
-            try killed_atoms.putNoClobber(atom_index, res.atom.*);
+            atom.flags.literal_pool = true;
+            try atom.addExtra(.{ .literal_index = res.index }, macho_file);
         } else if (Object.isPtrLiteral(header)) {
             const atom = macho_file.getAtom(atom_index).?;
             const relocs = atom.getRelocs(macho_file);
@@ -145,32 +141,45 @@ pub fn dedupLiterals(self: InternalObject, lp: *MachO.LiteralPool, macho_file: *
             buffer.clearRetainingCapacity();
             if (!res.found_existing) {
                 res.atom.* = atom_index;
-                continue;
             }
-            atom.flags.alive = false;
-            try killed_atoms.putNoClobber(atom_index, res.atom.*);
+            atom.flags.literal_pool = true;
+            try atom.addExtra(.{ .literal_index = res.index }, macho_file);
         }
     }
+}
 
+pub fn dedupLiterals(self: InternalObject, lp: MachO.LiteralPool, macho_file: *MachO) void {
     for (self.atoms.items) |atom_index| {
-        if (killed_atoms.get(atom_index)) |_| continue;
         const atom = macho_file.getAtom(atom_index) orelse continue;
         if (!atom.flags.alive) continue;
         if (!atom.flags.relocs) continue;
 
         const relocs = blk: {
             const extra = atom.getExtra(macho_file).?;
-            const relocs = slice.items(.relocs)[atom.n_sect].items;
+            const relocs = self.sections.items(.relocs)[atom.n_sect].items;
             break :blk relocs[extra.rel_index..][0..extra.rel_count];
         };
         for (relocs) |*rel| switch (rel.tag) {
-            .local => if (killed_atoms.get(rel.target)) |new_target| {
-                rel.target = new_target;
+            .local => {
+                const target = macho_file.getAtom(rel.target).?;
+                if (target.getLiteralPoolIndex(macho_file)) |lp_index| {
+                    const lp_atom = lp.getAtom(lp_index, macho_file);
+                    if (target.atom_index != lp_atom.atom_index) {
+                        target.flags.alive = false;
+                        rel.target = lp_atom.atom_index;
+                    }
+                }
             },
             .@"extern" => {
-                const target = rel.getTargetSymbol(macho_file);
-                if (killed_atoms.get(target.atom)) |new_atom| {
-                    target.atom = new_atom;
+                const target_sym = rel.getTargetSymbol(macho_file);
+                if (target_sym.getAtom(macho_file)) |target_atom| {
+                    if (target_atom.getLiteralPoolIndex(macho_file)) |lp_index| {
+                        const lp_atom = lp.getAtom(lp_index, macho_file);
+                        if (target_atom.atom_index != lp_atom.atom_index) {
+                            target_atom.flags.alive = false;
+                            target_sym.atom = lp_atom.atom_index;
+                        }
+                    }
                 }
             },
         };
@@ -179,9 +188,15 @@ pub fn dedupLiterals(self: InternalObject, lp: *MachO.LiteralPool, macho_file: *
     for (self.symbols.items) |sym_index| {
         const sym = macho_file.getSymbol(sym_index);
         if (!sym.flags.objc_stubs) continue;
-        const extra = sym.getExtra(macho_file).?;
-        if (killed_atoms.get(extra.objc_selrefs)) |new_atom| {
-            try sym.addExtra(.{ .objc_selrefs = new_atom }, macho_file);
+        var extra = sym.getExtra(macho_file).?;
+        const atom = macho_file.getAtom(extra.objc_selrefs).?;
+        if (atom.getLiteralPoolIndex(macho_file)) |lp_index| {
+            const lp_atom = lp.getAtom(lp_index, macho_file);
+            if (atom.atom_index != lp_atom.atom_index) {
+                atom.flags.alive = false;
+                extra.objc_selrefs = lp_atom.atom_index;
+                sym.setExtra(extra, macho_file);
+            }
         }
     }
 }
src/link/MachO/Object.zig
@@ -527,12 +527,9 @@ fn initPointerLiterals(self: *Object, macho_file: *MachO) !void {
     }
 }
 
-pub fn dedupLiterals(self: Object, lp: *MachO.LiteralPool, macho_file: *MachO) !void {
+pub fn resolveLiterals(self: Object, lp: *MachO.LiteralPool, macho_file: *MachO) !void {
     const gpa = macho_file.base.comp.gpa;
 
-    var killed_atoms = std.AutoHashMap(Atom.Index, Atom.Index).init(gpa);
-    defer killed_atoms.deinit();
-
     var buffer = std.ArrayList(u8).init(gpa);
     defer buffer.deinit();
 
@@ -548,10 +545,9 @@ pub fn dedupLiterals(self: Object, lp: *MachO.LiteralPool, macho_file: *MachO) !
                 const res = try lp.insert(gpa, header.type(), atom_data);
                 if (!res.found_existing) {
                     res.atom.* = sub.atom;
-                    continue;
                 }
-                atom.flags.alive = false;
-                try killed_atoms.putNoClobber(sub.atom, res.atom.*);
+                atom.flags.literal_pool = true;
+                try atom.addExtra(.{ .literal_index = res.index }, macho_file);
             }
         } else if (isPtrLiteral(header)) {
             for (subs.items) |sub| {
@@ -572,33 +568,46 @@ pub fn dedupLiterals(self: Object, lp: *MachO.LiteralPool, macho_file: *MachO) !
                 buffer.clearRetainingCapacity();
                 if (!res.found_existing) {
                     res.atom.* = sub.atom;
-                    continue;
                 }
-                atom.flags.alive = false;
-                try killed_atoms.putNoClobber(sub.atom, res.atom.*);
+                atom.flags.literal_pool = true;
+                try atom.addExtra(.{ .literal_index = res.index }, macho_file);
             }
         }
     }
+}
 
+pub fn dedupLiterals(self: Object, lp: MachO.LiteralPool, macho_file: *MachO) void {
     for (self.atoms.items) |atom_index| {
-        if (killed_atoms.get(atom_index)) |_| continue;
         const atom = macho_file.getAtom(atom_index) orelse continue;
         if (!atom.flags.alive) continue;
         if (!atom.flags.relocs) continue;
 
         const relocs = blk: {
             const extra = atom.getExtra(macho_file).?;
-            const relocs = slice.items(.relocs)[atom.n_sect].items;
+            const relocs = self.sections.items(.relocs)[atom.n_sect].items;
             break :blk relocs[extra.rel_index..][0..extra.rel_count];
         };
         for (relocs) |*rel| switch (rel.tag) {
-            .local => if (killed_atoms.get(rel.target)) |new_target| {
-                rel.target = new_target;
+            .local => {
+                const target = macho_file.getAtom(rel.target).?;
+                if (target.getLiteralPoolIndex(macho_file)) |lp_index| {
+                    const lp_atom = lp.getAtom(lp_index, macho_file);
+                    if (target.atom_index != lp_atom.atom_index) {
+                        target.flags.alive = false;
+                        rel.target = lp_atom.atom_index;
+                    }
+                }
             },
             .@"extern" => {
-                const target = rel.getTargetSymbol(macho_file);
-                if (killed_atoms.get(target.atom)) |new_atom| {
-                    target.atom = new_atom;
+                const target_sym = rel.getTargetSymbol(macho_file);
+                if (target_sym.getAtom(macho_file)) |target_atom| {
+                    if (target_atom.getLiteralPoolIndex(macho_file)) |lp_index| {
+                        const lp_atom = lp.getAtom(lp_index, macho_file);
+                        if (target_atom.atom_index != lp_atom.atom_index) {
+                            target_atom.flags.alive = false;
+                            target_sym.atom = lp_atom.atom_index;
+                        }
+                    }
                 }
             },
         };
src/link/MachO/ZigObject.zig
@@ -314,7 +314,14 @@ pub fn checkDuplicates(self: *ZigObject, dupes: anytype, macho_file: *MachO) !vo
     }
 }
 
-pub fn dedupLiterals(self: *ZigObject, lp: *MachO.LiteralPool, macho_file: *MachO) !void {
+pub fn resolveLiterals(self: *ZigObject, lp: *MachO.LiteralPool, macho_file: *MachO) !void {
+    _ = self;
+    _ = lp;
+    _ = macho_file;
+    // TODO
+}
+
+pub fn dedupLiterals(self: *ZigObject, lp: MachO.LiteralPool, macho_file: *MachO) void {
     _ = self;
     _ = lp;
     _ = macho_file;
src/link/MachO.zig
@@ -1500,14 +1500,25 @@ pub fn dedupLiterals(self: *MachO) !void {
     const gpa = self.base.comp.gpa;
     var lp: LiteralPool = .{};
     defer lp.deinit(gpa);
+
     if (self.getZigObject()) |zo| {
-        try zo.dedupLiterals(&lp, self);
+        try zo.resolveLiterals(&lp, self);
     }
     for (self.objects.items) |index| {
-        try self.getFile(index).?.object.dedupLiterals(&lp, self);
+        try self.getFile(index).?.object.resolveLiterals(&lp, self);
     }
     if (self.getInternalObject()) |object| {
-        try object.dedupLiterals(&lp, self);
+        try object.resolveLiterals(&lp, self);
+    }
+
+    if (self.getZigObject()) |zo| {
+        zo.dedupLiterals(lp, self);
+    }
+    for (self.objects.items) |index| {
+        self.getFile(index).?.object.dedupLiterals(lp, self);
+    }
+    if (self.getInternalObject()) |object| {
+        object.dedupLiterals(lp, self);
     }
 }
 
@@ -4415,8 +4426,14 @@ pub const LiteralPool = struct {
         lp.data.deinit(allocator);
     }
 
+    pub fn getAtom(lp: LiteralPool, index: Index, macho_file: *MachO) *Atom {
+        assert(index < lp.values.items.len);
+        return macho_file.getAtom(lp.values.items[index]).?;
+    }
+
     const InsertResult = struct {
         found_existing: bool,
+        index: Index,
         atom: *Atom.Index,
     };
 
@@ -4434,6 +4451,7 @@ pub const LiteralPool = struct {
         }
         return .{
             .found_existing = gop.found_existing,
+            .index = @intCast(gop.index),
             .atom = &lp.values.items[gop.index],
         };
     }
@@ -4472,6 +4490,8 @@ pub const LiteralPool = struct {
             return key.hash(ctx.lp);
         }
     };
+
+    pub const Index = u32;
 };
 
 const HotUpdateState = struct {
test/link/macho.zig
@@ -38,6 +38,8 @@ pub fn testAll(b: *Build, build_opts: BuildOptions) *Step {
     macho_step.dependOn(testLayout(b, .{ .target = default_target }));
     macho_step.dependOn(testLinkingStaticLib(b, .{ .target = default_target }));
     macho_step.dependOn(testLinksection(b, .{ .target = default_target }));
+    macho_step.dependOn(testMergeLiterals(b, .{ .target = aarch64_target }));
+    macho_step.dependOn(testMergeLiterals2(b, .{ .target = aarch64_target }));
     macho_step.dependOn(testMhExecuteHeader(b, .{ .target = default_target }));
     macho_step.dependOn(testNoDeadStrip(b, .{ .target = default_target }));
     macho_step.dependOn(testNoExportsDylib(b, .{ .target = default_target }));
@@ -914,6 +916,215 @@ fn testLinksection(b: *Build, opts: Options) *Step {
     return test_step;
 }
 
+fn testMergeLiterals(b: *Build, opts: Options) *Step {
+    const test_step = addTestStep(b, "merge-literals", opts);
+
+    const a_o = addObject(b, opts, .{ .name = "a", .asm_source_bytes = 
+    \\.globl _q1
+    \\.globl _s1
+    \\
+    \\.align 4
+    \\_q1:
+    \\  adrp x8, L._q1@PAGE
+    \\  ldr d0, [x8, L._q1@PAGEOFF]
+    \\  ret
+    \\ 
+    \\.section __TEXT,__cstring,cstring_literals
+    \\l._s1:
+    \\  .asciz "hello"
+    \\
+    \\.section __TEXT,__literal8,8byte_literals
+    \\.align 8
+    \\L._q1:
+    \\  .double 1.2345
+    \\
+    \\.section __DATA,__data
+    \\.align 8
+    \\_s1:
+    \\  .quad l._s1
+    });
+
+    const b_o = addObject(b, opts, .{ .name = "b", .asm_source_bytes = 
+    \\.globl _q2
+    \\.globl _s2
+    \\.globl _s3
+    \\
+    \\.align 4
+    \\_q2:
+    \\  adrp x8, L._q2@PAGE
+    \\  ldr d0, [x8, L._q2@PAGEOFF]
+    \\  ret
+    \\ 
+    \\.section __TEXT,__cstring,cstring_literals
+    \\l._s2:
+    \\  .asciz "hello"
+    \\l._s3:
+    \\  .asciz "world"
+    \\
+    \\.section __TEXT,__literal8,8byte_literals
+    \\.align 8
+    \\L._q2:
+    \\  .double 1.2345
+    \\
+    \\.section __DATA,__data
+    \\.align 8
+    \\_s2:
+    \\   .quad l._s2
+    \\_s3:
+    \\   .quad l._s3
+    });
+
+    const main_o = addObject(b, opts, .{ .name = "main", .c_source_bytes = 
+    \\#include <stdio.h>
+    \\extern double q1();
+    \\extern double q2();
+    \\extern const char* s1;
+    \\extern const char* s2;
+    \\extern const char* s3;
+    \\int main() {
+    \\  printf("%s, %s, %s, %f, %f", s1, s2, s3, q1(), q2());
+    \\  return 0;
+    \\}
+    });
+
+    {
+        const exe = addExecutable(b, opts, .{ .name = "main1" });
+        exe.addObject(a_o);
+        exe.addObject(b_o);
+        exe.addObject(main_o);
+
+        const run = addRunArtifact(exe);
+        run.expectStdOutEqual("hello, hello, world, 1.234500, 1.234500");
+        test_step.dependOn(&run.step);
+
+        const check = exe.checkObject();
+        check.dumpSection("__TEXT,__const");
+        check.checkContains("\x8d\x97n\x12\x83\xc0\xf3?");
+        check.dumpSection("__TEXT,__cstring");
+        check.checkContains("hello\x00world\x00%s, %s, %s, %f, %f\x00");
+        test_step.dependOn(&check.step);
+    }
+
+    {
+        const exe = addExecutable(b, opts, .{ .name = "main2" });
+        exe.addObject(b_o);
+        exe.addObject(a_o);
+        exe.addObject(main_o);
+
+        const run = addRunArtifact(exe);
+        run.expectStdOutEqual("hello, hello, world, 1.234500, 1.234500");
+        test_step.dependOn(&run.step);
+
+        const check = exe.checkObject();
+        check.dumpSection("__TEXT,__const");
+        check.checkContains("\x8d\x97n\x12\x83\xc0\xf3?");
+        check.dumpSection("__TEXT,__cstring");
+        check.checkContains("hello\x00world\x00%s, %s, %s, %f, %f\x00");
+        test_step.dependOn(&check.step);
+    }
+
+    {
+        const c_o = addObject(b, opts, .{ .name = "c" });
+        c_o.addObject(a_o);
+        c_o.addObject(b_o);
+        c_o.addObject(main_o);
+
+        const exe = addExecutable(b, opts, .{ .name = "main3" });
+        exe.addObject(c_o);
+
+        const run = addRunArtifact(exe);
+        run.expectStdOutEqual("hello, hello, world, 1.234500, 1.234500");
+        test_step.dependOn(&run.step);
+
+        const check = exe.checkObject();
+        check.dumpSection("__TEXT,__const");
+        check.checkContains("\x8d\x97n\x12\x83\xc0\xf3?");
+        check.dumpSection("__TEXT,__cstring");
+        check.checkContains("hello\x00world\x00%s, %s, %s, %f, %f\x00");
+        test_step.dependOn(&check.step);
+    }
+
+    return test_step;
+}
+
+/// This particular test case will generate invalid machine code that will segfault at runtime.
+/// However, this is by design as we want to test that the linker does not panic when linking it
+/// which is also the case for the system linker and lld - linking succeeds, runtime segfaults.
+/// It should also be mentioned that runtime segfault is not due to the linker but faulty input asm.
+fn testMergeLiterals2(b: *Build, opts: Options) *Step {
+    const test_step = addTestStep(b, "merge-literals-2", opts);
+
+    const a_o = addObject(b, opts, .{ .name = "a", .asm_source_bytes = 
+    \\.globl _q1
+    \\.globl _s1
+    \\
+    \\.align 4
+    \\_q1:
+    \\  adrp x0, L._q1@PAGE
+    \\  ldr x0, [x0, L._q1@PAGEOFF]
+    \\  ret
+    \\ 
+    \\.section __TEXT,__cstring,cstring_literals
+    \\_s1:
+    \\  .asciz "hello"
+    \\
+    \\.section __TEXT,__literal8,8byte_literals
+    \\.align 8
+    \\L._q1:
+    \\  .double 1.2345
+    });
+
+    const b_o = addObject(b, opts, .{ .name = "b", .asm_source_bytes = 
+    \\.globl _q2
+    \\.globl _s2
+    \\.globl _s3
+    \\
+    \\.align 4
+    \\_q2:
+    \\  adrp x0, L._q2@PAGE
+    \\  ldr x0, [x0, L._q2@PAGEOFF]
+    \\  ret
+    \\ 
+    \\.section __TEXT,__cstring,cstring_literals
+    \\_s2:
+    \\  .asciz "hello"
+    \\_s3:
+    \\  .asciz "world"
+    \\
+    \\.section __TEXT,__literal8,8byte_literals
+    \\.align 8
+    \\L._q2:
+    \\  .double 1.2345
+    });
+
+    const main_o = addObject(b, opts, .{ .name = "main", .c_source_bytes = 
+    \\#include <stdio.h>
+    \\extern double q1();
+    \\extern double q2();
+    \\extern const char* s1;
+    \\extern const char* s2;
+    \\extern const char* s3;
+    \\int main() {
+    \\  printf("%s, %s, %s, %f, %f", s1, s2, s3, q1(), q2());
+    \\  return 0;
+    \\}
+    });
+
+    const exe = addExecutable(b, opts, .{ .name = "main1" });
+    exe.addObject(a_o);
+    exe.addObject(b_o);
+    exe.addObject(main_o);
+
+    const check = exe.checkObject();
+    check.dumpSection("__TEXT,__const");
+    check.checkContains("\x8d\x97n\x12\x83\xc0\xf3?");
+    check.dumpSection("__TEXT,__cstring");
+    check.checkContains("hello\x00world\x00%s, %s, %s, %f, %f\x00");
+    test_step.dependOn(&check.step);
+
+    return test_step;
+}
+
 fn testMhExecuteHeader(b: *Build, opts: Options) *Step {
     const test_step = addTestStep(b, "mh-execute-header", opts);