From 26153ce73a1b9c49bdf89055b8ab7f4d3173f153 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 21 Apr 2022 00:06:52 +0200 Subject: dwarf: clean up allocations in std.dwarf module While this code probably could do with some love and a redesign, this commit fixes the allocations by making sure we explicitly pass an allocator where required, and we use arenas for temporary or narrowly-scoped objects such as a `Die` (for `Die` in particular, not every `FormValue` will be allocated - we could duplicate, or we can use an arena which is the proposal of this commit). --- lib/std/debug.zig | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) (limited to 'lib/std/debug.zig') diff --git a/lib/std/debug.zig b/lib/std/debug.zig index b600f7245a..c6ee812c19 100644 --- a/lib/std/debug.zig +++ b/lib/std/debug.zig @@ -30,10 +30,8 @@ pub const LineInfo = struct { line: u64, column: u64, file_name: []const u8, - allocator: ?mem.Allocator, - pub fn deinit(self: LineInfo) void { - const allocator = self.allocator orelse return; + pub fn deinit(self: LineInfo, allocator: mem.Allocator) void { allocator.free(self.file_name); } }; @@ -43,9 +41,9 @@ pub const SymbolInfo = struct { compile_unit_name: []const u8 = "???", line_info: ?LineInfo = null, - pub fn deinit(self: @This()) void { + pub fn deinit(self: @This(), allocator: mem.Allocator) void { if (self.line_info) |li| { - li.deinit(); + li.deinit(allocator); } } }; @@ -695,7 +693,7 @@ pub fn printSourceAtAddress(debug_info: *DebugInfo, out_stream: anytype, address }; const symbol_info = try module.getSymbolAtAddress(address); - defer symbol_info.deinit(); + defer symbol_info.deinit(debug_info.allocator); return printLineInfo( out_stream, @@ -1568,10 +1566,17 @@ pub const ModuleDebugInfo = switch (native_os) { if (o_file_di.findCompileUnit(relocated_address_o)) |compile_unit| { return SymbolInfo{ .symbol_name = o_file_di.getSymbolName(relocated_address_o) orelse "???", - .compile_unit_name = compile_unit.die.getAttrString(o_file_di, DW.AT.name) catch |err| switch (err) { + .compile_unit_name = compile_unit.die.getAttrString( + o_file_di, + DW.AT.name, + ) catch |err| switch (err) { error.MissingDebugInfo, error.InvalidDebugInfo => "???", }, - .line_info = o_file_di.getLineNumberInfo(compile_unit.*, relocated_address_o + addr_off) catch |err| switch (err) { + .line_info = o_file_di.getLineNumberInfo( + self.allocator(), + compile_unit.*, + relocated_address_o + addr_off, + ) catch |err| switch (err) { error.MissingDebugInfo, error.InvalidDebugInfo => null, else => return err, }, -- cgit v1.2.3 From 96c1314443bdf26442a2c9fdffa03f2afffbcb8e Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 21 Apr 2022 00:45:01 +0200 Subject: debug: fix resource (de)allocation for MachO targets With this change, it is now possible to safely call `var di = std.debug.openSelfDebugInfo(gpa)`. Calling then `di.deinit()` on the object will correctly free all allocated resources. --- lib/std/debug.zig | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) (limited to 'lib/std/debug.zig') diff --git a/lib/std/debug.zig b/lib/std/debug.zig index c6ee812c19..58038ef522 100644 --- a/lib/std/debug.zig +++ b/lib/std/debug.zig @@ -692,7 +692,7 @@ pub fn printSourceAtAddress(debug_info: *DebugInfo, out_stream: anytype, address else => return err, }; - const symbol_info = try module.getSymbolAtAddress(address); + const symbol_info = try module.getSymbolAtAddress(debug_info.allocator, address); defer symbol_info.deinit(debug_info.allocator); return printLineInfo( @@ -1142,7 +1142,12 @@ pub const DebugInfo = struct { } pub fn deinit(self: *DebugInfo) void { - // TODO: resources https://github.com/ziglang/zig/issues/4353 + var it = self.address_map.iterator(); + while (it.next()) |entry| { + const mdi = entry.value_ptr.*; + mdi.deinit(self.allocator); + self.allocator.destroy(mdi); + } self.address_map.deinit(); } @@ -1392,11 +1397,18 @@ pub const ModuleDebugInfo = switch (native_os) { addr_table: std.StringHashMap(u64), }; - pub fn allocator(self: @This()) mem.Allocator { - return self.ofiles.allocator; + fn deinit(self: *@This(), allocator: mem.Allocator) void { + var it = self.ofiles.iterator(); + while (it.next()) |entry| { + const ofile = entry.value_ptr; + ofile.di.deinit(allocator); + ofile.addr_table.deinit(); + } + self.ofiles.deinit(); + allocator.free(self.symbols); } - fn loadOFile(self: *@This(), o_file_path: []const u8) !OFileInfo { + fn loadOFile(self: *@This(), allocator: mem.Allocator, o_file_path: []const u8) !OFileInfo { const o_file = try fs.cwd().openFile(o_file_path, .{ .intended_io_mode = .blocking }); const mapped_mem = try mapWholeFile(o_file); @@ -1448,7 +1460,7 @@ pub const ModuleDebugInfo = switch (native_os) { )[0..symtabcmd.?.nsyms]; // TODO handle tentative (common) symbols - var addr_table = std.StringHashMap(u64).init(self.allocator()); + var addr_table = std.StringHashMap(u64).init(allocator); try addr_table.ensureTotalCapacity(@intCast(u32, symtab.len)); for (symtab) |sym| { if (sym.n_strx == 0) continue; @@ -1517,7 +1529,7 @@ pub const ModuleDebugInfo = switch (native_os) { null, }; - try DW.openDwarfDebugInfo(&di, self.allocator()); + try DW.openDwarfDebugInfo(&di, allocator); var info = OFileInfo{ .di = di, .addr_table = addr_table, @@ -1529,7 +1541,7 @@ pub const ModuleDebugInfo = switch (native_os) { return info; } - pub fn getSymbolAtAddress(self: *@This(), address: usize) !SymbolInfo { + pub fn getSymbolAtAddress(self: *@This(), allocator: mem.Allocator, address: usize) !SymbolInfo { nosuspend { // Translate the VA into an address into this object const relocated_address = address - self.base_address; @@ -1546,7 +1558,7 @@ pub const ModuleDebugInfo = switch (native_os) { // Check if its debug infos are already in the cache var o_file_info = self.ofiles.get(o_file_path) orelse - (self.loadOFile(o_file_path) catch |err| switch (err) { + (self.loadOFile(allocator, o_file_path) catch |err| switch (err) { error.FileNotFound, error.MissingDebugInfo, error.InvalidDebugInfo, @@ -1573,7 +1585,7 @@ pub const ModuleDebugInfo = switch (native_os) { error.MissingDebugInfo, error.InvalidDebugInfo => "???", }, .line_info = o_file_di.getLineNumberInfo( - self.allocator(), + allocator, compile_unit.*, relocated_address_o + addr_off, ) catch |err| switch (err) { -- cgit v1.2.3 From 28ca203b7132ba0513a3854bd3bcbd0ee9bca067 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 21 Apr 2022 10:46:04 +0200 Subject: debug: fix resource (de)allocation for Elf and Coff targets With this change, it is now possible to safely call `var di = std.debug.openSelfDebugInfo(gpa)`. Calling then `di.deinit()` on the object will correctly free all allocated resources. Ensure we store the result of `mmap` with correct alignment. --- lib/std/coff.zig | 11 ++++++----- lib/std/debug.zig | 46 ++++++++++++++++++++++++++++++++++------------ lib/std/pdb.zig | 1 - 3 files changed, 40 insertions(+), 18 deletions(-) (limited to 'lib/std/debug.zig') diff --git a/lib/std/coff.zig b/lib/std/coff.zig index a2da0552be..8fabd8e214 100644 --- a/lib/std/coff.zig +++ b/lib/std/coff.zig @@ -4,8 +4,6 @@ const mem = std.mem; const os = std.os; const File = std.fs.File; -const ArrayList = std.ArrayList; - // CoffHeader.machine values // see https://msdn.microsoft.com/en-us/library/windows/desktop/ms680313(v=vs.85).aspx const IMAGE_FILE_MACHINE_I386 = 0x014c; @@ -117,7 +115,7 @@ pub const Coff = struct { coff_header: CoffHeader, pe_header: OptionalHeader, - sections: ArrayList(Section), + sections: std.ArrayListUnmanaged(Section) = .{}, guid: [16]u8, age: u32, @@ -128,12 +126,15 @@ pub const Coff = struct { .allocator = allocator, .coff_header = undefined, .pe_header = undefined, - .sections = ArrayList(Section).init(allocator), .guid = undefined, .age = undefined, }; } + pub fn deinit(self: *Coff) void { + self.sections.deinit(self.allocator); + } + pub fn loadHeader(self: *Coff) !void { const pe_pointer_offset = 0x3C; @@ -291,7 +292,7 @@ pub const Coff = struct { if (self.sections.items.len == self.coff_header.number_of_sections) return; - try self.sections.ensureTotalCapacityPrecise(self.coff_header.number_of_sections); + try self.sections.ensureTotalCapacityPrecise(self.allocator, self.coff_header.number_of_sections); const in = self.in_file.reader(); diff --git a/lib/std/debug.zig b/lib/std/debug.zig index 58038ef522..5ee4973c1a 100644 --- a/lib/std/debug.zig +++ b/lib/std/debug.zig @@ -41,7 +41,7 @@ pub const SymbolInfo = struct { compile_unit_name: []const u8 = "???", line_info: ?LineInfo = null, - pub fn deinit(self: @This(), allocator: mem.Allocator) void { + pub fn deinit(self: SymbolInfo, allocator: mem.Allocator) void { if (self.line_info) |li| { li.deinit(allocator); } @@ -50,6 +50,13 @@ pub const SymbolInfo = struct { const PdbOrDwarf = union(enum) { pdb: pdb.Pdb, dwarf: DW.DwarfInfo, + + fn deinit(self: *PdbOrDwarf, allocator: mem.Allocator) void { + switch (self.*) { + .pdb => |*inner| inner.deinit(), + .dwarf => |*inner| inner.deinit(allocator), + } + } }; var stderr_mutex = std.Thread.Mutex{}; @@ -793,6 +800,7 @@ fn readCoffDebugInfo(allocator: mem.Allocator, coff_file: File) !ModuleDebugInfo errdefer coff_file.close(); const coff_obj = try allocator.create(coff.Coff); + errdefer allocator.destroy(coff_obj); coff_obj.* = coff.Coff.init(allocator, coff_file); var di = ModuleDebugInfo{ @@ -1386,7 +1394,7 @@ pub const DebugInfo = struct { pub const ModuleDebugInfo = switch (native_os) { .macos, .ios, .watchos, .tvos => struct { base_address: usize, - mapped_memory: []const u8, + mapped_memory: []align(mem.page_size) const u8, symbols: []const MachoSymbol, strings: [:0]const u8, ofiles: OFileTable, @@ -1406,6 +1414,7 @@ pub const ModuleDebugInfo = switch (native_os) { } self.ofiles.deinit(); allocator.free(self.symbols); + os.munmap(self.mapped_memory); } fn loadOFile(self: *@This(), allocator: mem.Allocator, o_file_path: []const u8) !OFileInfo { @@ -1609,18 +1618,20 @@ pub const ModuleDebugInfo = switch (native_os) { debug_data: PdbOrDwarf, coff: *coff.Coff, - pub fn allocator(self: @This()) mem.Allocator { - return self.coff.allocator; + fn deinit(self: *@This(), allocator: mem.Allocator) void { + self.debug_data.deinit(allocator); + self.coff.deinit(); + allocator.destroy(self.coff); } - pub fn getSymbolAtAddress(self: *@This(), address: usize) !SymbolInfo { + pub fn getSymbolAtAddress(self: *@This(), allocator: mem.Allocator, address: usize) !SymbolInfo { // Translate the VA into an address into this object const relocated_address = address - self.base_address; switch (self.debug_data) { .dwarf => |*dwarf| { const dwarf_address = relocated_address + self.coff.pe_header.image_base; - return getSymbolFromDwarf(dwarf_address, dwarf); + return getSymbolFromDwarf(allocator, dwarf_address, dwarf); }, .pdb => { // fallthrough to pdb handling @@ -1666,17 +1677,28 @@ pub const ModuleDebugInfo = switch (native_os) { .linux, .netbsd, .freebsd, .dragonfly, .openbsd, .haiku, .solaris => struct { base_address: usize, dwarf: DW.DwarfInfo, - mapped_memory: []const u8, + mapped_memory: []align(mem.page_size) const u8, + + fn deinit(self: *@This(), allocator: mem.Allocator) void { + self.dwarf.deinit(allocator); + os.munmap(self.mapped_memory); + } - pub fn getSymbolAtAddress(self: *@This(), address: usize) !SymbolInfo { + pub fn getSymbolAtAddress(self: *@This(), allocator: mem.Allocator, address: usize) !SymbolInfo { // Translate the VA into an address into this object const relocated_address = address - self.base_address; - return getSymbolFromDwarf(relocated_address, &self.dwarf); + return getSymbolFromDwarf(allocator, relocated_address, &self.dwarf); } }, .wasi => struct { - pub fn getSymbolAtAddress(self: *@This(), address: usize) !SymbolInfo { + fn deinit(self: *@This(), allocator: mem.Allocator) void { + _ = self; + _ = allocator; + } + + pub fn getSymbolAtAddress(self: *@This(), allocator: mem.Allocator, address: usize) !SymbolInfo { _ = self; + _ = allocator; _ = address; return SymbolInfo{}; } @@ -1684,14 +1706,14 @@ pub const ModuleDebugInfo = switch (native_os) { else => DW.DwarfInfo, }; -fn getSymbolFromDwarf(address: u64, di: *DW.DwarfInfo) !SymbolInfo { +fn getSymbolFromDwarf(allocator: mem.Allocator, address: u64, di: *DW.DwarfInfo) !SymbolInfo { if (nosuspend di.findCompileUnit(address)) |compile_unit| { return SymbolInfo{ .symbol_name = nosuspend di.getSymbolName(address) orelse "???", .compile_unit_name = compile_unit.die.getAttrString(di, DW.AT.name) catch |err| switch (err) { error.MissingDebugInfo, error.InvalidDebugInfo => "???", }, - .line_info = nosuspend di.getLineNumberInfo(compile_unit.*, address) catch |err| switch (err) { + .line_info = nosuspend di.getLineNumberInfo(allocator, compile_unit.*, address) catch |err| switch (err) { error.MissingDebugInfo, error.InvalidDebugInfo => null, else => return err, }, diff --git a/lib/std/pdb.zig b/lib/std/pdb.zig index 03f78cb179..88ae849109 100644 --- a/lib/std/pdb.zig +++ b/lib/std/pdb.zig @@ -764,7 +764,6 @@ pub const Pdb = struct { const flags = @ptrCast(*LineNumberEntry.Flags, &line_num_entry.Flags); return debug.LineInfo{ - .allocator = self.allocator, .file_name = source_file_name, .line = flags.Start, .column = column, -- cgit v1.2.3 From bedd7efa2bca82aa1c101ca4144b6bce65c9ab87 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 21 Apr 2022 10:14:23 +0200 Subject: debug: add smoke test --- lib/std/debug.zig | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) (limited to 'lib/std/debug.zig') diff --git a/lib/std/debug.zig b/lib/std/debug.zig index 5ee4973c1a..683219c78d 100644 --- a/lib/std/debug.zig +++ b/lib/std/debug.zig @@ -682,7 +682,6 @@ test "machoSearchSymbols" { try testing.expectEqual(&symbols[2], machoSearchSymbols(&symbols, 5000).?); } -/// TODO resources https://github.com/ziglang/zig/issues/4353 pub fn printSourceAtAddress(debug_info: *DebugInfo, out_stream: anytype, address: usize, tty_config: TTY.Config) !void { const module = debug_info.getModuleForAddress(address) catch |err| switch (err) { error.MissingDebugInfo, error.InvalidDebugInfo => { @@ -768,7 +767,6 @@ pub const OpenSelfDebugInfoError = error{ UnsupportedOperatingSystem, }; -/// TODO resources https://github.com/ziglang/zig/issues/4353 pub fn openSelfDebugInfo(allocator: mem.Allocator) anyerror!DebugInfo { nosuspend { if (builtin.strip_debug_info) @@ -793,7 +791,6 @@ pub fn openSelfDebugInfo(allocator: mem.Allocator) anyerror!DebugInfo { /// This takes ownership of coff_file: users of this function should not close /// it themselves, even on error. -/// TODO resources https://github.com/ziglang/zig/issues/4353 /// TODO it's weird to take ownership even on error, rework this code. fn readCoffDebugInfo(allocator: mem.Allocator, coff_file: File) !ModuleDebugInfo { nosuspend { @@ -863,7 +860,6 @@ fn chopSlice(ptr: []const u8, offset: u64, size: u64) ![]const u8 { /// This takes ownership of elf_file: users of this function should not close /// it themselves, even on error. -/// TODO resources https://github.com/ziglang/zig/issues/4353 /// TODO it's weird to take ownership even on error, rework this code. pub fn readElfDebugInfo(allocator: mem.Allocator, elf_file: File) !ModuleDebugInfo { nosuspend { @@ -937,7 +933,6 @@ pub fn readElfDebugInfo(allocator: mem.Allocator, elf_file: File) !ModuleDebugIn } } -/// TODO resources https://github.com/ziglang/zig/issues/4353 /// This takes ownership of macho_file: users of this function should not close /// it themselves, even on error. /// TODO it's weird to take ownership even on error, rework this code. @@ -1934,3 +1929,16 @@ pub fn dumpStackPointerAddr(prefix: []const u8) void { ); std.debug.print("{} sp = 0x{x}\n", .{ prefix, sp }); } + +test "#4353: std.debug should manage resources correctly" { + if (builtin.os.tag == .wasi) return error.SkipZigTest; + + const writer = std.io.null_writer; + var di = try openSelfDebugInfo(testing.allocator); + defer di.deinit(); + try printSourceAtAddress(&di, writer, showMyTrace(), detectTTYConfig()); +} + +noinline fn showMyTrace() usize { + return @returnAddress(); +} -- cgit v1.2.3