From 306e085d1bca05f4572b341c71f873669fc69d9c Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 01:16:52 +0000 Subject: [PATCH 01/12] Add leak detection to Ona heap allocator --- source/coral/arena.zig | 2 +- source/coral/io.zig | 14 ++- source/coral/list.zig | 4 +- source/coral/slab.zig | 2 +- source/coral/table.zig | 2 +- source/ona/heap.zig | 180 +++++++++++++++++++++++++++------ source/ona/kym/Environment.zig | 2 +- source/ona/ona.zig | 2 + 8 files changed, 163 insertions(+), 45 deletions(-) diff --git a/source/coral/arena.zig b/source/coral/arena.zig index a9719bf..a6509ad 100755 --- a/source/coral/arena.zig +++ b/source/coral/arena.zig @@ -49,7 +49,7 @@ pub const Stacking = struct { } fn allocate_page(self: *Stacking, page_size: usize) io.AllocationError!*Page { - var buffer = try io.allocate_many(u8, page_size, self.base_allocator); + var buffer = try io.allocate_many(self.base_allocator, page_size, u8); errdefer io.deallocate(self.base_allocator, buffer); diff --git a/source/coral/io.zig b/source/coral/io.zig index 90cbb23..e8a70e2 100755 --- a/source/coral/io.zig +++ b/source/coral/io.zig @@ -176,7 +176,7 @@ pub const GrowingBuffer = struct { pub const Writer = Generator(?usize, []const u8); -pub fn allocate_many(comptime Type: type, amount: usize, allocator: Allocator) AllocationError![]Type { +pub fn allocate_many(allocator: Allocator, amount: usize, comptime Type: type) AllocationError![]Type { if (@sizeOf(Type) == 0) { @compileError("Cannot allocate memory for 0-byte type " ++ @typeName(Type)); } @@ -232,14 +232,12 @@ pub fn copy(target: []u8, source: []const u8) void { } pub fn deallocate(allocator: Allocator, allocation: anytype) void { - const Allocation = @TypeOf(allocation); - - switch (@typeInfo(Allocation)) { - .Pointer => |allocation_pointer| { + switch (@typeInfo(@TypeOf(allocation))) { + .Pointer => |pointer| { _ = allocator.invoke(.{ - .allocation = switch (allocation_pointer.size) { - .One => @ptrCast([*]u8, allocation)[0 .. @sizeOf(Allocation)], - .Slice => @ptrCast([*]u8, allocation.ptr)[0 .. (@sizeOf(Allocation) * allocation.len)], + .allocation = switch (pointer.size) { + .One => @ptrCast([*]u8, allocation)[0 .. @sizeOf(pointer.child)], + .Slice => @ptrCast([*]u8, allocation.ptr)[0 .. (@sizeOf(pointer.child) * allocation.len)], .Many, .C => @compileError("length of allocation must be known to deallocate"), }, diff --git a/source/coral/list.zig b/source/coral/list.zig index 9a70511..b21e081 100755 --- a/source/coral/list.zig +++ b/source/coral/list.zig @@ -87,7 +87,7 @@ pub fn Stack(comptime Value: type) type { /// pub fn grow(self: *Self, allocator: io.Allocator, growth_amount: usize) io.AllocationError!void { const grown_capacity = self.capacity + growth_amount; - const values = (try io.allocate_many(Value, grown_capacity, allocator))[0 .. self.values.len]; + const values = (try io.allocate_many(allocator, grown_capacity, Value))[0 .. self.values.len]; errdefer io.deallocate(allocator, values); @@ -96,7 +96,7 @@ pub fn Stack(comptime Value: type) type { values[index] = self.values[index]; } - io.deallocate(allocator, self.values); + io.deallocate(allocator, self.values.ptr[0 .. self.capacity]); } self.values = values; diff --git a/source/coral/slab.zig b/source/coral/slab.zig index c80f969..6d1957e 100644 --- a/source/coral/slab.zig +++ b/source/coral/slab.zig @@ -95,7 +95,7 @@ pub fn Map(comptime index_int: std.builtin.Type.Int, comptime Value: type) type /// pub fn grow(self: *Self, allocator: io.Allocator, growth_amount: usize) io.AllocationError!void { const grown_capacity = self.table.len + growth_amount; - const entries = try io.allocate_many(Entry, grown_capacity, allocator); + const entries = try io.allocate_many(allocator, grown_capacity, Entry); errdefer io.deallocate(allocator, entries); diff --git a/source/coral/table.zig b/source/coral/table.zig index 82fe1aa..c127e82 100755 --- a/source/coral/table.zig +++ b/source/coral/table.zig @@ -228,7 +228,7 @@ pub fn Hashed(comptime Key: type, comptime Value: type, comptime keyer: Keyer(Ke pub fn rehash(self: *Self, allocator: io.Allocator, requested_range: usize) io.AllocationError!void { const old_table = self.table; - self.table = try io.allocate_many(?Entry, math.max(requested_range, self.count), allocator); + self.table = try io.allocate_many(allocator, math.max(requested_range, self.count), ?Entry); errdefer { io.deallocate(allocator, self.table); diff --git a/source/ona/heap.zig b/source/ona/heap.zig index 25923f0..2b250f4 100644 --- a/source/ona/heap.zig +++ b/source/ona/heap.zig @@ -2,53 +2,171 @@ const coral = @import("coral"); const ext = @import("./ext.zig"); +const std = @import("std"); + +/// +/// +/// +const stack_trace_frame_size = if (std.debug.sys_can_stack_trace) 4 else 0; + +/// +/// +/// const Context = struct { - live_allocations: usize, + allocation_info_head: ?*AllocationInfo = null, - const Self = @This(); + /// + /// + /// + const AllocationInfo = struct { + stack_frames: [stack_trace_frame_size]usize, + stack_trace: std.builtin.StackTrace, + next_info: ?*AllocationInfo, + size: usize, + }; - const empty_allocation = [0]u8{}; + /// + /// + /// + fn allocate(self: *Context, size: usize) ?[]u8 { + const allocation_info_size = @sizeOf(AllocationInfo); + const total_allocation_size = allocation_info_size + size; + const allocation = ext.SDL_malloc(total_allocation_size) orelse return null; + const allocation_info = @ptrCast(*AllocationInfo, @alignCast(@alignOf(AllocationInfo), allocation)); - fn reallocate(self: *Self, options: coral.io.AllocationOptions) ?[]u8 { - if (options.size == 0) { - if (options.allocation) |allocation| { - if (allocation.ptr != &empty_allocation) { - ext.SDL_free(allocation.ptr); + allocation_info.* = .{ + .size = size, + .next_info = self.allocation_info_head, + .stack_frames = [_]usize{0} ** stack_trace_frame_size, + + .stack_trace = .{ + .instruction_addresses = &allocation_info.stack_frames, + .index = 0, + }, + }; + + std.debug.captureStackTrace(null, &allocation_info.stack_trace); + + self.allocation_info_head = allocation_info; + + return @ptrCast([*]u8, allocation)[allocation_info_size .. total_allocation_size]; + } + + /// + /// + /// + fn deallocate(self: *Context, allocation: []u8) void { + const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - @sizeOf(AllocationInfo)); + + if (target_allocation_info.size != allocation.len) { + @panic("incorrect allocation length"); + } + + if (self.allocation_info_head) |allocation_info_head| { + if (target_allocation_info == allocation_info_head) { + self.allocation_info_head = allocation_info_head.next_info; + + ext.SDL_free(target_allocation_info); + + return; + } + + var previous_allocation_info = allocation_info_head; + var current_allocation_info = allocation_info_head.next_info; + + while (current_allocation_info) |allocation_info| { + if (allocation_info == target_allocation_info) { + previous_allocation_info.next_info = allocation_info.next_info; + + ext.SDL_free(target_allocation_info); + + return; } - self.live_allocations -= 1; - - return null; - } - - self.live_allocations += 1; - - return &empty_allocation; - } - - if (options.allocation) |allocation| { - if (ext.SDL_realloc(allocation.ptr, options.size)) |reallocation| { - self.live_allocations += 1; - - return @ptrCast([*]u8, reallocation)[0 .. options.size]; + previous_allocation_info = allocation_info; + current_allocation_info = allocation_info.next_info; } } - if (ext.SDL_malloc(options.size)) |allocation| { - self.live_allocations += 1; + @panic("double-free detected"); + } - return @ptrCast([*]u8, allocation)[0 .. options.size]; + /// + /// + /// + fn reallocate(self: *Context, allocation: []u8, size: usize) ?[]u8 { + const allocation_info_size = @sizeOf(AllocationInfo); + const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - allocation_info_size); + + if (target_allocation_info.size != allocation.len) { + @panic("incorrect allocation length"); } - return null; + if (self.allocation_info_head) |allocation_info_head| { + if (target_allocation_info == allocation_info_head) { + self.allocation_info_head = allocation_info_head.next_info; + + return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { + return null; + })[allocation_info_size .. allocation_info_size + size]; + } + + var previous_allocation_info = allocation_info_head; + var current_allocation_info = allocation_info_head.next_info; + + while (current_allocation_info) |allocation_info| { + if (allocation_info == target_allocation_info) { + previous_allocation_info.next_info = allocation_info.next_info; + + return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { + return null; + })[allocation_info_size .. allocation_info_size + size]; + } + + previous_allocation_info = allocation_info; + current_allocation_info = allocation_info.next_info; + } + } + + @panic("use-after-free detected"); } }; -var context = Context{ - .live_allocations = 0, -}; +/// +/// +/// +var context = Context{}; /// /// Heap allocator. /// -pub const allocator = coral.io.Allocator.bind(Context, &context, Context.reallocate); +pub const allocator = coral.io.Allocator.bind(Context, &context, struct { + fn reallocate(self: *Context, options: coral.io.AllocationOptions) ?[]u8 { + if (options.size == 0) { + if (options.allocation) |allocation| { + self.deallocate(allocation); + + return null; + } + + return self.allocate(0); + } + + if (options.allocation) |allocation| { + return self.reallocate(allocation, options.size); + } + + return self.allocate(options.size); + } +}.reallocate); + +/// +/// +/// +pub fn trace_allocations() void { + var current_allocation_info = context.allocation_info_head; + + while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info) { + std.debug.dumpStackTrace(allocation_info.stack_trace); + } +} diff --git a/source/ona/kym/Environment.zig b/source/ona/kym/Environment.zig index b5fcc8c..2ea0782 100644 --- a/source/ona/kym/Environment.zig +++ b/source/ona/kym/Environment.zig @@ -361,7 +361,7 @@ pub fn new_array(self: *Self) coral.io.AllocationError!types.Val { } pub fn new_object(self: *Self, userdata: []const u8, info: ObjectInfo) coral.io.AllocationError!types.Val { - const allocation = try coral.io.allocate_many(u8, userdata.len, self.allocator); + const allocation = try coral.io.allocate_many(self.allocator, userdata.len, u8); errdefer coral.io.deallocate(self.allocator, allocation); diff --git a/source/ona/ona.zig b/source/ona/ona.zig index 13e36c2..81d14ea 100755 --- a/source/ona/ona.zig +++ b/source/ona/ona.zig @@ -135,4 +135,6 @@ pub fn run_app(base_file_system: file.System) void { ext.SDL_RenderPresent(renderer); } } + + heap.trace_allocations(); } From d4fd1bfb4355993611c71e5047b10fc04cc9a15a Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 01:34:21 +0000 Subject: [PATCH 02/12] Fix incorrect buffer slicing in stack list deinit --- source/coral/list.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/coral/list.zig b/source/coral/list.zig index b21e081..77cbb67 100755 --- a/source/coral/list.zig +++ b/source/coral/list.zig @@ -52,7 +52,7 @@ pub fn Stack(comptime Value: type) type { return; } - io.deallocate(allocator, self.values); + io.deallocate(allocator, self.values.ptr[0 .. self.capacity]); self.values = &.{}; self.capacity = 0; @@ -132,7 +132,7 @@ pub fn Stack(comptime Value: type) type { pub fn push_all(self: *Self, allocator: io.Allocator, values: []const Value) io.AllocationError!void { const new_length = self.values.len + values.len; - if (new_length >= self.capacity) { + if (new_length > self.capacity) { try self.grow(allocator, values.len + values.len); } From d63cfc23d69121ea4f2ed9936cf5c89937e9b7c5 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 01:55:05 +0000 Subject: [PATCH 03/12] Fix various memory leaks caught by new tracer --- source/ona/heap.zig | 5 +++-- source/ona/kym/Environment.zig | 9 +++++++++ source/ona/ona.zig | 14 +++++++------- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/source/ona/heap.zig b/source/ona/heap.zig index 2b250f4..2c6cecf 100644 --- a/source/ona/heap.zig +++ b/source/ona/heap.zig @@ -7,7 +7,7 @@ const std = @import("std"); /// /// /// -const stack_trace_frame_size = if (std.debug.sys_can_stack_trace) 4 else 0; +const stack_trace_frame_size = if (std.debug.sys_can_stack_trace) 16 else 0; /// /// @@ -166,7 +166,8 @@ pub const allocator = coral.io.Allocator.bind(Context, &context, struct { pub fn trace_allocations() void { var current_allocation_info = context.allocation_info_head; - while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info) { + while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info.next_info) { + std.debug.print("{d} byte leak detected", .{allocation_info.size}); std.debug.dumpStackTrace(allocation_info.stack_trace); } } diff --git a/source/ona/kym/Environment.zig b/source/ona/kym/Environment.zig index 2ea0782..41e9920 100644 --- a/source/ona/kym/Environment.zig +++ b/source/ona/kym/Environment.zig @@ -185,8 +185,17 @@ pub fn check(self: *Self, condition: bool, failure_message: []const u8) !void { } pub fn deinit(self: *Self) void { + var global_data = self.heap.fetch(self.global_object); + + if (global_data.release(self)) { + self.heap.remove(self.global_object); + } else { + self.heap.assign(self.global_object, global_data); + } + self.values.deinit(self.allocator); self.calls.deinit(self.allocator); + self.heap.deinit(self.allocator); } pub fn discard(self: *Self, val: types.Val) void { diff --git a/source/ona/ona.zig b/source/ona/ona.zig index 81d14ea..72ecb56 100755 --- a/source/ona/ona.zig +++ b/source/ona/ona.zig @@ -55,7 +55,9 @@ const AppManifest = struct { } }; -pub fn run_app(base_file_system: file.System) void { +pub fn run_app(_: file.System) void { + defer heap.trace_allocations(); + const Logger = struct { const Self = @This(); @@ -74,12 +76,12 @@ pub fn run_app(base_file_system: file.System) void { defer script_environment.deinit(); - const app_file_name = "app.ona"; + // const app_file_name = "app.ona"; var app_manifest = AppManifest{}; - app_manifest.load_script(&script_environment, base_file_system, app_file_name) catch { - return ext.SDL_LogError(ext.SDL_LOG_CATEGORY_APPLICATION, "failed to load %s\n", app_file_name); - }; + // app_manifest.load_script(&script_environment, base_file_system, app_file_name) catch { + // return ext.SDL_LogError(ext.SDL_LOG_CATEGORY_APPLICATION, "failed to load %s\n", app_file_name); + // }; if (ext.SDL_Init(ext.SDL_INIT_EVERYTHING) != 0) { return ext.SDL_LogError(ext.SDL_LOG_CATEGORY_APPLICATION, "%s\n", ext.SDL_GetError()); @@ -135,6 +137,4 @@ pub fn run_app(base_file_system: file.System) void { ext.SDL_RenderPresent(renderer); } } - - heap.trace_allocations(); } From 2ec39484dcb51662a4c2da257bb674ddb45c6710 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 02:00:31 +0000 Subject: [PATCH 04/12] Add more detail to memory leak traces --- source/ona/heap.zig | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/source/ona/heap.zig b/source/ona/heap.zig index 2c6cecf..755b0b0 100644 --- a/source/ona/heap.zig +++ b/source/ona/heap.zig @@ -9,22 +9,22 @@ const std = @import("std"); /// const stack_trace_frame_size = if (std.debug.sys_can_stack_trace) 16 else 0; +/// +/// +/// +const AllocationInfo = struct { + stack_frames: [stack_trace_frame_size]usize, + stack_trace: std.builtin.StackTrace, + next_info: ?*AllocationInfo, + size: usize, +}; + /// /// /// const Context = struct { allocation_info_head: ?*AllocationInfo = null, - /// - /// - /// - const AllocationInfo = struct { - stack_frames: [stack_trace_frame_size]usize, - stack_trace: std.builtin.StackTrace, - next_info: ?*AllocationInfo, - size: usize, - }; - /// /// /// @@ -167,7 +167,10 @@ pub fn trace_allocations() void { var current_allocation_info = context.allocation_info_head; while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info.next_info) { - std.debug.print("{d} byte leak detected", .{allocation_info.size}); - std.debug.dumpStackTrace(allocation_info.stack_trace); + std.debug.print("{d} byte leak at 0x{x} detected: {}", .{ + allocation_info.size, + @ptrToInt(allocation_info) + @sizeOf(AllocationInfo), + allocation_info.stack_trace + }); } } From 08785b5b5473b85dadbff36819448aac0e6d8c97 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 02:23:07 +0000 Subject: [PATCH 05/12] Improve clarity of stack trace allocation origin --- source/coral/io.zig | 16 ++++++++----- source/ona/heap.zig | 55 ++++++++++++++++++++++++++++----------------- source/ona/ona.zig | 12 +++++----- 3 files changed, 50 insertions(+), 33 deletions(-) diff --git a/source/coral/io.zig b/source/coral/io.zig index e8a70e2..ad5e1ad 100755 --- a/source/coral/io.zig +++ b/source/coral/io.zig @@ -7,6 +7,7 @@ pub const AllocationError = error { }; pub const AllocationOptions = struct { + return_address: usize, allocation: ?[]u8 = null, size: usize, }; @@ -181,9 +182,10 @@ pub fn allocate_many(allocator: Allocator, amount: usize, comptime Type: type) A @compileError("Cannot allocate memory for 0-byte type " ++ @typeName(Type)); } - return @ptrCast([*]Type, @alignCast(@alignOf(Type), allocator.invoke(.{.size = @sizeOf(Type) * amount}) orelse { - return error.OutOfMemory; - }))[0 .. amount]; + return @ptrCast([*]Type, @alignCast(@alignOf(Type), allocator.invoke(.{ + .size = @sizeOf(Type) * amount, + .return_address = @returnAddress(), + }) orelse return error.OutOfMemory))[0 .. amount]; } pub fn allocate_one(allocator: Allocator, value: anytype) AllocationError!*@TypeOf(value) { @@ -193,9 +195,10 @@ pub fn allocate_one(allocator: Allocator, value: anytype) AllocationError!*@Type @compileError("Cannot allocate memory for 0-byte type " ++ @typeName(Type)); } - const allocation = @ptrCast(*Type, @alignCast(@alignOf(Type), allocator.invoke(.{.size = @sizeOf(Type)}) orelse { - return error.OutOfMemory; - })); + const allocation = @ptrCast(*Type, @alignCast(@alignOf(Type), allocator.invoke(.{ + .size = @sizeOf(Type), + .return_address = @returnAddress(), + }) orelse return error.OutOfMemory)); allocation.* = value; @@ -241,6 +244,7 @@ pub fn deallocate(allocator: Allocator, allocation: anytype) void { .Many, .C => @compileError("length of allocation must be known to deallocate"), }, + .return_address = @returnAddress(), .size = 0, }); }, diff --git a/source/ona/heap.zig b/source/ona/heap.zig index 755b0b0..9c9a5eb 100644 --- a/source/ona/heap.zig +++ b/source/ona/heap.zig @@ -4,17 +4,11 @@ const ext = @import("./ext.zig"); const std = @import("std"); -/// -/// -/// -const stack_trace_frame_size = if (std.debug.sys_can_stack_trace) 16 else 0; - /// /// /// const AllocationInfo = struct { - stack_frames: [stack_trace_frame_size]usize, - stack_trace: std.builtin.StackTrace, + trace: std.debug.Trace, next_info: ?*AllocationInfo, size: usize, }; @@ -26,9 +20,16 @@ const Context = struct { allocation_info_head: ?*AllocationInfo = null, /// + /// Attempts to allocate a buffer of `size` length from `self`, with `return_address` as the location of the + /// allocation request origin. /// + /// A reference to the allocated buffer is returned via a slice if the allocation was successful, otherwise `null` + /// is returned. /// - fn allocate(self: *Context, size: usize) ?[]u8 { + /// *Note* the returned buffer must be deallocated with [deallocate] before program exit or it will cause a memory + /// leak. + /// + fn allocate(self: *Context, size: usize, return_address: usize) ?[]u8 { const allocation_info_size = @sizeOf(AllocationInfo); const total_allocation_size = allocation_info_size + size; const allocation = ext.SDL_malloc(total_allocation_size) orelse return null; @@ -37,15 +38,10 @@ const Context = struct { allocation_info.* = .{ .size = size, .next_info = self.allocation_info_head, - .stack_frames = [_]usize{0} ** stack_trace_frame_size, - - .stack_trace = .{ - .instruction_addresses = &allocation_info.stack_frames, - .index = 0, - }, + .trace = .{}, }; - std.debug.captureStackTrace(null, &allocation_info.stack_trace); + allocation_info.trace.addAddr(return_address, ""); self.allocation_info_head = allocation_info; @@ -53,7 +49,10 @@ const Context = struct { } /// + /// Deallocates a the allocation buffer referenced by `allocation`. /// + /// *Note* the pointer and length of `allocation` must match valid values known to `allocator` otherwise safety- + /// checked behavior will occur. /// fn deallocate(self: *Context, allocation: []u8) void { const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - @sizeOf(AllocationInfo)); @@ -92,7 +91,19 @@ const Context = struct { } /// + /// Attempts to reallocate the buffer referenced by `allocation` to be `size` length from `self`. /// + /// A reference to the reallocated buffer is returned via a slice if the reallocation was successful, otherwise + /// `null` is returned. + /// + /// *Note* the returned buffer must be deallocated with [deallocate] before program exit or it will cause a memory + /// leak. + /// + /// *Note* the pointer and length of `allocation` must match valid values known to `allocator` otherwise safety- + /// checked behavior will occur. + /// + /// *Note* the allocation referenced by `allocation` should be considered invalid once the function returns, + /// discarding it in favor of the return value. /// fn reallocate(self: *Context, allocation: []u8, size: usize) ?[]u8 { const allocation_info_size = @sizeOf(AllocationInfo); @@ -133,7 +144,7 @@ const Context = struct { }; /// -/// +/// Heap context. /// var context = Context{}; @@ -149,28 +160,30 @@ pub const allocator = coral.io.Allocator.bind(Context, &context, struct { return null; } - return self.allocate(0); + return self.allocate(0, options.return_address); } if (options.allocation) |allocation| { return self.reallocate(allocation, options.size); } - return self.allocate(options.size); + return self.allocate(options.size, options.return_address); } }.reallocate); /// +/// Checks for any allocations belonging to the process heap allocated through the [allocator] interface that are still +/// alive and reports the stack traces of any detected allocations to stderr along with the allocation address and +/// length. /// -/// -pub fn trace_allocations() void { +pub fn trace_leaks() void { var current_allocation_info = context.allocation_info_head; while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info.next_info) { std.debug.print("{d} byte leak at 0x{x} detected: {}", .{ allocation_info.size, @ptrToInt(allocation_info) + @sizeOf(AllocationInfo), - allocation_info.stack_trace + allocation_info.trace }); } } diff --git a/source/ona/ona.zig b/source/ona/ona.zig index 72ecb56..33e5fb2 100755 --- a/source/ona/ona.zig +++ b/source/ona/ona.zig @@ -55,8 +55,8 @@ const AppManifest = struct { } }; -pub fn run_app(_: file.System) void { - defer heap.trace_allocations(); +pub fn run_app(base_file_system: file.System) void { + defer heap.trace_leaks(); const Logger = struct { const Self = @This(); @@ -76,12 +76,12 @@ pub fn run_app(_: file.System) void { defer script_environment.deinit(); - // const app_file_name = "app.ona"; + const app_file_name = "app.ona"; var app_manifest = AppManifest{}; - // app_manifest.load_script(&script_environment, base_file_system, app_file_name) catch { - // return ext.SDL_LogError(ext.SDL_LOG_CATEGORY_APPLICATION, "failed to load %s\n", app_file_name); - // }; + app_manifest.load_script(&script_environment, base_file_system, app_file_name) catch { + return ext.SDL_LogError(ext.SDL_LOG_CATEGORY_APPLICATION, "failed to load %s\n", app_file_name); + }; if (ext.SDL_Init(ext.SDL_INIT_EVERYTHING) != 0) { return ext.SDL_LogError(ext.SDL_LOG_CATEGORY_APPLICATION, "%s\n", ext.SDL_GetError()); From 331d86224682b8881a4ec23ca002a418a26e2841 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 02:39:57 +0000 Subject: [PATCH 06/12] Fix leaks and double-frees in Kym VM environment --- source/ona/kym/Environment.zig | 59 +++++++++++++++------------------- 1 file changed, 26 insertions(+), 33 deletions(-) diff --git a/source/ona/kym/Environment.zig b/source/ona/kym/Environment.zig index 41e9920..97055a8 100644 --- a/source/ona/kym/Environment.zig +++ b/source/ona/kym/Environment.zig @@ -69,21 +69,6 @@ const Object = struct { self.ref_count += 1; } - - pub fn release(self: *Object, env: *Self) bool { - coral.debug.assert(self.ref_count != 0); - - self.ref_count -= 1; - - if (self.ref_count == 0) { - coral.io.deallocate(env.allocator, self.state.userdata); - self.state.fields.deinit(env.allocator); - - return false; - } - - return true; - } }; pub const ObjectInfo = struct { @@ -185,14 +170,7 @@ pub fn check(self: *Self, condition: bool, failure_message: []const u8) !void { } pub fn deinit(self: *Self) void { - var global_data = self.heap.fetch(self.global_object); - - if (global_data.release(self)) { - self.heap.remove(self.global_object); - } else { - self.heap.assign(self.global_object, global_data); - } - + self.discard(.{.object = self.global_object}); self.values.deinit(self.allocator); self.calls.deinit(self.allocator); self.heap.deinit(self.allocator); @@ -203,7 +181,19 @@ pub fn discard(self: *Self, val: types.Val) void { .object => |object| { var data = self.heap.fetch(object); - if (data.release(self)) { + coral.debug.assert(data.ref_count != 0); + + data.ref_count -= 1; + + if (data.ref_count == 0) { + data.state.info.deinitializer(.{ + .env = self, + .obj = val.as_ref(), + }); + + coral.io.deallocate(self.allocator, data.state.userdata); + data.state.fields.deinit(self.allocator); + self.heap.remove(object); } else { self.heap.assign(object, data); @@ -223,17 +213,21 @@ pub fn execute_data(self: *Self, source: DataSource) types.RuntimeError!types.Va } }; - var chunk = try Chunk.init(self, source.name); + var compiled_chunk = init_compiled_chunk: { + var chunk = try Chunk.init(self, source.name); - errdefer chunk.deinit(); + errdefer chunk.deinit(); - chunk.compile(source.data) catch |compile_error| { - self.reporter.invoke(chunk.error_details()); + chunk.compile(source.data) catch |compile_error| { + self.reporter.invoke(chunk.error_details()); - return compile_error; + return compile_error; + }; + + break: init_compiled_chunk chunk; }; - const script = try self.new_object(coral.io.bytes_of(&chunk), .{ + const script = try self.new_object(coral.io.bytes_of(&compiled_chunk), .{ .identity = typeid, .deinitializer = Behaviors.deinitialize, }); @@ -355,12 +349,11 @@ pub fn native_cast(self: *Self, castable: types.Ref, id: *const anyopaque, compt try self.check(castable == .object, "invalid type conversion: object"); const object = self.heap.fetch(castable.object); - const alignment = @alignOf(Type); - const is_expected_type = (object.state.info.identity == id) and (object.state.userdata.len == alignment); + const is_expected_type = (object.state.info.identity == id) and (object.state.userdata.len == @sizeOf(Type)); try self.check(is_expected_type, "invalid object cast: native type"); - return @ptrCast(*Type, @alignCast(alignment, object.state.userdata)); + return @ptrCast(*Type, @alignCast(@alignOf(Type), object.state.userdata)); } pub fn new_array(self: *Self) coral.io.AllocationError!types.Val { From 9c871aac97bf6dffc1fa86ed514cfece562325f6 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 12:34:48 +0000 Subject: [PATCH 07/12] Clarify panic wording in Ona heap implementation --- source/ona/heap.zig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/ona/heap.zig b/source/ona/heap.zig index 9c9a5eb..35fc21d 100644 --- a/source/ona/heap.zig +++ b/source/ona/heap.zig @@ -58,7 +58,7 @@ const Context = struct { const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - @sizeOf(AllocationInfo)); if (target_allocation_info.size != allocation.len) { - @panic("incorrect allocation length"); + @panic("incorrect allocation length for deallocating"); } if (self.allocation_info_head) |allocation_info_head| { @@ -87,7 +87,7 @@ const Context = struct { } } - @panic("double-free detected"); + @panic("incorrect allocation address for deallocating"); } /// @@ -110,7 +110,7 @@ const Context = struct { const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - allocation_info_size); if (target_allocation_info.size != allocation.len) { - @panic("incorrect allocation length"); + @panic("incorrect allocation length for reallocating"); } if (self.allocation_info_head) |allocation_info_head| { @@ -139,7 +139,7 @@ const Context = struct { } } - @panic("use-after-free detected"); + @panic("incorrect allocation address for reallocating"); } }; From 341710bfbc0675ee234ae170fff6a288cff6b168 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 13:09:30 +0000 Subject: [PATCH 08/12] Fix so, so many memory leaks --- source/coral/io.zig | 41 ----------------------- source/coral/list.zig | 59 ++++++++++++++++++++++++++-------- source/coral/table.zig | 24 ++++++++++++++ source/ona/kym/Chunk.zig | 9 ++++-- source/ona/kym/Environment.zig | 30 ++++++++++++----- 5 files changed, 98 insertions(+), 65 deletions(-) diff --git a/source/coral/io.zig b/source/coral/io.zig index ad5e1ad..400c564 100755 --- a/source/coral/io.zig +++ b/source/coral/io.zig @@ -134,47 +134,6 @@ pub const FixedBuffer = struct { } }; -pub const GrowingBuffer = struct { - allocator: Allocator, - appender: Appender, - - const AppendOptions = struct { - allocator: Allocator, - bytes: []const u8, - }; - - const Appender = Generator(AllocationError!void, AppendOptions); - - pub fn as_writer(self: *GrowingBuffer) Writer { - return Writer.bind(GrowingBuffer, self, struct { - fn write(growing_buffer: *GrowingBuffer, bytes: []const u8) ?usize { - growing_buffer.write(bytes) catch return null; - - return bytes.len; - } - }.write); - } - - pub fn bind(comptime State: type, allocator: Allocator, state: *State, comptime appender: fn (capture: *State, allocator: Allocator, bytes: []const u8) AllocationError!void) GrowingBuffer { - return .{ - .appender = Appender.bind(State, state, struct { - fn append(self: *State, options: AppendOptions) AllocationError!void { - return appender(self, options.allocator, options.bytes); - } - }.append), - - .allocator = allocator, - }; - } - - pub fn write(self: GrowingBuffer, bytes: []const u8) AllocationError!void { - return self.appender.invoke(.{ - .allocator = self.allocator, - .bytes = bytes, - }); - } -}; - pub const Writer = Generator(?usize, []const u8); pub fn allocate_many(allocator: Allocator, amount: usize, comptime Type: type) AllocationError![]Type { diff --git a/source/coral/list.zig b/source/coral/list.zig index 77cbb67..d501be2 100755 --- a/source/coral/list.zig +++ b/source/coral/list.zig @@ -17,19 +17,6 @@ pub fn Stack(comptime Value: type) type { /// const Self = @This(); - /// - /// Returns a [io.GrowableBuffer] bound to `self` and `allocator`. - /// - /// The returned buffer may be used to write to the stack without needing to explicitly pass an allocator - /// context, as well decay further into a generic [io.Writer] type. - /// - /// *Note* if `capacity` is a non-zero value, `allocator` must reference the same allocation strategy as the one - /// originally used to allocate the current internal buffer. - /// - pub fn as_buffer(self: *Self, allocator: io.Allocator) io.GrowingBuffer { - return io.GrowingBuffer.bind(Self, allocator, self, push_all); - } - /// /// Clears all elements from `self` while preserving the current internal buffer. /// @@ -194,3 +181,49 @@ pub fn Stack(comptime Value: type) type { } }; } + +/// +/// Bridge context between a list type implement as part of the list module and an allocator, allowing the list resource +/// referenced by the [Writable] instance to be written to directly or virtually via the [io.Writer] interface. +/// +/// *Note* if the given list contains an existing allocation, the provided [io.Allocator] instance must reference the +/// same allocation strategy as the one originally used to allocate the list type memory. +/// +pub const Writable = struct { + allocator: io.Allocator, + + list: union (enum) { + stack: *ByteStack, + }, + + /// + /// Stack of bytes. + /// + const ByteStack = Stack(u8); + + /// + /// Returns a [io.Writer] instance that binds a reference of `self` to the [write] operation. + /// + pub fn as_writer(self: *Writable) io.Writer { + return io.Writer.bind(Writable, self, struct { + fn write(writable: *Writable, bytes: []const u8) ?usize { + writable.write(bytes) catch return null; + + return bytes.len; + } + }.write); + } + + /// + /// Attempts to call the appropriate multi-element writing function for the current list referenced by `self`, + /// passing `bytes` along. + /// + /// The function returns [io.AllocationError] if `allocator` could not commit the memory by the list implementation + /// referenced by `self`. See the specific implementation details of the respective list type for more information. + /// + pub fn write(self: *Writable, bytes: []const u8) io.AllocationError!void { + return switch (self.list) { + .stack => |stack| stack.push_all(self.allocator, bytes), + }; + } +}; diff --git a/source/coral/table.zig b/source/coral/table.zig index c127e82..d80a5a2 100755 --- a/source/coral/table.zig +++ b/source/coral/table.zig @@ -65,6 +65,30 @@ pub fn Hashed(comptime Key: type, comptime Value: type, comptime keyer: Keyer(Ke } }; + /// + /// Iterable wrapper for [Hashed] instances to make unordered traversal of key-value entries relatively trivial. + /// + pub const Iterable = struct { + hashed_map: *Self, + iterations: usize = 0, + + /// + /// Attempts to move past the current iteration of `self` and onto the next key-value entry, returning it or + /// `null` if there are no more elements in the referenced map. + /// + pub fn next(self: *Iterable) ?Entry { + while (self.iterations < self.hashed_map.table.len) { + defer self.iterations += 1; + + if (self.hashed_map.table[self.iterations]) |entry| { + return entry; + } + } + + return null; + } + }; + /// /// Table type. /// diff --git a/source/ona/kym/Chunk.zig b/source/ona/kym/Chunk.zig index 30add13..a2c10f1 100644 --- a/source/ona/kym/Chunk.zig +++ b/source/ona/kym/Chunk.zig @@ -53,7 +53,7 @@ fn clear_error_details(self: *Self) void { pub fn compile(self: *Self, data: []const u8) types.RuntimeError!void { var ast = try Ast.init(self.env.allocator); - errdefer ast.deinit(); + defer ast.deinit(); { var tokenizer = tokens.Tokenizer{.source = data}; @@ -62,9 +62,12 @@ pub fn compile(self: *Self, data: []const u8) types.RuntimeError!void { if (init_error == error.BadSyntax) { self.clear_error_details(); - var message_buffer = self.message_data.as_buffer(self.env.allocator); + var writable_data = coral.list.Writable{ + .allocator = self.env.allocator, + .list = .{.stack = &self.message_data}, + }; - coral.utf8.print_formatted(message_buffer.as_writer(), "@({line}): {name}", .{ + coral.utf8.print_formatted(writable_data.as_writer(), "@({line}): {name}", .{ .line = tokenizer.lines_stepped, .name = ast.error_message, }) catch return error.OutOfMemory; diff --git a/source/ona/kym/Environment.zig b/source/ona/kym/Environment.zig index 97055a8..4969f3b 100644 --- a/source/ona/kym/Environment.zig +++ b/source/ona/kym/Environment.zig @@ -171,6 +171,16 @@ pub fn check(self: *Self, condition: bool, failure_message: []const u8) !void { pub fn deinit(self: *Self) void { self.discard(.{.object = self.global_object}); + + { + var interned_iterable = InternTable.Iterable{.hashed_map = &self.interned}; + + while (interned_iterable.next()) |entry| { + self.discard(.{.object = entry.value}); + } + } + + self.interned.deinit(self.allocator); self.values.deinit(self.allocator); self.calls.deinit(self.allocator); self.heap.deinit(self.allocator); @@ -191,9 +201,9 @@ pub fn discard(self: *Self, val: types.Val) void { .obj = val.as_ref(), }); - coral.io.deallocate(self.allocator, data.state.userdata); + // TODO: Free individual key-value pairs of fields data.state.fields.deinit(self.allocator); - + coral.io.deallocate(self.allocator, data.state.userdata); self.heap.remove(object); } else { self.heap.assign(object, data); @@ -242,25 +252,29 @@ pub fn execute_file(self: *Self, fs: file.System, file_path: file.Path) ExecuteF defer readable_file.close(); - var file_source = coral.list.Stack(u8){}; + var file_data = coral.list.Stack(u8){}; const file_size = (try fs.query_info(file_path)).size; - try file_source.grow(self.allocator, file_size); + try file_data.grow(self.allocator, file_size); - defer file_source.deinit(self.allocator); + defer file_data.deinit(self.allocator); { - var file_buffer = file_source.as_buffer(self.allocator); + var writable_data = coral.list.Writable{ + .allocator = self.allocator, + .list = .{.stack = &file_data}, + }; + var stream_buffer = [_]u8{0} ** 4096; - if ((try coral.io.stream(file_buffer.as_writer(), readable_file.as_reader(), &stream_buffer)) != file_size) { + if ((try coral.io.stream(writable_data.as_writer(), readable_file.as_reader(), &stream_buffer)) != file_size) { return error.ReadFailure; } } return try self.execute_data(.{ .name = try file_path.to_string(), - .data = file_source.values, + .data = file_data.values, }); } From d378939f30340d1de4359b0f00c13de977cbb196 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 13:37:26 +0000 Subject: [PATCH 09/12] Remove allocation checks in optimized release builds --- source/ona/heap.zig | 215 ++++++++++++++++++++++++++++---------------- 1 file changed, 136 insertions(+), 79 deletions(-) diff --git a/source/ona/heap.zig b/source/ona/heap.zig index 35fc21d..4d32118 100644 --- a/source/ona/heap.zig +++ b/source/ona/heap.zig @@ -1,3 +1,5 @@ +const builtin = @import("builtin"); + const coral = @import("coral"); const ext = @import("./ext.zig"); @@ -8,11 +10,19 @@ const std = @import("std"); /// /// const AllocationInfo = struct { - trace: std.debug.Trace, + trace: AllocationTrace, next_info: ?*AllocationInfo, size: usize, }; +/// +/// +/// +const AllocationTrace = std.debug.ConfigurableTrace(2, 4, switch (builtin.mode) { + .Debug, .ReleaseSafe => true, + .ReleaseFast, .ReleaseSmall => false, +}); + /// /// /// @@ -29,23 +39,40 @@ const Context = struct { /// *Note* the returned buffer must be deallocated with [deallocate] before program exit or it will cause a memory /// leak. /// + /// *Note* allocation checks are disabled in release builds optimized for speed or size. + /// fn allocate(self: *Context, size: usize, return_address: usize) ?[]u8 { - const allocation_info_size = @sizeOf(AllocationInfo); - const total_allocation_size = allocation_info_size + size; - const allocation = ext.SDL_malloc(total_allocation_size) orelse return null; - const allocation_info = @ptrCast(*AllocationInfo, @alignCast(@alignOf(AllocationInfo), allocation)); + switch (builtin.mode) { + .Debug, .ReleaseSafe => { + const allocation_info_size = @sizeOf(AllocationInfo); + const total_allocation_size = allocation_info_size + size; + const allocation = ext.SDL_malloc(total_allocation_size) orelse return null; + const allocation_info = @ptrCast(*AllocationInfo, @alignCast(@alignOf(AllocationInfo), allocation)); - allocation_info.* = .{ - .size = size, - .next_info = self.allocation_info_head, - .trace = .{}, - }; + allocation_info.* = .{ + .size = size, + .next_info = self.allocation_info_head, + .trace = .{}, + }; - allocation_info.trace.addAddr(return_address, ""); + allocation_info.trace.addAddr(return_address, ""); - self.allocation_info_head = allocation_info; + self.allocation_info_head = allocation_info; - return @ptrCast([*]u8, allocation)[allocation_info_size .. total_allocation_size]; + return @ptrCast([*]u8, allocation)[allocation_info_size .. total_allocation_size]; + }, + + .ReleaseFast, .ReleaseSmall => { + return @ptrCast([*]u8, ext.SDL_malloc(size) orelse return null)[0 .. size]; + }, + } + } + + /// + /// + /// + fn allocation_info_of(allocation: [*]u8) *AllocationInfo { + return @intToPtr(*AllocationInfo, @ptrToInt(allocation) - @sizeOf(AllocationInfo)); } /// @@ -54,40 +81,50 @@ const Context = struct { /// *Note* the pointer and length of `allocation` must match valid values known to `allocator` otherwise safety- /// checked behavior will occur. /// + /// *Note* allocation checks are disabled in release builds optimized for speed or size. + /// fn deallocate(self: *Context, allocation: []u8) void { - const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - @sizeOf(AllocationInfo)); + switch (builtin.mode) { + .Debug, .ReleaseSafe => { + const target_allocation_info = allocation_info_of(allocation.ptr); - if (target_allocation_info.size != allocation.len) { - @panic("incorrect allocation length for deallocating"); - } - - if (self.allocation_info_head) |allocation_info_head| { - if (target_allocation_info == allocation_info_head) { - self.allocation_info_head = allocation_info_head.next_info; - - ext.SDL_free(target_allocation_info); - - return; - } - - var previous_allocation_info = allocation_info_head; - var current_allocation_info = allocation_info_head.next_info; - - while (current_allocation_info) |allocation_info| { - if (allocation_info == target_allocation_info) { - previous_allocation_info.next_info = allocation_info.next_info; - - ext.SDL_free(target_allocation_info); - - return; + if (target_allocation_info.size != allocation.len) { + @panic("incorrect allocation length for deallocating"); } - previous_allocation_info = allocation_info; - current_allocation_info = allocation_info.next_info; - } - } + if (self.allocation_info_head) |allocation_info_head| { + if (target_allocation_info == allocation_info_head) { + self.allocation_info_head = allocation_info_head.next_info; - @panic("incorrect allocation address for deallocating"); + ext.SDL_free(target_allocation_info); + + return; + } + + var previous_allocation_info = allocation_info_head; + var current_allocation_info = allocation_info_head.next_info; + + while (current_allocation_info) |allocation_info| { + if (allocation_info == target_allocation_info) { + previous_allocation_info.next_info = allocation_info.next_info; + + ext.SDL_free(target_allocation_info); + + return; + } + + previous_allocation_info = allocation_info; + current_allocation_info = allocation_info.next_info; + } + } + + @panic("incorrect allocation address for deallocating"); + }, + + .ReleaseFast, .ReleaseSmall => { + ext.SDL_free(allocation.ptr); + }, + } } /// @@ -105,41 +142,52 @@ const Context = struct { /// *Note* the allocation referenced by `allocation` should be considered invalid once the function returns, /// discarding it in favor of the return value. /// + /// *Note* allocation checks are disabled in release builds optimized for speed or size. + /// fn reallocate(self: *Context, allocation: []u8, size: usize) ?[]u8 { - const allocation_info_size = @sizeOf(AllocationInfo); - const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - allocation_info_size); + switch (builtin.mode) { + .Debug, .ReleaseSafe => { + const target_allocation_info = allocation_info_of(allocation.ptr); - if (target_allocation_info.size != allocation.len) { - @panic("incorrect allocation length for reallocating"); - } - - if (self.allocation_info_head) |allocation_info_head| { - if (target_allocation_info == allocation_info_head) { - self.allocation_info_head = allocation_info_head.next_info; - - return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { - return null; - })[allocation_info_size .. allocation_info_size + size]; - } - - var previous_allocation_info = allocation_info_head; - var current_allocation_info = allocation_info_head.next_info; - - while (current_allocation_info) |allocation_info| { - if (allocation_info == target_allocation_info) { - previous_allocation_info.next_info = allocation_info.next_info; - - return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { - return null; - })[allocation_info_size .. allocation_info_size + size]; + if (target_allocation_info.size != allocation.len) { + @panic("incorrect allocation length for reallocating"); } - previous_allocation_info = allocation_info; - current_allocation_info = allocation_info.next_info; - } - } + const allocation_info_size = @sizeOf(AllocationInfo); - @panic("incorrect allocation address for reallocating"); + if (self.allocation_info_head) |allocation_info_head| { + if (target_allocation_info == allocation_info_head) { + self.allocation_info_head = allocation_info_head.next_info; + + return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { + return null; + })[allocation_info_size .. allocation_info_size + size]; + } + + var previous_allocation_info = allocation_info_head; + var current_allocation_info = allocation_info_head.next_info; + + while (current_allocation_info) |allocation_info| { + if (allocation_info == target_allocation_info) { + previous_allocation_info.next_info = allocation_info.next_info; + + return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { + return null; + })[allocation_info_size .. allocation_info_size + size]; + } + + previous_allocation_info = allocation_info; + current_allocation_info = allocation_info.next_info; + } + } + + @panic("incorrect allocation address for reallocating"); + }, + + .ReleaseFast, .ReleaseSmall => { + return @ptrCast([*]u8, ext.SDL_realloc(allocation.ptr, size) orelse return null)[0 .. size]; + }, + } } }; @@ -176,14 +224,23 @@ pub const allocator = coral.io.Allocator.bind(Context, &context, struct { /// alive and reports the stack traces of any detected allocations to stderr along with the allocation address and /// length. /// +/// *Note* this function becomes a no-op in release builds optimized for speed or size. +/// pub fn trace_leaks() void { - var current_allocation_info = context.allocation_info_head; + switch (builtin.mode) { + .Debug, .ReleaseSafe => { + var current_allocation_info = context.allocation_info_head; - while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info.next_info) { - std.debug.print("{d} byte leak at 0x{x} detected: {}", .{ - allocation_info.size, - @ptrToInt(allocation_info) + @sizeOf(AllocationInfo), - allocation_info.trace - }); + while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info.next_info) { + std.debug.print("{d} byte leak at 0x{x} detected:\n", .{ + allocation_info.size, + @ptrToInt(allocation_info) + @sizeOf(AllocationInfo), + }); + + allocation_info.trace.dump(); + } + }, + + .ReleaseFast, .ReleaseSmall => {}, } } From 155645a308844ee4ac18c0602209f23a44c7898b Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 13:44:27 +0000 Subject: [PATCH 10/12] Fix allocation size info not updating in reallocs --- source/ona/heap.zig | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/source/ona/heap.zig b/source/ona/heap.zig index 4d32118..b6a28f9 100644 --- a/source/ona/heap.zig +++ b/source/ona/heap.zig @@ -7,7 +7,7 @@ const ext = @import("./ext.zig"); const std = @import("std"); /// -/// +/// Recorded allocation info state. /// const AllocationInfo = struct { trace: AllocationTrace, @@ -16,7 +16,9 @@ const AllocationInfo = struct { }; /// +/// Recorded stack trace of allocation call site. /// +/// *Note* this structure is reduced to zero bytes in released builds optimized for speed or size. /// const AllocationTrace = std.debug.ConfigurableTrace(2, 4, switch (builtin.mode) { .Debug, .ReleaseSafe => true, @@ -24,7 +26,7 @@ const AllocationTrace = std.debug.ConfigurableTrace(2, 4, switch (builtin.mode) }); /// -/// +/// Heap allocation context. /// const Context = struct { allocation_info_head: ?*AllocationInfo = null, @@ -69,7 +71,7 @@ const Context = struct { } /// - /// + /// Returns the assumed pointer to the [AllocationInfo] address of `allocation`. /// fn allocation_info_of(allocation: [*]u8) *AllocationInfo { return @intToPtr(*AllocationInfo, @ptrToInt(allocation) - @sizeOf(AllocationInfo)); @@ -159,9 +161,12 @@ const Context = struct { if (target_allocation_info == allocation_info_head) { self.allocation_info_head = allocation_info_head.next_info; - return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { - return null; - })[allocation_info_size .. allocation_info_size + size]; + const allocation_address = ext.SDL_realloc(target_allocation_info, size) orelse return null; + + target_allocation_info.size = size; + + return @ptrCast([*]u8, allocation_address)[ + allocation_info_size .. (allocation_info_size + size)]; } var previous_allocation_info = allocation_info_head; @@ -171,9 +176,12 @@ const Context = struct { if (allocation_info == target_allocation_info) { previous_allocation_info.next_info = allocation_info.next_info; - return @ptrCast([*]u8, ext.SDL_realloc(target_allocation_info, size) orelse { - return null; - })[allocation_info_size .. allocation_info_size + size]; + const allocation_address = ext.SDL_realloc(target_allocation_info, size) orelse return null; + + target_allocation_info.size = size; + + return @ptrCast([*]u8, allocation_address)[ + allocation_info_size .. (allocation_info_size + size)]; } previous_allocation_info = allocation_info; From 5500de299b0eed9eeb8987714332533fe18ba931 Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 14:00:21 +0000 Subject: [PATCH 11/12] Fix slab count not being properly updated --- source/coral/slab.zig | 31 ++++++++++++++++++++----------- source/ona/kym/Environment.zig | 1 + 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/source/coral/slab.zig b/source/coral/slab.zig index 6d1957e..4dd6d30 100644 --- a/source/coral/slab.zig +++ b/source/coral/slab.zig @@ -52,17 +52,6 @@ pub fn Map(comptime index_int: std.builtin.Type.Int, comptime Value: type) type entry.value = value; } - /// - /// Fetches the value referenced by `index` in `self`, returning it. - /// - pub fn fetch(self: *Self, index: Index) Value { - const entry = &self.table[index]; - - debug.assert(entry.* == .value); - - return entry.value; - } - /// /// Deinitializes `self` and sets it to an invalid state, freeing all memory allocated by `allocator`. /// @@ -81,6 +70,17 @@ pub fn Map(comptime index_int: std.builtin.Type.Int, comptime Value: type) type self.free_index = 0; } + /// + /// Fetches the value referenced by `index` in `self`, returning it. + /// + pub fn fetch(self: *Self, index: Index) Value { + const entry = &self.table[index]; + + debug.assert(entry.* == .value); + + return entry.value; + } + /// /// Attempts to grow the internal buffer of `self` by `growth_amount` using `allocator`. /// @@ -147,12 +147,20 @@ pub fn Map(comptime index_int: std.builtin.Type.Int, comptime Value: type) type debug.assert(entry.* == .free_index); + self.count += 1; self.free_index = entry.free_index; entry.* = .{.value = value}; return entry_index; } + /// + /// Returns `true` if `self` contains no values, otherwise `false`. + /// + pub fn is_empty(self: Self) bool { + return self.count == 0; + } + /// /// Removes the value referenced by `index` from `self`. /// @@ -161,6 +169,7 @@ pub fn Map(comptime index_int: std.builtin.Type.Int, comptime Value: type) type debug.assert(entry.* == .value); + self.count -= 1; entry.* = .{.free_index = self.free_index}; self.free_index = index; } diff --git a/source/ona/kym/Environment.zig b/source/ona/kym/Environment.zig index 4969f3b..b23c243 100644 --- a/source/ona/kym/Environment.zig +++ b/source/ona/kym/Environment.zig @@ -183,6 +183,7 @@ pub fn deinit(self: *Self) void { self.interned.deinit(self.allocator); self.values.deinit(self.allocator); self.calls.deinit(self.allocator); + coral.debug.assert(self.heap.is_empty()); self.heap.deinit(self.allocator); } From 37942599490dd4aec2d4e9db26e26753bf393b0a Mon Sep 17 00:00:00 2001 From: kayomn Date: Sun, 4 Jun 2023 14:06:28 +0000 Subject: [PATCH 12/12] Amend code review comments --- source/ona/kym/Environment.zig | 49 +++++++++++++++++----------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/source/ona/kym/Environment.zig b/source/ona/kym/Environment.zig index b23c243..e35742b 100644 --- a/source/ona/kym/Environment.zig +++ b/source/ona/kym/Environment.zig @@ -170,13 +170,13 @@ pub fn check(self: *Self, condition: bool, failure_message: []const u8) !void { } pub fn deinit(self: *Self) void { - self.discard(.{.object = self.global_object}); + self.object_release(self.global_object); { var interned_iterable = InternTable.Iterable{.hashed_map = &self.interned}; while (interned_iterable.next()) |entry| { - self.discard(.{.object = entry.value}); + self.object_release(entry.value); } } @@ -189,28 +189,7 @@ pub fn deinit(self: *Self) void { pub fn discard(self: *Self, val: types.Val) void { switch (val) { - .object => |object| { - var data = self.heap.fetch(object); - - coral.debug.assert(data.ref_count != 0); - - data.ref_count -= 1; - - if (data.ref_count == 0) { - data.state.info.deinitializer(.{ - .env = self, - .obj = val.as_ref(), - }); - - // TODO: Free individual key-value pairs of fields - data.state.fields.deinit(self.allocator); - coral.io.deallocate(self.allocator, data.state.userdata); - self.heap.remove(object); - } else { - self.heap.assign(object, data); - } - }, - + .object => |object| self.object_release(object), else => {}, } } @@ -417,6 +396,28 @@ pub fn new_string(self: *Self, data: []const u8) coral.io.AllocationError!types. }); } +pub fn object_release(self: *Self, object: types.Object) void { + var data = self.heap.fetch(object); + + coral.debug.assert(data.ref_count != 0); + + data.ref_count -= 1; + + if (data.ref_count == 0) { + data.state.info.deinitializer(.{ + .env = self, + .obj = .{.object = object}, + }); + + // TODO: Free individual key-value pairs of fields + data.state.fields.deinit(self.allocator); + coral.io.deallocate(self.allocator, data.state.userdata); + self.heap.remove(object); + } else { + self.heap.assign(object, data); + } +} + pub fn set_global(self: *Self, global_name: []const u8, value: types.Ref) coral.io.AllocationError!void { try self.globals.assign(self.allocator, global_name, value); }