safety: show error return trace when unwrapping error in switch by Vexu · Pull Request #12807 · ziglang/zig

Vexu · 2022-09-10T20:18:41Z

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

Example situation:

fn foo() !void {
    return error.Foo;
}
fn bar() !void {
    try foo();
}
fn baz() 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 · 2022-09-12T04:55:54Z

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 · 2022-09-14T03:12:14Z

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 · 2022-09-14T20:47:44Z

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 · 2022-09-14T20:56:02Z

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 · 2022-09-14T22:13:08Z

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 · 2022-09-14T23:05:33Z

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

Vexu · 2022-09-14T23:06:40Z

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

andrewrk · 2022-09-14T23:08:39Z

-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.

Vexu force-pushed the stage2-safety branch from cdcfe6b to 32a08fb Compare September 11, 2022 19:14

Vexu mentioned this pull request Sep 12, 2022

Properly reset error return trace index #12825

Closed

andrewrk force-pushed the stage2-safety branch from 6233823 to 72f132d Compare September 14, 2022 03:11

topolarity mentioned this pull request Sep 14, 2022

stage2: Pop error trace frames for handled errors (#1923) #12837

Merged

andrewrk requested changes Sep 14, 2022

View reviewed changes

Vexu force-pushed the stage2-safety branch from 72f132d to 390db7f Compare September 14, 2022 21:45

Vexu force-pushed the stage2-safety branch 3 times, most recently from c340698 to 19b4503 Compare September 15, 2022 00:22

Vexu added 2 commits September 16, 2022 23:48

safety: show error return trace when unwrapping error in switch

8509cc4

std: add return address parameter to panic fn

2d22519

Vexu force-pushed the stage2-safety branch from 19b4503 to 2d22519 Compare September 16, 2022 20:48

Vexu mentioned this pull request Sep 17, 2022

safety: show error return trace when unwrapping error in switch #12889

Merged

Vexu closed this Sep 17, 2022

Vexu deleted the stage2-safety branch September 17, 2022 20:23

gwenzek mentioned this pull request Oct 15, 2022

There should be a way to disable formatting in panic handlers #13174

Closed

Vexu mentioned this pull request Dec 7, 2022

else => unreachable should be handled like catch unreachable when the type is an error set #3418

Closed

codethief mentioned this pull request Mar 5, 2023

Show error return trace whenever catch block reaches unreachable #14807

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

safety: show error return trace when unwrapping error in switch#12807

safety: show error return trace when unwrapping error in switch#12807
Vexu wants to merge 2 commits intoziglang:masterfrom
Vexu:stage2-safety

Vexu commented Sep 10, 2022

Uh oh!

andrewrk commented Sep 12, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

andrewrk left a comment

Uh oh!

Vexu commented Sep 14, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

Vexu commented Sep 14, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

Vexu commented Sep 14, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Vexu commented Sep 10, 2022

Uh oh!

andrewrk commented Sep 12, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Vexu commented Sep 14, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

Vexu commented Sep 14, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

Vexu commented Sep 14, 2022

Uh oh!

andrewrk commented Sep 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants