builtin: Added declarations for other panic handlers, permitting user override.

[amp-59]

With these additions the following panic-related functions permit override: panic, panicSentinelMismatch, panicUnwrapError, panicOutOfBounds, panicStartGreaterThanEnd, panicInactiveUnionField, checkNonScalarSentinel, returnError, addErrRetTraceAddr.

[Vexu]

Duplicate of #15392, do you have a use case?

[amp-59]

This is how I have used the declarations added by my patch for over a year:

https://github.com/amp-59/zig_lib/blob/main/top/debug.zig#L681

I had added the declarations manually for each compiler build/update, and offered this patch to any who asked about overriding panic handlers--that is the source code seen in the 'duplicate'. I am not sure why @wrongnull did not ask me about it before making the PR.

You probably understand, but the reason this patch is necessary is because without it users of my library are forced to compile bits of the standard library heavy machinery which are never reused, and in the best possible case these are around 80 times the size of my alternatives.

[Vexu]

users of my library are forced to compile bits of the standard library heavy machinery which are never reused, and in the best possible case these are around 80 times the size of my alternatives.

There is also -fformatted-panics for that.

Overriding the panic functions is somewhat reasonable but returnError and addErrRetTraceAddr should not be and overriding checkNonScalarSentinel is of questionable usefulness, since as the name suggests it is only used for non-scalar sentinels.

[amp-59]

There is also -fformatted-panics for that.

My library is a complete standard library rewrite and redesign--with a focus on performance and efficient (fast-compiling) implementations. I have no control over which flags the user decides for their compile jobs. Moreover, -fno-formatted-panics only saves my implementation 1.7KiB.

Overriding the panic functions is somewhat reasonable but returnError and addErrRetTraceAddr should not be

With all due respect, these are not hypothetical changes, and whether they are reasonable or generally useful is irrelevant. The patch is a temporary workaround required by a design decision, and I have no opinion on whether these functions 'should' permit override. I just want my library to work correctly without compiling random functions from an unrelated implementation.

[trgwii]

If I don't link libc, I don't get libc functions. There should be a way for me to not get zig stdlib panic handlers if I don't use zig stdlib and provide my own panic handlers, even in debug builds.

[N00byEdge]

I would like to vouch for this for my OS as I don't use std.fmt for code size reasons. The default panic handlers are the last places that still use it.
I think this makes a lot of sense for situations with constrained code size where you still want to build with safety.

[Vexu]

I still don't think returnError and addErrRetTraceAddr should be configurable since they're only in builtin.zig for convenience and used to be generated by the compiler.

[N00byEdge]

Yeah, I was commenting on the panic handlers, I don't know about the other stuff. As long as they don't pull any big code in, sounds good

[amp-59]

I still don't think returnError and addErrRetTraceAddr should be configurable since they're only in builtin.zig for convenience and used to be generated by the compiler.

There is no strong practical case for the inclusion of returnError and addErrRetTraceAddr declarations without also permitting override of StackTrace. They alone make no difference to this patch. However, I also think there is no case to prevent user override, considering:

• The few users who know of the existence of these functions will understand the extent of their meddling should they choose to override.
• Compile commands which override sources such as -fno-compiler-rt, --build-runner, --test-runner, and --zig-lib-dir allow inexperienced users to break their compilation much more easily and with zero knowledge of undocumented interfaces.

As a compromise, I have removed override declarations for returnError and addErrRetTraceAddr, relocated the default definitions of these functions within the container definition of StackTrace, and added an override declaration for StackTrace alone. This achieves more (all) of the intended outcomes of the patch, and guarantees that all three definitions are coherent in case of override.

I have added file-scope simple public declarations for returnError and addErrRetTraceAddr. These have no design purpose, and may be removed with an update to src/Sema.zig, retWithErrTracing to lookup returnError in builtin.StackTrace instead of builtin. The public declaration for addErrRetTraceAddr is apparently never required.

[andrewrk]

I don't like -fformatted-panics either. I liked it better when one could override the panic handler with a single pub fn panic in the root source file.

However, I don't like this patch either. It sacrifices the standard library being improved in favor of making it possible to override everything. It shouldn't make StackTrace generic, for example.

Instead, the panic handling functions in this file should be modified to not depend on formatted printing, and instead populate a local stack buffer, then call the one singular panic handler with a []const u8, and -fformatted-panics should be deleted from being a CLI option.

Also, the format function should be deleted from StackTrace.

[castholm]

I just want to chime in with another (albeit niche) use case that would benefit from being able to override these panic handlers: environments with severely limited stack space. One example of such an environment is the Playdate console, which has a stack that is only a measly 10 KiB.

These panic handlers currently call std.debug.panicExtra which in turn reserves a little over 4 KiB of stack space for message formatting. This means that even if you provide a root.panic override that uses the Playdate's internal "exit with error message" API, if you happen trigger one of these panic handlers (e.g. via try ... catch unreachable with safety-checked UB enabled) while in the last 40% of the stack, a stack overflow crash will occur before control reaches builtin.panic.

So for this use case, it would be useful to at the very least be able to either disable formatted panics or be able to override the formatting function with a custom function (e.g. for the Playdate it would usually be fine to try to format using a heap-allocated buffer).

[N00byEdge]

I also don't like the idea of that one solution will work for everyone. There are plenty of embedded applications where neither the strings nor printing code will fit to start out with. What you then do is to turn the string into an ID and just put the parameters after the ID, and let the client reformat things.

For example

"Hello, my format string wants to say: {} and it's awesome", .{42}

would transmit

0 42

over the wire, then your debug terminal would look up the id of the format string and show the values

[andrewrk]

What you then do is to turn the string into an ID and just put the parameters after the ID, and let the client reformat things.

I think this is a key observation. If we focus on this, we end up with an interface 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.

How does this work with everyone's use cases?

[N00byEdge]

I won't make any statements on anyone else's behalf but yeah, seems sensible to me, nice solution! Should also be easier for the compiler to optimize depending on how you use the values.

[amp-59]

@andrewrk Here is a WIP interface and example library implementation for PanicCause: https://github.com/amp-59/zig_lib/blob/main/top/safety.zig#L71

All together these functions produce a .text section between 4KiB and 18KiB in size, depending on optimization mode.

However that is unrelated to this patch and PR. This patch is only intended to remove the inconvenience of the existing panic functions while a replacement is implemented.

This inconvenience has made my 220KLoC Zig library unusable for the last 15 months, and has meant that every compiler update I have needed to apply this patch to the standard library.

If that makes no difference then this PR has no purpose and may be closed.

[andrewrk]

I created #17969 to track the intended changes. Thanks for pointing out this flaw, @amp-59