simplify the panic handler function to be only one function; remove -fformatted-panics

[andrewrk]

There should only be 1 panic handler function and it should be possible to override it by only providing one function. Furthermore, it should be the entry point from the language; there should be no other dependencies on the std lib such as formatted printing.

It should look something like this:

pub const PanicCause = union(enum) {
    unreach,
    unwrap_null,
    cast_to_null,
    incorrect_alignment,
    out_of_bounds: struct {
        index: usize,
        len: usize,
    },
    sentinel_mismatch_usize: usize,
    sentinel_mismatch_isize: isize,
    sentinel_mismatch_other,
    /// This one comes from a call to `@panic`.
    application_msg: []const u8,
    // ...
};

pub fn panic(cause: PanicCause, error_return_trace: ?*StackTrace, ret_addr: ?usize) noreturn {
    // ...
}

This provides complete flexibility to the application overriding the default handler, while keeping it to a single function to be overridden.

[amp-59]

Reporting non-scalar sentinels can not work with the solution you have suggested.

[amp-59]

@andrewrk The incompatibility with non-scalar sentinels is this:

• The return type of the single function is noreturn.
• Non-scalar sentinels require generic test code (meta.eql) to be compiled within the panic function as with the existing function (checkNonScalarSentinel). If the expected and found values are equal the function returns, so the return type must be void.
• Non-scalar sentinels require an anytype renderer to represent the difference between found and expected values in case of a panic.

[AssortedFantasy]

How about also adding an argument containing the std.builtin.SourceLocation?
So a signature more like this:

pub fn panic(cause: PanicCause, error_return_trace: ?*StackTrace, ret_addr: ?usize, source: ?std.builtin.SourceLocation ) noreturn {
    // ...
}

This would let a basic panic handler at least point to where the panic happened even if you don't want to get into std.debug.DebugInfo and std.debug.dumpStackTrace.

[amp-59]

How about also adding an argument containing the std.builtin.SourceLocation?

That is a large additional cost for common checked arithmetic operations, such as addition, subtraction, and multiplication. Each occurrence of these operations in source would probably cost an additional 64 bytes in program segments; 16 in .text, 48 in .rodata:

fn useSrcLoc(src: ?zl.builtin.SourceLocation) void {
    if (src) |_| {}
}
pub fn main() void {}

Simulating just the call with @src():

fn useSrcLoc(src: ?zl.builtin.SourceLocation) void {
    if (src) |_| {}
}
pub fn main() void {
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
    useSrcLoc(@src());
}

In a real program the cost would be higher, as the strings for fn_name and file are reused in the example.

[AssortedFantasy]

Why wouldn't the function name and file be reused? Surely it could be done smartly so its only whatever bytes for the function name and the file name and then per each instance of a call to panic you only need the u32 for line and u32 for column?

[xdBronch]

Why wouldn't the function name and file be reused?

because it wont always be called from the same function and file

[AssortedFantasy]

Here is an example of what I mean. This is roughly how the panic could work at the implementation level. Not embedding the full source struct in every panic location.

const std = @import("std");
const SourceLocation = std.builtin.SourceLocation;

fn panicImpl(src: SourceLocation) noreturn {
    _ = src;
    @panic("");
}

fn panic_zig_file(partial: SourceLocation) noreturn {
    panicImpl(.{
        .file = "main.zig", // One copy only
        .fn_name = partial.fn_name,
        .column = partial.column,
        .line = partial.line,
    });
}

fn panic_main_fn(line: u32, column: u32) noreturn {
    panic_zig_file(.{
        .file = undefined,
        .fn_name = "main", // One per function
        .column = column,
        .line = line,
    });
}

pub fn main() !void {
    panic_main_fn(30, 5);
    panic_main_fn(31, 5);
    panic_main_fn(32, 5);
    panic_main_fn(33, 5);
    panic_main_fn(34, 5);
    panic_main_fn(35, 5);
    panic_main_fn(36, 5);
    panic_main_fn(37, 5);
    panic_main_fn(38, 5);
    panic_main_fn(39, 5);
    panic_main_fn(40, 5);
    panic_main_fn(41, 5);
    panic_main_fn(42, 5);
    panic_main_fn(43, 5);
    panic_main_fn(44, 5);
    panic_main_fn(45, 5);
}

