From 7349f2c6a3a02885449c951852af4bc4a7678b8a Mon Sep 17 00:00:00 2001 From: Hanif Bin Ariffin Date: Sat, 25 Apr 2020 19:30:23 -0400 Subject: [PATCH 1/6] Added unsafety documentation to shift_head --- src/libcore/slice/sort.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/libcore/slice/sort.rs b/src/libcore/slice/sort.rs index be3e7aaa2e8..3c14647f3c7 100644 --- a/src/libcore/slice/sort.rs +++ b/src/libcore/slice/sort.rs @@ -1,6 +1,6 @@ //! 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 //! //! Unstable sorting is compatible with libcore because it doesn't allocate memory, unlike our @@ -32,6 +32,20 @@ where F: FnMut(&T, &T) -> bool, { 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. FIXME: Guarantees that the elements are properly aligned? + // + // See comments below for further detail. unsafe { // If the first two elements are out-of-order... if len >= 2 && is_less(v.get_unchecked(1), v.get_unchecked(0)) { From 9e8b42c02bfa348b024ad07652e860b125345acf Mon Sep 17 00:00:00 2001 From: Hanif Bin Ariffin Date: Sat, 25 Apr 2020 19:39:40 -0400 Subject: [PATCH 2/6] Added unsafety documentation to shift_tail This is just the reverse of shift_head. --- src/libcore/slice/sort.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libcore/slice/sort.rs b/src/libcore/slice/sort.rs index 3c14647f3c7..0177c5a9ffd 100644 --- a/src/libcore/slice/sort.rs +++ b/src/libcore/slice/sort.rs @@ -76,6 +76,20 @@ where F: FnMut(&T, &T) -> bool, { let len = v.len(); + // SAFETY: As with shift_head, 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. FIXME: Guarantees that the elements are properly aligned? + // + // See comments below for further detail. unsafe { // If the last two elements are out-of-order... if len >= 2 && is_less(v.get_unchecked(len - 1), v.get_unchecked(len - 2)) { From 9e1e989f7cfbc9bb35511acdeb51b3122bf717a2 Mon Sep 17 00:00:00 2001 From: Hanif Bin Ariffin Date: Sat, 25 Apr 2020 19:46:53 -0400 Subject: [PATCH 3/6] Document unsafety in partial_insertion_sort We already implicitly (or explicitly??) do the bound checking for the indexing. --- src/libcore/slice/sort.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libcore/slice/sort.rs b/src/libcore/slice/sort.rs index 0177c5a9ffd..937995e2fe0 100644 --- a/src/libcore/slice/sort.rs +++ b/src/libcore/slice/sort.rs @@ -131,6 +131,8 @@ where let mut i = 1; for _ in 0..MAX_STEPS { + // SAFETY: We already explicitly done the bound checking with `i Date: Sat, 25 Apr 2020 21:11:59 -0400 Subject: [PATCH 4/6] Added unsafety documentation with partition and partition equal These are simply indexing safety. --- src/libcore/slice/sort.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libcore/slice/sort.rs b/src/libcore/slice/sort.rs index 937995e2fe0..046b2f6c13a 100644 --- a/src/libcore/slice/sort.rs +++ b/src/libcore/slice/sort.rs @@ -250,6 +250,7 @@ where let mut offsets_l = [MaybeUninit::::uninit(); BLOCK]; // 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 block_r = BLOCK; let mut start_r = ptr::null_mut(); @@ -435,12 +436,14 @@ where let mut l = 0; let mut r = v.len(); unsafe { - // Find the first element greater then or equal to the pivot. + // Find the first element greater than or equal to the pivot. + // SAFETY: We already do the bound checking here with `l Date: Sat, 25 Apr 2020 21:18:47 -0400 Subject: [PATCH 5/6] Added some unsafety documentation to partition_equal --- src/libcore/slice/sort.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libcore/slice/sort.rs b/src/libcore/slice/sort.rs index 046b2f6c13a..2ec4f43b1f7 100644 --- a/src/libcore/slice/sort.rs +++ b/src/libcore/slice/sort.rs @@ -435,15 +435,17 @@ where // Find the first pair of out-of-order elements. let mut l = 0; let mut r = v.len(); + + // SAFETY: The unsafety below involves indexing an array. + // For the first one: we already do the bound checking here with `l Date: Sat, 13 Jun 2020 19:02:26 -0400 Subject: [PATCH 6/6] Added some more documentations to unsafety blocks in slice/sort.rs --- src/libcore/slice/sort.rs | 50 ++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/src/libcore/slice/sort.rs b/src/libcore/slice/sort.rs index 2ec4f43b1f7..8b2ac294764 100644 --- a/src/libcore/slice/sort.rs +++ b/src/libcore/slice/sort.rs @@ -20,6 +20,9 @@ struct CopyOnDrop { impl Drop for CopyOnDrop { 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 { ptr::copy_nonoverlapping(self.src, self.dest, 1); } @@ -43,7 +46,8 @@ where // 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. FIXME: Guarantees that the elements are properly aligned? + // 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 { @@ -76,18 +80,19 @@ where F: FnMut(&T, &T) -> bool, { let len = v.len(); - // SAFETY: As with shift_head, the unsafe operations below involves indexing without a bound check (`get_unchecked` and `get_unchecked_mut`) + // 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. + // 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. FIXME: Guarantees that the elements are properly aligned? + // 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 { @@ -131,8 +136,8 @@ where let mut i = 1; for _ in 0..MAX_STEPS { - // SAFETY: We already explicitly done the bound checking with `i