Rollup merge of #71568 - hbina:document_unsafety_slice_sort, r=joshtriplett
Document unsafety in slice/sort.rs Let me know if these documentations are accurate c: I don't think I am capable enough to document the safety of `partition_blocks`, however. Related issue #66219
This commit is contained in:
commit
85e1c3baca
1 changed files with 70 additions and 3 deletions
|
@ -1,6 +1,6 @@
|
||||||
//! Slice sorting
|
//! Slice sorting
|
||||||
//!
|
//!
|
||||||
//! This module contains an sort algorithm based on Orson Peters' pattern-defeating quicksort,
|
//! This module contains a sorting algorithm based on Orson Peters' pattern-defeating quicksort,
|
||||||
//! published at: https://github.com/orlp/pdqsort
|
//! published at: https://github.com/orlp/pdqsort
|
||||||
//!
|
//!
|
||||||
//! Unstable sorting is compatible with libcore because it doesn't allocate memory, unlike our
|
//! Unstable sorting is compatible with libcore because it doesn't allocate memory, unlike our
|
||||||
|
@ -20,6 +20,9 @@ struct CopyOnDrop<T> {
|
||||||
|
|
||||||
impl<T> Drop for CopyOnDrop<T> {
|
impl<T> Drop for CopyOnDrop<T> {
|
||||||
fn drop(&mut self) {
|
fn drop(&mut self) {
|
||||||
|
// SAFETY: This is a helper class.
|
||||||
|
// Please refer to its usage for correctness.
|
||||||
|
// Namely, one must be sure that `src` and `dst` does not overlap as required by `ptr::copy_nonoverlapping`.
|
||||||
unsafe {
|
unsafe {
|
||||||
ptr::copy_nonoverlapping(self.src, self.dest, 1);
|
ptr::copy_nonoverlapping(self.src, self.dest, 1);
|
||||||
}
|
}
|
||||||
|
@ -32,6 +35,21 @@ where
|
||||||
F: FnMut(&T, &T) -> bool,
|
F: FnMut(&T, &T) -> bool,
|
||||||
{
|
{
|
||||||
let len = v.len();
|
let len = v.len();
|
||||||
|
// SAFETY: The unsafe operations below involves indexing without a bound check (`get_unchecked` and `get_unchecked_mut`)
|
||||||
|
// and copying memory (`ptr::copy_nonoverlapping`).
|
||||||
|
//
|
||||||
|
// a. Indexing:
|
||||||
|
// 1. We checked the size of the array to >=2.
|
||||||
|
// 2. All the indexing that we will do is always between {0 <= index < len} at most.
|
||||||
|
//
|
||||||
|
// b. Memory copying
|
||||||
|
// 1. We are obtaining pointers to references which are guaranteed to be valid.
|
||||||
|
// 2. They cannot overlap because we obtain pointers to difference indices of the slice.
|
||||||
|
// Namely, `i` and `i-1`.
|
||||||
|
// 3. If the slice is properly aligned, the elements are properly aligned.
|
||||||
|
// It is the caller's responsibility to make sure the slice is properly aligned.
|
||||||
|
//
|
||||||
|
// See comments below for further detail.
|
||||||
unsafe {
|
unsafe {
|
||||||
// If the first two elements are out-of-order...
|
// If the first two elements are out-of-order...
|
||||||
if len >= 2 && is_less(v.get_unchecked(1), v.get_unchecked(0)) {
|
if len >= 2 && is_less(v.get_unchecked(1), v.get_unchecked(0)) {
|
||||||
|
@ -62,6 +80,21 @@ where
|
||||||
F: FnMut(&T, &T) -> bool,
|
F: FnMut(&T, &T) -> bool,
|
||||||
{
|
{
|
||||||
let len = v.len();
|
let len = v.len();
|
||||||
|
// SAFETY: The unsafe operations below involves indexing without a bound check (`get_unchecked` and `get_unchecked_mut`)
|
||||||
|
// and copying memory (`ptr::copy_nonoverlapping`).
|
||||||
|
//
|
||||||
|
// a. Indexing:
|
||||||
|
// 1. We checked the size of the array to >= 2.
|
||||||
|
// 2. All the indexing that we will do is always between `0 <= index < len-1` at most.
|
||||||
|
//
|
||||||
|
// b. Memory copying
|
||||||
|
// 1. We are obtaining pointers to references which are guaranteed to be valid.
|
||||||
|
// 2. They cannot overlap because we obtain pointers to difference indices of the slice.
|
||||||
|
// Namely, `i` and `i+1`.
|
||||||
|
// 3. If the slice is properly aligned, the elements are properly aligned.
|
||||||
|
// It is the caller's responsibility to make sure the slice is properly aligned.
|
||||||
|
//
|
||||||
|
// See comments below for further detail.
|
||||||
unsafe {
|
unsafe {
|
||||||
// If the last two elements are out-of-order...
|
// If the last two elements are out-of-order...
|
||||||
if len >= 2 && is_less(v.get_unchecked(len - 1), v.get_unchecked(len - 2)) {
|
if len >= 2 && is_less(v.get_unchecked(len - 1), v.get_unchecked(len - 2)) {
|
||||||
|
@ -103,6 +136,8 @@ where
|
||||||
let mut i = 1;
|
let mut i = 1;
|
||||||
|
|
||||||
for _ in 0..MAX_STEPS {
|
for _ in 0..MAX_STEPS {
|
||||||
|
// SAFETY: We already explicitly did the bound checking with `i < len`.
|
||||||
|
// All our subsequent indexing is only in the range `0 <= index < len`
|
||||||
unsafe {
|
unsafe {
|
||||||
// Find the next pair of adjacent out-of-order elements.
|
// Find the next pair of adjacent out-of-order elements.
|
||||||
while i < len && !is_less(v.get_unchecked(i), v.get_unchecked(i - 1)) {
|
while i < len && !is_less(v.get_unchecked(i), v.get_unchecked(i - 1)) {
|
||||||
|
@ -220,6 +255,7 @@ where
|
||||||
let mut offsets_l = [MaybeUninit::<u8>::uninit(); BLOCK];
|
let mut offsets_l = [MaybeUninit::<u8>::uninit(); BLOCK];
|
||||||
|
|
||||||
// The current block on the right side (from `r.sub(block_r)` to `r`).
|
// The current block on the right side (from `r.sub(block_r)` to `r`).
|
||||||
|
// SAFETY: The documentation for .add() specifically mention that `vec.as_ptr().add(vec.len())` is always safe`
|
||||||
let mut r = unsafe { l.add(v.len()) };
|
let mut r = unsafe { l.add(v.len()) };
|
||||||
let mut block_r = BLOCK;
|
let mut block_r = BLOCK;
|
||||||
let mut start_r = ptr::null_mut();
|
let mut start_r = ptr::null_mut();
|
||||||
|
@ -268,6 +304,16 @@ where
|
||||||
let mut elem = l;
|
let mut elem = l;
|
||||||
|
|
||||||
for i in 0..block_l {
|
for i in 0..block_l {
|
||||||
|
// SAFETY: The unsafety operations below involve the usage of the `offset`.
|
||||||
|
// According to the conditions required by the function, we satisfy them because:
|
||||||
|
// 1. `offsets_l` is stack-allocated, and thus considered separate allocated object.
|
||||||
|
// 2. The function `is_less` returns a `bool`.
|
||||||
|
// Casting a `bool` will never overflow `isize`.
|
||||||
|
// 3. We have guaranteed that `block_l` will be `<= BLOCK`.
|
||||||
|
// Plus, `end_l` was initially set to the begin pointer of `offsets_` which was declared on the stack.
|
||||||
|
// Thus, we know that even in the worst case (all invocations of `is_less` returns false) we will only be at most 1 byte pass the end.
|
||||||
|
// Another unsafety operation here is dereferencing `elem`.
|
||||||
|
// However, `elem` was initially the begin pointer to the slice which is always valid.
|
||||||
unsafe {
|
unsafe {
|
||||||
// Branchless comparison.
|
// Branchless comparison.
|
||||||
*end_l = i as u8;
|
*end_l = i as u8;
|
||||||
|
@ -284,6 +330,17 @@ where
|
||||||
let mut elem = r;
|
let mut elem = r;
|
||||||
|
|
||||||
for i in 0..block_r {
|
for i in 0..block_r {
|
||||||
|
// SAFETY: The unsafety operations below involve the usage of the `offset`.
|
||||||
|
// According to the conditions required by the function, we satisfy them because:
|
||||||
|
// 1. `offsets_r` is stack-allocated, and thus considered separate allocated object.
|
||||||
|
// 2. The function `is_less` returns a `bool`.
|
||||||
|
// Casting a `bool` will never overflow `isize`.
|
||||||
|
// 3. We have guaranteed that `block_r` will be `<= BLOCK`.
|
||||||
|
// Plus, `end_r` was initially set to the begin pointer of `offsets_` which was declared on the stack.
|
||||||
|
// Thus, we know that even in the worst case (all invocations of `is_less` returns true) we will only be at most 1 byte pass the end.
|
||||||
|
// Another unsafety operation here is dereferencing `elem`.
|
||||||
|
// However, `elem` was initially `1 * sizeof(T)` past the end and we decrement it by `1 * sizeof(T)` before accessing it.
|
||||||
|
// Plus, `block_r` was asserted to be less than `BLOCK` and `elem` will therefore at most be pointing to the beginning of the slice.
|
||||||
unsafe {
|
unsafe {
|
||||||
// Branchless comparison.
|
// Branchless comparison.
|
||||||
elem = elem.offset(-1);
|
elem = elem.offset(-1);
|
||||||
|
@ -404,8 +461,13 @@ where
|
||||||
// Find the first pair of out-of-order elements.
|
// Find the first pair of out-of-order elements.
|
||||||
let mut l = 0;
|
let mut l = 0;
|
||||||
let mut r = v.len();
|
let mut r = v.len();
|
||||||
|
|
||||||
|
// SAFETY: The unsafety below involves indexing an array.
|
||||||
|
// For the first one: We already do the bounds checking here with `l < r`.
|
||||||
|
// For the second one: We initially have `l == 0` and `r == v.len()` and we checked that `l < r` at every indexing operation.
|
||||||
|
// From here we know that `r` must be at least `r == l` which was shown to be valid from the first one.
|
||||||
unsafe {
|
unsafe {
|
||||||
// Find the first element greater then or equal to the pivot.
|
// Find the first element greater than or equal to the pivot.
|
||||||
while l < r && is_less(v.get_unchecked(l), pivot) {
|
while l < r && is_less(v.get_unchecked(l), pivot) {
|
||||||
l += 1;
|
l += 1;
|
||||||
}
|
}
|
||||||
|
@ -444,6 +506,7 @@ where
|
||||||
|
|
||||||
// Read the pivot into a stack-allocated variable for efficiency. If a following comparison
|
// Read the pivot into a stack-allocated variable for efficiency. If a following comparison
|
||||||
// operation panics, the pivot will be automatically written back into the slice.
|
// operation panics, the pivot will be automatically written back into the slice.
|
||||||
|
// SAFETY: The pointer here is valid because it is obtained from a reference to a slice.
|
||||||
let mut tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot) });
|
let mut tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot) });
|
||||||
let _pivot_guard = CopyOnDrop { src: &mut *tmp, dest: pivot };
|
let _pivot_guard = CopyOnDrop { src: &mut *tmp, dest: pivot };
|
||||||
let pivot = &*tmp;
|
let pivot = &*tmp;
|
||||||
|
@ -452,8 +515,12 @@ where
|
||||||
let mut l = 0;
|
let mut l = 0;
|
||||||
let mut r = v.len();
|
let mut r = v.len();
|
||||||
loop {
|
loop {
|
||||||
|
// SAFETY: The unsafety below involves indexing an array.
|
||||||
|
// For the first one: We already do the bounds checking here with `l < r`.
|
||||||
|
// For the second one: We initially have `l == 0` and `r == v.len()` and we checked that `l < r` at every indexing operation.
|
||||||
|
// From here we know that `r` must be at least `r == l` which was shown to be valid from the first one.
|
||||||
unsafe {
|
unsafe {
|
||||||
// Find the first element greater that the pivot.
|
// Find the first element greater than the pivot.
|
||||||
while l < r && !is_less(pivot, v.get_unchecked(l)) {
|
while l < r && !is_less(pivot, v.get_unchecked(l)) {
|
||||||
l += 1;
|
l += 1;
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in a new issue