Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an assume that the index is inbounds to slice::get_unchecked #116915

Merged
merged 1 commit into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion library/core/src/slice/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,10 @@ unsafe impl<T> SliceIndex<[T]> for usize {
// cannot be longer than `isize::MAX`. They also guarantee that
// `self` is in bounds of `slice` so `self` cannot overflow an `isize`,
// so the call to `add` is safe.
unsafe { slice.as_ptr().add(self) }
unsafe {
crate::intrinsics::assume(self < slice.len());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I just noticed here: why is there an assume in get_unchecked, but not in get_unchecked_mut below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't figure out how to justify the compile-time regression: #120762

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it seems there's a pretty robust effect on the size of libstd.so that I just didn't notice before. Hunh.

slice.as_ptr().add(self)
}
}

#[inline]
Expand Down
3 changes: 1 addition & 2 deletions src/tools/miri/tests/fail/stacked_borrows/zst_slice.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
//@compile-flags: -Zmiri-strict-provenance
//@error-in-other-file: /retag .* tag does not exist in the borrow stack/

fn main() {
unsafe {
let a = [1, 2, 3];
let s = &a[0..0];
assert_eq!(s.len(), 0);
assert_eq!(*s.get_unchecked(1), 2);
assert_eq!(*s.as_ptr().add(1), 2); //~ ERROR: /retag .* tag does not exist in the borrow stack/
}
}
24 changes: 10 additions & 14 deletions src/tools/miri/tests/fail/stacked_borrows/zst_slice.stderr
Original file line number Diff line number Diff line change
@@ -1,26 +1,22 @@
error: Undefined Behavior: trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location
--> RUSTLIB/core/src/slice/mod.rs:LL:CC
--> $DIR/zst_slice.rs:LL:CC
|
LL | unsafe { &*index.get_unchecked(self) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location
| this error occurs as part of retag at ALLOC[0x4..0x8]
LL | assert_eq!(*s.as_ptr().add(1), 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location
| this error occurs as part of retag at ALLOC[0x4..0x8]
|
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <TAG> would have been created here, but this is a zero-size retag ([0x0..0x0]) so the tag in question does not exist anywhere
--> $DIR/zst_slice.rs:LL:CC
|
LL | assert_eq!(*s.get_unchecked(1), 2);
| ^^^^^^^^^^^^^^^^^^
LL | assert_eq!(*s.as_ptr().add(1), 2);
| ^^^^^^^^^^
= note: BACKTRACE (of the first span):
= note: inside `core::slice::<impl [i32]>::get_unchecked::<usize>` at RUSTLIB/core/src/slice/mod.rs:LL:CC
note: inside `main`
--> $DIR/zst_slice.rs:LL:CC
|
LL | assert_eq!(*s.get_unchecked(1), 2);
| ^^^^^^^^^^^^^^^^^^
= note: inside `main` at RUSTLIB/core/src/macros/mod.rs:LL:CC
= note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/tests/fail/uninit/uninit_byte_read.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//@compile-flags: -Zmiri-disable-stacked-borrows
fn main() {
let v: Vec<u8> = Vec::with_capacity(10);
let undef = unsafe { *v.get_unchecked(5) }; //~ ERROR: uninitialized
let undef = unsafe { *v.as_ptr().add(5) }; //~ ERROR: uninitialized
let x = undef + 1;
panic!("this should never print: {}", x);
}
4 changes: 2 additions & 2 deletions src/tools/miri/tests/fail/uninit/uninit_byte_read.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
--> $DIR/uninit_byte_read.rs:LL:CC
|
LL | let undef = unsafe { *v.get_unchecked(5) };
| ^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
LL | let undef = unsafe { *v.as_ptr().add(5) };
| ^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
Expand Down
4 changes: 2 additions & 2 deletions src/tools/miri/tests/pass/float_nan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ use NaNKind::*;
#[track_caller]
fn check_all_outcomes<T: Eq + Hash + fmt::Display>(expected: HashSet<T>, generate: impl Fn() -> T) {
let mut seen = HashSet::new();
// Let's give it 8x as many tries as we are expecting values.
let tries = expected.len() * 8;
// Let's give it sixteen times as many tries as we are expecting values.
let tries = expected.len() * 16;
for _ in 0..tries {
let val = generate();
assert!(expected.contains(&val), "got an unexpected value: {val}");
Expand Down
13 changes: 13 additions & 0 deletions tests/codegen/issues/issue-116878.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// no-system-llvm
// compile-flags: -O
// ignore-debug: the debug assertions get in the way
#![crate_type = "lib"]

/// Make sure no bounds checks are emitted after a `get_unchecked`.
// CHECK-LABEL: @unchecked_slice_no_bounds_check
#[no_mangle]
pub unsafe fn unchecked_slice_no_bounds_check(s: &[u8]) -> u8 {
let a = *s.get_unchecked(1);
// CHECK-NOT: panic_bounds_check
a + s[0]
}
Loading