Full unit test suite passing Valgrind (#8319)

This contains the various changes necessary to get the full unit test
suite passing Valgrind, and configures CI to run this.

I disabled relatively few (less than 10) tests under Valgrind because
they're way too slow: all `verifyIntegrity` tests, because those run
anyways in debug and check their own memory health, a font test that
fills out font map, and the sprite render test. Everything else runs
as-is.

I found a number of issues, most were in the tests themselves. A couple
in actual code. A funny one was some undefined memory on tabstop resize
if you exceed the default number of tabstops. I don't know any real
world program that ever even did that (memory issue aside), and that
whole file hasn't been touched since 2022, so that was funny.

No memory leaks in actual code, but a number of leaks in tests. All
resolved.

I think we're still missing some reports because of the Zig bug:
https://github.com/ziglang/zig/issues/19148 so I'm gong to audit our
codebase after this and look for cases of that.

.github/workflows/test.yml

@@ -1072,9 +1072,6 @@ jobs:
           sudo apt update -y
           sudo apt install -y valgrind libc6-dbg
 
-      # Currently, the entire Ghostty test suite does not pass Valgrind.
-      # As we incrementally add areas that pass, we'll add more filters here.
-      # Ultimately, we'll remove the filter and run the full suite.
       - name: valgrind
         run: |
-          nix develop -c zig build test-valgrind -Dtest-filter="OSC"
+          nix develop -c zig build test-valgrind

src/font/Collection.zig

@@ -937,6 +937,9 @@ test init {
 }
 
 test "add full" {
+    // This test is way too slow to run under Valgrind, unfortunately.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     const testing = std.testing;
     const alloc = testing.allocator;
     const testFont = font.embedded.regular;

src/font/DeferredFace.zig

@@ -413,6 +413,7 @@ test "fontconfig" {
     // Get a deferred face from fontconfig
     var def = def: {
         var fc = discovery.Fontconfig.init();
+        defer fc.deinit();
         var it = try fc.discover(alloc, .{ .family = "monospace", .size = 12 });
         defer it.deinit();
         break :def (try it.next()).?;

src/font/discovery.zig

@@ -897,6 +897,7 @@ test "fontconfig" {
     const alloc = testing.allocator;
 
     var fc = Fontconfig.init();
+    defer fc.deinit();
     var it = try fc.discover(alloc, .{ .family = "monospace", .size = 12 });
     defer it.deinit();
 }
@@ -908,12 +909,14 @@ test "fontconfig codepoint" {
     const alloc = testing.allocator;
 
     var fc = Fontconfig.init();
+    defer fc.deinit();
     var it = try fc.discover(alloc, .{ .codepoint = 'A', .size = 12 });
     defer it.deinit();
 
     // The first result should have the codepoint. Later ones may not
     // because fontconfig returns all fonts sorted.
-    const face = (try it.next()).?;
+    var face = (try it.next()).?;
+    defer face.deinit();
     try testing.expect(face.hasCodepoint('A', null));
 
     // Should have other codepoints too

src/font/sprite/Face.zig

@@ -511,6 +511,9 @@ fn testDrawRanges(
 }
 
 test "sprite face render all sprites" {
+    // This test is way too slow to run under Valgrind, unfortunately.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     // Renders all sprites to an atlas and compares
     // it to a ground truth for regression testing.
 

src/terminal/PageList.zig

@@ -2395,8 +2395,8 @@ pub fn eraseRows(
                 break;
             }
 
-            self.erasePage(chunk.node);
             erased += chunk.node.data.size.rows;
+            self.erasePage(chunk.node);
             continue;
         }
 

src/terminal/Screen.zig

@@ -233,6 +233,11 @@ pub fn deinit(self: *Screen) void {
 /// ensure they're also calling page integrity checks if necessary.
 pub fn assertIntegrity(self: *const Screen) void {
     if (build_config.slow_runtime_safety) {
+        // We don't run integrity checks on Valgrind because its soooooo slow,
+        // Valgrind is our integrity checker, and we run these during unit
+        // tests (non-Valgrind) anyways so we're verifying anyways.
+        if (std.valgrind.runningOnValgrind() > 0) return;
+
         assert(self.cursor.x < self.pages.cols);
         assert(self.cursor.y < self.pages.rows);
 

src/terminal/Tabstops.zig

@@ -129,6 +129,7 @@ pub fn resize(self: *Tabstops, alloc: Allocator, cols: usize) !void {
 
     // Note: we can probably try to realloc here but I'm not sure it matters.
     const new = try alloc.alloc(Unit, size);
+    @memset(new, 0);
     if (self.dynamic_stops.len > 0) {
         fastmem.copy(Unit, new, self.dynamic_stops);
         alloc.free(self.dynamic_stops);

src/terminal/Terminal.zig

@@ -2824,14 +2824,21 @@ test "Terminal: input glitch text" {
     var t = try init(alloc, .{ .cols = 30, .rows = 30 });
     defer t.deinit(alloc);
 
-    const page = t.screen.pages.pages.first.?;
-    const grapheme_cap = page.data.capacity.grapheme_bytes;
+    // Get our initial grapheme capacity.
+    const grapheme_cap = cap: {
+        const page = t.screen.pages.pages.first.?;
+        break :cap page.data.capacity.grapheme_bytes;
+    };
 
-    while (page.data.capacity.grapheme_bytes == grapheme_cap) {
+    // Print glitch text until our capacity changes
+    while (true) {
+        const page = t.screen.pages.pages.first.?;
+        if (page.data.capacity.grapheme_bytes != grapheme_cap) break;
         try t.printString(glitch);
     }
 
     // We're testing to make sure that grapheme capacity gets increased.
+    const page = t.screen.pages.pages.first.?;
     try testing.expect(page.data.capacity.grapheme_bytes > grapheme_cap);
 }
 

src/terminal/page.zig

@@ -346,6 +346,11 @@ pub const Page = struct {
         //   used for the same reason as styles above.
         //
 
+        // We don't run integrity checks on Valgrind because its soooooo slow,
+        // Valgrind is our integrity checker, and we run these during unit
+        // tests (non-Valgrind) anyways so we're verifying anyways.
+        if (std.valgrind.runningOnValgrind() > 0) return;
+
         if (build_config.slow_runtime_safety) {
             if (self.pause_integrity_checks > 0) return;
         }
@@ -2038,10 +2043,13 @@ pub const Cell = packed struct(u64) {
 
     /// Helper to make a cell that just has a codepoint.
     pub fn init(cp: u21) Cell {
-        return .{
-            .content_tag = .codepoint,
-            .content = .{ .codepoint = cp },
-        };
+        // We have to use this bitCast here to ensure that our memory is
+        // zeroed. Otherwise, the content below will leave some uninitialized
+        // memory in the packed union. Valgrind verifies this.
+        var cell: Cell = @bitCast(@as(u64, 0));
+        cell.content_tag = .codepoint;
+        cell.content = .{ .codepoint = cp };
+        return cell;
     }
 
     pub fn isZero(self: Cell) bool {
@@ -3034,6 +3042,10 @@ test "Page moveCells graphemes" {
 }
 
 test "Page verifyIntegrity graphemes good" {
+    // Too slow, and not really necessary because the integrity tests are
+    // only run in debug builds and unit tests verify they work well enough.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     var page = try Page.init(.{
         .cols = 10,
         .rows = 10,
@@ -3055,6 +3067,10 @@ test "Page verifyIntegrity graphemes good" {
 }
 
 test "Page verifyIntegrity grapheme row not marked" {
+    // Too slow, and not really necessary because the integrity tests are
+    // only run in debug builds and unit tests verify they work well enough.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     var page = try Page.init(.{
         .cols = 10,
         .rows = 10,
@@ -3082,6 +3098,10 @@ test "Page verifyIntegrity grapheme row not marked" {
 }
 
 test "Page verifyIntegrity styles good" {
+    // Too slow, and not really necessary because the integrity tests are
+    // only run in debug builds and unit tests verify they work well enough.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     var page = try Page.init(.{
         .cols = 10,
         .rows = 10,
@@ -3114,6 +3134,10 @@ test "Page verifyIntegrity styles good" {
 }
 
 test "Page verifyIntegrity styles ref count mismatch" {
+    // Too slow, and not really necessary because the integrity tests are
+    // only run in debug builds and unit tests verify they work well enough.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     var page = try Page.init(.{
         .cols = 10,
         .rows = 10,
@@ -3152,6 +3176,10 @@ test "Page verifyIntegrity styles ref count mismatch" {
 }
 
 test "Page verifyIntegrity zero rows" {
+    // Too slow, and not really necessary because the integrity tests are
+    // only run in debug builds and unit tests verify they work well enough.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     var page = try Page.init(.{
         .cols = 10,
         .rows = 10,
@@ -3166,6 +3194,10 @@ test "Page verifyIntegrity zero rows" {
 }
 
 test "Page verifyIntegrity zero cols" {
+    // Too slow, and not really necessary because the integrity tests are
+    // only run in debug builds and unit tests verify they work well enough.
+    if (std.valgrind.runningOnValgrind() > 0) return error.SkipZigTest;
+
     var page = try Page.init(.{
         .cols = 10,
         .rows = 10,

src/terminal/search.zig

@@ -454,6 +454,11 @@ const SlidingWindow = struct {
     fn assertIntegrity(self: *const SlidingWindow) void {
         if (comptime !std.debug.runtime_safety) return;
 
+        // We don't run integrity checks on Valgrind because its soooooo slow,
+        // Valgrind is our integrity checker, and we run these during unit
+        // tests (non-Valgrind) anyways so we're verifying anyways.
+        if (std.valgrind.runningOnValgrind() > 0) return;
+
         // Integrity check: verify our data matches our metadata exactly.
         var meta_it = self.meta.iterator(.forward);
         var data_len: usize = 0;

valgrind.supp

@@ -574,6 +574,18 @@
    ...
 }
 
+{
+   pango language
+   Memcheck:Leak
+   match-leak-kinds: possible
+   fun:calloc
+   fun:g_malloc0
+   fun:pango_language_from_string
+   fun:pango_language_get_default
+   fun:gtk_get_locale_direction
+   fun:gtk_init_check
+   ...
+}
 {
    Adwaita Stylesheet Load
    Memcheck:Leak