From 196ba706a05046b2209529744d2df47215819691 Mon Sep 17 00:00:00 2001 From: Luuk de Gram Date: Wed, 28 Feb 2024 06:31:26 +0100 Subject: [PATCH] wasm: gc fixes and re-enable linker tests Certain symbols were left unmarked, meaning they would not be emit into the final binary incorrectly. We now mark the synthetic symbols to ensure they are emit as they are already created under the circumstance they're needed for. This also re-enables disabled tests that were left disabled in a previous merge conflict. Lastly, this adds the shared-memory test to the test harnass as it was previously forgotten and therefore regressed. --- src/link/Wasm.zig | 55 +++++++++++------ src/link/Wasm/Object.zig | 2 +- src/link/Wasm/ZigObject.zig | 10 +-- test/link.zig | 22 ++++--- test/link/wasm/archive/build.zig | 3 +- test/link/wasm/export-data/build.zig | 2 +- test/link/wasm/shared-memory/build.zig | 85 ++++++++++++-------------- test/link/wasm/type/build.zig | 33 +++++----- 8 files changed, 117 insertions(+), 95 deletions(-) diff --git a/src/link/Wasm.zig b/src/link/Wasm.zig index db0e275810dacd9de0c925e64da6ddc38856cb0d..9a59484aa1404bda6f7aa09c04a6b402410491a9 100644 --- a/src/link/Wasm.zig +++ b/src/link/Wasm.zig @@ -11,6 +11,7 @@ const leb = std.leb; const link = @import("../link.zig"); const lldMain = @import("../main.zig").lldMain; const log = std.log.scoped(.link); +const gc_log = std.log.scoped(.gc); const mem = std.mem; const trace = @import("../tracy.zig").trace; const types = @import("Wasm/types.zig"); @@ -525,6 +526,7 @@ pub fn createEmpty( const symbol = loc.getSymbol(wasm); symbol.setFlag(.WASM_SYM_VISIBILITY_HIDDEN); symbol.index = @intCast(wasm.imported_globals_count + wasm.wasm_globals.items.len); + symbol.mark(); try wasm.wasm_globals.append(gpa, .{ .global_type = .{ .valtype = .i32, .mutable = true }, .init = .{ .i32_const = undefined }, @@ -535,6 +537,7 @@ pub fn createEmpty( const symbol = loc.getSymbol(wasm); symbol.setFlag(.WASM_SYM_VISIBILITY_HIDDEN); symbol.index = @intCast(wasm.imported_globals_count + wasm.wasm_globals.items.len); + symbol.mark(); try wasm.wasm_globals.append(gpa, .{ .global_type = .{ .valtype = .i32, .mutable = false }, .init = .{ .i32_const = undefined }, @@ -545,6 +548,7 @@ pub fn createEmpty( const symbol = loc.getSymbol(wasm); symbol.setFlag(.WASM_SYM_VISIBILITY_HIDDEN); symbol.index = @intCast(wasm.imported_globals_count + wasm.wasm_globals.items.len); + symbol.mark(); try wasm.wasm_globals.append(gpa, .{ .global_type = .{ .valtype = .i32, .mutable = false }, .init = .{ .i32_const = undefined }, @@ -968,6 +972,8 @@ fn setupInitMemoryFunction(wasm: *Wasm) !void { if (!wasm.hasPassiveInitializationSegments()) { return; } + const sym_loc = try wasm.createSyntheticSymbol("__wasm_init_memory", .function); + sym_loc.getSymbol(wasm).mark(); const flag_address: u32 = if (shared_memory) address: { // when we have passive initialization segments and shared memory @@ -1130,7 +1136,8 @@ fn setupTLSRelocationsFunction(wasm: *Wasm) !void { return; } - // const loc = try wasm.createSyntheticSymbol("__wasm_apply_global_tls_relocs"); + const loc = try wasm.createSyntheticSymbol("__wasm_apply_global_tls_relocs", .function); + loc.getSymbol(wasm).mark(); var function_body = std.ArrayList(u8).init(gpa); defer function_body.deinit(); const writer = function_body.writer(); @@ -1833,8 +1840,7 @@ fn createSyntheticFunction( function_body: *std.ArrayList(u8), ) !void { const gpa = wasm.base.comp.gpa; - const loc = wasm.findGlobalSymbol(symbol_name) orelse - try wasm.createSyntheticSymbol(symbol_name, .function); + const loc = wasm.findGlobalSymbol(symbol_name).?; // forgot to create symbol? const symbol = loc.getSymbol(wasm); if (symbol.isDead()) { return; @@ -1884,6 +1890,9 @@ fn initializeTLSFunction(wasm: *Wasm) !void { if (!shared_memory) return; + // ensure function is marked as we must emit it + wasm.findGlobalSymbol("__wasm_init_tls").?.getSymbol(wasm).mark(); + var function_body = std.ArrayList(u8).init(gpa); defer function_body.deinit(); const writer = function_body.writer(); @@ -1932,6 +1941,7 @@ fn initializeTLSFunction(wasm: *Wasm) !void { if (wasm.findGlobalSymbol("__wasm_apply_global_tls_relocs")) |loc| { try writer.writeByte(std.wasm.opcode(.call)); try leb.writeULEB128(writer, loc.getSymbol(wasm).index); + loc.getSymbol(wasm).mark(); } try writer.writeByte(std.wasm.opcode(.end)); @@ -2039,14 +2049,19 @@ fn mergeSections(wasm: *Wasm) !void { // We found an alias to the same function, discard this symbol in favor of // the original symbol and point the discard function to it. This ensures // we only emit a single function, instead of duplicates. - symbol.unmark(); - try wasm.discarded.putNoClobber( - gpa, - sym_loc, - .{ .file = gop.key_ptr.*.file, .index = gop.value_ptr.*.sym_index }, - ); - try removed_duplicates.append(sym_loc); - continue; + // we favor keeping the global over a local. + const original_loc: SymbolLoc = .{ .file = gop.key_ptr.file, .index = gop.value_ptr.sym_index }; + const original_sym = original_loc.getSymbol(wasm); + if (original_sym.isLocal() and symbol.isGlobal()) { + original_sym.unmark(); + try wasm.discarded.put(gpa, original_loc, sym_loc); + try removed_duplicates.append(original_loc); + } else { + symbol.unmark(); + try wasm.discarded.putNoClobber(gpa, sym_loc, original_loc); + try removed_duplicates.append(sym_loc); + continue; + } } gop.value_ptr.* = .{ .func = obj_file.function(sym_loc.index), .sym_index = sym_loc.index }; symbol.index = @as(u32, @intCast(gop.index)) + wasm.imported_functions_count; @@ -2073,6 +2088,7 @@ fn mergeSections(wasm: *Wasm) !void { // For any removed duplicates, remove them from the resolved symbols list for (removed_duplicates.items) |sym_loc| { assert(wasm.resolved_symbols.swapRemove(sym_loc)); + gc_log.debug("Removed duplicate for function '{s}'", .{sym_loc.getName(wasm)}); } log.debug("Merged ({d}) functions", .{wasm.functions.count()}); @@ -2119,12 +2135,7 @@ fn mergeTypes(wasm: *Wasm) !void { log.debug("Completed merging and deduplicating types. Total count: ({d})", .{wasm.func_types.items.len}); } -fn setupExports(wasm: *Wasm) !void { - const comp = wasm.base.comp; - const gpa = comp.gpa; - if (comp.config.output_mode == .Obj) return; - log.debug("Building exports from symbols", .{}); - +fn checkExportNames(wasm: *Wasm) !void { const force_exp_names = wasm.export_symbol_names; if (force_exp_names.len > 0) { var failed_exports = false; @@ -2144,6 +2155,13 @@ fn setupExports(wasm: *Wasm) !void { return error.FlushFailure; } } +} + +fn setupExports(wasm: *Wasm) !void { + const comp = wasm.base.comp; + const gpa = comp.gpa; + if (comp.config.output_mode == .Obj) return; + log.debug("Building exports from symbols", .{}); for (wasm.resolved_symbols.keys()) |sym_loc| { const symbol = sym_loc.getSymbol(wasm); @@ -2272,6 +2290,7 @@ fn setupMemory(wasm: *Wasm) !void { memory_ptr = mem.alignForward(u64, memory_ptr, 4); const loc = try wasm.createSyntheticSymbol("__wasm_init_memory_flag", .data); const sym = loc.getSymbol(wasm); + sym.mark(); sym.virtual_address = @as(u32, @intCast(memory_ptr)); memory_ptr += 4; } @@ -2561,6 +2580,7 @@ pub fn flushModule(wasm: *Wasm, arena: Allocator, prog_node: *std.Progress.Node) if (comp.link_errors.items.len > 0) return error.FlushFailure; try wasm.resolveLazySymbols(); try wasm.checkUndefinedSymbols(); + try wasm.checkExportNames(); try wasm.setupInitFunctions(); if (comp.link_errors.items.len > 0) return error.FlushFailure; @@ -4044,6 +4064,7 @@ fn mark(wasm: *Wasm, loc: SymbolLoc) !void { return; } symbol.mark(); + gc_log.debug("Marked symbol '{s}'", .{loc.getName(wasm)}); if (symbol.isUndefined()) { // undefined symbols do not have an associated `Atom` and therefore also // do not contain relocations. diff --git a/src/link/Wasm/Object.zig b/src/link/Wasm/Object.zig index ca9f746d51e053250bba8e1e397f799e58d8163d..b2f41a33b0fc2cc6639bf90983d4e050b4abe3e4 100644 --- a/src/link/Wasm/Object.zig +++ b/src/link/Wasm/Object.zig @@ -15,7 +15,7 @@ const Allocator = std.mem.Allocator; const leb = std.leb; const meta = std.meta; -const log = std.log.scoped(.link); +const log = std.log.scoped(.object); /// Index into the list of relocatable object files within the linker driver. index: File.Index = .null, diff --git a/src/link/Wasm/ZigObject.zig b/src/link/Wasm/ZigObject.zig index 9cb3e82c9067be92a1d020bae090bc99dd688f3b..7d017e20c80f716f5cec0ae3e754775da27c88c7 100644 --- a/src/link/Wasm/ZigObject.zig +++ b/src/link/Wasm/ZigObject.zig @@ -315,7 +315,7 @@ fn finishUpdateDecl( const atom_index = decl_info.atom; const atom = wasm_file.getAtomPtr(atom_index); const sym = zig_object.symbol(atom.sym_index); - const full_name = mod.intern_pool.stringToSlice(try decl.getFullyQualifiedName(mod)); + const full_name = mod.intern_pool.stringToSlice(try decl.fullyQualifiedName(mod)); sym.name = try zig_object.string_table.insert(gpa, full_name); try atom.code.appendSlice(gpa, code); atom.size = @intCast(code.len); @@ -401,7 +401,7 @@ pub fn getOrCreateAtomForDecl(zig_object: *ZigObject, wasm_file: *Wasm, decl_ind gop.value_ptr.* = .{ .atom = try wasm_file.createAtom(sym_index, zig_object.index) }; const mod = wasm_file.base.comp.module.?; const decl = mod.declPtr(decl_index); - const full_name = mod.intern_pool.stringToSlice(try decl.getFullyQualifiedName(mod)); + const full_name = mod.intern_pool.stringToSlice(try decl.fullyQualifiedName(mod)); const sym = zig_object.symbol(sym_index); sym.name = try zig_object.string_table.insert(gpa, full_name); } @@ -455,7 +455,7 @@ pub fn lowerUnnamedConst(zig_object: *ZigObject, wasm_file: *Wasm, tv: TypedValu const parent_atom_index = try zig_object.getOrCreateAtomForDecl(wasm_file, decl_index); const parent_atom = wasm_file.getAtom(parent_atom_index); const local_index = parent_atom.locals.items.len; - const fqn = mod.intern_pool.stringToSlice(try decl.getFullyQualifiedName(mod)); + const fqn = mod.intern_pool.stringToSlice(try decl.fullyQualifiedName(mod)); const name = try std.fmt.allocPrintZ(gpa, "__unnamed_{s}_{d}", .{ fqn, local_index, }); @@ -838,6 +838,7 @@ pub fn updateExports( const atom = wasm_file.getAtom(atom_index); const atom_sym = atom.symbolLoc().getSymbol(wasm_file).*; const gpa = mod.gpa; + log.debug("Updating exports for decl '{s}'", .{mod.intern_pool.stringToSlice(decl.name)}); for (exports) |exp| { if (mod.intern_pool.stringToSliceUnwrap(exp.opts.section)) |section| { @@ -888,6 +889,7 @@ pub fn updateExports( if (exp.opts.visibility == .hidden) { sym.setFlag(.WASM_SYM_VISIBILITY_HIDDEN); } + log.debug(" with name '{s}' - {}", .{ export_string, sym }); try zig_object.global_syms.put(gpa, export_name, sym_index); try wasm_file.symbol_atom.put(gpa, .{ .file = zig_object.index, .index = sym_index }, atom_index); } @@ -1061,7 +1063,7 @@ pub fn createDebugSectionForIndex(zig_object: *ZigObject, wasm_file: *Wasm, inde pub fn updateDeclLineNumber(zig_object: *ZigObject, mod: *Module, decl_index: InternPool.DeclIndex) !void { if (zig_object.dwarf) |*dw| { const decl = mod.declPtr(decl_index); - const decl_name = mod.intern_pool.stringToSlice(try decl.getFullyQualifiedName(mod)); + const decl_name = mod.intern_pool.stringToSlice(try decl.fullyQualifiedName(mod)); log.debug("updateDeclLineNumber {s}{*}", .{ decl_name, decl }); try dw.updateDeclLineNumber(mod, decl_index); diff --git a/test/link.zig b/test/link.zig index 731736c553dc793fac6ee277ba20f081baeb773a..58b2dc9f96953e5c6b00f54cc64e39e1e298323c 100644 --- a/test/link.zig +++ b/test/link.zig @@ -35,11 +35,10 @@ pub const cases = [_]Case{ }, // WASM Cases - // https://github.com/ziglang/zig/issues/16938 - //.{ - // .build_root = "test/link/wasm/archive", - // .import = @import("link/wasm/archive/build.zig"), - //}, + .{ + .build_root = "test/link/wasm/archive", + .import = @import("link/wasm/archive/build.zig"), + }, .{ .build_root = "test/link/wasm/basic-features", .import = @import("link/wasm/basic-features/build.zig"), @@ -52,11 +51,10 @@ pub const cases = [_]Case{ .build_root = "test/link/wasm/export", .import = @import("link/wasm/export/build.zig"), }, - // https://github.com/ziglang/zig/issues/16937 - //.{ - // .build_root = "test/link/wasm/export-data", - // .import = @import("link/wasm/export-data/build.zig"), - //}, + .{ + .build_root = "test/link/wasm/export-data", + .import = @import("link/wasm/export-data/build.zig"), + }, .{ .build_root = "test/link/wasm/extern", .import = @import("link/wasm/extern/build.zig"), @@ -81,6 +79,10 @@ pub const cases = [_]Case{ .build_root = "test/link/wasm/segments", .import = @import("link/wasm/segments/build.zig"), }, + .{ + .build_root = "test/link/wasm/shared-memory", + .import = @import("link/wasm/shared-memory/build.zig"), + }, .{ .build_root = "test/link/wasm/stack_pointer", .import = @import("link/wasm/stack_pointer/build.zig"), diff --git a/test/link/wasm/archive/build.zig b/test/link/wasm/archive/build.zig index 1d5e031848b91c948bc95ba34e737ab4c825eb02..cd91feae65dd1015c6282c186ae3736bea27e145 100644 --- a/test/link/wasm/archive/build.zig +++ b/test/link/wasm/archive/build.zig @@ -19,12 +19,13 @@ fn add(b: *std.Build, test_step: *std.Build.Step, optimize: std.builtin.Optimize .name = "main", .root_source_file = .{ .path = "main.zig" }, .optimize = optimize, - .target = .{ .cpu_arch = .wasm32, .os_tag = .freestanding }, + .target = b.resolveTargetQuery(.{ .cpu_arch = .wasm32, .os_tag = .freestanding }), .strip = false, }); lib.entry = .disabled; lib.use_llvm = false; lib.use_lld = false; + lib.root_module.export_symbol_names = &.{"foo"}; const check = lib.checkObject(); check.checkInHeaders(); diff --git a/test/link/wasm/export-data/build.zig b/test/link/wasm/export-data/build.zig index bba00d5c12813b576f913da6c90b57348b424fd6..05b2ca161bbfb24d2907d3af5fcde9ea2c658747 100644 --- a/test/link/wasm/export-data/build.zig +++ b/test/link/wasm/export-data/build.zig @@ -13,7 +13,7 @@ pub fn build(b: *std.Build) void { .name = "lib", .root_source_file = .{ .path = "lib.zig" }, .optimize = .ReleaseSafe, // to make the output deterministic in address positions - .target = .{ .cpu_arch = .wasm32, .os_tag = .freestanding }, + .target = b.resolveTargetQuery(.{ .cpu_arch = .wasm32, .os_tag = .freestanding }), }); lib.entry = .disabled; lib.use_lld = false; diff --git a/test/link/wasm/shared-memory/build.zig b/test/link/wasm/shared-memory/build.zig index f8751d99c049d83e19e3e5187e88fff116a8fdd6..0cad2560cbe6fc7fb814b6c99b5c2a21871c2d32 100644 --- a/test/link/wasm/shared-memory/build.zig +++ b/test/link/wasm/shared-memory/build.zig @@ -11,37 +11,39 @@ pub fn build(b: *std.Build) void { } fn add(b: *std.Build, test_step: *std.Build.Step, optimize_mode: std.builtin.OptimizeMode) void { - const lib = b.addExecutable(.{ + const exe = b.addExecutable(.{ .name = "lib", .root_source_file = .{ .path = "lib.zig" }, - .target = .{ + .target = b.resolveTargetQuery(.{ .cpu_arch = .wasm32, .cpu_model = .{ .explicit = &std.Target.wasm.cpu.mvp }, .cpu_features_add = std.Target.wasm.featureSet(&.{ .atomics, .bulk_memory }), .os_tag = .freestanding, - }, + }), .optimize = optimize_mode, .strip = false, .single_threaded = false, }); - lib.entry = .disabled; - lib.use_lld = false; - lib.import_memory = true; - lib.export_memory = true; - lib.shared_memory = true; - lib.max_memory = 67108864; - lib.root_module.export_symbol_names = &.{"foo"}; + exe.entry = .disabled; + exe.use_lld = false; + exe.import_memory = true; + exe.export_memory = true; + exe.shared_memory = true; + exe.max_memory = 67108864; + exe.root_module.export_symbol_names = &.{"foo"}; - const check_lib = lib.checkObject(); + const check_exe = exe.checkObject(); - check_lib.checkStart("Section import"); - check_lib.checkNext("entries 1"); - check_lib.checkNext("module env"); - check_lib.checkNext("name memory"); // ensure we are importing memory + check_exe.checkInHeaders(); + check_exe.checkExact("Section import"); + check_exe.checkExact("entries 1"); + check_exe.checkExact("module env"); + check_exe.checkExact("name memory"); // ensure we are importing memory - check_lib.checkStart("Section export"); - check_lib.checkNext("entries 2"); - check_lib.checkNext("name memory"); // ensure we also export memory again + check_exe.checkInHeaders(); + check_exe.checkExact("Section export"); + check_exe.checkExact("entries 2"); + check_exe.checkExact("name memory"); // ensure we also export memory again // This section *must* be emit as the start function is set to the index // of __wasm_init_memory @@ -49,49 +51,42 @@ fn add(b: *std.Build, test_step: *std.Build.Step, optimize_mode: std.builtin.Opt // This means we won't have __wasm_init_memory in such case, and therefore // should also not have a section "start" if (optimize_mode == .Debug) { - check_lib.checkStart("Section start"); + check_exe.checkInHeaders(); + check_exe.checkExact("Section start"); } // This section is only and *must* be emit when shared-memory is enabled // release modes will have the TLS segment optimized out in our test-case. if (optimize_mode == .Debug) { - check_lib.checkStart("Section data_count"); - check_lib.checkNext("count 3"); + check_exe.checkInHeaders(); + check_exe.checkExact("Section data_count"); + check_exe.checkExact("count 1"); } - check_lib.checkStart("Section custom"); - check_lib.checkNext("name name"); - check_lib.checkNext("type function"); + check_exe.checkInHeaders(); + check_exe.checkExact("Section custom"); + check_exe.checkExact("name name"); + check_exe.checkExact("type function"); if (optimize_mode == .Debug) { - check_lib.checkNext("name __wasm_init_memory"); + check_exe.checkExact("name __wasm_init_memory"); } - check_lib.checkNext("name __wasm_init_tls"); - check_lib.checkNext("type global"); + check_exe.checkExact("name __wasm_init_tls"); + check_exe.checkExact("type global"); // In debug mode the symbol __tls_base is resolved to an undefined symbol // from the object file, hence its placement differs than in release modes // where the entire tls segment is optimized away, and tls_base will have // its original position. - if (optimize_mode == .Debug) { - check_lib.checkNext("name __tls_size"); - check_lib.checkNext("name __tls_align"); - check_lib.checkNext("name __tls_base"); - } else { - check_lib.checkNext("name __tls_base"); - check_lib.checkNext("name __tls_size"); - check_lib.checkNext("name __tls_align"); - } + check_exe.checkExact("name __tls_base"); + check_exe.checkExact("name __tls_size"); + check_exe.checkExact("name __tls_align"); - check_lib.checkNext("type data_segment"); + check_exe.checkExact("type data_segment"); if (optimize_mode == .Debug) { - check_lib.checkNext("names 3"); - check_lib.checkNext("index 0"); - check_lib.checkNext("name .rodata"); - check_lib.checkNext("index 1"); - check_lib.checkNext("name .bss"); - check_lib.checkNext("index 2"); - check_lib.checkNext("name .tdata"); + check_exe.checkExact("names 1"); + check_exe.checkExact("index 0"); + check_exe.checkExact("name .tdata"); } - test_step.dependOn(&check_lib.step); + test_step.dependOn(&check_exe.step); } diff --git a/test/link/wasm/type/build.zig b/test/link/wasm/type/build.zig index a318ddb1f97d9688d685a73e0151d4ebce9ea4e8..b62886c74eb3000494a5d71c189e5050f7abba4a 100644 --- a/test/link/wasm/type/build.zig +++ b/test/link/wasm/type/build.zig @@ -13,31 +13,32 @@ pub fn build(b: *std.Build) void { } fn add(b: *std.Build, test_step: *std.Build.Step, optimize: std.builtin.OptimizeMode) void { - const lib = b.addExecutable(.{ + const exe = b.addExecutable(.{ .name = "lib", .root_source_file = .{ .path = "lib.zig" }, .target = b.resolveTargetQuery(.{ .cpu_arch = .wasm32, .os_tag = .freestanding }), .optimize = optimize, .strip = false, }); - lib.entry = .disabled; - lib.use_llvm = false; - lib.use_lld = false; - b.installArtifact(lib); + exe.entry = .disabled; + exe.use_llvm = false; + exe.use_lld = false; + exe.root_module.export_symbol_names = &.{"foo"}; + b.installArtifact(exe); - const check_lib = lib.checkObject(); - check_lib.checkInHeaders(); - check_lib.checkExact("Section type"); + const check_exe = exe.checkObject(); + check_exe.checkInHeaders(); + check_exe.checkExact("Section type"); // only 2 entries, although we have more functions. // This is to test functions with the same function signature // have their types deduplicated. - check_lib.checkExact("entries 2"); - check_lib.checkExact("params 1"); - check_lib.checkExact("type i32"); - check_lib.checkExact("returns 1"); - check_lib.checkExact("type i64"); - check_lib.checkExact("params 0"); - check_lib.checkExact("returns 0"); + check_exe.checkExact("entries 2"); + check_exe.checkExact("params 1"); + check_exe.checkExact("type i32"); + check_exe.checkExact("returns 1"); + check_exe.checkExact("type i64"); + check_exe.checkExact("params 0"); + check_exe.checkExact("returns 0"); - test_step.dependOn(&check_lib.step); + test_step.dependOn(&check_exe.step); } -- 2.54.0