[amp-59]

@AssortedFantasy Yes, that is the current behaviour and the reason why the cost implied by the example is not realistic. It only shows one fn_name and file being reused every time. The difference is not great because the 16 bytes required per slice is already large in proportion to the function name and file pathname.

All things considered I think what you want is possible and could be useful to some users. But it can not be the default behaviour because most of Zig users rely on debug sections for runtime source locations, and for those users all of the work required to generate the builtin source locations would be redundant.

[andrewrk]

I don't think non-scalar sentinels need to block progress on this issue. They can have a simple panic message with no formatted printing.

[trgwii]

If checkNonScalarSentinel remains un-overridable, that forces instances of std.meta.eql to be compiled into all safe builds, regardless of if panic was overridden.

This means that safe builds would still depend on the std lib when panic is overridden by the user.

Edit:

[the panic handler] should be the entry point from the language

If this is to be possible, checkNonScalarSentinel can't even really exist at all, since presumably the compiler would be forced to insert calls to that to check at runtime if a call to panic is warranted at any particular source location.

Either it needs to be overridable as a separate function, or panic must become much uglier and return void such that it can avoid actually panicking in cases where the overridden panic determines that sentinels do in fact match.

[andrewrk]

std.meta.eql should not be compiled into safe builds. This was a mistake and never should have been done.

[trgwii]

Given that std.meta.eql is seemingly ONLY used by checkNonScalarSentinel, should a PR just make no equality comparison at all for non-scalar sentinels?

[amp-59]

Do we make panic return void, make checkNonScalarSentinel permit override, or remove non-scalar sentinels from the language?

[andrewrk]

remove non-scalar sentinels from the language

[wooster0]

I started working on this a bit here: https://github.com/wooster0/zig/commits/panic/
There is only one commit right now: wooster0@279d93a

It's not finished yet.

I have some questions:

1. Should std.debug.panic be changed to accept std.builtin.PanicCause or should the signature stay comptime format: []const u8, args: anytype? See lib/c.zig and lib/compiler_rt/common.zig in the commit for examples of why it might be a problem to keep it comptime format: []const u8, args: anytype.
2. I tested it manually a bit and it seems to work fine so far (including panic causes that are non-void) but I haven't been able to run the test suite yet mainly because I can't really build from source and "builtin.zig is corrupt" errors are also causing me some problems so sometimes I resorted to copy-pasting PanicCause wherever it was needed in src/ compiler files. Would it be fine to test my code with just the GitHub CI later when I'm reasonably confident about my changes? I assume in that case I don't need any of those workarounds.
3. Should printPanicCause in lib/std/builtin.zig not use std.fmt.bufPrint? Should it append to a local stack buffer?
4. I removed StackTrace.format. Do I need to do anything else?
5. Is a vector used as a sentinel a scalar sentinel? For now I removed support for vectors as sentinels as well. So you can only use non-vectors and non-array values as sentinels.
6. Doesn't there need to be another sentinel_mismatch_float: struct { expected: f32, actual: f32 } field in PanicCause in order to print out float sentinels properly? If so which float type should I use?
7. Should there be another field added for pointer sentinels too?
8. All I did to remove non-scalar sentinels from the language is add "non-scalar sentinel '{}' not allowed" errors and I added a test (test/cases/compile_errors/non_scalar_sentinels.zig). Is there any code after that point that I can get rid of?

[amp-59]

@wooster0 I have already completed the implementation. I am on Discord if you would like to discuss the details.

[castholm]

@amp-59 I was just wondering, what is the status on your implementation? Do you have any plans to open a PR with your changes? If you don't have any plans to continue working on this it might be a good idea to let us all know so that someone else can pick up where you left off.