zig
zig copied to clipboard
simplify the panic handler function to be only one function; remove `-fformatted-panics`
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.
Reporting non-scalar sentinels can not work with the solution you have suggested.
@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 bevoid
. - Non-scalar sentinels require an
anytype
renderer to represent the difference between found and expected values in case of a panic.
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.
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.
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?
Why wouldn't the function name and file be reused?
because it wont always be called from the same function and file
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);
}
@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.
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.
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.
std.meta.eql
should not be compiled into safe builds. This was a mistake and never should have been done.
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?
Do we make panic return void
, make checkNonScalarSentinel
permit override, or remove non-scalar sentinels from the language?
remove non-scalar sentinels from the language
I started working on this a bit here: https://github.com/wooster0/zig/commits/panic/ There is only one commit right now: https://github.com/wooster0/zig/commit/279d93ab5d846af6d1051aa2ace84413c4b7a8bc
It's not finished yet.
I have some questions:
- Should
std.debug.panic
be changed to acceptstd.builtin.PanicCause
or should the signature staycomptime format: []const u8, args: anytype
? Seelib/c.zig
andlib/compiler_rt/common.zig
in the commit for examples of why it might be a problem to keep itcomptime format: []const u8, args: anytype
. - 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-pastingPanicCause
wherever it was needed insrc/
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. - Should
printPanicCause
inlib/std/builtin.zig
not usestd.fmt.bufPrint
? Should it append to a local stack buffer? - I removed
StackTrace.format
. Do I need to do anything else? - 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.
- Doesn't there need to be another
sentinel_mismatch_float: struct { expected: f32, actual: f32 }
field inPanicCause
in order to print out float sentinels properly? If so which float type should I use? - Should there be another field added for pointer sentinels too?
- 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?
@wooster0 I have already completed the implementation. I am on Discord if you would like to discuss the details.
@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.