From 2b592d7e3cf328deb1b8ffa7ea88389d785837ff Mon Sep 17 00:00:00 2001 From: kcbanner Date: Sun, 23 Apr 2023 19:09:12 -0400 Subject: [PATCH 1/2] sema: Rework Decl.value_arena to fix another memory corruption issue This fixes a bug where resolveStructLayout to was promoting from stale value_arena state which was then overwrriten when another ArenaAllocator higher in the call stack saved its state back. This resulted in the memory for struct_obj.optmized_order overlapping existing allocations. My initial fix in c7067ef wasn't sufficient, as it only checked if the struct being resolved had the same owner as the current sema instance. However, it's possible for resolveStructLayout to be called when the sema instance has a different owner, but the struct decl's value_arena is currently in use higher up in the callstack. This change introduces ValueArena, which holds the arena state as well as tracks if an arena has already been promoted from it. This allows callers to use the value_arena storage without needing to be aware of another user of this same storage higher up in the call stack. --- src/Module.zig | 65 ++++++++++++++++++++++++++------- src/Sema.zig | 38 +++++++++---------- test/cases/decl_value_arena.zig | 21 +++++++++++ 3 files changed, 91 insertions(+), 33 deletions(-) create mode 100644 test/cases/decl_value_arena.zig diff --git a/src/Module.zig b/src/Module.zig index 6738322d01..c148ee44e3 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -411,6 +411,43 @@ pub const WipCaptureScope = struct { } }; +const ValueArena = struct { + state: std.heap.ArenaAllocator.State, + state_acquired: ?*std.heap.ArenaAllocator.State = null, + + /// If non-zero, then an ArenaAllocator has been promoted from `state`, + /// and `state_acquired` points to its state field. + ref_count: usize = 0, + + /// Returns an allocator backed by either promoting `state`, or by the existing ArenaAllocator + /// that has already promoted `state`. `out_arena_allocator` provides storage for the initial promotion, + /// and must live until the matching call to release() + pub fn acquire(self: *ValueArena, child_allocator: Allocator, out_arena_allocator: *std.heap.ArenaAllocator) Allocator { + defer self.ref_count += 1; + + if (self.state_acquired) |state_acquired| { + const arena_allocator = @fieldParentPtr( + std.heap.ArenaAllocator, + "state", + state_acquired, + ); + return arena_allocator.allocator(); + } + + out_arena_allocator.* = self.state.promote(child_allocator); + self.state_acquired = &out_arena_allocator.state; + return out_arena_allocator.allocator(); + } + + pub fn release(self: *ValueArena) void { + self.ref_count -= 1; + if (self.ref_count == 0) { + self.state = self.state_acquired.?.*; + self.state_acquired = null; + } + } +}; + pub const Decl = struct { /// Allocated with Module's allocator; outlives the ZIR code. name: [*:0]const u8, @@ -429,7 +466,7 @@ pub const Decl = struct { @"addrspace": std.builtin.AddressSpace, /// The memory for ty, val, align, linksection, and captures. /// If this is `null` then there is no memory management needed. - value_arena: ?*std.heap.ArenaAllocator.State = null, + value_arena: ?*ValueArena = null, /// The direct parent namespace of the Decl. /// Reference to externally owned memory. /// In the case of the Decl corresponding to a file, this is @@ -607,7 +644,7 @@ pub const Decl = struct { variable.deinit(gpa); gpa.destroy(variable); } - if (decl.value_arena) |arena_state| { + if (decl.value_arena) |value_arena| { if (decl.owns_tv) { if (decl.val.castTag(.str_lit)) |str_lit| { mod.string_literal_table.getPtrContext(str_lit.data, .{ @@ -615,7 +652,8 @@ pub const Decl = struct { }).?.* = .none; } } - arena_state.promote(gpa).deinit(); + assert(value_arena.ref_count == 0); + value_arena.state.promote(gpa).deinit(); decl.value_arena = null; decl.has_tv = false; decl.owns_tv = false; @@ -624,9 +662,9 @@ pub const Decl = struct { pub fn finalizeNewArena(decl: *Decl, arena: *std.heap.ArenaAllocator) !void { assert(decl.value_arena == null); - const arena_state = try arena.allocator().create(std.heap.ArenaAllocator.State); - arena_state.* = arena.state; - decl.value_arena = arena_state; + const value_arena = try arena.allocator().create(ValueArena); + value_arena.* = .{ .state = arena.state }; + decl.value_arena = value_arena; } /// This name is relative to the containing namespace of the decl. @@ -4538,14 +4576,15 @@ fn semaDecl(mod: *Module, decl_index: Decl.Index) !bool { var decl_arena = std.heap.ArenaAllocator.init(gpa); const decl_arena_allocator = decl_arena.allocator(); - const decl_arena_state = blk: { + const decl_value_arena = blk: { errdefer decl_arena.deinit(); - const s = try decl_arena_allocator.create(std.heap.ArenaAllocator.State); + const s = try decl_arena_allocator.create(ValueArena); + s.* = .{ .state = undefined }; break :blk s; }; defer { - decl_arena_state.* = decl_arena.state; - decl.value_arena = decl_arena_state; + decl_value_arena.state = decl_arena.state; + decl.value_arena = decl_value_arena; } var analysis_arena = std.heap.ArenaAllocator.init(gpa); @@ -5493,9 +5532,9 @@ pub fn analyzeFnBody(mod: *Module, func: *Fn, arena: Allocator) SemaError!Air { const decl = mod.declPtr(decl_index); // Use the Decl's arena for captured values. - var decl_arena = decl.value_arena.?.promote(gpa); - defer decl.value_arena.?.* = decl_arena.state; - const decl_arena_allocator = decl_arena.allocator(); + var decl_arena: std.heap.ArenaAllocator = undefined; + const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); + defer decl.value_arena.?.release(); var sema: Sema = .{ .mod = mod, diff --git a/src/Sema.zig b/src/Sema.zig index 14c1789858..e836f1610a 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -2856,9 +2856,9 @@ fn zirEnumDecl( const decl_val = try sema.analyzeDeclVal(block, src, new_decl_index); done = true; - var decl_arena = new_decl.value_arena.?.promote(gpa); - defer new_decl.value_arena.?.* = decl_arena.state; - const decl_arena_allocator = decl_arena.allocator(); + var decl_arena: std.heap.ArenaAllocator = undefined; + const decl_arena_allocator = new_decl.value_arena.?.acquire(gpa, &decl_arena); + defer new_decl.value_arena.?.release(); extra_index = try mod.scanNamespace(&enum_obj.namespace, extra_index, decls_len, new_decl); @@ -26999,13 +26999,12 @@ const ComptimePtrMutationKit = struct { fn beginArena(self: *ComptimePtrMutationKit, mod: *Module) Allocator { const decl = mod.declPtr(self.decl_ref_mut.decl_index); - self.decl_arena = decl.value_arena.?.promote(mod.gpa); - return self.decl_arena.allocator(); + return decl.value_arena.?.acquire(mod.gpa, &self.decl_arena); } fn finishArena(self: *ComptimePtrMutationKit, mod: *Module) void { const decl = mod.declPtr(self.decl_ref_mut.decl_index); - decl.value_arena.?.* = self.decl_arena.state; + decl.value_arena.?.release(); self.decl_arena = undefined; } }; @@ -27036,6 +27035,7 @@ fn beginComptimePtrMutation( .elem_ptr => { const elem_ptr = ptr_val.castTag(.elem_ptr).?.data; var parent = try sema.beginComptimePtrMutation(block, src, elem_ptr.array_ptr, elem_ptr.elem_ty); + switch (parent.pointee) { .direct => |val_ptr| switch (parent.ty.zigTypeTag()) { .Array, .Vector => { @@ -30653,10 +30653,9 @@ fn resolveStructLayout(sema: *Sema, ty: Type) CompileError!void { try sema.perm_arena.alloc(u32, struct_obj.fields.count()) else blk: { const decl = sema.mod.declPtr(struct_obj.owner_decl); - var decl_arena = decl.value_arena.?.promote(sema.mod.gpa); - defer decl.value_arena.?.* = decl_arena.state; - const decl_arena_allocator = decl_arena.allocator(); - + var decl_arena: std.heap.ArenaAllocator = undefined; + const decl_arena_allocator = decl.value_arena.?.acquire(sema.mod.gpa, &decl_arena); + defer decl.value_arena.?.release(); break :blk try decl_arena_allocator.alloc(u32, struct_obj.fields.count()); }; @@ -30700,9 +30699,9 @@ fn semaBackingIntType(mod: *Module, struct_obj: *Module.Struct) CompileError!voi const decl_index = struct_obj.owner_decl; const decl = mod.declPtr(decl_index); - var decl_arena = decl.value_arena.?.promote(gpa); - defer decl.value_arena.?.* = decl_arena.state; - const decl_arena_allocator = decl_arena.allocator(); + var decl_arena: std.heap.ArenaAllocator = undefined; + const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); + defer decl.value_arena.?.release(); const zir = struct_obj.namespace.file_scope.zir; const extended = zir.instructions.items(.data)[struct_obj.zir_index].extended; @@ -31394,9 +31393,9 @@ fn semaStructFields(mod: *Module, struct_obj: *Module.Struct) CompileError!void } const decl = mod.declPtr(decl_index); - var decl_arena = decl.value_arena.?.promote(gpa); - defer decl.value_arena.?.* = decl_arena.state; - const decl_arena_allocator = decl_arena.allocator(); + var decl_arena: std.heap.ArenaAllocator = undefined; + const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); + defer decl.value_arena.?.release(); var analysis_arena = std.heap.ArenaAllocator.init(gpa); defer analysis_arena.deinit(); @@ -31734,10 +31733,9 @@ fn semaUnionFields(mod: *Module, union_obj: *Module.Union) CompileError!void { extra_index += body.len; const decl = mod.declPtr(decl_index); - - var decl_arena = decl.value_arena.?.promote(gpa); - defer decl.value_arena.?.* = decl_arena.state; - const decl_arena_allocator = decl_arena.allocator(); + var decl_arena: std.heap.ArenaAllocator = undefined; + const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); + defer decl.value_arena.?.release(); var analysis_arena = std.heap.ArenaAllocator.init(gpa); defer analysis_arena.deinit(); diff --git a/test/cases/decl_value_arena.zig b/test/cases/decl_value_arena.zig new file mode 100644 index 0000000000..02062c4543 --- /dev/null +++ b/test/cases/decl_value_arena.zig @@ -0,0 +1,21 @@ +pub const Protocols: struct { + list: *const fn(*Connection) void = undefined, + handShake: type = struct { + const stepStart: u8 = 0; + }, +} = .{}; + +pub const Connection = struct { + streamBuffer: [0]u8 = undefined, + __lastReceivedPackets: [0]u8 = undefined, + + handShakeState: u8 = Protocols.handShake.stepStart, +}; + +pub fn main() void { + var conn: Connection = undefined; + _ = conn; +} + +// run +// From 15dafd16e65a8cd8ea434f0b4783f08875254cd4 Mon Sep 17 00:00:00 2001 From: kcbanner Date: Thu, 27 Apr 2023 01:11:41 -0400 Subject: [PATCH 2/2] sema: add `prev` to ValueArena to allow freeing previous arenas when new ones are created during re-analysis In semaDecl, it was possible for a new ArenaAllocators state to replace an existing one that hadn't been freed yet. Instead of the ref_count (which was made redundant by adding the allocator parameter to `release`), I now store a pointer to the previous arena, if one exists. This allows a recursive deinit to happen when the last arena created is destroyed. --- src/Module.zig | 44 +++++++++++++++++++++++++------------------- src/Sema.zig | 12 ++++++------ 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/src/Module.zig b/src/Module.zig index c148ee44e3..93e247f7a3 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -415,23 +415,15 @@ const ValueArena = struct { state: std.heap.ArenaAllocator.State, state_acquired: ?*std.heap.ArenaAllocator.State = null, - /// If non-zero, then an ArenaAllocator has been promoted from `state`, - /// and `state_acquired` points to its state field. - ref_count: usize = 0, + /// If this ValueArena replaced an existing one during re-analysis, this is the previous instance + prev: ?*ValueArena = null, /// Returns an allocator backed by either promoting `state`, or by the existing ArenaAllocator /// that has already promoted `state`. `out_arena_allocator` provides storage for the initial promotion, - /// and must live until the matching call to release() + /// and must live until the matching call to release(). pub fn acquire(self: *ValueArena, child_allocator: Allocator, out_arena_allocator: *std.heap.ArenaAllocator) Allocator { - defer self.ref_count += 1; - if (self.state_acquired) |state_acquired| { - const arena_allocator = @fieldParentPtr( - std.heap.ArenaAllocator, - "state", - state_acquired, - ); - return arena_allocator.allocator(); + return @fieldParentPtr(std.heap.ArenaAllocator, "state", state_acquired).allocator(); } out_arena_allocator.* = self.state.promote(child_allocator); @@ -439,13 +431,24 @@ const ValueArena = struct { return out_arena_allocator.allocator(); } - pub fn release(self: *ValueArena) void { - self.ref_count -= 1; - if (self.ref_count == 0) { + /// Releases the allocator acquired by `acquire. `arena_allocator` must match the one passed to `acquire`. + pub fn release(self: *ValueArena, arena_allocator: *std.heap.ArenaAllocator) void { + if (@fieldParentPtr(std.heap.ArenaAllocator, "state", self.state_acquired.?) == arena_allocator) { self.state = self.state_acquired.?.*; self.state_acquired = null; } } + + pub fn deinit(self: ValueArena, child_allocator: Allocator) void { + assert(self.state_acquired == null); + + const prev = self.prev; + self.state.promote(child_allocator).deinit(); + + if (prev) |p| { + p.deinit(child_allocator); + } + } }; pub const Decl = struct { @@ -652,8 +655,7 @@ pub const Decl = struct { }).?.* = .none; } } - assert(value_arena.ref_count == 0); - value_arena.state.promote(gpa).deinit(); + value_arena.deinit(gpa); decl.value_arena = null; decl.has_tv = false; decl.owns_tv = false; @@ -4575,7 +4577,6 @@ fn semaDecl(mod: *Module, decl_index: Decl.Index) !bool { // We need the memory for the Type to go into the arena for the Decl var decl_arena = std.heap.ArenaAllocator.init(gpa); const decl_arena_allocator = decl_arena.allocator(); - const decl_value_arena = blk: { errdefer decl_arena.deinit(); const s = try decl_arena_allocator.create(ValueArena); @@ -4583,6 +4584,11 @@ fn semaDecl(mod: *Module, decl_index: Decl.Index) !bool { break :blk s; }; defer { + if (decl.value_arena) |value_arena| { + assert(value_arena.state_acquired == null); + decl_value_arena.prev = value_arena; + } + decl_value_arena.state = decl_arena.state; decl.value_arena = decl_value_arena; } @@ -5534,7 +5540,7 @@ pub fn analyzeFnBody(mod: *Module, func: *Fn, arena: Allocator) SemaError!Air { // Use the Decl's arena for captured values. var decl_arena: std.heap.ArenaAllocator = undefined; const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); - defer decl.value_arena.?.release(); + defer decl.value_arena.?.release(&decl_arena); var sema: Sema = .{ .mod = mod, diff --git a/src/Sema.zig b/src/Sema.zig index e836f1610a..8b47f1877b 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -2858,7 +2858,7 @@ fn zirEnumDecl( var decl_arena: std.heap.ArenaAllocator = undefined; const decl_arena_allocator = new_decl.value_arena.?.acquire(gpa, &decl_arena); - defer new_decl.value_arena.?.release(); + defer new_decl.value_arena.?.release(&decl_arena); extra_index = try mod.scanNamespace(&enum_obj.namespace, extra_index, decls_len, new_decl); @@ -27004,7 +27004,7 @@ const ComptimePtrMutationKit = struct { fn finishArena(self: *ComptimePtrMutationKit, mod: *Module) void { const decl = mod.declPtr(self.decl_ref_mut.decl_index); - decl.value_arena.?.release(); + decl.value_arena.?.release(&self.decl_arena); self.decl_arena = undefined; } }; @@ -30655,7 +30655,7 @@ fn resolveStructLayout(sema: *Sema, ty: Type) CompileError!void { const decl = sema.mod.declPtr(struct_obj.owner_decl); var decl_arena: std.heap.ArenaAllocator = undefined; const decl_arena_allocator = decl.value_arena.?.acquire(sema.mod.gpa, &decl_arena); - defer decl.value_arena.?.release(); + defer decl.value_arena.?.release(&decl_arena); break :blk try decl_arena_allocator.alloc(u32, struct_obj.fields.count()); }; @@ -30701,7 +30701,7 @@ fn semaBackingIntType(mod: *Module, struct_obj: *Module.Struct) CompileError!voi const decl = mod.declPtr(decl_index); var decl_arena: std.heap.ArenaAllocator = undefined; const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); - defer decl.value_arena.?.release(); + defer decl.value_arena.?.release(&decl_arena); const zir = struct_obj.namespace.file_scope.zir; const extended = zir.instructions.items(.data)[struct_obj.zir_index].extended; @@ -31395,7 +31395,7 @@ fn semaStructFields(mod: *Module, struct_obj: *Module.Struct) CompileError!void const decl = mod.declPtr(decl_index); var decl_arena: std.heap.ArenaAllocator = undefined; const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); - defer decl.value_arena.?.release(); + defer decl.value_arena.?.release(&decl_arena); var analysis_arena = std.heap.ArenaAllocator.init(gpa); defer analysis_arena.deinit(); @@ -31735,7 +31735,7 @@ fn semaUnionFields(mod: *Module, union_obj: *Module.Union) CompileError!void { const decl = mod.declPtr(decl_index); var decl_arena: std.heap.ArenaAllocator = undefined; const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena); - defer decl.value_arena.?.release(); + defer decl.value_arena.?.release(&decl_arena); var analysis_arena = std.heap.ArenaAllocator.init(gpa); defer analysis_arena.deinit();