safety: show error return trace when unwrapping error in switch

[Vexu]

A small quality-of-life improvement I thought of while trying to figure out where an error.GenericPoison was coming from.

Example situation:

fnfoo() !void {
returnerror.Foo;
}
fnbar() !void {
tryfoo();
}
fnbaz() void {
// bar() catch unreachable;// bar() catch @panic("foo");// const S = struct {// const S = struct {// const fooo = "foo";// };// };bar() catch|err|switch (err) {
// error.Foo => @panic(S.S.fooo),error.Foo=>unreachable,
};
}
test {
baz();
}

before:

Test [1/1] test_0... thread 18432 panic: reached unreachable code
./a.zig:17:22: 0x20f0fc in baz (test)
error.Foo => unreachable,
^
./a.zig:21:8: 0x20f078 in test_0 (test)
baz();
^
/home/vexu/Documents/zig/zig/lib/test_runner.zig:79:28: 0x2105b5 in main (test)
} else test_fn.func();
^
/home/vexu/Documents/zig/zig/lib/std/start.zig:568:22: 0x20f9bd in posixCallMainAndExit (test)
root.main();
^
/home/vexu/Documents/zig/zig/lib/std/start.zig:340:5: 0x20f3c2 in _start (test)
@call(.{ .modifier = .never_inline }, posixCallMainAndExit, .{});
^
error: the following test command crashed:

after:

Test [1/1] test_0... thread 18500 panic: attempt to unwrap error: Foo
./a.zig:2:5: 0x20f998 in foo (test)
return error.Foo;
^
./a.zig:5:5: 0x20fa83 in bar (test)
try foo();
^
/home/vexu/Documents/zig/zig/lib/std/debug.zig:311:22: 0x210b34 in panicExtra__anon_2885 (test)
std.builtin.panic(msg, trace);
^
/home/vexu/Documents/zig/zig/lib/std/builtin.zig:864:25: 0x20fad3 in panicUnwrapError (test)
std.debug.panicExtra(st, "attempt to unwrap error: {s}", .{@errorName(err)});
^
./a.zig:17:22: 0x20f097 in baz (test)
error.Foo => unreachable,
^
./a.zig:21:8: 0x20f018 in test_0 (test)
baz();
^
/home/vexu/Documents/zig/zig/lib/test_runner.zig:79:28: 0x210595 in main (test)
} else test_fn.func();
^
/home/vexu/Documents/zig/zig/lib/std/start.zig:568:22: 0x20f95d in posixCallMainAndExit (test)
root.main();
^
/home/vexu/Documents/zig/zig/lib/std/start.zig:340:5: 0x20f362 in _start (test)
@call(.{ .modifier = .never_inline }, posixCallMainAndExit, .{});
^
error: the following test command crashed:

This also makes @panic show error return trace in these same situations which you can test by uncommenting lines in the example.

[andrewrk]

Looks great. It would be nice to get rid of these frames from the stack trace too:

/home/vexu/Documents/zig/zig/lib/std/debug.zig:311:22: 0x210b34 in panicExtra__anon_2885 (test)
std.builtin.panic(msg, trace);
^
/home/vexu/Documents/zig/zig/lib/std/builtin.zig:864:25: 0x20fad3 in panicUnwrapError (test)
std.debug.panicExtra(st, "attempt to unwrap error: {s}", .{@errorName(err)});
^

[andrewrk]

I rebased this against master branch, updated the CI tarballs for windows and macos, and force pushed.

[andrewrk]

I suggest instead of inline calls, to have these functions use @returnAddress() and then pass this forward into the panic system so that this value ends up passed to panicImpl as the first_trace_addr parameter. Maybe this means we have to make a breaking change to the panic function signature to add this parameter. If that is the case, I'm fine with it.

This will be more appropriate for cold functions to do, and result in less bloat from inlined things.

As a side note, I'm a little uncomfortable with how much std lib formatting is being dragged in by builtin panic helper functions, and by the fact that overriding the panic handler still results in a bunch of formatting stuff being outputted. But, I don't have any concrete suggestions for improvement on that, and it's not relevant to this PR, and I think that the helpful debugging messages are worth it in the end.

[Vexu]

I suggest instead of inline calls, to have these functions use @returnAddress() and then pass this forward into the panic system so that this value ends up passed to panicImpl as the first_trace_addr parameter. Maybe this means we have to make a breaking change to the panic function signature to add this parameter. If that is the case, I'm fine with it.

That would require changing PanicFn from fn ([]const u8, ?*StackTrace) noreturn to fn ([]const u8, ?*StackTrace, ?usize) noreturn but I also think that's an improvement.

As a side note, I'm a little uncomfortable with how much std lib formatting is being dragged in by builtin panic helper functions, and by the fact that overriding the panic handler still results in a bunch of formatting stuff being outputted. But, I don't have any concrete suggestions for improvement on that, and it's not relevant to this PR, and I think that the helpful debugging messages are worth it in the end.

How about a @import("root").simple_panics option?

[andrewrk]

That would require changing PanicFn from fn ([]const u8, ?*StackTrace) noreturn to fn ([]const u8, ?*StackTrace, ?usize) noreturn but I also think that's an improvement.

Let's do it.

How about a @import("root").simple_panics option?

Worth considering. Now that you mention it, I can think of a couple other options worth considering as well. Sounds like we should discuss in a separate proposal that does not block this PR, yeah?

[Vexu]

Call parameter type does not match function signature!
%"?usize" { i64 undef, i1 false }
ptr call fastcc void @std.builtin.default_panic(ptr @11218, ptr null, %"?usize" { i64 undef, i1 false }), !dbg !1431870

Any ideas to what could be causing this? I don't think panic is being called in any other place in stage1 and my change there seems correct enough to build stage1?

[andrewrk]

Can I see the corresponding function signature from the LLVM module?

[Vexu]

How do I pass -femit-llvm-ir to CMake?

[andrewrk]

-femit-llvm-ir gives you the LLVM IR after optimization passes (even in debug builds there is still some processing). In this case --verbose-llvm-ir is the more helpful flag.

My suggestion is to invoke make like this: make VERBOSE=1 which will make it print the commands it runs before it runs them, then you can copy paste the command, adding --verbose-llvm-ir 2>stderr.txt to the end.