From b41a0b4768d368d81d0d33c779f919d8f315e622 Mon Sep 17 00:00:00 2001 From: mlugg Date: Wed, 6 Mar 2024 21:24:38 +0000 Subject: [PATCH] Package.Module: deduplicate identical builtin modules Previously, when multiple modules had builtin modules with identical sources, two distinct `Module`s and `File`s were created pointing at the same file path. This led to a bug later in the frontend. These modules are now deduplicated with a simple hashmap on the builtin source. --- src/Compilation.zig | 4 ++++ src/Package/Module.zig | 16 +++++++++++++++- src/Sema.zig | 1 + src/glibc.zig | 1 + src/libcxx.zig | 2 ++ src/libtsan.zig | 1 + src/libunwind.zig | 1 + src/main.zig | 18 +++++++++++++++--- src/musl.zig | 1 + 9 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/Compilation.zig b/src/Compilation.zig index 539a11faa0..798fbc9a4c 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -1325,6 +1325,7 @@ pub fn create(gpa: Allocator, arena: Allocator, options: CreateOptions) !*Compil .global = options.config, .parent = options.root_mod, .builtin_mod = options.root_mod.getBuiltinDependency(), + .builtin_modules = null, // `builtin_mod` is set }); try options.root_mod.deps.putNoClobber(arena, "compiler_rt", compiler_rt_mod); } @@ -1429,6 +1430,7 @@ pub fn create(gpa: Allocator, arena: Allocator, options: CreateOptions) !*Compil .global = options.config, .parent = options.root_mod, .builtin_mod = options.root_mod.getBuiltinDependency(), + .builtin_modules = null, // `builtin_mod` is set }); const zcu = try arena.create(Module); @@ -6104,6 +6106,7 @@ fn buildOutputFromZig( .cc_argv = &.{}, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }); const root_name = src_basename[0 .. src_basename.len - std.fs.path.extension(src_basename).len]; const target = comp.getTarget(); @@ -6216,6 +6219,7 @@ pub fn build_crt_file( .cc_argv = &.{}, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }); for (c_source_files) |*item| { diff --git a/src/Package/Module.zig b/src/Package/Module.zig index 6fadbf1dd5..e5376ed93a 100644 --- a/src/Package/Module.zig +++ b/src/Package/Module.zig @@ -63,6 +63,11 @@ pub const CreateOptions = struct { builtin_mod: ?*Package.Module, + /// Allocated into the given `arena`. Should be shared across all module creations in a Compilation. + /// Ignored if `builtin_mod` is passed or if `!have_zcu`. + /// Otherwise, may be `null` only if this Compilation consists of a single module. + builtin_modules: ?*std.StringHashMapUnmanaged(*Module), + pub const Paths = struct { root: Package.Path, /// Relative to `root`. May contain path separators. @@ -364,11 +369,20 @@ pub fn create(arena: Allocator, options: CreateOptions) !*Package.Module { .wasi_exec_model = options.global.wasi_exec_model, }, arena); + const new = if (options.builtin_modules) |builtins| new: { + const gop = try builtins.getOrPut(arena, generated_builtin_source); + if (gop.found_existing) break :b gop.value_ptr.*; + errdefer builtins.removeByPtr(gop.key_ptr); + const new = try arena.create(Module); + gop.value_ptr.* = new; + break :new new; + } else try arena.create(Module); + errdefer if (options.builtin_modules) |builtins| assert(builtins.remove(generated_builtin_source)); + const new_file = try arena.create(File); const digest = Cache.HashHelper.oneShot(generated_builtin_source); const builtin_sub_path = try arena.dupe(u8, "b" ++ std.fs.path.sep_str ++ digest); - const new = try arena.create(Module); new.* = .{ .root = .{ .root_dir = options.global_cache_directory, diff --git a/src/Sema.zig b/src/Sema.zig index 2ea7432c0e..cff7404a3b 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -5858,6 +5858,7 @@ fn zirCImport(sema: *Sema, parent_block: *Block, inst: Zir.Inst.Index) CompileEr .global = comp.config, .parent = parent_mod, .builtin_mod = parent_mod.getBuiltinDependency(), + .builtin_modules = null, // `builtin_mod` is set }) catch |err| switch (err) { // None of these are possible because we are creating a package with // the exact same configuration as the parent package, which already diff --git a/src/glibc.zig b/src/glibc.zig index a5d768d64c..5a2caa6809 100644 --- a/src/glibc.zig +++ b/src/glibc.zig @@ -1118,6 +1118,7 @@ fn buildSharedLib( .cc_argv = &.{}, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }); const c_source_files = [1]Compilation.CSourceFile{ diff --git a/src/libcxx.zig b/src/libcxx.zig index d47ec71a64..980836b682 100644 --- a/src/libcxx.zig +++ b/src/libcxx.zig @@ -181,6 +181,7 @@ pub fn buildLibCXX(comp: *Compilation, prog_node: *std.Progress.Node) !void { .cc_argv = &.{}, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }); var c_source_files = try std.ArrayList(Compilation.CSourceFile).initCapacity(arena, libcxx_files.len); @@ -395,6 +396,7 @@ pub fn buildLibCXXABI(comp: *Compilation, prog_node: *std.Progress.Node) !void { .cc_argv = &.{}, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }); var c_source_files = try std.ArrayList(Compilation.CSourceFile).initCapacity(arena, libcxxabi_files.len); diff --git a/src/libtsan.zig b/src/libtsan.zig index 803dd546ed..95b44dad76 100644 --- a/src/libtsan.zig +++ b/src/libtsan.zig @@ -92,6 +92,7 @@ pub fn buildTsan(comp: *Compilation, prog_node: *std.Progress.Node) BuildError!v .cc_argv = &common_flags, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }) catch |err| { comp.setMiscFailure( .libtsan, diff --git a/src/libunwind.zig b/src/libunwind.zig index 5f0613c361..8de6f5a99a 100644 --- a/src/libunwind.zig +++ b/src/libunwind.zig @@ -58,6 +58,7 @@ pub fn buildStaticLib(comp: *Compilation, prog_node: *std.Progress.Node) !void { .cc_argv = &.{}, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }); const root_name = "unwind"; diff --git a/src/main.zig b/src/main.zig index 72c0773044..54ce02ff13 100644 --- a/src/main.zig +++ b/src/main.zig @@ -2697,7 +2697,9 @@ fn buildOutputType( create_module.opts.emit_bin = emit_bin != .no; create_module.opts.any_c_source_files = create_module.c_source_files.items.len != 0; - const main_mod = try createModule(gpa, arena, &create_module, 0, null, zig_lib_directory); + var builtin_modules: std.StringHashMapUnmanaged(*Package.Module) = .{}; + // `builtin_modules` allocated into `arena`, so no deinit + const main_mod = try createModule(gpa, arena, &create_module, 0, null, zig_lib_directory, &builtin_modules); for (create_module.modules.keys(), create_module.modules.values()) |key, cli_mod| { if (cli_mod.resolved == null) fatal("module '{s}' declared but not used", .{key}); @@ -2742,6 +2744,7 @@ fn buildOutputType( .global = create_module.resolved_options, .parent = main_mod, .builtin_mod = main_mod.getBuiltinDependency(), + .builtin_modules = null, // `builtin_mod` is specified }); test_mod.deps = try main_mod.deps.clone(arena); break :test_mod test_mod; @@ -2760,6 +2763,7 @@ fn buildOutputType( .global = create_module.resolved_options, .parent = main_mod, .builtin_mod = main_mod.getBuiltinDependency(), + .builtin_modules = null, // `builtin_mod` is specified }); break :root_mod test_mod; @@ -3467,6 +3471,7 @@ fn createModule( index: usize, parent: ?*Package.Module, zig_lib_directory: Cache.Directory, + builtin_modules: *std.StringHashMapUnmanaged(*Package.Module), ) Allocator.Error!*Package.Module { const cli_mod = &create_module.modules.values()[index]; if (cli_mod.resolved) |m| return m; @@ -3919,6 +3924,7 @@ fn createModule( .global = create_module.resolved_options, .parent = parent, .builtin_mod = null, + .builtin_modules = builtin_modules, }) catch |err| switch (err) { error.ValgrindUnsupportedOnTarget => fatal("unable to create module '{s}': valgrind does not support the selected target CPU architecture", .{name}), error.TargetRequiresSingleThreaded => fatal("unable to create module '{s}': the selected target does not support multithreading", .{name}), @@ -3941,7 +3947,7 @@ fn createModule( for (cli_mod.deps) |dep| { const dep_index = create_module.modules.getIndex(dep.value) orelse fatal("module '{s}' depends on non-existent module '{s}'", .{ name, dep.key }); - const dep_mod = try createModule(gpa, arena, create_module, dep_index, mod, zig_lib_directory); + const dep_mod = try createModule(gpa, arena, create_module, dep_index, mod, zig_lib_directory, builtin_modules); try mod.deps.put(arena, dep.key, dep_mod); } @@ -5237,6 +5243,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void { .global = config, .parent = null, .builtin_mod = null, + .builtin_modules = null, // all modules will inherit this one's builtin }); const builtin_mod = root_mod.getBuiltinDependency(); @@ -5253,6 +5260,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void { .global = config, .parent = root_mod, .builtin_mod = builtin_mod, + .builtin_modules = null, // `builtin_mod` is specified }); var cleanup_build_dir: ?fs.Dir = null; @@ -5387,6 +5395,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void { .global = config, .parent = root_mod, .builtin_mod = builtin_mod, + .builtin_modules = null, // `builtin_mod` is specified }); const hash_cloned = try arena.dupe(u8, &hash); deps_mod.deps.putAssumeCapacityNoClobber(hash_cloned, m); @@ -5636,6 +5645,7 @@ fn jitCmd( .global = config, .parent = null, .builtin_mod = null, + .builtin_modules = null, // all modules will inherit this one's builtin }); if (options.depend_on_aro) { @@ -5658,6 +5668,7 @@ fn jitCmd( .global = config, .parent = null, .builtin_mod = root_mod.getBuiltinDependency(), + .builtin_modules = null, // `builtin_mod` is specified }); try root_mod.deps.put(arena, "aro", aro_mod); } @@ -7204,10 +7215,11 @@ fn createDependenciesModule( }, .fully_qualified_name = "root.@dependencies", .parent = main_mod, - .builtin_mod = builtin_mod, .cc_argv = &.{}, .inherited = .{}, .global = global_options, + .builtin_mod = builtin_mod, + .builtin_modules = null, // `builtin_mod` is specified }); try main_mod.deps.put(arena, "@dependencies", deps_mod); return deps_mod; diff --git a/src/musl.zig b/src/musl.zig index 03011248e0..ed943b7bf5 100644 --- a/src/musl.zig +++ b/src/musl.zig @@ -250,6 +250,7 @@ pub fn buildCRTFile(comp: *Compilation, crt_file: CRTFile, prog_node: *std.Progr .cc_argv = cc_argv, .parent = null, .builtin_mod = null, + .builtin_modules = null, // there is only one module in this compilation }); const sub_compilation = try Compilation.create(comp.gpa, arena, .{