From 9d08ed32eeb77bf62309cc15b81483fb9881959e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 31 Aug 2024 09:47:05 -0700 Subject: [PATCH] terminal: make error sets more explicit, capture explicit errors --- src/terminal/PageList.zig | 24 ++++++++++----- src/terminal/Screen.zig | 2 +- src/terminal/Terminal.zig | 63 +++++++++++++++++++++++++++++++-------- src/terminal/page.zig | 7 +++-- 4 files changed, 73 insertions(+), 23 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index c0ff5a548..4cbe8f2ef 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1758,6 +1758,8 @@ pub const AdjustCapacity = struct { string_bytes: ?usize = null, }; +pub const AdjustCapacityError = Allocator.Error || Page.CloneFromError; + /// Adjust the capcaity of the given page in the list. This should /// be used in cases where OutOfMemory is returned by some operation /// i.e to increase style counts, grapheme counts, etc. @@ -1778,25 +1780,31 @@ pub fn adjustCapacity( self: *PageList, page: *List.Node, adjustment: AdjustCapacity, -) !*List.Node { +) AdjustCapacityError!*List.Node { // We always start with the base capacity of the existing page. This // ensures we never shrink from what we need. var cap = page.data.capacity; + // All ceilPowerOfTwo is unreachable because we're always same or less + // bit width so maxInt is always possible. if (adjustment.styles) |v| { - const aligned = try std.math.ceilPowerOfTwo(usize, v); + comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); + const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; cap.styles = @max(cap.styles, aligned); } if (adjustment.grapheme_bytes) |v| { - const aligned = try std.math.ceilPowerOfTwo(usize, v); + comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); + const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; cap.grapheme_bytes = @max(cap.grapheme_bytes, aligned); } if (adjustment.hyperlink_bytes) |v| { - const aligned = try std.math.ceilPowerOfTwo(usize, v); + comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); + const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; cap.hyperlink_bytes = @max(cap.hyperlink_bytes, aligned); } if (adjustment.string_bytes) |v| { - const aligned = try std.math.ceilPowerOfTwo(usize, v); + comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); + const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; cap.string_bytes = @max(cap.string_bytes, aligned); } @@ -1830,7 +1838,7 @@ pub fn adjustCapacity( fn createPage( self: *PageList, cap: Capacity, -) !*List.Node { +) Allocator.Error!*List.Node { // log.debug("create page cap={}", .{cap}); return try createPageExt(&self.pool, cap, &self.page_size); } @@ -1839,7 +1847,7 @@ fn createPageExt( pool: *MemoryPool, cap: Capacity, total_size: ?*usize, -) !*List.Node { +) Allocator.Error!*List.Node { var page = try pool.nodes.create(); errdefer pool.nodes.destroy(page); @@ -2292,7 +2300,7 @@ pub fn pin(self: *const PageList, pt: point.Point) ?Pin { /// automatically updated as the pagelist is modified. If the point the /// pin points to is removed completely, the tracked pin will be updated /// to the top-left of the screen. -pub fn trackPin(self: *PageList, p: Pin) !*Pin { +pub fn trackPin(self: *PageList, p: Pin) Allocator.Error!*Pin { if (comptime std.debug.runtime_safety) assert(self.pinIsValid(p)); // Create our tracked pin diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index ad7511982..bb0374e8e 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -437,7 +437,7 @@ pub fn adjustCapacity( self: *Screen, page: *PageList.List.Node, adjustment: PageList.AdjustCapacity, -) !*PageList.List.Node { +) PageList.AdjustCapacityError!*PageList.List.Node { // If the page being modified isn't our cursor page then // this is a quick operation because we have no additional // accounting. diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 363650ef2..0479846dc 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1444,6 +1444,7 @@ pub fn insertLines(self: *Terminal, count: usize) void { const start_y = self.screen.cursor.y; defer { self.screen.cursorAbsolute(self.scrolling_region.left, start_y); + // Always unset pending wrap self.screen.cursor.pending_wrap = false; } @@ -1464,8 +1465,15 @@ pub fn insertLines(self: *Terminal, count: usize) void { var cur_p = self.screen.pages.trackPin( self.screen.cursor.page_pin.down(rem - 1).?, ) catch |err| { - std.log.err("TODO: insertLines handle trackPin error err={}", .{err}); - @panic("TODO: insertLines handle trackPin error"); + comptime assert(@TypeOf(err) == error{OutOfMemory}); + + // This error scenario means that our GPA is OOM. This is not a + // situation we can gracefully handle. We can't just ignore insertLines + // because it'll result in a corrupted screen. Ideally in the future + // we flag the state as broken and show an error message to the user. + // For now, we panic. + log.err("insertLines trackPin error err={}", .{err}); + @panic("insertLines trackPin OOM"); }; defer self.screen.pages.untrackPin(cur_p); @@ -1525,7 +1533,7 @@ pub fn insertLines(self: *Terminal, count: usize) void { // Increase style memory error.StyleSetOutOfMemory, - => .{.styles = cap.styles * 2 }, + => .{ .styles = cap.styles * 2 }, // Increase string memory error.StringAllocOutOfMemory, @@ -1541,10 +1549,27 @@ pub fn insertLines(self: *Terminal, count: usize) void { error.GraphemeAllocOutOfMemory, => .{ .grapheme_bytes = cap.grapheme_bytes * 2 }, }, - ) catch |e| { - std.log.err("TODO: insertLines handle adjustCapacity error err={}", .{e}); - @panic("TODO: insertLines handle adjustCapacity error"); + ) catch |e| switch (e) { + // This shouldn't be possible because above we're only + // adjusting capacity _upwards_. So it should have all + // the existing capacity it had to fit the adjusted + // data. Panic since we don't expect this. + error.StyleSetOutOfMemory, + error.StyleSetNeedsRehash, + error.StringAllocOutOfMemory, + error.HyperlinkSetOutOfMemory, + error.HyperlinkSetNeedsRehash, + error.HyperlinkMapOutOfMemory, + error.GraphemeMapOutOfMemory, + error.GraphemeAllocOutOfMemory, + => @panic("adjustCapacity resulted in capacity errors"), + + // The system allocator is OOM. We can't currently do + // anything graceful here. We panic. + error.OutOfMemory, + => @panic("adjustCapacity system allocator OOM"), }; + // Continue the loop to try handling this row again. continue; }; @@ -1643,8 +1668,10 @@ pub fn deleteLines(self: *Terminal, count: usize) void { var cur_p = self.screen.pages.trackPin( self.screen.cursor.page_pin.*, ) catch |err| { - std.log.err("TODO: deleteLines handle trackPin error err={}", .{err}); - @panic("TODO: deleteLines handle trackPin error"); + // See insertLines + comptime assert(@TypeOf(err) == error{OutOfMemory}); + log.err("deleteLines trackPin error err={}", .{err}); + @panic("deleteLines trackPin OOM"); }; defer self.screen.pages.untrackPin(cur_p); @@ -1704,7 +1731,7 @@ pub fn deleteLines(self: *Terminal, count: usize) void { // Increase style memory error.StyleSetOutOfMemory, - => .{.styles = cap.styles * 2 }, + => .{ .styles = cap.styles * 2 }, // Increase string memory error.StringAllocOutOfMemory, @@ -1720,10 +1747,22 @@ pub fn deleteLines(self: *Terminal, count: usize) void { error.GraphemeAllocOutOfMemory, => .{ .grapheme_bytes = cap.grapheme_bytes * 2 }, }, - ) catch |e| { - std.log.err("TODO: deleteLines handle adjustCapacity error err={}", .{e}); - @panic("TODO: deleteLines handle adjustCapacity error"); + ) catch |e| switch (e) { + // See insertLines which has the same error capture. + error.StyleSetOutOfMemory, + error.StyleSetNeedsRehash, + error.StringAllocOutOfMemory, + error.HyperlinkSetOutOfMemory, + error.HyperlinkSetNeedsRehash, + error.HyperlinkMapOutOfMemory, + error.GraphemeMapOutOfMemory, + error.GraphemeAllocOutOfMemory, + => @panic("adjustCapacity resulted in capacity errors"), + + error.OutOfMemory, + => @panic("adjustCapacity system allocator OOM"), }; + // Continue the loop to try handling this row again. continue; }; diff --git a/src/terminal/page.zig b/src/terminal/page.zig index f4d315cb0..31b973e9b 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -708,7 +708,7 @@ pub const Page = struct { const cells = dst_row.cells.ptr(self.memory)[x_start..x_end]; // If our destination has styles or graphemes then we need to - // clear some state. + // clear some state. This will free up the managed memory as well. if (dst_row.managedMemory()) self.clearCells(dst_row, x_start, x_end); // Copy all the row metadata but keep our cells offset @@ -771,6 +771,8 @@ pub const Page = struct { // Copy the grapheme codepoints const cps = other.lookupGrapheme(src_cell).?; + // Safe to use setGraphemes because we cleared all + // managed memory for our destination cell range. try self.setGraphemes(dst_row, dst_cell, cps); } if (src_cell.hyperlink) hyperlink: { @@ -801,10 +803,11 @@ pub const Page = struct { if (self.hyperlink_set.lookupContext( self.memory, other_link.*, - .{ .page = @constCast(other) }, + // `lookupContext` uses the context for hashing, and // that doesn't write to the page, so this constCast // is completely safe. + .{ .page = @constCast(other) }, )) |i| { self.hyperlink_set.use(self.memory, i); break :dst_id i;