diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/CHANGELOG.md | 24 | ||||
| -rw-r--r-- | src/crash.zig | 134 | ||||
| -rw-r--r-- | src/macos.zig | 2 | ||||
| -rw-r--r-- | src/main.zig | 21 |
4 files changed, 113 insertions, 68 deletions
diff --git a/src/CHANGELOG.md b/src/CHANGELOG.md index 4cef906d..bb7e9ad5 100644 --- a/src/CHANGELOG.md +++ b/src/CHANGELOG.md @@ -15,14 +15,22 @@ fact. `src/crash.zig` runs inside the panic handler, so it takes no lock of this program's, allocates nothing of its own, and every failure is swallowed: a crash file that could not be written must not become the crash, and stderr - still gets its own copy either way. The file gets RETURN ADDRESSES rather - than the symbolised trace, and that is measured rather than chosen — - `writeCurrentStackTrace` called from a panic handler *before* `defaultPanic` - wedges the process at 0% CPU: symbolising reads DWARF, that read can panic, - and the staging which turns a nested panic into "aborting due to recursive - panic" is `defaultPanic`'s own and private. Walking frames is safe, so the - addresses go in the file and `addr2line -e` against the build named on the - line above them finishes the job. The AppKit shell got a panic handler of its + still gets its own copy either way. The file carries NO STACK TRACE, and that + is measured rather than chosen: `writeCurrentStackTrace` called from a panic + handler *before* `defaultPanic` wedges the process at 0% CPU, and + `captureCurrentStackTrace` — which looks like the safe half of it — takes + `SelfInfo`'s rwlock exclusively on its first call, so a panic inside the walk + leaves that lock held and `defaultPanic` waits on it for the life of the + process. A crash that becomes a hang is worse than the crash. What makes + `defaultPanic` itself survive that is its private `panic_stage`, reachable + from nowhere outside `std.debug`, so the frames stay on stderr where they + already work. ONE record per process, because the nested panic std's trace + printer raises comes back through the handler: the first version of this + wrote the real message and then "reached unreachable code" underneath it, + which is the panic handler's own second `vaxis.recover()` double-closing the + tty — `recover()` never cleared the global saying there was one. That call is + guarded now too, in both the panic and the segfault handler, which is a fix + older than this file. The AppKit shell got a panic handler of its own in the process: the macOS build roots at `macos.zig`, so the one in `main.zig` had never run there — in the shell with the least useful stderr of the four. diff --git a/src/crash.zig b/src/crash.zig index 8de2abed..d9c47715 100644 --- a/src/crash.zig +++ b/src/crash.zig @@ -10,9 +10,10 @@ //! that produced it: a bug report needs the version and the commit, and a user //! who runs pardes already knows where its config lives. //! -//! The record is the metadata line, the panic message, and the RETURN -//! ADDRESSES — not the symbolised trace stderr gets. `record` says why in as -//! many words: symbolising from out here hangs the process, measured. +//! The record is the metadata line and the panic message, and NOT the frames: +//! every way of collecting those from out here can wedge the process instead of +//! ending it. `record` says exactly why, at length, because the reasoning is +//! not visible from the call it does not make. //! //! Panics only. A SIGSEGV never reaches a panic handler (main.zig `debug` //! hooks that path separately) and unwinding one needs the signal's saved cpu @@ -56,20 +57,18 @@ else /// the fd is opened `O_APPEND` and one `write(2)` of the lot is what keeps two /// crashing processes from interleaving their records line by line. Static /// rather than a local: a panic handler may be running on a thread stack of a -/// few pages. +/// few pages. Two lines fit several times over — the panic message is the only +/// part with no bound of its own, and a longer one is simply cut. var scratch: [8 << 10]u8 = undefined; -/// Deep enough to reach `main` through any of this program's loops, and the -/// only sizing decision here: the addresses are 8 bytes each and cost a line. -var addr_buf: [64]usize = undefined; - -/// One at a time, and never inside itself. `defaultPanic` gets both properties -/// from a private `panic_stage` and from the stderr lock, and neither is -/// reachable from out here: two threads panicking at once would interleave -/// `@memcpy`s into one `scratch` and each write out the mixture, and a panic -/// raised INSIDE this function — the walk below is the plausible place — would -/// re-enter it with the fd still open. Both end here instead, and the loser -/// falls through to `defaultPanic`, which still says everything on stderr. +/// One record per process, taken by whoever panics first. `defaultPanic` gets +/// the same property from a private `panic_stage` and from the stderr lock, and +/// neither is reachable from out here: two threads panicking at once would +/// interleave `@memcpy`s into one `scratch` and each write out the mixture, and +/// the nested panic `defaultPanic`'s own trace printer can raise comes back +/// through the handler and appends its wreckage under the real message. Both +/// end here instead, and the loser falls through to `defaultPanic`, which still +/// says everything on stderr. var recording: std.atomic.Value(bool) = .init(false); /// Append one crash record, or silently do nothing. Called FROM the panic @@ -77,12 +76,15 @@ var recording: std.atomic.Value(bool) = .init(false); /// own, and does its own file I/O: a static buffer, one `open`, one `write`. /// Every failure is swallowed — a crash file that could not be written must not /// become the crash, and stderr is about to get the message either way. -pub fn record(msg: []const u8, first_trace_addr: ?usize) void { +pub fn record(msg: []const u8) void { if (dir_len == 0) return; + // ONCE PER PROCESS, and never released. A process panics once and dies; a + // SECOND record is always the wreckage of the first. Observed with the + // flag released on the way out: `defaultPanic`'s own trace printing raised + // a nested panic, which came back through the handler and appended + // "panic: reached unreachable code" under the real message — burying the + // one line the file exists to keep. A test resets it deliberately. if (recording.swap(true, .seq_cst)) return; - // Released on the way out, which matters to nobody at panic time — the - // process is about to abort — and lets a test call this twice. - defer recording.store(false, .seq_cst); const config_dir = dir_buf[0..dir_len]; var path_buf: [1024:0]u8 = undefined; if (config_dir.len + 1 + name.len >= path_buf.len) return; @@ -113,8 +115,14 @@ pub fn record(msg: []const u8, first_trace_addr: ?usize) void { // The metadata line. Same clock and the same civil-time arithmetic as // dump.zig's filename, printed as UTC because a crash file is read by // whoever the reporter sends it to and their zone is not the reporter's. - var ts: std.c.timespec = undefined; - _ = std.c.clock_gettime(.REALTIME, &ts); + // + // The return IS checked, unlike dump.zig's, because this one runs where a + // panic cannot be afforded: a failed `clock_gettime` leaves `ts` undefined, + // and an undefined large-positive `sec` walks `calculateYearDay`'s `u16` + // year past 65535 and overflow-panics INSIDE the panic handler. Debug fills + // it with 0xaa and lands in 1970, which is why it reads as harmless. + var ts: std.c.timespec = .{ .sec = 0, .nsec = 0 }; + if (std.c.clock_gettime(.REALTIME, &ts) != 0) ts = .{ .sec = 0, .nsec = 0 }; const es: std.time.epoch.EpochSeconds = .{ .secs = @intCast(@max(0, ts.sec)) }; const yd = es.getEpochDay().calculateYearDay(); const md = yd.calculateMonthDay(); @@ -134,34 +142,43 @@ pub fn record(msg: []const u8, first_trace_addr: ?usize) void { @as(u32, @intCast(std.c.getpid())), }) catch {}; - // ...and under it the message stderr is about to print, then the return - // addresses behind it. + // ...and under it the line stderr is about to print. + // + // NO STACK TRACE, AND NOT EVEN THE RAW RETURN ADDRESSES. This is the whole + // design decision in this file, and it was arrived at by measurement, so it + // is written down here rather than left to be rediscovered. + // + // `std.debug.writeCurrentStackTrace` — the call `defaultPanic` makes a + // moment later, into a writer of its own — HANGS THE PROCESS when it is + // made from a panic handler BEFORE `defaultPanic` has run. Observed at 0% + // CPU in `futex_do_wait` with libc's debug info half-open, identically in a + // test binary and in a standalone build carrying this program's + // `std_options_debug_io`. Calling `debug_io.vtable.crashHandler` first does + // not help, and neither does holding `lockStderr` around it. // - // ADDRESSES AND NOT THE SYMBOLISED TRACE, which is the one thing here that - // is not simply "the same bytes stderr gets". `std.debug.writeCurrentStack - // Trace` — the call `defaultPanic` makes a moment later, into a writer of - // its own — HANGS THE PROCESS when it is made from a panic handler before - // `defaultPanic` has run. Symbolising reads DWARF, and on a machine with - // debug info to fetch that read can itself panic; `defaultPanic` survives - // that because its private `panic_stage` turns the nested panic into - // "aborting due to recursive panic" and an abort. Outside it there is no - // such staging, and the recursion ends in a futex nobody will post: a - // panicking pardes stopped dead instead of dying. Measured, not guessed — - // with the stack-trace call the process wedged at 0% CPU with libc's debug - // info half-open, and it did it identically inside a test binary and in a - // standalone build with this program's `std_options_debug_io`. + // `captureCurrentStackTrace` looks like the safe half of that and is NOT. + // `StackIterator.init` picks the `.di` strategy whenever `SelfInfo` can + // unwind, `stratOk` accepts `.di` no matter what `allow_unsafe_unwind` + // says, and `.di` unwinding takes `SelfInfo`'s rwlock EXCLUSIVELY on its + // first call — the only kind of call a panic record ever makes — while it + // runs `dl_iterate_phdr` and a DWARF CFI machine and allocates. A panic in + // there (a smashed stack is a leading cause of getting here at all) leaves + // that lock held forever, because the unlock is a `defer` in a frame that + // never returns. `defaultPanic` then asks for the same lock and waits on it + // for the life of the process: A CRASH BECOMES A HANG, which is strictly + // worse than the behaviour this file was added to improve on. What makes + // `defaultPanic` itself survive that is its own PRIVATE `panic_stage`, + // reachable from nowhere out here. // - // `captureCurrentStackTrace` only walks frames, so it is safe here, and the - // addresses resolve with `addr2line -e <the pardes binary>` against the - // build the line above names. stderr still gets the symbolised trace from - // `defaultPanic`, unchanged. + // So the frames stay on stderr, where `defaultPanic` prints them under the + // staging that makes them safe, and this file keeps what it can gather + // without asking the process any questions: which build, when, where, and + // what it said. That is the half a reporter cannot reconstruct afterwards. w.print("panic: {s}\n", .{msg}) catch {}; - const trace = std.debug.captureCurrentStackTrace(.{ - .first_address = first_trace_addr orelse @returnAddress(), - .allow_unsafe_unwind = true, - }, &addr_buf); - for (trace.return_addresses) |a| w.print(" 0x{x}\n", .{a}) catch {}; + // One `write(2)`, and the loop is for a short write rather than for a + // second record: `O_APPEND` puts it at the end whatever else is writing, + // and nothing above here can fail in a way that leaves it half-built. const written = w.buffered(); var off: usize = 0; while (off < written.len) { @@ -187,8 +204,12 @@ test "a crash record names the build, appends, and carries the panic message" { defer dir_len = saved_len; setDir(config_dir); - record("first boom", null); - record("second boom", null); + record("first boom"); + // Deliberately released, which the panic path never does — see `recording`. + // Two records is how the APPEND is proved, and this is the only caller + // allowed to ask for a second one. + recording.store(false, .seq_cst); + record("second boom"); const path = try std.fs.path.join(gpa, &.{ config_dir, name }); defer gpa.free(path); @@ -207,19 +228,20 @@ test "a crash record names the build, appends, and carries the panic message" { try std.testing.expect(stamp < second); try std.testing.expect(std.mem.indexOf(u8, bytes, "1970-") == null); - // ...and at least one return address under the FIRST message, which is the - // only part of this that can quietly produce nothing: an unwinder that - // refuses the strategy hands back an empty trace and the loop writes no - // lines at all. Asserted between the two records so it cannot be satisfied - // by the second one's. - const first_msg = std.mem.indexOf(u8, bytes, "panic: first boom\n").?; - const addr = std.mem.indexOfPos(u8, bytes, first_msg, "\n 0x").?; - try std.testing.expect(addr < second); + // Two lines per record and no third: the frames deliberately do NOT come + // here (see `record`), and a future edit that starts collecting them again + // is the thing this pins. Counted rather than pattern-matched, because the + // failure being guarded is "something extra appeared", which no assertion + // about known content can see. + // Three newlines a record: the blank one that separates records, the end of + // the metadata line, and the end of the `panic:` line. + try std.testing.expectEqual(@as(usize, 6), std.mem.count(u8, bytes, "\n")); // No directory, no file: a core that never resolved a config directory // panics exactly as it did before this existed. dir_len = 0; - record("unrecorded", null); + recording.store(false, .seq_cst); + record("unrecorded"); const again = try std.Io.Dir.cwd().readFileAlloc(io, path, gpa, .limited(1 << 20)); defer gpa.free(again); try std.testing.expectEqual(bytes.len, again.len); diff --git a/src/macos.zig b/src/macos.zig index a9c9d0f3..f2e68a85 100644 --- a/src/macos.zig +++ b/src/macos.zig @@ -50,7 +50,7 @@ const crash = @import("crash.zig"); /// terminal to restore either, which is the rest of what main.zig's does. pub const panic = std.debug.FullPanic(struct { fn call(msg: []const u8, ret_addr: ?usize) noreturn { - crash.record(msg, ret_addr); + crash.record(msg); std.debug.defaultPanic(msg, ret_addr); } }.call); diff --git a/src/main.zig b/src/main.zig index 60491052..92b59bc2 100644 --- a/src/main.zig +++ b/src/main.zig @@ -53,18 +53,33 @@ fn logFn( // exactly the behaviour that was here before. pub const panic = if (is_emscripten) std.debug.FullPanic(std.debug.defaultPanic) else std.debug.FullPanic(struct { fn call(msg: []const u8, ret_addr: ?usize) noreturn { - @import("vaxis").recover(); - @import("crash.zig").record(msg, ret_addr); + // ONCE. This handler is re-entered whenever something panics while it + // runs, and std's own trace printer does exactly that — `defaultPanic` + // survives its own recursion through a private `panic_stage`, and + // everything up here is in front of that guard. `recover()` is not + // idempotent: it closes the vaxis tty and never clears the global that + // says there is one, so a second call double-closes, which std answers + // with `recoverableOsBugDetected` and an `unreachable` in a Debug + // build. Measured with the crash file in place: one panic left TWO + // records, the real message and then "reached unreachable code" from + // this line under it. + if (!recovering.swap(true, .seq_cst)) @import("vaxis").recover(); + @import("crash.zig").record(msg); std.debug.defaultPanic(msg, ret_addr); } }.call); +/// Whether this process has already restored its terminal — see `panic` above, +/// and note that `debug.handleSegfault` below shares it: a SIGSEGV raised while +/// the panic handler runs must not double-close either. +var recovering: std.atomic.Value(bool) = .init(false); + // Fatal signals (SIGSEGV/SIGILL/SIGBUS/SIGFPE) bypass the panic handler and // no defer/errdefer ever runs — hook std.debug's segfault path the same way // so the terminal is restored before the trace prints. pub const debug = if (is_emscripten) struct {} else struct { pub fn handleSegfault(addr: ?usize, name: []const u8, opt_ctx: anytype) noreturn { - @import("vaxis").recover(); + if (!recovering.swap(true, .seq_cst)) @import("vaxis").recover(); return std.debug.defaultHandleSegfault(addr, name, opt_ctx); } }; |