From 6ff267dc2635177b57bda1d398374ab96ed40179 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Tue, 29 Oct 2024 21:17:11 +0100 Subject: [PATCH] link/Elf: ensure we always sort all relocations by r_offset in -r mode According to a comment in mold, this is the expected (and desired) condition by the linkers, except for some architectures (RISCV and Loongarch) where this condition does not have to upheld. If you follow the changes in this patch and in particular doc comments I have linked the comment/code in mold that explains and implements this. I have also modified `testEhFrameRelocatable` test to now test both cases such that `zig ld -r a.o b.o -o c.o` and `zig ld -r b.o a.o -o d.o`. In both cases, `c.o` and `d.o` should produce valid object files which was not the case before this patch. --- src/link/Elf/Object.zig | 4 +++ src/link/Elf/eh_frame.zig | 11 +++--- src/link/Elf/relocatable.zig | 36 ++++++++++++-------- test/link/elf.zig | 66 +++++++++++++++++++++++------------- 4 files changed, 72 insertions(+), 45 deletions(-) diff --git a/src/link/Elf/Object.zig b/src/link/Elf/Object.zig index d1ff168341b44605d1c162b21bc3452718e167b0..688e51f3f141b8a631d0ccfe8931f900e0b85519 100644 --- a/src/link/Elf/Object.zig +++ b/src/link/Elf/Object.zig @@ -398,6 +398,10 @@ fn parseEhFrame( defer gpa.free(relocs); const rel_start: u32 = @intCast(self.relocs.items.len); try self.relocs.appendUnalignedSlice(gpa, relocs); + + // We expect relocations to be sorted by r_offset as per this comment in mold linker: + // https://github.com/rui314/mold/blob/8e4f7b53832d8af4f48a633a8385cbc932d1944e/src/input-files.cc#L653 + // Except for RISCV and Loongarch which do not seem to be uphold this convention. if (target.cpu.arch == .riscv64) { sortRelocs(self.relocs.items[rel_start..][0..relocs.len]); } diff --git a/src/link/Elf/eh_frame.zig b/src/link/Elf/eh_frame.zig index 1af1236526e8481bb77a43fa4084d2a7de63ffbd..4df05d49d3d52dd804bf45664a3f2c1985dea73f 100644 --- a/src/link/Elf/eh_frame.zig +++ b/src/link/Elf/eh_frame.zig @@ -453,7 +453,7 @@ fn emitReloc(elf_file: *Elf, r_offset: u64, sym: *const Symbol, rel: elf.Elf64_R }; } -pub fn writeEhFrameRelocs(elf_file: *Elf, writer: anytype) !void { +pub fn writeEhFrameRelocs(elf_file: *Elf, relocs: *std.ArrayList(elf.Elf64_Rela)) !void { relocs_log.debug("{x}: .eh_frame", .{ elf_file.sections.items(.shdr)[elf_file.section_indexes.eh_frame.?].sh_addr, }); @@ -466,8 +466,7 @@ pub fn writeEhFrameRelocs(elf_file: *Elf, writer: anytype) !void { for (atom_ptr.relocs(elf_file)) |rel| { const ref = zo.resolveSymbol(rel.r_sym(), elf_file); const target = elf_file.symbol(ref).?; - const out_rel = emitReloc(elf_file, rel.r_offset, target, rel); - try writer.writeStruct(out_rel); + try relocs.append(emitReloc(elf_file, rel.r_offset, target, rel)); } } @@ -480,8 +479,7 @@ pub fn writeEhFrameRelocs(elf_file: *Elf, writer: anytype) !void { const ref = object.resolveSymbol(rel.r_sym(), elf_file); const sym = elf_file.symbol(ref).?; const r_offset = cie.address(elf_file) + rel.r_offset - cie.offset; - const out_rel = emitReloc(elf_file, r_offset, sym, rel); - try writer.writeStruct(out_rel); + try relocs.append(emitReloc(elf_file, r_offset, sym, rel)); } } @@ -491,8 +489,7 @@ pub fn writeEhFrameRelocs(elf_file: *Elf, writer: anytype) !void { const ref = object.resolveSymbol(rel.r_sym(), elf_file); const sym = elf_file.symbol(ref).?; const r_offset = fde.address(elf_file) + rel.r_offset - fde.offset; - const out_rel = emitReloc(elf_file, r_offset, sym, rel); - try writer.writeStruct(out_rel); + try relocs.append(emitReloc(elf_file, r_offset, sym, rel)); } } } diff --git a/src/link/Elf/relocatable.zig b/src/link/Elf/relocatable.zig index b4943604462b96aad4cf12f29461e7b9009ffea1..3035c33790faad58164241ea0675e0a1ac86e5bc 100644 --- a/src/link/Elf/relocatable.zig +++ b/src/link/Elf/relocatable.zig @@ -362,6 +362,14 @@ fn writeSyntheticSections(elf_file: *Elf) !void { const gpa = elf_file.base.comp.gpa; const slice = elf_file.sections.slice(); + const SortRelocs = struct { + pub fn lessThan(ctx: void, lhs: elf.Elf64_Rela, rhs: elf.Elf64_Rela) bool { + _ = ctx; + assert(lhs.r_offset != rhs.r_offset); + return lhs.r_offset < rhs.r_offset; + } + }; + for (slice.items(.shdr), slice.items(.atom_list), 0..) |shdr, atom_list, shndx| { if (shdr.sh_type != elf.SHT_RELA) continue; if (atom_list.items.len == 0) continue; @@ -378,15 +386,8 @@ fn writeSyntheticSections(elf_file: *Elf) !void { try atom_ptr.writeRelocs(elf_file, &relocs); } assert(relocs.items.len == num_relocs); - - const SortRelocs = struct { - pub fn lessThan(ctx: void, lhs: elf.Elf64_Rela, rhs: elf.Elf64_Rela) bool { - _ = ctx; - assert(lhs.r_offset != rhs.r_offset); - return lhs.r_offset < rhs.r_offset; - } - }; - + // Sort output relocations by r_offset which is usually an expected (and desired) condition + // by the linkers. mem.sortUnstable(elf.Elf64_Rela, relocs.items, {}, SortRelocs.lessThan); log.debug("writing {s} from 0x{x} to 0x{x}", .{ @@ -418,16 +419,21 @@ fn writeSyntheticSections(elf_file: *Elf) !void { } if (elf_file.section_indexes.eh_frame_rela) |shndx| { const shdr = slice.items(.shdr)[shndx]; - const sh_size = math.cast(usize, shdr.sh_size) orelse return error.Overflow; - var buffer = try std.ArrayList(u8).initCapacity(gpa, sh_size); - defer buffer.deinit(); - try eh_frame.writeEhFrameRelocs(elf_file, buffer.writer()); - assert(buffer.items.len == sh_size); + const num_relocs = math.cast(usize, @divExact(shdr.sh_size, shdr.sh_entsize)) orelse + return error.Overflow; + var relocs = try std.ArrayList(elf.Elf64_Rela).initCapacity(gpa, num_relocs); + defer relocs.deinit(); + try eh_frame.writeEhFrameRelocs(elf_file, &relocs); + assert(relocs.items.len == num_relocs); + // Sort output relocations by r_offset which is usually an expected (and desired) condition + // by the linkers. + mem.sortUnstable(elf.Elf64_Rela, relocs.items, {}, SortRelocs.lessThan); + log.debug("writing .rela.eh_frame from 0x{x} to 0x{x}", .{ shdr.sh_offset, shdr.sh_offset + shdr.sh_size, }); - try elf_file.base.file.?.pwriteAll(buffer.items, shdr.sh_offset); + try elf_file.base.file.?.pwriteAll(mem.sliceAsBytes(relocs.items), shdr.sh_offset); } try writeComdatGroups(elf_file); diff --git a/test/link/elf.zig b/test/link/elf.zig index 7d387a07623b426a49467bb3c1bf945ff297dc75..5014d20c9862fc7ccca2c24e9be097a91b6d1970 100644 --- a/test/link/elf.zig +++ b/test/link/elf.zig @@ -2744,32 +2744,52 @@ fn testRelocatableEhFrame(b: *Build, opts: Options) *Step { , }); obj2.linkLibCpp(); + const obj3 = addObject(b, opts, .{ .name = "obj3", .cpp_source_bytes = + \\#include + \\#include + \\extern int try_again(); + \\int main() { + \\ try { + \\ try_again(); + \\ } catch (const std::exception &e) { + \\ std::cout << "exception=" << e.what(); + \\ } + \\ return 0; + \\} + }); + obj3.linkLibCpp(); - const obj = addObject(b, opts, .{ .name = "obj" }); - obj.addObject(obj1); - obj.addObject(obj2); - obj.linkLibCpp(); + { + const obj = addObject(b, opts, .{ .name = "obj" }); + obj.addObject(obj1); + obj.addObject(obj2); + obj.linkLibCpp(); - const exe = addExecutable(b, opts, .{ .name = "test1" }); - addCppSourceBytes(exe, - \\#include - \\#include - \\extern int try_again(); - \\int main() { - \\ try { - \\ try_again(); - \\ } catch (const std::exception &e) { - \\ std::cout << "exception=" << e.what(); - \\ } - \\ return 0; - \\} - , &.{}); - exe.addObject(obj); - exe.linkLibCpp(); + const exe = addExecutable(b, opts, .{ .name = "test1" }); + exe.addObject(obj3); + exe.addObject(obj); + exe.linkLibCpp(); - const run = addRunArtifact(exe); - run.expectStdOutEqual("exception=Oh no!"); - test_step.dependOn(&run.step); + const run = addRunArtifact(exe); + run.expectStdOutEqual("exception=Oh no!"); + test_step.dependOn(&run.step); + } + { + // Flipping the order should not influence the end result. + const obj = addObject(b, opts, .{ .name = "obj" }); + obj.addObject(obj2); + obj.addObject(obj1); + obj.linkLibCpp(); + + const exe = addExecutable(b, opts, .{ .name = "test2" }); + exe.addObject(obj3); + exe.addObject(obj); + exe.linkLibCpp(); + + const run = addRunArtifact(exe); + run.expectStdOutEqual("exception=Oh no!"); + test_step.dependOn(&run.step); + } return test_step; } -- 2.54.0