From 6053d5a9ae3e6d6d30cca88a6e039e50df6e8cc8 Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Sat, 30 Dec 2023 17:03:36 -0800 Subject: [PATCH 1/4] Add missing reserve assumes for Vec and String Signed-off-by: Alex Saveau --- library/alloc/src/raw_vec.rs | 10 +++++--- library/alloc/src/string.rs | 30 ++++++++++++++++++---- library/alloc/src/vec/mod.rs | 28 +++++++++++++++++--- tests/ui/hygiene/panic-location.run.stderr | 2 +- 4 files changed, 58 insertions(+), 12 deletions(-) diff --git a/library/alloc/src/raw_vec.rs b/library/alloc/src/raw_vec.rs index 74fa30456eb95..a20e9d6d25226 100644 --- a/library/alloc/src/raw_vec.rs +++ b/library/alloc/src/raw_vec.rs @@ -308,6 +308,10 @@ impl RawVec { if self.needs_to_grow(len, additional) { do_reserve_and_handle(self, len, additional); } + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(!self.needs_to_grow(len, additional)); + } } /// A specialized version of `reserve()` used only by the hot and @@ -325,7 +329,7 @@ impl RawVec { } unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - core::intrinsics::assume(!self.needs_to_grow(len, additional)); + intrinsics::assume(!self.needs_to_grow(len, additional)); } Ok(()) } @@ -363,7 +367,7 @@ impl RawVec { } unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - core::intrinsics::assume(!self.needs_to_grow(len, additional)); + intrinsics::assume(!self.needs_to_grow(len, additional)); } Ok(()) } @@ -387,7 +391,7 @@ impl RawVec { impl RawVec { /// Returns if the buffer needs to grow to fulfill the needed extra capacity. /// Mainly used to make inlining reserve-calls possible without inlining `grow`. - fn needs_to_grow(&self, len: usize, additional: usize) -> bool { + pub(crate) fn needs_to_grow(&self, len: usize, additional: usize) -> bool { additional > self.capacity().wrapping_sub(len) } diff --git a/library/alloc/src/string.rs b/library/alloc/src/string.rs index 4d6968157dedd..00d89b929d5b0 100644 --- a/library/alloc/src/string.rs +++ b/library/alloc/src/string.rs @@ -43,7 +43,6 @@ #![stable(feature = "rust1", since = "1.0.0")] use core::error::Error; -use core::fmt; use core::hash; #[cfg(not(no_global_oom_handling))] use core::iter::from_fn; @@ -60,6 +59,7 @@ use core::slice; use core::str::pattern::Pattern; #[cfg(not(no_global_oom_handling))] use core::str::Utf8Chunks; +use core::{fmt, intrinsics}; #[cfg(not(no_global_oom_handling))] use crate::borrow::{Cow, ToOwned}; @@ -1149,7 +1149,11 @@ impl String { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn reserve(&mut self, additional: usize) { - self.vec.reserve(additional) + self.vec.reserve(additional); + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + } } /// Reserves the minimum capacity for at least `additional` bytes more than @@ -1199,7 +1203,11 @@ impl String { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn reserve_exact(&mut self, additional: usize) { - self.vec.reserve_exact(additional) + self.vec.reserve_exact(additional); + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + } } /// Tries to reserve capacity for at least `additional` bytes more than the @@ -1233,8 +1241,14 @@ impl String { /// # process_data("rust").expect("why is the test harness OOMing on 4 bytes?"); /// ``` #[stable(feature = "try_reserve", since = "1.57.0")] + #[inline] pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> { - self.vec.try_reserve(additional) + self.vec.try_reserve(additional)?; + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + } + Ok(()) } /// Tries to reserve the minimum capacity for at least `additional` bytes @@ -1274,8 +1288,14 @@ impl String { /// # process_data("rust").expect("why is the test harness OOMing on 4 bytes?"); /// ``` #[stable(feature = "try_reserve", since = "1.57.0")] + #[inline] pub fn try_reserve_exact(&mut self, additional: usize) -> Result<(), TryReserveError> { - self.vec.try_reserve_exact(additional) + self.vec.try_reserve_exact(additional)?; + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + } + Ok(()) } /// Shrinks the capacity of this `String` to match its length. diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index e8a096cac869e..562c767425f66 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -56,7 +56,6 @@ #[cfg(not(no_global_oom_handling))] use core::cmp; use core::cmp::Ordering; -use core::fmt; use core::hash::{Hash, Hasher}; #[cfg(not(no_global_oom_handling))] use core::iter; @@ -65,6 +64,7 @@ use core::mem::{self, ManuallyDrop, MaybeUninit, SizedTypeProperties}; use core::ops::{self, Index, IndexMut, Range, RangeBounds}; use core::ptr::{self, NonNull}; use core::slice::{self, SliceIndex}; +use core::{fmt, intrinsics}; use crate::alloc::{Allocator, Global}; use crate::borrow::{Cow, ToOwned}; @@ -907,8 +907,13 @@ impl Vec { /// ``` #[cfg(not(no_global_oom_handling))] #[stable(feature = "rust1", since = "1.0.0")] + #[inline] pub fn reserve(&mut self, additional: usize) { self.buf.reserve(self.len, additional); + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); + } } /// Reserves the minimum capacity for at least `additional` more elements to @@ -937,8 +942,13 @@ impl Vec { /// ``` #[cfg(not(no_global_oom_handling))] #[stable(feature = "rust1", since = "1.0.0")] + #[inline] pub fn reserve_exact(&mut self, additional: usize) { self.buf.reserve_exact(self.len, additional); + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); + } } /// Tries to reserve capacity for at least `additional` more elements to be inserted @@ -974,8 +984,14 @@ impl Vec { /// # process_data(&[1, 2, 3]).expect("why is the test harness OOMing on 12 bytes?"); /// ``` #[stable(feature = "try_reserve", since = "1.57.0")] + #[inline] pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> { - self.buf.try_reserve(self.len, additional) + self.buf.try_reserve(self.len, additional)?; + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); + } + Ok(()) } /// Tries to reserve the minimum capacity for at least `additional` @@ -1017,8 +1033,14 @@ impl Vec { /// # process_data(&[1, 2, 3]).expect("why is the test harness OOMing on 12 bytes?"); /// ``` #[stable(feature = "try_reserve", since = "1.57.0")] + #[inline] pub fn try_reserve_exact(&mut self, additional: usize) -> Result<(), TryReserveError> { - self.buf.try_reserve_exact(self.len, additional) + self.buf.try_reserve_exact(self.len, additional)?; + unsafe { + // Inform the optimizer that the reservation has succeeded or wasn't needed + intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); + } + Ok(()) } /// Shrinks the capacity of the vector as much as possible. diff --git a/tests/ui/hygiene/panic-location.run.stderr b/tests/ui/hygiene/panic-location.run.stderr index 5c552411da7f3..915274fd9ef8d 100644 --- a/tests/ui/hygiene/panic-location.run.stderr +++ b/tests/ui/hygiene/panic-location.run.stderr @@ -1,3 +1,3 @@ -thread 'main' panicked at library/alloc/src/raw_vec.rs:571:5: +thread 'main' panicked at library/alloc/src/raw_vec.rs:575:5: capacity overflow note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace From f94a27d0b11e7dc4af5f52d1dedded4e26352241 Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Sat, 30 Dec 2023 22:11:13 -0800 Subject: [PATCH 2/4] Add codegen tests Signed-off-by: Alex Saveau --- tests/codegen/string_reserve_push.rs | 61 ++++++++++++++++++++++++++++ tests/codegen/vec_reserve_extend.rs | 61 ++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 tests/codegen/string_reserve_push.rs create mode 100644 tests/codegen/vec_reserve_extend.rs diff --git a/tests/codegen/string_reserve_push.rs b/tests/codegen/string_reserve_push.rs new file mode 100644 index 0000000000000..ce76e0ed814ec --- /dev/null +++ b/tests/codegen/string_reserve_push.rs @@ -0,0 +1,61 @@ +// compile-flags: -O + +#![crate_type = "lib"] + +use std::arch::asm; + +#[no_mangle] +// CHECK-LABEL: @reserve_push_str( +pub fn reserve_push_str(v: &mut String, s: &str) { + // CHECK: do_reserve_and_handle + // CHECK: "nop" + // CHECK-NOT: do_reserve_and_handle + // CHECK: ret + v.reserve(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.push_str(s); +} + +#[no_mangle] +// CHECK-LABEL: @reserve_exact_push_str( +pub fn reserve_exact_push_str(v: &mut String, s: &str) { + // CHECK: finish_grow + // CHECK: "nop" + // CHECK-NOT: finish_grow + // CHECK: ret + v.reserve_exact(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.push_str(s); +} + +#[no_mangle] +// CHECK-LABEL: @try_reserve_push_str( +pub fn try_reserve_push_str(v: &mut String, s: &str) { + // CHECK: finish_grow + // CHECK: "nop" + // CHECK-NOT: finish_grow + // CHECK: ret + v.try_reserve(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.push_str(s); +} + +#[no_mangle] +// CHECK-LABEL: @try_reserve_exact_push_str( +pub fn try_reserve_exact_push_str(v: &mut String, s: &str) { + // CHECK: finish_grow + // CHECK: "nop" + // CHECK-NOT: finish_grow + // CHECK: ret + v.try_reserve_exact(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.push_str(s); +} diff --git a/tests/codegen/vec_reserve_extend.rs b/tests/codegen/vec_reserve_extend.rs new file mode 100644 index 0000000000000..abd2674a9ca35 --- /dev/null +++ b/tests/codegen/vec_reserve_extend.rs @@ -0,0 +1,61 @@ +// compile-flags: -O + +#![crate_type = "lib"] + +use std::arch::asm; + +#[no_mangle] +// CHECK-LABEL: @reserve_extend( +pub fn reserve_extend(v: &mut Vec, s: &[u8]) { + // CHECK: do_reserve_and_handle + // CHECK: "nop" + // CHECK-NOT: do_reserve_and_handle + // CHECK: ret + v.reserve(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.extend_from_slice(s); +} + +#[no_mangle] +// CHECK-LABEL: @reserve_exact_extend( +pub fn reserve_exact_extend(v: &mut Vec, s: &[u8]) { + // CHECK: finish_grow + // CHECK: "nop" + // CHECK-NOT: finish_grow + // CHECK: ret + v.reserve_exact(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.extend_from_slice(s); +} + +#[no_mangle] +// CHECK-LABEL: @try_reserve_extend( +pub fn try_reserve_extend(v: &mut Vec, s: &[u8]) { + // CHECK: finish_grow + // CHECK: "nop" + // CHECK-NOT: finish_grow + // CHECK: ret + v.try_reserve(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.extend_from_slice(s); +} + +#[no_mangle] +// CHECK-LABEL: @try_reserve_exact_extend( +pub fn try_reserve_exact_extend(v: &mut Vec, s: &[u8]) { + // CHECK: finish_grow + // CHECK: "nop" + // CHECK-NOT: finish_grow + // CHECK: ret + v.try_reserve_exact(s.len()); + unsafe { + asm!("nop"); // Used to make the check logic easier + } + v.extend_from_slice(s); +} From 692788aa22041b73ad5c3286be1ac5a5a9f4f416 Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Sat, 30 Dec 2023 22:12:26 -0800 Subject: [PATCH 3/4] Move to unchecked_sub as suggested by @the8472 Signed-off-by: Alex Saveau --- .../alloc/src/collections/vec_deque/mod.rs | 8 +-- library/alloc/src/raw_vec.rs | 50 ++++++++++------- library/alloc/src/raw_vec/tests.rs | 53 +++++++++++++------ library/alloc/src/string.rs | 8 +-- library/alloc/src/vec/mod.rs | 20 +++++-- library/alloc/src/vec/splice.rs | 4 +- 6 files changed, 96 insertions(+), 47 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index d062587b8f533..2b9b6520b9efb 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -756,8 +756,8 @@ impl VecDeque { let old_cap = self.capacity(); if new_cap > old_cap { - self.buf.reserve_exact(self.len, additional); unsafe { + self.buf.reserve_exact(self.len, additional); self.handle_capacity_increase(old_cap); } } @@ -787,8 +787,8 @@ impl VecDeque { if new_cap > old_cap { // we don't need to reserve_exact(), as the size doesn't have // to be a power of 2. - self.buf.reserve(self.len, additional); unsafe { + self.buf.reserve(self.len, additional); self.handle_capacity_increase(old_cap); } } @@ -838,8 +838,8 @@ impl VecDeque { let old_cap = self.capacity(); if new_cap > old_cap { - self.buf.try_reserve_exact(self.len, additional)?; unsafe { + self.buf.try_reserve_exact(self.len, additional)?; self.handle_capacity_increase(old_cap); } } @@ -886,8 +886,8 @@ impl VecDeque { let old_cap = self.capacity(); if new_cap > old_cap { - self.buf.try_reserve(self.len, additional)?; unsafe { + self.buf.try_reserve(self.len, additional)?; self.handle_capacity_increase(old_cap); } } diff --git a/library/alloc/src/raw_vec.rs b/library/alloc/src/raw_vec.rs index a20e9d6d25226..1776da2d834bf 100644 --- a/library/alloc/src/raw_vec.rs +++ b/library/alloc/src/raw_vec.rs @@ -276,10 +276,6 @@ impl RawVec { /// *O*(1) behavior. Will limit this behavior if it would needlessly cause /// itself to panic. /// - /// If `len` exceeds `self.capacity()`, this may fail to actually allocate - /// the requested space. This is not really unsafe, but the unsafe - /// code *you* write that relies on the behavior of this function may break. - /// /// This is ideal for implementing a bulk-push operation like `extend`. /// /// # Panics @@ -289,9 +285,13 @@ impl RawVec { /// # Aborts /// /// Aborts on OOM. + /// + /// # Safety + /// + /// `len` must be less than or equal to the capacity of this [`RawVec`]. #[cfg(not(no_global_oom_handling))] #[inline] - pub fn reserve(&mut self, len: usize, additional: usize) { + pub unsafe fn reserve(&mut self, len: usize, additional: usize) { // Callers expect this function to be very cheap when there is already sufficient capacity. // Therefore, we move all the resizing and error-handling logic from grow_amortized and // handle_reserve behind a call, while making sure that this function is likely to be @@ -305,7 +305,7 @@ impl RawVec { handle_reserve(slf.grow_amortized(len, additional)); } - if self.needs_to_grow(len, additional) { + if unsafe { self.needs_to_grow(len, additional) } { do_reserve_and_handle(self, len, additional); } unsafe { @@ -323,8 +323,16 @@ impl RawVec { } /// The same as `reserve`, but returns on errors instead of panicking or aborting. - pub fn try_reserve(&mut self, len: usize, additional: usize) -> Result<(), TryReserveError> { - if self.needs_to_grow(len, additional) { + /// + /// # Safety + /// + /// `len` must be less than or equal to the capacity of this [`RawVec`]. + pub unsafe fn try_reserve( + &mut self, + len: usize, + additional: usize, + ) -> Result<(), TryReserveError> { + if unsafe { self.needs_to_grow(len, additional) } { self.grow_amortized(len, additional)?; } unsafe { @@ -340,10 +348,6 @@ impl RawVec { /// exactly the amount of memory necessary, but in principle the allocator /// is free to give back more than we asked for. /// - /// If `len` exceeds `self.capacity()`, this may fail to actually allocate - /// the requested space. This is not really unsafe, but the unsafe code - /// *you* write that relies on the behavior of this function may break. - /// /// # Panics /// /// Panics if the new capacity exceeds `isize::MAX` _bytes_. @@ -351,18 +355,28 @@ impl RawVec { /// # Aborts /// /// Aborts on OOM. + /// + /// # Safety + /// + /// `len` must be less than or equal to the capacity of this [`RawVec`]. #[cfg(not(no_global_oom_handling))] - pub fn reserve_exact(&mut self, len: usize, additional: usize) { - handle_reserve(self.try_reserve_exact(len, additional)); + pub unsafe fn reserve_exact(&mut self, len: usize, additional: usize) { + unsafe { + handle_reserve(self.try_reserve_exact(len, additional)); + } } /// The same as `reserve_exact`, but returns on errors instead of panicking or aborting. - pub fn try_reserve_exact( + /// + /// # Safety + /// + /// `len` must be less than or equal to the capacity of this [`RawVec`]. + pub unsafe fn try_reserve_exact( &mut self, len: usize, additional: usize, ) -> Result<(), TryReserveError> { - if self.needs_to_grow(len, additional) { + if unsafe { self.needs_to_grow(len, additional) } { self.grow_exact(len, additional)?; } unsafe { @@ -391,8 +405,8 @@ impl RawVec { impl RawVec { /// Returns if the buffer needs to grow to fulfill the needed extra capacity. /// Mainly used to make inlining reserve-calls possible without inlining `grow`. - pub(crate) fn needs_to_grow(&self, len: usize, additional: usize) -> bool { - additional > self.capacity().wrapping_sub(len) + pub(crate) unsafe fn needs_to_grow(&self, len: usize, additional: usize) -> bool { + unsafe { additional > self.capacity().unchecked_sub(len) } } /// # Safety: diff --git a/library/alloc/src/raw_vec/tests.rs b/library/alloc/src/raw_vec/tests.rs index f8cada01c0309..f73cf33fb702c 100644 --- a/library/alloc/src/raw_vec/tests.rs +++ b/library/alloc/src/raw_vec/tests.rs @@ -43,7 +43,10 @@ fn allocator_param() { let a = BoundedAlloc { fuel: Cell::new(500) }; let mut v: RawVec = RawVec::with_capacity_in(50, a); assert_eq!(v.alloc.fuel.get(), 450); - v.reserve(50, 150); // (causes a realloc, thus using 50 + 150 = 200 units of fuel) + unsafe { + // (causes a realloc, thus using 50 + 150 = 200 units of fuel) + v.reserve(50, 150); + } assert_eq!(v.alloc.fuel.get(), 250); } @@ -52,25 +55,35 @@ fn reserve_does_not_overallocate() { { let mut v: RawVec = RawVec::new(); // First, `reserve` allocates like `reserve_exact`. - v.reserve(0, 9); + unsafe { + v.reserve(0, 9); + } assert_eq!(9, v.capacity()); } { let mut v: RawVec = RawVec::new(); - v.reserve(0, 7); + unsafe { + v.reserve(0, 7); + } assert_eq!(7, v.capacity()); // 97 is more than double of 7, so `reserve` should work // like `reserve_exact`. - v.reserve(7, 90); + unsafe { + v.reserve(7, 90); + } assert_eq!(97, v.capacity()); } { let mut v: RawVec = RawVec::new(); - v.reserve(0, 12); + unsafe { + v.reserve(0, 12); + } assert_eq!(12, v.capacity()); - v.reserve(12, 3); + unsafe { + v.reserve(12, 3); + } // 3 is less than half of 12, so `reserve` must grow // exponentially. At the time of writing this test grow // factor is 2, so new capacity is 24, however, grow factor @@ -116,24 +129,28 @@ fn zst() { // Check all these operations work as expected with zero-sized elements. - assert!(!v.needs_to_grow(100, usize::MAX - 100)); - assert!(v.needs_to_grow(101, usize::MAX - 100)); + assert!(unsafe { !v.needs_to_grow(100, usize::MAX - 100) }); + assert!(unsafe { v.needs_to_grow(101, usize::MAX - 100) }); zst_sanity(&v); - v.reserve(100, usize::MAX - 100); + unsafe { + v.reserve(100, usize::MAX - 100); + } //v.reserve(101, usize::MAX - 100); // panics, in `zst_reserve_panic` below zst_sanity(&v); - v.reserve_exact(100, usize::MAX - 100); + unsafe { + v.reserve_exact(100, usize::MAX - 100); + } //v.reserve_exact(101, usize::MAX - 100); // panics, in `zst_reserve_exact_panic` below zst_sanity(&v); - assert_eq!(v.try_reserve(100, usize::MAX - 100), Ok(())); - assert_eq!(v.try_reserve(101, usize::MAX - 100), cap_err); + assert_eq!(unsafe { v.try_reserve(100, usize::MAX - 100) }, Ok(())); + assert_eq!(unsafe { v.try_reserve(101, usize::MAX - 100) }, cap_err); zst_sanity(&v); - assert_eq!(v.try_reserve_exact(100, usize::MAX - 100), Ok(())); - assert_eq!(v.try_reserve_exact(101, usize::MAX - 100), cap_err); + assert_eq!(unsafe { v.try_reserve_exact(100, usize::MAX - 100) }, Ok(())); + assert_eq!(unsafe { v.try_reserve_exact(101, usize::MAX - 100) }, cap_err); zst_sanity(&v); assert_eq!(v.grow_amortized(100, usize::MAX - 100), cap_err); @@ -151,7 +168,9 @@ fn zst_reserve_panic() { let mut v: RawVec = RawVec::new(); zst_sanity(&v); - v.reserve(101, usize::MAX - 100); + unsafe { + v.reserve(101, usize::MAX - 100); + } } #[test] @@ -160,7 +179,9 @@ fn zst_reserve_exact_panic() { let mut v: RawVec = RawVec::new(); zst_sanity(&v); - v.reserve_exact(101, usize::MAX - 100); + unsafe { + v.reserve_exact(101, usize::MAX - 100); + } } #[test] diff --git a/library/alloc/src/string.rs b/library/alloc/src/string.rs index 00d89b929d5b0..68306ceb082f2 100644 --- a/library/alloc/src/string.rs +++ b/library/alloc/src/string.rs @@ -1152,7 +1152,7 @@ impl String { self.vec.reserve(additional); unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); } } @@ -1206,7 +1206,7 @@ impl String { self.vec.reserve_exact(additional); unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); } } @@ -1246,7 +1246,7 @@ impl String { self.vec.try_reserve(additional)?; unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); } Ok(()) } @@ -1293,7 +1293,7 @@ impl String { self.vec.try_reserve_exact(additional)?; unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); + intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); } Ok(()) } diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index 562c767425f66..bd9565a8452d6 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -909,7 +909,10 @@ impl Vec { #[stable(feature = "rust1", since = "1.0.0")] #[inline] pub fn reserve(&mut self, additional: usize) { - self.buf.reserve(self.len, additional); + // SAFETY: len <= capacity + unsafe { + self.buf.reserve(self.len, additional); + } unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); @@ -944,7 +947,10 @@ impl Vec { #[stable(feature = "rust1", since = "1.0.0")] #[inline] pub fn reserve_exact(&mut self, additional: usize) { - self.buf.reserve_exact(self.len, additional); + // SAFETY: len <= capacity + unsafe { + self.buf.reserve_exact(self.len, additional); + } unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); @@ -986,7 +992,10 @@ impl Vec { #[stable(feature = "try_reserve", since = "1.57.0")] #[inline] pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> { - self.buf.try_reserve(self.len, additional)?; + // SAFETY: len <= capacity + unsafe { + self.buf.try_reserve(self.len, additional)?; + } unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); @@ -1035,7 +1044,10 @@ impl Vec { #[stable(feature = "try_reserve", since = "1.57.0")] #[inline] pub fn try_reserve_exact(&mut self, additional: usize) -> Result<(), TryReserveError> { - self.buf.try_reserve_exact(self.len, additional)?; + // SAFETY: len <= capacity + unsafe { + self.buf.try_reserve_exact(self.len, additional)?; + } unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); diff --git a/library/alloc/src/vec/splice.rs b/library/alloc/src/vec/splice.rs index 852fdcc3f5ce7..c5d731147390a 100644 --- a/library/alloc/src/vec/splice.rs +++ b/library/alloc/src/vec/splice.rs @@ -126,7 +126,9 @@ impl Drain<'_, T, A> { unsafe fn move_tail(&mut self, additional: usize) { let vec = unsafe { self.vec.as_mut() }; let len = self.tail_start + self.tail_len; - vec.buf.reserve(len, additional); + unsafe { + vec.buf.reserve(len, additional); + } let new_tail_start = self.tail_start + additional; unsafe { From a4b629e35f656e82d631d7de918960c58a9fc813 Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Tue, 2 Jan 2024 09:38:23 -0800 Subject: [PATCH 4/4] Revert "Move to unchecked_sub as suggested by @the8472" This reverts commit 1dfff2e4afff50123a3e09e455b345bdeba4810f. --- .../alloc/src/collections/vec_deque/mod.rs | 8 +-- library/alloc/src/raw_vec.rs | 50 +++++++---------- library/alloc/src/raw_vec/tests.rs | 53 ++++++------------- library/alloc/src/string.rs | 8 +-- library/alloc/src/vec/mod.rs | 20 ++----- library/alloc/src/vec/splice.rs | 4 +- 6 files changed, 47 insertions(+), 96 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index 2b9b6520b9efb..d062587b8f533 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -756,8 +756,8 @@ impl VecDeque { let old_cap = self.capacity(); if new_cap > old_cap { + self.buf.reserve_exact(self.len, additional); unsafe { - self.buf.reserve_exact(self.len, additional); self.handle_capacity_increase(old_cap); } } @@ -787,8 +787,8 @@ impl VecDeque { if new_cap > old_cap { // we don't need to reserve_exact(), as the size doesn't have // to be a power of 2. + self.buf.reserve(self.len, additional); unsafe { - self.buf.reserve(self.len, additional); self.handle_capacity_increase(old_cap); } } @@ -838,8 +838,8 @@ impl VecDeque { let old_cap = self.capacity(); if new_cap > old_cap { + self.buf.try_reserve_exact(self.len, additional)?; unsafe { - self.buf.try_reserve_exact(self.len, additional)?; self.handle_capacity_increase(old_cap); } } @@ -886,8 +886,8 @@ impl VecDeque { let old_cap = self.capacity(); if new_cap > old_cap { + self.buf.try_reserve(self.len, additional)?; unsafe { - self.buf.try_reserve(self.len, additional)?; self.handle_capacity_increase(old_cap); } } diff --git a/library/alloc/src/raw_vec.rs b/library/alloc/src/raw_vec.rs index 1776da2d834bf..a20e9d6d25226 100644 --- a/library/alloc/src/raw_vec.rs +++ b/library/alloc/src/raw_vec.rs @@ -276,6 +276,10 @@ impl RawVec { /// *O*(1) behavior. Will limit this behavior if it would needlessly cause /// itself to panic. /// + /// If `len` exceeds `self.capacity()`, this may fail to actually allocate + /// the requested space. This is not really unsafe, but the unsafe + /// code *you* write that relies on the behavior of this function may break. + /// /// This is ideal for implementing a bulk-push operation like `extend`. /// /// # Panics @@ -285,13 +289,9 @@ impl RawVec { /// # Aborts /// /// Aborts on OOM. - /// - /// # Safety - /// - /// `len` must be less than or equal to the capacity of this [`RawVec`]. #[cfg(not(no_global_oom_handling))] #[inline] - pub unsafe fn reserve(&mut self, len: usize, additional: usize) { + pub fn reserve(&mut self, len: usize, additional: usize) { // Callers expect this function to be very cheap when there is already sufficient capacity. // Therefore, we move all the resizing and error-handling logic from grow_amortized and // handle_reserve behind a call, while making sure that this function is likely to be @@ -305,7 +305,7 @@ impl RawVec { handle_reserve(slf.grow_amortized(len, additional)); } - if unsafe { self.needs_to_grow(len, additional) } { + if self.needs_to_grow(len, additional) { do_reserve_and_handle(self, len, additional); } unsafe { @@ -323,16 +323,8 @@ impl RawVec { } /// The same as `reserve`, but returns on errors instead of panicking or aborting. - /// - /// # Safety - /// - /// `len` must be less than or equal to the capacity of this [`RawVec`]. - pub unsafe fn try_reserve( - &mut self, - len: usize, - additional: usize, - ) -> Result<(), TryReserveError> { - if unsafe { self.needs_to_grow(len, additional) } { + pub fn try_reserve(&mut self, len: usize, additional: usize) -> Result<(), TryReserveError> { + if self.needs_to_grow(len, additional) { self.grow_amortized(len, additional)?; } unsafe { @@ -348,6 +340,10 @@ impl RawVec { /// exactly the amount of memory necessary, but in principle the allocator /// is free to give back more than we asked for. /// + /// If `len` exceeds `self.capacity()`, this may fail to actually allocate + /// the requested space. This is not really unsafe, but the unsafe code + /// *you* write that relies on the behavior of this function may break. + /// /// # Panics /// /// Panics if the new capacity exceeds `isize::MAX` _bytes_. @@ -355,28 +351,18 @@ impl RawVec { /// # Aborts /// /// Aborts on OOM. - /// - /// # Safety - /// - /// `len` must be less than or equal to the capacity of this [`RawVec`]. #[cfg(not(no_global_oom_handling))] - pub unsafe fn reserve_exact(&mut self, len: usize, additional: usize) { - unsafe { - handle_reserve(self.try_reserve_exact(len, additional)); - } + pub fn reserve_exact(&mut self, len: usize, additional: usize) { + handle_reserve(self.try_reserve_exact(len, additional)); } /// The same as `reserve_exact`, but returns on errors instead of panicking or aborting. - /// - /// # Safety - /// - /// `len` must be less than or equal to the capacity of this [`RawVec`]. - pub unsafe fn try_reserve_exact( + pub fn try_reserve_exact( &mut self, len: usize, additional: usize, ) -> Result<(), TryReserveError> { - if unsafe { self.needs_to_grow(len, additional) } { + if self.needs_to_grow(len, additional) { self.grow_exact(len, additional)?; } unsafe { @@ -405,8 +391,8 @@ impl RawVec { impl RawVec { /// Returns if the buffer needs to grow to fulfill the needed extra capacity. /// Mainly used to make inlining reserve-calls possible without inlining `grow`. - pub(crate) unsafe fn needs_to_grow(&self, len: usize, additional: usize) -> bool { - unsafe { additional > self.capacity().unchecked_sub(len) } + pub(crate) fn needs_to_grow(&self, len: usize, additional: usize) -> bool { + additional > self.capacity().wrapping_sub(len) } /// # Safety: diff --git a/library/alloc/src/raw_vec/tests.rs b/library/alloc/src/raw_vec/tests.rs index f73cf33fb702c..f8cada01c0309 100644 --- a/library/alloc/src/raw_vec/tests.rs +++ b/library/alloc/src/raw_vec/tests.rs @@ -43,10 +43,7 @@ fn allocator_param() { let a = BoundedAlloc { fuel: Cell::new(500) }; let mut v: RawVec = RawVec::with_capacity_in(50, a); assert_eq!(v.alloc.fuel.get(), 450); - unsafe { - // (causes a realloc, thus using 50 + 150 = 200 units of fuel) - v.reserve(50, 150); - } + v.reserve(50, 150); // (causes a realloc, thus using 50 + 150 = 200 units of fuel) assert_eq!(v.alloc.fuel.get(), 250); } @@ -55,35 +52,25 @@ fn reserve_does_not_overallocate() { { let mut v: RawVec = RawVec::new(); // First, `reserve` allocates like `reserve_exact`. - unsafe { - v.reserve(0, 9); - } + v.reserve(0, 9); assert_eq!(9, v.capacity()); } { let mut v: RawVec = RawVec::new(); - unsafe { - v.reserve(0, 7); - } + v.reserve(0, 7); assert_eq!(7, v.capacity()); // 97 is more than double of 7, so `reserve` should work // like `reserve_exact`. - unsafe { - v.reserve(7, 90); - } + v.reserve(7, 90); assert_eq!(97, v.capacity()); } { let mut v: RawVec = RawVec::new(); - unsafe { - v.reserve(0, 12); - } + v.reserve(0, 12); assert_eq!(12, v.capacity()); - unsafe { - v.reserve(12, 3); - } + v.reserve(12, 3); // 3 is less than half of 12, so `reserve` must grow // exponentially. At the time of writing this test grow // factor is 2, so new capacity is 24, however, grow factor @@ -129,28 +116,24 @@ fn zst() { // Check all these operations work as expected with zero-sized elements. - assert!(unsafe { !v.needs_to_grow(100, usize::MAX - 100) }); - assert!(unsafe { v.needs_to_grow(101, usize::MAX - 100) }); + assert!(!v.needs_to_grow(100, usize::MAX - 100)); + assert!(v.needs_to_grow(101, usize::MAX - 100)); zst_sanity(&v); - unsafe { - v.reserve(100, usize::MAX - 100); - } + v.reserve(100, usize::MAX - 100); //v.reserve(101, usize::MAX - 100); // panics, in `zst_reserve_panic` below zst_sanity(&v); - unsafe { - v.reserve_exact(100, usize::MAX - 100); - } + v.reserve_exact(100, usize::MAX - 100); //v.reserve_exact(101, usize::MAX - 100); // panics, in `zst_reserve_exact_panic` below zst_sanity(&v); - assert_eq!(unsafe { v.try_reserve(100, usize::MAX - 100) }, Ok(())); - assert_eq!(unsafe { v.try_reserve(101, usize::MAX - 100) }, cap_err); + assert_eq!(v.try_reserve(100, usize::MAX - 100), Ok(())); + assert_eq!(v.try_reserve(101, usize::MAX - 100), cap_err); zst_sanity(&v); - assert_eq!(unsafe { v.try_reserve_exact(100, usize::MAX - 100) }, Ok(())); - assert_eq!(unsafe { v.try_reserve_exact(101, usize::MAX - 100) }, cap_err); + assert_eq!(v.try_reserve_exact(100, usize::MAX - 100), Ok(())); + assert_eq!(v.try_reserve_exact(101, usize::MAX - 100), cap_err); zst_sanity(&v); assert_eq!(v.grow_amortized(100, usize::MAX - 100), cap_err); @@ -168,9 +151,7 @@ fn zst_reserve_panic() { let mut v: RawVec = RawVec::new(); zst_sanity(&v); - unsafe { - v.reserve(101, usize::MAX - 100); - } + v.reserve(101, usize::MAX - 100); } #[test] @@ -179,9 +160,7 @@ fn zst_reserve_exact_panic() { let mut v: RawVec = RawVec::new(); zst_sanity(&v); - unsafe { - v.reserve_exact(101, usize::MAX - 100); - } + v.reserve_exact(101, usize::MAX - 100); } #[test] diff --git a/library/alloc/src/string.rs b/library/alloc/src/string.rs index 68306ceb082f2..00d89b929d5b0 100644 --- a/library/alloc/src/string.rs +++ b/library/alloc/src/string.rs @@ -1152,7 +1152,7 @@ impl String { self.vec.reserve(additional); unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); } } @@ -1206,7 +1206,7 @@ impl String { self.vec.reserve_exact(additional); unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); } } @@ -1246,7 +1246,7 @@ impl String { self.vec.try_reserve(additional)?; unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); } Ok(()) } @@ -1293,7 +1293,7 @@ impl String { self.vec.try_reserve_exact(additional)?; unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed - intrinsics::assume(additional <= self.capacity().unchecked_sub(self.len())); + intrinsics::assume(additional <= self.capacity().wrapping_sub(self.len())); } Ok(()) } diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index bd9565a8452d6..562c767425f66 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -909,10 +909,7 @@ impl Vec { #[stable(feature = "rust1", since = "1.0.0")] #[inline] pub fn reserve(&mut self, additional: usize) { - // SAFETY: len <= capacity - unsafe { - self.buf.reserve(self.len, additional); - } + self.buf.reserve(self.len, additional); unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); @@ -947,10 +944,7 @@ impl Vec { #[stable(feature = "rust1", since = "1.0.0")] #[inline] pub fn reserve_exact(&mut self, additional: usize) { - // SAFETY: len <= capacity - unsafe { - self.buf.reserve_exact(self.len, additional); - } + self.buf.reserve_exact(self.len, additional); unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); @@ -992,10 +986,7 @@ impl Vec { #[stable(feature = "try_reserve", since = "1.57.0")] #[inline] pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> { - // SAFETY: len <= capacity - unsafe { - self.buf.try_reserve(self.len, additional)?; - } + self.buf.try_reserve(self.len, additional)?; unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); @@ -1044,10 +1035,7 @@ impl Vec { #[stable(feature = "try_reserve", since = "1.57.0")] #[inline] pub fn try_reserve_exact(&mut self, additional: usize) -> Result<(), TryReserveError> { - // SAFETY: len <= capacity - unsafe { - self.buf.try_reserve_exact(self.len, additional)?; - } + self.buf.try_reserve_exact(self.len, additional)?; unsafe { // Inform the optimizer that the reservation has succeeded or wasn't needed intrinsics::assume(!self.buf.needs_to_grow(self.len, additional)); diff --git a/library/alloc/src/vec/splice.rs b/library/alloc/src/vec/splice.rs index c5d731147390a..852fdcc3f5ce7 100644 --- a/library/alloc/src/vec/splice.rs +++ b/library/alloc/src/vec/splice.rs @@ -126,9 +126,7 @@ impl Drain<'_, T, A> { unsafe fn move_tail(&mut self, additional: usize) { let vec = unsafe { self.vec.as_mut() }; let len = self.tail_start + self.tail_len; - unsafe { - vec.buf.reserve(len, additional); - } + vec.buf.reserve(len, additional); let new_tail_start = self.tail_start + additional; unsafe {