stage2: free Package resources

Without this commit we leak file descriptors and memory

2 files changed, 60 insertions, 58 deletions

diff --git a/src/Package.zig b/src/Package.zig
index 8a0e89f883..33ff4766ca 100644
--- a/src/Package.zig
+++ b/src/Package.zig
@@ -1,3 +1,12 @@
+const Package = @This();
+
+const std = @import("std");
+const fs = std.fs;
+const mem = std.mem;
+const Allocator = mem.Allocator;
+
+const Compilation = @import("Compilation.zig");
+
 pub const Table = std.StringHashMapUnmanaged(*Package);
 
 root_src_directory: Compilation.Directory,
@@ -6,57 +15,60 @@ root_src_path: []const u8,
 table: Table = .{},
 parent: ?*Package = null,
 
-const std = @import("std");
-const mem = std.mem;
-const Allocator = std.mem.Allocator;
-const assert = std.debug.assert;
-const Package = @This();
-const Compilation = @import("Compilation.zig");
-
-/// No references to `root_src_dir` and `root_src_path` are kept.
+/// Allocate a Package. No references to the slices passed are kept.
 pub fn create(
     gpa: *Allocator,
-    base_directory: Compilation.Directory,
-    /// Relative to `base_directory`.
-    root_src_dir: []const u8,
-    /// Relative to `root_src_dir`.
+    /// Null indicates the current working directory
+    root_src_dir_path: ?[]const u8,
+    /// Relative to root_src_dir_path
     root_src_path: []const u8,
 ) !*Package {
     const ptr = try gpa.create(Package);
     errdefer gpa.destroy(ptr);
 
-    const root_src_dir_path = try base_directory.join(gpa, &[_][]const u8{root_src_dir});
-    errdefer gpa.free(root_src_dir_path);
+    const owned_dir_path = if (root_src_dir_path) |p| try gpa.dupe(u8, p) else null;
+    errdefer if (owned_dir_path) |p| gpa.free(p);
 
-    const root_src_path_dupe = try mem.dupe(gpa, u8, root_src_path);
-    errdefer gpa.free(root_src_path_dupe);
+    const owned_src_path = try gpa.dupe(u8, root_src_path);
+    errdefer gpa.free(owned_src_path);
 
     ptr.* = .{
         .root_src_directory = .{
-            .path = root_src_dir_path,
-            .handle = try base_directory.handle.openDir(root_src_dir, .{}),
+            .path = owned_dir_path,
+            .handle = if (owned_dir_path) |p| try fs.cwd().openDir(p, .{}) else fs.cwd(),
         },
-        .root_src_path = root_src_path_dupe,
+        .root_src_path = owned_src_path,
     };
+
     return ptr;
 }
 
+/// Free all memory associated with this package and recursively call destroy
+/// on all packages in its table
 pub fn destroy(pkg: *Package, gpa: *Allocator) void {
-    pkg.root_src_directory.handle.close();
     gpa.free(pkg.root_src_path);
-    if (pkg.root_src_directory.path) |p| gpa.free(p);
+
+    // If root_src_directory.path is null then the handle is the cwd()
+    // which shouldn't be closed.
+    if (pkg.root_src_directory.path) |p| {
+        gpa.free(p);
+        pkg.root_src_directory.handle.close();
+    }
+
     {
         var it = pkg.table.iterator();
         while (it.next()) |kv| {
+            kv.value.destroy(gpa);
             gpa.free(kv.key);
         }
     }
+
     pkg.table.deinit(gpa);
     gpa.destroy(pkg);
 }
 
 pub fn add(pkg: *Package, gpa: *Allocator, name: []const u8, package: *Package) !void {
-    try pkg.table.ensureCapacity(gpa, pkg.table.items().len + 1);
+    try pkg.table.ensureCapacity(gpa, pkg.table.count() + 1);
     const name_dupe = try mem.dupe(gpa, u8, name);
     pkg.table.putAssumeCapacityNoClobber(name_dupe, package);
 }
