authorgravatar for kcbanner@gmail.comCasey Banner <kcbanner@gmail.com> 2023-04-23 19:09:12-04:00
committergravatar for kcbanner@gmail.comCasey Banner <kcbanner@gmail.com> 2023-04-27 01:11:57-04:00
log2b592d7e3cf328deb1b8ffa7ea88389d785837ff
tree111892a8c4ef5301207a497382fe4fc6f51a3f4d
parent396bd51c4818752f6309ff10f2d316f41598d5cd

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.

3 files changed, 91 insertions(+), 33 deletions(-)

src/Module.zig+52-13
......@@ -411,6 +411,43 @@ pub const WipCaptureScope = struct {
411411 }
412412};
413413
414const ValueArena = struct {
415 state: std.heap.ArenaAllocator.State,
416 state_acquired: ?*std.heap.ArenaAllocator.State = null,
417
418 /// If non-zero, then an ArenaAllocator has been promoted from `state`,
419 /// and `state_acquired` points to its state field.
420 ref_count: usize = 0,
421
422 /// Returns an allocator backed by either promoting `state`, or by the existing ArenaAllocator
423 /// that has already promoted `state`. `out_arena_allocator` provides storage for the initial promotion,
424 /// and must live until the matching call to release()
425 pub fn acquire(self: *ValueArena, child_allocator: Allocator, out_arena_allocator: *std.heap.ArenaAllocator) Allocator {
426 defer self.ref_count += 1;
427
428 if (self.state_acquired) |state_acquired| {
429 const arena_allocator = @fieldParentPtr(
430 std.heap.ArenaAllocator,
431 "state",
432 state_acquired,
433 );
434 return arena_allocator.allocator();
435 }
436
437 out_arena_allocator.* = self.state.promote(child_allocator);
438 self.state_acquired = &out_arena_allocator.state;
439 return out_arena_allocator.allocator();
440 }
441
442 pub fn release(self: *ValueArena) void {
443 self.ref_count -= 1;
444 if (self.ref_count == 0) {
445 self.state = self.state_acquired.?.*;
446 self.state_acquired = null;
447 }
448 }
449};
450
414451pub const Decl = struct {
415452 /// Allocated with Module's allocator; outlives the ZIR code.
416453 name: [*:0]const u8,
......@@ -429,7 +466,7 @@ pub const Decl = struct {
429466 @"addrspace": std.builtin.AddressSpace,
430467 /// The memory for ty, val, align, linksection, and captures.
431468 /// If this is `null` then there is no memory management needed.
432 value_arena: ?*std.heap.ArenaAllocator.State = null,
469 value_arena: ?*ValueArena = null,
433470 /// The direct parent namespace of the Decl.
434471 /// Reference to externally owned memory.
435472 /// In the case of the Decl corresponding to a file, this is
......@@ -607,7 +644,7 @@ pub const Decl = struct {
607644 variable.deinit(gpa);
608645 gpa.destroy(variable);
609646 }
610 if (decl.value_arena) |arena_state| {
647 if (decl.value_arena) |value_arena| {
611648 if (decl.owns_tv) {
612649 if (decl.val.castTag(.str_lit)) |str_lit| {
613650 mod.string_literal_table.getPtrContext(str_lit.data, .{
......@@ -615,7 +652,8 @@ pub const Decl = struct {
615652 }).?.* = .none;
616653 }
617654 }
618 arena_state.promote(gpa).deinit();
655 assert(value_arena.ref_count == 0);
656 value_arena.state.promote(gpa).deinit();
619657 decl.value_arena = null;
620658 decl.has_tv = false;
621659 decl.owns_tv = false;
......@@ -624,9 +662,9 @@ pub const Decl = struct {
624662
625663 pub fn finalizeNewArena(decl: *Decl, arena: *std.heap.ArenaAllocator) !void {
626664 assert(decl.value_arena == null);
627 const arena_state = try arena.allocator().create(std.heap.ArenaAllocator.State);
628 arena_state.* = arena.state;
629 decl.value_arena = arena_state;
665 const value_arena = try arena.allocator().create(ValueArena);
666 value_arena.* = .{ .state = arena.state };
667 decl.value_arena = value_arena;
630668 }
631669
632670 /// This name is relative to the containing namespace of the decl.
......@@ -4538,14 +4576,15 @@ fn semaDecl(mod: *Module, decl_index: Decl.Index) !bool {
45384576 var decl_arena = std.heap.ArenaAllocator.init(gpa);
45394577 const decl_arena_allocator = decl_arena.allocator();
45404578
4541 const decl_arena_state = blk: {
4579 const decl_value_arena = blk: {
45424580 errdefer decl_arena.deinit();
4543 const s = try decl_arena_allocator.create(std.heap.ArenaAllocator.State);
4581 const s = try decl_arena_allocator.create(ValueArena);
4582 s.* = .{ .state = undefined };
45444583 break :blk s;
45454584 };
45464585 defer {
4547 decl_arena_state.* = decl_arena.state;
4548 decl.value_arena = decl_arena_state;
4586 decl_value_arena.state = decl_arena.state;
4587 decl.value_arena = decl_value_arena;
45494588 }
45504589
45514590 var analysis_arena = std.heap.ArenaAllocator.init(gpa);
......@@ -5493,9 +5532,9 @@ pub fn analyzeFnBody(mod: *Module, func: *Fn, arena: Allocator) SemaError!Air {
54935532 const decl = mod.declPtr(decl_index);
54945533
54955534 // Use the Decl's arena for captured values.
5496 var decl_arena = decl.value_arena.?.promote(gpa);
5497 defer decl.value_arena.?.* = decl_arena.state;
5498 const decl_arena_allocator = decl_arena.allocator();
5535 var decl_arena: std.heap.ArenaAllocator = undefined;
5536 const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena);
5537 defer decl.value_arena.?.release();
54995538
55005539 var sema: Sema = .{
55015540 .mod = mod,
src/Sema.zig+18-20
......@@ -2856,9 +2856,9 @@ fn zirEnumDecl(
28562856 const decl_val = try sema.analyzeDeclVal(block, src, new_decl_index);
28572857 done = true;
28582858
2859 var decl_arena = new_decl.value_arena.?.promote(gpa);
2860 defer new_decl.value_arena.?.* = decl_arena.state;
2861 const decl_arena_allocator = decl_arena.allocator();
2859 var decl_arena: std.heap.ArenaAllocator = undefined;
2860 const decl_arena_allocator = new_decl.value_arena.?.acquire(gpa, &decl_arena);
2861 defer new_decl.value_arena.?.release();
28622862
28632863 extra_index = try mod.scanNamespace(&enum_obj.namespace, extra_index, decls_len, new_decl);
28642864
......@@ -26999,13 +26999,12 @@ const ComptimePtrMutationKit = struct {
2699926999
2700027000 fn beginArena(self: *ComptimePtrMutationKit, mod: *Module) Allocator {
2700127001 const decl = mod.declPtr(self.decl_ref_mut.decl_index);
27002 self.decl_arena = decl.value_arena.?.promote(mod.gpa);
27003 return self.decl_arena.allocator();
27002 return decl.value_arena.?.acquire(mod.gpa, &self.decl_arena);
2700427003 }
2700527004
2700627005 fn finishArena(self: *ComptimePtrMutationKit, mod: *Module) void {
2700727006 const decl = mod.declPtr(self.decl_ref_mut.decl_index);
27008 decl.value_arena.?.* = self.decl_arena.state;
27007 decl.value_arena.?.release();
2700927008 self.decl_arena = undefined;
2701027009 }
2701127010};
......@@ -27036,6 +27035,7 @@ fn beginComptimePtrMutation(
2703627035 .elem_ptr => {
2703727036 const elem_ptr = ptr_val.castTag(.elem_ptr).?.data;
2703827037 var parent = try sema.beginComptimePtrMutation(block, src, elem_ptr.array_ptr, elem_ptr.elem_ty);
27038
2703927039 switch (parent.pointee) {
2704027040 .direct => |val_ptr| switch (parent.ty.zigTypeTag()) {
2704127041 .Array, .Vector => {
......@@ -30653,10 +30653,9 @@ fn resolveStructLayout(sema: *Sema, ty: Type) CompileError!void {
3065330653 try sema.perm_arena.alloc(u32, struct_obj.fields.count())
3065430654 else blk: {
3065530655 const decl = sema.mod.declPtr(struct_obj.owner_decl);
30656 var decl_arena = decl.value_arena.?.promote(sema.mod.gpa);
30657 defer decl.value_arena.?.* = decl_arena.state;
30658 const decl_arena_allocator = decl_arena.allocator();
30659
30656 var decl_arena: std.heap.ArenaAllocator = undefined;
30657 const decl_arena_allocator = decl.value_arena.?.acquire(sema.mod.gpa, &decl_arena);
30658 defer decl.value_arena.?.release();
3066030659 break :blk try decl_arena_allocator.alloc(u32, struct_obj.fields.count());
3066130660 };
3066230661
......@@ -30700,9 +30699,9 @@ fn semaBackingIntType(mod: *Module, struct_obj: *Module.Struct) CompileError!voi
3070030699
3070130700 const decl_index = struct_obj.owner_decl;
3070230701 const decl = mod.declPtr(decl_index);
30703 var decl_arena = decl.value_arena.?.promote(gpa);
30704 defer decl.value_arena.?.* = decl_arena.state;
30705 const decl_arena_allocator = decl_arena.allocator();
30702 var decl_arena: std.heap.ArenaAllocator = undefined;
30703 const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena);
30704 defer decl.value_arena.?.release();
3070630705
3070730706 const zir = struct_obj.namespace.file_scope.zir;
3070830707 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
3139431393 }
3139531394
3139631395 const decl = mod.declPtr(decl_index);
31397 var decl_arena = decl.value_arena.?.promote(gpa);
31398 defer decl.value_arena.?.* = decl_arena.state;
31399 const decl_arena_allocator = decl_arena.allocator();
31396 var decl_arena: std.heap.ArenaAllocator = undefined;
31397 const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena);
31398 defer decl.value_arena.?.release();
3140031399
3140131400 var analysis_arena = std.heap.ArenaAllocator.init(gpa);
3140231401 defer analysis_arena.deinit();
......@@ -31734,10 +31733,9 @@ fn semaUnionFields(mod: *Module, union_obj: *Module.Union) CompileError!void {
3173431733 extra_index += body.len;
3173531734
3173631735 const decl = mod.declPtr(decl_index);
31737
31738 var decl_arena = decl.value_arena.?.promote(gpa);
31739 defer decl.value_arena.?.* = decl_arena.state;
31740 const decl_arena_allocator = decl_arena.allocator();
31736 var decl_arena: std.heap.ArenaAllocator = undefined;
31737 const decl_arena_allocator = decl.value_arena.?.acquire(gpa, &decl_arena);
31738 defer decl.value_arena.?.release();
3174131739
3174231740 var analysis_arena = std.heap.ArenaAllocator.init(gpa);
3174331741 defer analysis_arena.deinit();
test/cases/decl_value_arena.zig created+21
......@@ -0,0 +1,21 @@
1pub const Protocols: struct {
2 list: *const fn(*Connection) void = undefined,
3 handShake: type = struct {
4 const stepStart: u8 = 0;
5 },
6} = .{};
7
8pub const Connection = struct {
9 streamBuffer: [0]u8 = undefined,
10 __lastReceivedPackets: [0]u8 = undefined,
11
12 handShakeState: u8 = Protocols.handShake.stepStart,
13};
14
15pub fn main() void {
16 var conn: Connection = undefined;
17 _ = conn;
18}
19
20// run
21//