From 1ab1a96f87279375e656bba35280a85b62973255 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 11 May 2021 22:12:36 -0700 Subject: [PATCH] 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 --- src/Compilation.zig | 8 ++--- src/Module.zig | 87 +++++++++++++++++++++++++++------------------ src/Sema.zig | 6 ++++ src/link/C.zig | 2 +- test/stage2/cbe.zig | 8 ++--- 5 files changed, 67 insertions(+), 44 deletions(-) diff --git a/src/Compilation.zig b/src/Compilation.zig index 90e229c4a9..e805b86ee0 100644 --- a/src/Compilation.zig +++ b/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); } } diff --git a/src/Module.zig b/src/Module.zig index dcadb8e23e..3a877ad9e4 100644 --- a/src/Module.zig +++ b/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. diff --git a/src/Sema.zig b/src/Sema.zig index cf4895aea5..abb4f9e56a 100644 --- a/src/Sema.zig +++ b/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); } diff --git a/src/link/C.zig b/src/link/C.zig index 79afe90380..da61dda488 100644 --- a/src/link/C.zig +++ b/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(); diff --git a/test/stage2/cbe.zig b/test/stage2/cbe.zig index 60ab51ab1a..21eed62292 100644 --- a/test/stage2/cbe.zig +++ b/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", }); }