diff --git a/src/main.zig b/src/main.zig
index 9654fc9565..d0614a7b1c 100644
--- a/src/main.zig
+++ b/src/main.zig
@@ -560,12 +560,15 @@ fn buildOutputType(
     var test_exec_args = std.ArrayList(?[]const u8).init(gpa);
     defer test_exec_args.deinit();
 
-    var root_pkg_memory: Package = .{
-        .root_src_directory = undefined,
-        .root_src_path = undefined,
+    const pkg_tree_root = try gpa.create(Package);
+    // This package only exists to clean up the code parsing --pkg-begin and
+    // --pkg-end flags. Use dummy values that are safe for the destroy call.
+    pkg_tree_root.* = .{
+        .root_src_directory = .{ .path = null, .handle = fs.cwd() },
+        .root_src_path = &[0]u8{},
     };
-    defer root_pkg_memory.table.deinit(gpa);
-    var cur_pkg: *Package = &root_pkg_memory;
+    defer pkg_tree_root.destroy(gpa);
+    var cur_pkg: *Package = pkg_tree_root;
 
     switch (arg_mode) {
         .build, .translate_c, .zig_test, .run => {
@@ -619,22 +622,13 @@ fn buildOutputType(
                         i += 1;
                         const pkg_path = args[i];
 
-                        const new_cur_pkg = try arena.create(Package);
-                        new_cur_pkg.* = .{
-                            .root_src_directory = if (fs.path.dirname(pkg_path)) |dirname|
-                                .{
-                                    .path = dirname,
-                                    .handle = try fs.cwd().openDir(dirname, .{}), // TODO close this fd
-                                }
-                            else
-                                .{
-                                    .path = null,
-                                    .handle = fs.cwd(),
-                                },
-                            .root_src_path = fs.path.basename(pkg_path),
-                            .parent = cur_pkg,
-                        };
-                        try cur_pkg.table.put(gpa, pkg_name, new_cur_pkg);
+                        const new_cur_pkg = try Package.create(
+                            gpa,
+                            fs.path.dirname(pkg_path),
+                            fs.path.basename(pkg_path),
+                        );
+                        new_cur_pkg.parent = cur_pkg;
+                        try cur_pkg.add(gpa, pkg_name, new_cur_pkg);
                         cur_pkg = new_cur_pkg;
                     } else if (mem.eql(u8, arg, "--pkg-end")) {
                         cur_pkg = cur_pkg.parent orelse
@@ -1583,26 +1577,22 @@ fn buildOutputType(
         .yes => |p| p,
     };
 
-    var cleanup_root_dir: ?fs.Dir = null;
-    defer if (cleanup_root_dir) |*dir| dir.close();
-
     const root_pkg: ?*Package = if (root_src_file) |src_path| blk: {
         if (main_pkg_path) |p| {
-            const dir = try fs.cwd().openDir(p, .{});
-            cleanup_root_dir = dir;
-            root_pkg_memory.root_src_directory = .{ .path = p, .handle = dir };
-            root_pkg_memory.root_src_path = try fs.path.relative(arena, p, src_path);
-        } else if (fs.path.dirname(src_path)) |p| {
-            const dir = try fs.cwd().openDir(p, .{});
-            cleanup_root_dir = dir;
-            root_pkg_memory.root_src_directory = .{ .path = p, .handle = dir };
-            root_pkg_memory.root_src_path = fs.path.basename(src_path);
+            const rel_src_path = try fs.path.relative(gpa, p, src_path);
+            defer gpa.free(rel_src_path);
+            break :blk try Package.create(gpa, p, rel_src_path);
         } else {
-            root_pkg_memory.root_src_directory = .{ .path = null, .handle = fs.cwd() };
-            root_pkg_memory.root_src_path = src_path;
+            break :blk try Package.create(gpa, fs.path.dirname(src_path), fs.path.basename(src_path));
         }
-        break :blk &root_pkg_memory;
     } else null;
+    defer if (root_pkg) |p| p.destroy(gpa);
+
+    // Transfer packages added with --pkg-begin/--pkg-end to the root package
+    if (root_pkg) |pkg| {
+        pkg.table = pkg_tree_root.table;
+        pkg_tree_root.table = .{};
+    }
 
     const self_exe_path = try fs.selfExePathAlloc(arena);
     var zig_lib_directory: Compilation.Directory = if (override_lib_dir) |lib_dir|