From be09d1ec7abd71f4e52f05942a941d76b6d420f2 Mon Sep 17 00:00:00 2001 From: Gabriel Schneider Date: Mon, 28 Sep 2026 16:26:08 -0300 Subject: Post chain review fixes: glslc on stdin, level A at the display's refresh Stage 9 review: a user's Shadertoy file goes to glslc on its stdin, so no source file is written anywhere (the old predictable name under XDG_RUNTIME_DIR or /tmp was a symlink-clobber hazard). std's spawn does all its allocation before fork and searches PATH on the stack after it, said at the call. Level A redraws are paced by the display's refresh rate (SDL_GetCurrentDisplayMode), not a fixed 16 ms. Shared files touched: gui.zig. --- src/gui/Post.zig | 83 ++++++++++++++++++++++++++++++++++---------------------- src/gui/gui.zig | 21 ++++++++++++-- 2 files changed, 68 insertions(+), 36 deletions(-) (limited to 'src/gui') diff --git a/src/gui/Post.zig b/src/gui/Post.zig index d92623d7..c529efc0 100644 --- a/src/gui/Post.zig +++ b/src/gui/Post.zig @@ -15,7 +15,6 @@ const libc = std.c; const ghostty_vt = @import("ghostty-vt"); const pardes = @import("../pardes.zig"); const filesystem = @import("../fs.zig"); -const host_io = @import("../host_io.zig"); const gui = @import("gui.zig"); const c = gui.c; @@ -191,8 +190,9 @@ fn collect(post: *Post, gpa: std.mem.Allocator, device: *c.SDL_GPUDevice, format } } -/// The compile thread: each file behind the prefix, through glslc exactly as -/// the build calls it (build.zig compileGlsl), its SPIR-V on stdout. +/// The compile thread: each file behind the prefix, through glslc with the +/// build's own flags (build.zig compileGlsl), source on stdin, SPIR-V on +/// stdout. fn compile(job: *Job) void { defer job.done.store(true, .release); defer { @@ -200,10 +200,10 @@ fn compile(job: *Job) void { event.type = c.SDL_EVENT_USER; _ = c.SDL_PushEvent(&event); } - for (job.paths[0..job.len], 0..) |path, i| job.results[i] = compileOne(job.gpa, job.io, path, i); + for (job.paths[0..job.len], 0..) |path, i| job.results[i] = compileOne(job.gpa, job.io, path); } -fn compileOne(gpa: std.mem.Allocator, io: std.Io, path: []const u8, index: usize) Job.Result { +fn compileOne(gpa: std.mem.Allocator, io: std.Io, path: []const u8) Job.Result { var home_buf: [4096]u8 = undefined; const expanded = if (std.mem.startsWith(u8, path, "~/")) if (libc.getenv("HOME")) |home| std.fmt.bufPrint(&home_buf, "{s}/{s}", .{ std.mem.span(home), path[2..] }) catch path @@ -212,37 +212,54 @@ fn compileOne(gpa: std.mem.Allocator, io: std.Io, path: []const u8, index: usize const body = filesystem.readFile(gpa, expanded) catch |err| return .{ .failed = std.fmt.allocPrint(gpa, "cannot read it ({t})", .{err}) catch return .none }; defer gpa.free(body); - const dir = if (libc.getenv("XDG_RUNTIME_DIR")) |d| std.mem.span(d) else "/tmp"; - var source_buf: [4096]u8 = undefined; - const source = std.fmt.bufPrintSentinel(&source_buf, "{s}/pardes-post-{d}-{d}.frag.glsl", .{ dir, libc.getpid(), index }, 0) catch return .none; - defer _ = libc.unlink(source); - { - const fd = libc.open(source, .{ .ACCMODE = .WRONLY, .CREAT = true, .TRUNC = true }, @as(libc.mode_t, 0o600)); - if (fd < 0) return .{ .failed = gpa.dupe(u8, "cannot write its source for glslc") catch return .none }; - defer _ = libc.close(fd); - if (!host_io.writeFd(fd, prefix) or !host_io.writeFd(fd, body)) - return .{ .failed = gpa.dupe(u8, "cannot write its source for glslc") catch return .none }; - } - const run = std.process.run(gpa, io, .{ - .argv = &.{ "glslc", "-fshader-stage=fragment", source, "-o", "-" }, - .stdout_limit = .limited(16 << 20), - .stderr_limit = .limited(64 << 10), + // The prefix and the file go to glslc on its stdin: no file of ours is + // written anywhere. std's spawn does every allocation before fork and + // searches PATH on the stack after it (Io/Threaded.zig spawnPosix, + // posixExecv), so a child forked from this thread cannot wedge on a + // lock another thread held (the forkShell hazard). + var child = std.process.spawn(io, .{ + .argv = &.{ "glslc", "-fshader-stage=fragment", "-o", "-", "-" }, + .stdin = .pipe, + .stdout = .pipe, + .stderr = .pipe, }) catch |err| return switch (err) { error.FileNotFound => .missing, else => .{ .failed = std.fmt.allocPrint(gpa, "glslc did not run ({t})", .{err}) catch return .none }, }; - const clean = switch (run.term) { + defer child.kill(io); + // glslc reads all of its input before it writes a byte, so writing it + // first cannot deadlock against a full output pipe. + { + var stdin = child.stdin.?; + child.stdin = null; + defer stdin.close(io); + var buf: [4096]u8 = undefined; + var writer = stdin.writer(io, &buf); + writer.interface.writeAll(prefix) catch {}; + writer.interface.writeAll(body) catch {}; + writer.interface.flush() catch {}; + } + var streams: std.Io.File.MultiReader.Buffer(2) = undefined; + var reader: std.Io.File.MultiReader = undefined; + reader.init(gpa, io, streams.toStreams(), &.{ child.stdout.?, child.stderr.? }); + defer reader.deinit(); + while (reader.fill(64, .none)) |_| { + if (reader.reader(0).buffered().len > 16 << 20 or reader.reader(1).buffered().len > 64 << 10) + return .{ .failed = gpa.dupe(u8, "glslc said too much") catch return .none }; + } else |err| switch (err) { + error.EndOfStream => {}, + else => return .{ .failed = gpa.dupe(u8, "reading glslc failed") catch return .none }, + } + const term = child.wait(io) catch return .{ .failed = gpa.dupe(u8, "glslc did not finish") catch return .none }; + const clean = switch (term) { .exited => |code| code == 0, else => false, }; - if (clean) { - gpa.free(run.stderr); - return .{ .spirv = run.stdout }; - } - gpa.free(run.stdout); - // glslc names the temporary source; the file is what a person knows. - defer gpa.free(run.stderr); - const named = std.mem.replaceOwned(u8, gpa, run.stderr, source, path) catch return .{ .failed = gpa.dupe(u8, "glslc failed") catch return .none }; + if (clean) return .{ .spirv = reader.toOwnedSlice(0) catch return .none }; + const said = reader.toOwnedSlice(1) catch return .none; + // glslc calls its input ; the file is what a person knows. + defer gpa.free(said); + const named = std.mem.replaceOwned(u8, gpa, said, "", path) catch return .{ .failed = gpa.dupe(u8, "glslc failed") catch return .none }; return .{ .failed = named }; } @@ -535,9 +552,9 @@ test "ghostty's test shaders compile behind the prefix, and its invalid one fail if (std.mem.startsWith(u8, entry.name, "ghostty-")) break entry.name; } else return error.SkipZigTest; var buf: [512]u8 = undefined; - for ([_][]const u8{ "crt", "focus", "invalid" }, 0..) |name, i| { + for ([_][]const u8{ "crt", "focus", "invalid" }) |name| { const path = try std.fmt.bufPrint(&buf, "zig-pkg/{s}/src/renderer/shaders/test_shadertoy_{s}.glsl", .{ ghostty, name }); - const result = compileOne(gpa, io, path, i); + const result = compileOne(gpa, io, path); switch (result) { .missing => return error.SkipZigTest, // no glslc here .spirv => |spirv| { @@ -549,9 +566,9 @@ test "ghostty's test shaders compile behind the prefix, and its invalid one fail .failed => |text| { defer gpa.free(text); try std.testing.expectEqualStrings("invalid", name); - // Named by the file, not by the temporary source glslc read. + // Named by the file, not by glslc's . try std.testing.expect(std.mem.indexOf(u8, text, path) != null); - try std.testing.expect(std.mem.indexOf(u8, text, "pardes-post-") == null); + try std.testing.expect(std.mem.indexOf(u8, text, "") == null); }, .none => return error.TestUnexpectedResult, } diff --git a/src/gui/gui.zig b/src/gui/gui.zig index ab1875b7..aa39cf32 100644 --- a/src/gui/gui.zig +++ b/src/gui/gui.zig @@ -4086,7 +4086,12 @@ fn waitInput(ctx: ?*anyopaque, timeout_ms: u32) void { const minimized = !s.test_mode and c.SDL_GetWindowFlags(g.window) & (c.SDL_WINDOW_MINIMIZED | c.SDL_WINDOW_OCCLUDED) != 0; // A latency trace is fed on stdin, whose poll below paces the loop: // waiting here too would make every tick cost two frames. - const ms: c_int = if (latency_fd >= 0) 0 else if (minimized) -1 else if (timeout_ms != 0) @intCast(timeout_ms) else if (polls) 16 else -1; + // A chain that moves on its own wakes once a display refresh. + const poll_ms: c_int = if (g.post.animating(core.settings.shader_animation)) + @intCast(@max(1, refreshNs(g) / std.time.ns_per_ms)) + else + 16; + const ms: c_int = if (latency_fd >= 0) 0 else if (minimized) -1 else if (timeout_ms != 0) @min(@as(c_int, @intCast(@min(timeout_ms, std.math.maxInt(c_int)))), if (polls) poll_ms else std.math.maxInt(c_int)) else if (polls) poll_ms else -1; // The wait is the 9P connections' turn with the core. pardes.turn.rest(); const got = c.SDL_WaitEventTimeout(&sev, ms); @@ -4168,9 +4173,19 @@ fn pollFrame(ctx: ?*anyopaque) void { if (updateCoreResize(core, geom.cols, geom.rows, g.cell_w, g.cell_h, g.tagline_width, g.tagline_height)) resetScroll(g); stepScroll(g, core, s.gpa); g.post.sync(s.gpa, s.io, g.device, g.swapchain_format, core); - // Between the core's frames, a chain that moves on its own redraws alone. + // Between the core's frames, a chain that moves on its own redraws + // alone, once a refresh of the display the window is on (a millisecond + // early is on time: the wait that paces it is in whole milliseconds). if (g.post.animating(core.settings.shader_animation) and !core.needs_frame and s.presented and - c.SDL_GetTicksNS() -| g.post.last_ns >= pardes.animation.frame_ns) redrawPost(g, s.gpa); + c.SDL_GetTicksNS() -| g.post.last_ns + std.time.ns_per_ms >= refreshNs(g)) redrawPost(g, s.gpa); +} + +/// One refresh of the display the window is on, in ns; a frame of the +/// core's where the display does not say. +fn refreshNs(g: *const Gui) u64 { + const mode = c.SDL_GetCurrentDisplayMode(c.SDL_GetDisplayForWindow(g.window)) orelse return pardes.animation.frame_ns; + if (!(mode.*.refresh_rate > 1)) return pardes.animation.frame_ns; + return @intFromFloat(@as(f64, std.time.ns_per_s) / mode.*.refresh_rate); } fn finishWindowOpacity(core: *pardes.Pardes, applied: *u8, failure: ?[]const u8) void { -- cgit v1.3