Commit 1ab1a96f87

Andrew Kelley <andrew@ziglang.org>
2021-05-12 07:12:36
stage2: improve Decl lifetime management
* Compilation: iteration over the deletion_set only tries to delete the first one, relying on Decl destroy to remove itself from the deletion set. * link: `freeDecl` now has to handle the possibility of freeing a Decl that was never called with `allocateDeclIndexes`. * `deleteDecl` recursively iterates over a Decl's Namespace sub-Decl objects and calls `deleteDecl` on them. - Prevents Decl objects from being destroyed when they are still in `deletion_set`. * Sema: fix cleanup of anonymous Decl objects when an error occurs during semantic analysis. * tests: update test cases for fully qualified names
1 parent d7567c0
Changed files (5)
src/link/C.zig
@@ -79,7 +79,7 @@ pub fn deinit(self: *C) void {
 pub fn allocateDeclIndexes(self: *C, decl: *Module.Decl) !void {}
 
 pub fn freeDecl(self: *C, decl: *Module.Decl) void {
-    self.decl_table.removeAssertDiscard(decl);
+    _ = self.decl_table.swapRemove(decl);
     decl.link.c.code.deinit(self.base.allocator);
     decl.fn_link.c.fwd_decl.deinit(self.base.allocator);
     var it = decl.fn_link.c.typedefs.iterator();
src/Compilation.zig
@@ -1616,14 +1616,12 @@ pub fn update(self: *Compilation) !void {
             // Process the deletion set. We use a while loop here because the
             // deletion set may grow as we call `deleteDecl` within this loop,
             // and more unreferenced Decls are revealed.
-            var entry_i: usize = 0;
-            while (entry_i < module.deletion_set.entries.items.len) : (entry_i += 1) {
-                const decl = module.deletion_set.entries.items[entry_i].key;
+            while (module.deletion_set.entries.items.len != 0) {
+                const decl = module.deletion_set.entries.items[0].key;
                 assert(decl.deletion_flag);
-                assert(decl.dependants.items().len == 0);
+                assert(decl.dependants.count() == 0);
                 try module.deleteDecl(decl, null);
             }
-            module.deletion_set.shrinkRetainingCapacity(0);
         }
     }
 
src/Module.zig
@@ -283,7 +283,9 @@ pub const Decl = struct {
     pub fn destroy(decl: *Decl, module: *Module) void {
         const gpa = module.gpa;
         log.debug("destroy {*} ({s})", .{ decl, decl.name });
-        decl.clearName(gpa);
+        if (decl.deletion_flag) {
+            module.deletion_set.swapRemoveAssertDiscard(decl);
+        }
         if (decl.has_tv) {
             if (decl.getInnerNamespace()) |namespace| {
                 namespace.clearDecls(module);
@@ -292,6 +294,7 @@ pub const Decl = struct {
         }
         decl.dependants.deinit(gpa);
         decl.dependencies.deinit(gpa);
+        decl.clearName(gpa);
         if (module.emit_h != null) {
             const decl_plus_emit_h = @fieldParentPtr(DeclPlusEmitH, "decl", decl);
             decl_plus_emit_h.emit_h.fwd_decl.deinit(gpa);
@@ -546,28 +549,6 @@ pub const Decl = struct {
     fn removeDependency(decl: *Decl, other: *Decl) void {
         decl.dependencies.removeAssertDiscard(other);
     }
-
-    fn hasLinkAllocation(decl: Decl) bool {
-        return switch (decl.analysis) {
-            .unreferenced,
-            .in_progress,
-            .dependency_failure,
-            .file_failure,
-            .sema_failure,
-            .sema_failure_retryable,
-            .codegen_failure,
-            .codegen_failure_retryable,
-            => false,
-
-            .complete,
-            .outdated,
-            => {
-                if (!decl.owns_tv)
-                    return false;
-                return decl.ty.hasCodeGenBits();
-            },
-        };
-    }
 };
 
 /// This state is attached to every Decl when Module emit_h is non-null.
@@ -929,6 +910,32 @@ pub const Scope = struct {
             anon_decls.deinit(gpa);
         }
 
+        pub fn deleteAllDecls(
+            ns: *Namespace,
+            mod: *Module,
+            outdated_decls: ?*std.AutoArrayHashMap(*Decl, void),
+        ) !void {
+            const gpa = mod.gpa;
+
+            log.debug("deleteAllDecls {*}", .{ns});
+
+            while (ns.decls.count() != 0) {
+                const last_entry = ns.decls.entries.items[ns.decls.entries.items.len - 1];
+                const child_decl = last_entry.value;
+                try mod.deleteDecl(child_decl, outdated_decls);
+            }
+            ns.decls.deinit(gpa);
+            ns.decls = .{};
+
+            while (ns.anon_decls.count() != 0) {
+                const last_entry = ns.anon_decls.entries.items[ns.anon_decls.entries.items.len - 1];
+                const child_decl = last_entry.key;
+                try mod.deleteDecl(child_decl, outdated_decls);
+            }
+            ns.anon_decls.deinit(gpa);
+            ns.anon_decls = .{};
+        }
+
         pub fn removeDecl(ns: *Namespace, child: *Decl) void {
             if (child.zir_decl_index == 0) {
                 _ = ns.anon_decls.swapRemove(child);
@@ -2646,7 +2653,7 @@ fn updateZirRefs(gpa: *Allocator, file: *Scope.File, old_zir: Zir) !void {
             }
         }
 
-        if (!decl.has_tv) continue;
+        if (!decl.owns_tv) continue;
 
         if (decl.getStruct()) |struct_obj| {
             struct_obj.zir_index = inst_map.get(struct_obj.zir_index) orelse {
@@ -3401,17 +3408,19 @@ pub fn deleteDecl(
     mod: *Module,
     decl: *Decl,
     outdated_decls: ?*std.AutoArrayHashMap(*Decl, void),
-) !void {
+) Allocator.Error!void {
     const tracy = trace(@src());
     defer tracy.end();
 
     log.debug("deleting {*} ({s})", .{ decl, decl.name });
 
+    const gpa = mod.gpa;
+    try mod.deletion_set.ensureUnusedCapacity(gpa, decl.dependencies.count());
+
     if (outdated_decls) |map| {
         _ = map.swapRemove(decl);
         try map.ensureUnusedCapacity(decl.dependants.count());
     }
-    try mod.deletion_set.ensureUnusedCapacity(mod.gpa, decl.dependencies.count());
 
     // Remove from the namespace it resides in.
     decl.namespace.removeDecl(decl);
@@ -3443,18 +3452,23 @@ pub fn deleteDecl(
         }
     }
     if (mod.failed_decls.swapRemove(decl)) |entry| {
-        entry.value.destroy(mod.gpa);
+        entry.value.destroy(gpa);
     }
     if (mod.emit_h) |emit_h| {
         if (emit_h.failed_decls.swapRemove(decl)) |entry| {
-            entry.value.destroy(mod.gpa);
+            entry.value.destroy(gpa);
         }
         emit_h.decl_table.removeAssertDiscard(decl);
     }
     _ = mod.compile_log_decls.swapRemove(decl);
     mod.deleteDeclExports(decl);
-    if (decl.hasLinkAllocation()) {
-        mod.comp.bin_file.freeDecl(decl);
+    mod.comp.bin_file.freeDecl(decl);
+
+    if (decl.has_tv) {
+        if (decl.getInnerNamespace()) |namespace| {
+            try namespace.deleteAllDecls(mod, outdated_decls);
+        }
+        decl.clearValues(gpa);
     }
 
     decl.destroy(mod);
@@ -3827,6 +3841,12 @@ pub fn constIntBig(mod: *Module, arena: *Allocator, src: LazySrcLoc, ty: Type, b
     }
 }
 
+pub fn deleteAnonDecl(mod: *Module, scope: *Scope, decl: *Decl) void {
+    const scope_decl = scope.ownerDecl().?;
+    scope_decl.namespace.anon_decls.swapRemoveAssertDiscard(decl);
+    decl.destroy(mod);
+}
+
 /// Takes ownership of `name` even if it returns an error.
 pub fn createAnonymousDeclNamed(
     mod: *Module,
@@ -4814,6 +4834,8 @@ pub fn processOutdatedAndDeletedDecls(mod: *Module) !void {
         for (file.outdated_decls.items) |decl| {
             outdated_decls.putAssumeCapacity(decl, {});
         }
+        file.outdated_decls.clearRetainingCapacity();
+
         // Handle explicitly deleted decls from the source code. This is one of two
         // places that Decl deletions happen. The other is in `Compilation`, after
         // `performAllTheWork`, where we iterate over `Module.deletion_set` and
@@ -4824,12 +4846,9 @@ pub fn processOutdatedAndDeletedDecls(mod: *Module) !void {
         // deletion set at this time.
         for (file.deleted_decls.items) |decl| {
             log.debug("deleted from source: {*} ({s})", .{ decl, decl.name });
-            if (decl.deletion_flag) {
-                log.debug("{*} ({s}) redundantly in deletion set; removing", .{ decl, decl.name });
-                mod.deletion_set.removeAssertDiscard(decl);
-            }
             try mod.deleteDecl(decl, &outdated_decls);
         }
+        file.deleted_decls.clearRetainingCapacity();
     }
     // Finally we can queue up re-analysis tasks after we have processed
     // the deleted decls.
src/Sema.zig
@@ -725,6 +725,7 @@ fn zirStructDecl(
         .ty = Type.initTag(.type),
         .val = struct_val,
     }, type_name);
+    errdefer sema.mod.deleteAnonDecl(&block.base, new_decl);
     struct_obj.* = .{
         .owner_decl = new_decl,
         .fields = .{},
@@ -842,6 +843,8 @@ fn zirEnumDecl(
         .ty = Type.initTag(.type),
         .val = enum_val,
     }, type_name);
+    errdefer sema.mod.deleteAnonDecl(&block.base, new_decl);
+
     enum_obj.* = .{
         .owner_decl = new_decl,
         .tag_ty = tag_ty,
@@ -1005,6 +1008,7 @@ fn zirUnionDecl(
         .ty = Type.initTag(.type),
         .val = union_val,
     }, type_name);
+    errdefer sema.mod.deleteAnonDecl(&block.base, new_decl);
     union_obj.* = .{
         .owner_decl = new_decl,
         .tag_ty = Type.initTag(.@"null"),
@@ -1071,6 +1075,7 @@ fn zirErrorSetDecl(
         .ty = Type.initTag(.type),
         .val = error_set_val,
     }, type_name);
+    errdefer sema.mod.deleteAnonDecl(&block.base, new_decl);
     const names = try new_decl_arena.allocator.alloc([]const u8, fields.len);
     for (fields) |str_index, i| {
         names[i] = try new_decl_arena.allocator.dupe(u8, sema.code.nullTerminatedString(str_index));
@@ -1575,6 +1580,7 @@ fn zirStr(sema: *Sema, block: *Scope.Block, inst: Zir.Inst.Index) InnerError!*In
         .ty = decl_ty,
         .val = decl_val,
     });
+    errdefer sema.mod.deleteAnonDecl(&block.base, new_decl);
     try new_decl.finalizeNewArena(&new_decl_arena);
     return sema.analyzeDeclRef(block, .unneeded, new_decl);
 }
test/stage2/cbe.zig
@@ -708,7 +708,7 @@ pub fn addCases(ctx: *TestContext) !void {
             \\    const b = @intToEnum(E, 3);
             \\}
         , &.{
-            ":3:15: error: enum 'E' has no tag with value 3",
+            ":3:15: error: enum 'test_case.E' has no tag with value 3",
             ":1:11: note: enum declared here",
         });
 
@@ -724,7 +724,7 @@ pub fn addCases(ctx: *TestContext) !void {
         , &.{
             ":4:5: error: switch must handle all possibilities",
             ":4:5: note: unhandled enumeration value: 'b'",
-            ":1:11: note: enum 'E' declared here",
+            ":1:11: note: enum 'test_case.E' declared here",
         });
 
         case.addError(
@@ -779,7 +779,7 @@ pub fn addCases(ctx: *TestContext) !void {
             \\    var x = E.d;
             \\}
         , &.{
-            ":3:14: error: enum 'E' has no member named 'd'",
+            ":3:14: error: enum 'test_case.E' has no member named 'd'",
             ":1:11: note: enum declared here",
         });
 
@@ -789,7 +789,7 @@ pub fn addCases(ctx: *TestContext) !void {
             \\    var x: E = .d;
             \\}
         , &.{
-            ":3:17: error: enum 'E' has no field named 'd'",
+            ":3:17: error: enum 'test_case.E' has no field named 'd'",
             ":1:11: note: enum declared here",
         });
     }