Slice fixes #21426
Open
Slice fixes #21426
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`comptime`-known `undefined` pointer operand by
runtime-known length.
|
Thank you for taking the time to make this contribution tractable. I will be sure to give you a timely review. edit: I will wait until it is passing the CI to review it, however |
|
The CI is failing because one of the previously omitted sentinel checks is catching an undefined null-terminator for a buffer sliced by The error is only encountered on Windows, and the unexpected sentinel value is 43690, which is This runtime check for implicitly propagated sentinels is new with this implementation, as implicitly propagated sentinels were previously only checked at compile time (under certain conditions). A similar bug was found in I searched for the uninitialised buffer, but was unable to find it or test anything. Without help from Windows contributors (@squeek502?) the only solution seems to be to re-disable the sentinel check. |
mlugg
reviewed
Sep 17, 2024
* Various superficial changes.
* Removed bounds checks for pointers with `comptime`-known upper bounds.
1) Except when the actual upper bound may be used to rule out the
existence of a sentinel.
2) Except when those bounds are explicitly defined, such as by
`comptime`-known slices `.len` or by array type information.
Any available explicitly defined bounds will be used instead of
actual bounds in all safety checks.
* Removed unused functions and fields. * Added validation for start, end, and length operands when slicing `comptime`-only types.
amp-59
commented
Sep 25, 2024
| const dest_sent_val: Value = val: { | ||
| if (dest_sent_opt != .none) { | ||
| sa.dest_sent = .known; | ||
| try checkSentinelType(sema, block, dest_sent_src, elem_ty); |
amp-59
commented
Sep 25, 2024
|
|
||
| // Disable all bounds checks for pointers without explicit lengths. | ||
| if (!src_ptr_explicit_len and sa.eq_sentinel != .known) { | ||
| sa.end_le_len = .unknown; |
There was a problem hiding this comment.
Updated with previous commits, as requested.
amp-59
commented
Sep 25, 2024
Comment on lines
+38992
to
+39012
| if (try elem_ty.comptimeOnlySema(sema.pt)) require_comptime: { | ||
| assert(sa.src_ptr == .known); | ||
| const src_note = src_note: { | ||
| if (sa.dest_start == .variable) { | ||
| break :src_note .{ dest_start_src, "start index must be comptime-known" }; | ||
| } | ||
| if (dest_len_opt != .none and sa.dest_len == .variable) { | ||
| break :src_note .{ dest_end_src, "length must be comptime-known" }; | ||
| } | ||
| if (dest_end_opt != .none and sa.dest_end == .variable) { | ||
| break :src_note .{ dest_end_src, "end index must be comptime-known" }; | ||
| } | ||
| break :require_comptime; | ||
| }; | ||
| const msg: *Zcu.ErrorMsg = try sema.errMsg(src, "unable to resolve operand slicing comptime-only type '{}'", .{operand_ty.fmt(sema.pt)}); | ||
| { | ||
| errdefer msg.destroy(sema.gpa); | ||
| try sema.errNote(src_note[0], msg, "{s}", .{src_note[1]}); | ||
| } | ||
| return sema.failWithOwnedErrorMsg(block, msg); | ||
| } |
There was a problem hiding this comment.
This validation is not present in master, and because of this it is quite easy to crash the compiler by slicing comptime-only types with runtime-known operands.
For example, compile the example program with zig build-obj runtime_indices_slicing_comptime_only_pointers.zig
runtime_indices_slicing_comptime_only_pointers.zig:
export fn end() void {
comptime var buf: [1]comptime_int = .{0};
var dest_end: usize = 0;
dest_end +%= 1;
const slice: []const comptime_int = buf[0..dest_end];
const ptr: [*]const comptime_int = slice.ptr;
_ = ptr[0..5];
}
export fn start() void {
comptime var buf: [1]comptime_int = .{0};
var dest_start: usize = 0;
dest_start +%= 1;
const slice: []const comptime_int = buf[dest_start..][0..9];
_ = slice[0];
}
export fn len() void {
comptime var buf: [1]comptime_int = .{0};
var dest_len: usize = 0;
dest_len +%= 1;
const slice: []const comptime_int = buf[0..][0..dest_len];
_ = slice[0];
}
export fn end_const() void {
const buf: [1]type = .{void};
var dest_end: usize = 0;
dest_end +%= 1;
const slice: []const type = buf[0..dest_end];
_ = slice[0];
}
export fn start_const() void {
const buf: [1]type = .{void};
var dest_start: usize = 0;
dest_start +%= 1;
const slice: []const type = buf[dest_start..][0..9];
_ = slice[0];
}
export fn len_const() void {
const buf: [1]type = .{void};
var dest_len: usize = 0;
dest_len +%= 1;
const slice: []const type = buf[0..][0..dest_len];
_ = slice[0];
}Output (master)
zig build-obj runtime_indices_slicing_comptime_only_pointers.zig
Trace/breakpoint trap
This crash is produced by example function end. If end is rewritten to be like the other functions, the output is several apparently self-contradictory compile errors:
zig build-obj runtime_indices_slicing_comptime_only_pointers.zig
runtime_indices_slicing_comptime_only_pointers.zig:6:15: error: values of type '[]const comptime_int' must be comptime-known, but index value is runtime-known
_ = slice[0];
^
runtime_indices_slicing_comptime_only_pointers.zig:13:15: error: values of type '[]const comptime_int' must be comptime-known, but index value is runtime-known
_ = slice[0];
^
runtime_indices_slicing_comptime_only_pointers.zig:20:15: error: values of type '[]const comptime_int' must be comptime-known, but index value is runtime-known
_ = slice[0];
^
runtime_indices_slicing_comptime_only_pointers.zig:27:15: error: values of type '[]const type' must be comptime-known, but index value is runtime-known
_ = slice[0];
^
runtime_indices_slicing_comptime_only_pointers.zig:27:14: note: types are not available at runtime
_ = slice[0];
~~~~~^~~
runtime_indices_slicing_comptime_only_pointers.zig:34:15: error: values of type '[]const type' must be comptime-known, but index value is runtime-known
_ = slice[0];
^
runtime_indices_slicing_comptime_only_pointers.zig:34:14: note: types are not available at runtime
_ = slice[0];
~~~~~^~~
runtime_indices_slicing_comptime_only_pointers.zig:41:15: error: values of type '[]const type' must be comptime-known, but index value is runtime-known
_ = slice[0];
^
runtime_indices_slicing_comptime_only_pointers.zig:41:14: note: types are not available at runtime
_ = slice[0];
~~~~~^~~
Output (slices_1)
This is what the new compile errors look like:
zig build-obj runtime_indices_slicing_comptime_only_pointers.zig
runtime_indices_slicing_comptime_only_pointers.zig:5:44: error: unable to resolve operand slicing comptime-only type '[1]comptime_int'
const slice: []const comptime_int = buf[0..dest_end];
~~~^~~~~~~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:5:48: note: end index must be comptime-known
const slice: []const comptime_int = buf[0..dest_end];
^~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:13:58: error: unable to resolve operand slicing comptime-only type '[1]comptime_int'
const slice: []const comptime_int = buf[dest_start..][0..9];
~~~~~~~~~~~~~~~~~^~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:13:45: note: start index must be comptime-known
const slice: []const comptime_int = buf[dest_start..][0..9];
^~~~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:20:49: error: unable to resolve operand slicing comptime-only type '[1]comptime_int'
const slice: []const comptime_int = buf[0..][0..dest_len];
~~~~~~~~^~~~~~~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:20:53: note: length must be comptime-known
const slice: []const comptime_int = buf[0..][0..dest_len];
^~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:27:36: error: unable to resolve operand slicing comptime-only type '[1]type'
const slice: []const type = buf[0..dest_end];
~~~^~~~~~~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:27:40: note: end index must be comptime-known
const slice: []const type = buf[0..dest_end];
^~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:34:50: error: unable to resolve operand slicing comptime-only type '[1]type'
const slice: []const type = buf[dest_start..][0..9];
~~~~~~~~~~~~~~~~~^~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:34:37: note: start index must be comptime-known
const slice: []const type = buf[dest_start..][0..9];
^~~~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:41:41: error: unable to resolve operand slicing comptime-only type '[1]type'
const slice: []const type = buf[0..][0..dest_len];
~~~~~~~~^~~~~~~~~~~~~
runtime_indices_slicing_comptime_only_pointers.zig:41:45: note: length must be comptime-known
const slice: []const type = buf[0..][0..dest_len];
^~~~~~~~
|
The only remaining differences between this patch and master (besides the fixes) are:
If (2) is not desirable, I will just remove it and create issues for the related crashes. If it is desirable I will add an appropriate test case. If (3) is not desirable, #19798 should be closed, and I need to know which combinations of inputs should allow slicing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
These fixes had been required to test the single-function panic interface.
Merging this patch will close the following issues:
slice_sentinelandslice_length(with sentinel) produce runtime safety checks testing incorrect (underestimated) upper bounds #19794slice_sentinelandslice_length(with sentinel) produce compile-time assertions testing incorrect (overestimated) upper bounds #19795Compatibility
The function added by this patch (
Sema.analyzeSlice2) is intended as a drop-in replacement forSema.analyzeSlice, with these exceptions:Required by #19798: Will limit usage of
comptime-knownundefinedpointer operands. This limitation is described by the third option suggested in the expected behaviour section of #19798. This behaviour is controlled by the call optionallow_limited_slice_of_undefinedand is enabled by default. Disabling this option will prevent slicingcomptime-knownundefinedpointer operands entirely.Will not implicitly propagate sentinels when slicing pointers-to-many without an end or length operand. It may be possible to restore this behaviour for
ptr[0..]ifptr[start.. :sentinel]is removed entirely.Performance
This patch reduces the (machine code) size of safety checked slice operations by 46 percent on average. This advantage is consistent between
stage2_llvm(x86-64) andstage2_x86_64.The size of this improvement may cause readers to question the thoroughness of this implementation, and the following sections are prepared to answer those concerns. The short answer is that this implementation only appears to be highly optimised because the current implementation is not optimal.
Background
The most significant unit of cost for safety checked operations is the conditional branch added for runtime safety checks. In the case of slices, these are produced for index bounds, sentinel values, null pointers, and in rare cases integer overflow. Each branch has a
voidsuccess block and a fail block with a call tobuiltin.panic*. The sizes of these branches vary between 25B and 60B depending on the panic condition.When the term 'variant' is used, this refers to the
comptimestate (known-ness) and values of all the operands, combined with the base syntax.ptr,start,end(optional),len(optional),sentinel(optional).slice_start(ptr[start..]),slice_end(ptr[start..end]),slice_sentinel(ptr[start..end:sentinel],ptr[start..:sentinel]), andslice_length(ptr[start..][0..len],ptr[start..][0..len:sentinel]).The following sections will give examples to explain why this implementation adds far fewer conditional branches than the current implementation. The variants shown in these examples are not isolated cases of bad performance; they each represent broad subsets of the syntax.
Example A: Slice operation with one redundant call to panic
This example shows slicing a pointer-to-one array with
comptime-knownstartandendoperands.The reduction to AIR instructions is 54 percent (117B to 54B), and the reduction to symbol size is 58 percent (46B to 19B).
Compile with
zig build-obj -fstrip -fno-unwind-tables -fomit-frame-pointer -fformatted-panics one_unreachable.zigone_unreachable.zig:Output (master)
Air:Debug:Output (slices_1)
Air:Debug:Explanation
Zig users might expect that no runtime check would be added for this operation, because the bounds had been validated at compile time. This is not caused by any obvious programming error; there is simply no attempt or mechanism to prevent redundant checks for this requirement.
This check will be optimised (removed) by Dead Code Elimination (DCE).
Example B: Slice operation with two redundant calls to panic
This example shows slicing a pointer-to-many with a runtime-known
endoperand.The reduction to AIR instructions is 68 percent (171B to 54B), and the reduction to symbol size is 77 percent (109B to 25B).
Compile with
zig build-obj -fstrip -fno-unwind-tables -fomit-frame-pointer -fformatted-panics two_unreachable.zigtwo_unreachable.zig:Output (master)
Air:Debug:ReleaseSafe:Output (slices_1)
Air:Debug:ReleaseSafe:Explanation
Zig users might expect that no runtime safety checks would be added for this operation, because there is no upper bound and because
0 <= endis a fact.These are the panic functions called by the assembly output by master for the example function
fn0:builtin.panicStartGreaterThanEndfromstart <= end: This check is added because the analysis does not check whether thestartoperand is known to be zero at compile time.builtin.panicOutOfBoundsfromstart <= end: This check is added because the mechanism (var checked_start_lte_end: bool) intended to prevent duplicate checks for this requirement is not observed by the block adding the second check. It is also not engaged by the block adding the first check. It is only engaged by the compile time validation, and only observed by the block adding the first check. This means that even when bothstartandendoperands are known at compile time the second check will still be added, and--yet again--the fail block of this check will be unreachable because any invalid result would have already failed compilation.Both checks will be optimised by DCE, as shown by the
ReleaseSafeassembly above.Example C: Slice operation with five redundant calls to panic
This example shows slicing a slice with the only difference being the sentinel value.
The reduction to AIR instructions is 58 percent (324B to 135B), and the reduction to symbol size is 73 percent (304B to 82B).
Compile with
zig build-obj -fstrip -fno-unwind-tables -fomit-frame-pointer -fformatted-panics five_unreachable.zigfive_unreachable.zig:Output (master)
Air:Debug:ReleaseSafe:Output (slices_1)
Air:Debug:ReleaseSafe:Explanation
Zig users might expect that only one (sentinel) runtime safety check would be added for this operation.
This is the source code for
five_unreachable.zig(again):These are the panic functions called by the assembly output by master for the example function
fn2000:builtin.panicStartGreaterThanEndfromstart <= end: This check is not what it appears to be, because there is noendoperand for this slice operation. In this contextendis equal toslice.lenas shown by the AIR instructions (%4,%5), and the remainder of the function will ignore that. As in the previous example, the success condition0 <= slice.lenis a fact.builtin.default_panicfromend + 1: This check is for the addition operation extending theendindex, 'accounting' for the sentinel in the output type. The theory behind this technique is that pointers with sentinels have an 'actual length' equal to.len + 1, and these lengths are used to construct the conditions for bounds checks instead of the input bounds. Commitment to this technique is the most consistent--apparently deliberate--performance disadvantage of the current implementation for variants with sentinels. Using these values also produces panic messages which are inconsistent with the compile error messages for the same requirement.builtin.default_panicfromslice.len + 1: This check is for the addition operation extendingslice.len, 'accounting' for the sentinel in the input type. This is unnecessary because both input and output pointer types have sentinels. If the end index is out-of-bounds so is the sentinel index. The only case whereslice.len + 1is necessary is when the input pointer has a sentinel and the output pointer does not.builtin.panicOutOfBoundsfromend <= len: This check comparesactual_end <= actual_len. These values are equal, as shown by the AIR instructions (%4,%11,%13,%14). The success conditionslice.len + 1 <= slice.len + 1is a fact.builtin.panicOutOfBoundsfromstart <= end: This check is the second instance ofstart <= end. This is covered in more detail by the previous example.builtin.panicSentinelMismatchfromptr[end] == sentinel: This check is valid.Four of these checks will be optimised by DCE, as shown by the
ReleaseSafeassembly above. The optimised output for master retains a single overflow check (cmp rax, -1), but the addition itself is eliminated because the panic function (that would use the index as panic information) is unreachable.Performance counters compiling 5620 unique success variants
These measurements are provided to give an indication of the compile time performance advantage of this patch. This implementation performs above average for this set, with a machine code size reduction of 54 percent per variant instead of the general average of 46 percent. This emerges from the fact that success variants will often not require any runtime checks, while panic variants will always require at least one runtime check.
Compiled with
zig build-exe -fno-formatted-panics -fstrip -fomit-frame-pointer -fno-unwind-tables slice_success_variants.zigCounters (master)
Counters (slices_1)