Commit b41a0b4768

mlugg <mlugg@mlugg.co.uk>
2024-03-06 22:24:38
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.
1 parent 4c05a9a
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,
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| {
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{
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);
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,
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";
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;
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, .{
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