From 8a82a55455bef11fd5575f1dc26051d933cc916e Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 23 Sep 2022 10:10:18 -0700 Subject: [PATCH 1/6] dtor --- utils/zerovec/src/zerovec/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/utils/zerovec/src/zerovec/mod.rs b/utils/zerovec/src/zerovec/mod.rs index dc7e83752c5..57808d71ced 100644 --- a/utils/zerovec/src/zerovec/mod.rs +++ b/utils/zerovec/src/zerovec/mod.rs @@ -109,6 +109,14 @@ impl<'a, T: AsULE> Deref for ZeroVec<'a, T> { } } +impl<'a, T: AsULE> Drop for ZeroVec<'a, T> { + #[inline] + fn drop(&mut self) { + let this = mem::take(self); + let _ = this.into_cow(); + } +} + impl<'a, T: AsULE> Clone for ZeroVec<'a, T> { fn clone(&self) -> Self { if self.is_owned() { @@ -786,6 +794,10 @@ where Cow::Owned(vec) } else { let slice = unsafe { &*self.buf }; + // The borrowed destructor is a no-op, but we want to prevent + // the check being run (and also prevent recursion in our Drop + // impl, which calls into_cow) + mem::forget(self); Cow::Borrowed(slice) } } From 5e70e0bfe41894801318206c59e03bceb2485b8d Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 23 Sep 2022 11:09:04 -0700 Subject: [PATCH 2/6] docs and tests --- utils/zerovec/src/zerovec/mod.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/utils/zerovec/src/zerovec/mod.rs b/utils/zerovec/src/zerovec/mod.rs index 57808d71ced..074613e1513 100644 --- a/utils/zerovec/src/zerovec/mod.rs +++ b/utils/zerovec/src/zerovec/mod.rs @@ -88,6 +88,8 @@ where T: AsULE, { /// Pointer to data + /// This pointer is *always* valid, the reason it is represented as a raw pointer + /// is that it may logically represent an `&[T::ULE]` or the ptr,len of a `Vec` buf: *mut [T::ULE], /// Borrowed if zero. Capacity of buffer above if not capacity: usize, @@ -97,14 +99,20 @@ where marker: PhantomData<(Vec, &'a [T::ULE])>, } +// Send inherits as long as all fields are Send, but also references are Send only +// when their contents are Sync (this is the core purpose of Sync), so +// we need a Send+Sync bound since this struct can logically be a vector or a slice. unsafe impl<'a, T: AsULE> Send for ZeroVec<'a, T> where T::ULE: Send + Sync {} +// Sync typically inherits as long as all fields are Sync unsafe impl<'a, T: AsULE> Sync for ZeroVec<'a, T> where T::ULE: Sync {} impl<'a, T: AsULE> Deref for ZeroVec<'a, T> { type Target = ZeroSlice; #[inline] fn deref(&self) -> &Self::Target { - let slice = unsafe { &*self.buf }; + // `buf` is either a valid vector or slice of `T::ULE`s, either + // way it's always valid + let slice: &[T::ULE] = unsafe { &*self.buf }; ZeroSlice::from_ule_slice(slice) } } @@ -224,6 +232,9 @@ where /// [`Self::alloc_from_slice()`]. #[inline] pub fn new_owned(vec: Vec) -> Self { + // Deconstruct the vector into parts + // This is the only part of the code that goes from Vec + // to ZeroVec, all other such operations should use this function let slice: &[T::ULE] = &*vec; let slice = slice as *const [_] as *mut [_]; let capacity = vec.capacity(); @@ -480,6 +491,8 @@ where if self.is_owned() { None } else { + // `buf` is either a valid vector or slice of `T::ULE`s, either + // way it's always valid let ule_slice = unsafe { &*self.buf }; Some(ZeroSlice::from_ule_slice(ule_slice)) } @@ -726,7 +739,7 @@ where /// /// # Example /// - /// ```rust,ignore + /// ```rust /// # use crate::zerovec::ule::AsULE; /// use zerovec::ZeroVec; /// @@ -756,7 +769,7 @@ where /// /// # Example /// - /// ```rust,ignore + /// ```rust /// # use crate::zerovec::ule::AsULE; /// use zerovec::ZeroVec; /// @@ -764,11 +777,13 @@ where /// let mut zerovec: ZeroVec = ZeroVec::parse_byte_slice(bytes).expect("infallible"); /// assert!(!zerovec.is_owned()); /// - /// zerovec.to_mut_slice().push(12_u16.to_unaligned()); + /// zerovec.to_mut_slice()[1] = 5u16.to_unaligned(); /// assert!(zerovec.is_owned()); /// ``` pub fn to_mut_slice(&mut self) -> &mut [T::ULE] { if !self.is_owned() { + // `buf` is either a valid vector or slice of `T::ULE`s, either + // way it's always valid let slice = unsafe { &*self.buf }; *self = ZeroVec::new_owned(slice.into()); } @@ -785,6 +800,9 @@ where pub fn into_cow(self) -> Cow<'a, [T::ULE]> { if self.is_owned() { let vec = unsafe { + // If self is owned, then we know + // for a fact that it came from the parts of a Vec + // (via Self::new_owned()) let len = (&*self.buf).len(); let ptr = self.buf as *mut T::ULE; let capacity = self.capacity; @@ -793,6 +811,8 @@ where }; Cow::Owned(vec) } else { + // `buf` is either a valid vector or slice of `T::ULE`s, either + // way it's always valid let slice = unsafe { &*self.buf }; // The borrowed destructor is a no-op, but we want to prevent // the check being run (and also prevent recursion in our Drop From de21050581ed6cf28913fbeecf2aac47d3f878a5 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 23 Sep 2022 12:57:21 -0700 Subject: [PATCH 3/6] may_dangle --- utils/zerovec/src/zerovec/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/utils/zerovec/src/zerovec/mod.rs b/utils/zerovec/src/zerovec/mod.rs index 074613e1513..c84faddf3cf 100644 --- a/utils/zerovec/src/zerovec/mod.rs +++ b/utils/zerovec/src/zerovec/mod.rs @@ -117,6 +117,8 @@ impl<'a, T: AsULE> Deref for ZeroVec<'a, T> { } } +// Todo: after https://github.com/rust-lang/rust/issues/34761 stabilizes, +// use #[may_dangle] impl<'a, T: AsULE> Drop for ZeroVec<'a, T> { #[inline] fn drop(&mut self) { From 6d05d6f377c093fe53a0edf2d81b9a65593fc224 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 23 Sep 2022 14:16:41 -0700 Subject: [PATCH 4/6] Add EyepatchHackVector --- utils/zerovec/src/zerovec/mod.rs | 102 ++++++++++++++++++++----------- 1 file changed, 68 insertions(+), 34 deletions(-) diff --git a/utils/zerovec/src/zerovec/mod.rs b/utils/zerovec/src/zerovec/mod.rs index c84faddf3cf..3aa0feefdd5 100644 --- a/utils/zerovec/src/zerovec/mod.rs +++ b/utils/zerovec/src/zerovec/mod.rs @@ -87,12 +87,8 @@ pub struct ZeroVec<'a, T> where T: AsULE, { - /// Pointer to data - /// This pointer is *always* valid, the reason it is represented as a raw pointer - /// is that it may logically represent an `&[T::ULE]` or the ptr,len of a `Vec` - buf: *mut [T::ULE], - /// Borrowed if zero. Capacity of buffer above if not - capacity: usize, + vector: EyepatchHackVector, + /// Marker type, signalling variance and dropck behavior /// by containing all potential types this type represents #[allow(clippy::type_complexity)] // needed to get correct marker type behavior @@ -110,20 +106,53 @@ impl<'a, T: AsULE> Deref for ZeroVec<'a, T> { type Target = ZeroSlice; #[inline] fn deref(&self) -> &Self::Target { - // `buf` is either a valid vector or slice of `T::ULE`s, either - // way it's always valid - let slice: &[T::ULE] = unsafe { &*self.buf }; + // + let slice: &[T::ULE] = self.vector.as_slice(); ZeroSlice::from_ule_slice(slice) } } -// Todo: after https://github.com/rust-lang/rust/issues/34761 stabilizes, -// use #[may_dangle] -impl<'a, T: AsULE> Drop for ZeroVec<'a, T> { +// Represents an unsafe potentially-owned vector/slice type, without a lifetime +// working around dropck limitations. +// +// Must either be constructed by deconstructing a Vec, or from &[U] with capacity set to +// zero. Should not outlive its source &[U] in the borrowed case; this type does not in +// and of itself uphold this guarantee, but the .as_slice() method assumes it. +// +// After https://github.com/rust-lang/rust/issues/34761 stabilizes, +// we should remove this type and use #[may_dangle] +struct EyepatchHackVector { + /// Pointer to data + /// This pointer is *always* valid, the reason it is represented as a raw pointer + /// is that it may logically represent an `&[T::ULE]` or the ptr,len of a `Vec` + buf: *mut [U], + /// Borrowed if zero. Capacity of buffer above if not + capacity: usize, +} + +impl EyepatchHackVector { + // Return a slice to the inner data for an arbitrary caller-specified lifetime + #[inline] + unsafe fn as_arbitrary_slice<'a>(&self) -> &'a [U] { + &*self.buf + } + // Return a slice to the inner data + #[inline] + fn as_slice<'a>(&'a self) -> &'a [U] { + unsafe { &*self.buf } + } +} + +impl Drop for EyepatchHackVector { #[inline] fn drop(&mut self) { - let this = mem::take(self); - let _ = this.into_cow(); + if self.capacity != 0 { + let slice: &[U] = self.as_slice(); + let len = slice.len(); + unsafe { + let _: Vec = Vec::from_raw_parts(self.buf as *mut U, len, self.capacity); + } + } } } @@ -133,8 +162,10 @@ impl<'a, T: AsULE> Clone for ZeroVec<'a, T> { ZeroVec::new_owned(self.as_ule_slice().into()) } else { Self { - buf: self.buf, - capacity: 0, + vector: EyepatchHackVector { + buf: self.vector.buf, + capacity: 0, + }, marker: PhantomData, } } @@ -242,8 +273,10 @@ where let capacity = vec.capacity(); mem::forget(vec); Self { - buf: slice, - capacity, + vector: EyepatchHackVector { + buf: slice, + capacity, + }, marker: PhantomData, } } @@ -254,8 +287,10 @@ where pub const fn new_borrowed(slice: &'a [T::ULE]) -> Self { let slice = slice as *const [_] as *mut [_]; Self { - buf: slice, - capacity: 0, + vector: EyepatchHackVector { + buf: slice, + capacity: 0, + }, marker: PhantomData, } } @@ -483,7 +518,7 @@ where /// Check if this type is fully owned #[inline] pub fn is_owned(&self) -> bool { - self.capacity != 0 + self.vector.capacity != 0 } /// If this is a borrowed ZeroVec, return it as a slice that covers @@ -493,9 +528,9 @@ where if self.is_owned() { None } else { - // `buf` is either a valid vector or slice of `T::ULE`s, either - // way it's always valid - let ule_slice = unsafe { &*self.buf }; + // We can extend the lifetime of the slice to 'a + // since we know it is borrowed + let ule_slice = unsafe { self.vector.as_arbitrary_slice() }; Some(ZeroSlice::from_ule_slice(ule_slice)) } } @@ -786,10 +821,10 @@ where if !self.is_owned() { // `buf` is either a valid vector or slice of `T::ULE`s, either // way it's always valid - let slice = unsafe { &*self.buf }; + let slice = self.vector.as_slice(); *self = ZeroVec::new_owned(slice.into()); } - unsafe { &mut *self.buf } + unsafe { &mut *self.vector.buf } } /// Remove all elements from this ZeroVec and reset it to an empty borrowed state. pub fn clear(&mut self) { @@ -805,20 +840,19 @@ where // If self is owned, then we know // for a fact that it came from the parts of a Vec // (via Self::new_owned()) - let len = (&*self.buf).len(); - let ptr = self.buf as *mut T::ULE; - let capacity = self.capacity; + let len = (&*self.vector.buf).len(); + let ptr = self.vector.buf as *mut T::ULE; + let capacity = self.vector.capacity; mem::forget(self); Vec::from_raw_parts(ptr, len, capacity) }; Cow::Owned(vec) } else { - // `buf` is either a valid vector or slice of `T::ULE`s, either - // way it's always valid - let slice = unsafe { &*self.buf }; + // We can extend the lifetime of the slice to 'a + // since we know it is borrowed + let slice = unsafe { self.vector.as_arbitrary_slice() }; // The borrowed destructor is a no-op, but we want to prevent - // the check being run (and also prevent recursion in our Drop - // impl, which calls into_cow) + // the check being run mem::forget(self); Cow::Borrowed(slice) } From bb0788f3914edd0ca4c030875d594e0e4f3c41b9 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 23 Sep 2022 22:26:03 +0000 Subject: [PATCH 5/6] Update utils/zerovec/src/zerovec/mod.rs Co-authored-by: Shane F. Carr --- utils/zerovec/src/zerovec/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/utils/zerovec/src/zerovec/mod.rs b/utils/zerovec/src/zerovec/mod.rs index 3aa0feefdd5..8514c38ad23 100644 --- a/utils/zerovec/src/zerovec/mod.rs +++ b/utils/zerovec/src/zerovec/mod.rs @@ -106,7 +106,6 @@ impl<'a, T: AsULE> Deref for ZeroVec<'a, T> { type Target = ZeroSlice; #[inline] fn deref(&self) -> &Self::Target { - // let slice: &[T::ULE] = self.vector.as_slice(); ZeroSlice::from_ule_slice(slice) } From c85a85f66cb452805af769d6922ab66309e05974 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 23 Sep 2022 15:36:08 -0700 Subject: [PATCH 6/6] review --- utils/zerovec/src/zerovec/mod.rs | 34 +++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/utils/zerovec/src/zerovec/mod.rs b/utils/zerovec/src/zerovec/mod.rs index 3aa0feefdd5..ea3bff754ca 100644 --- a/utils/zerovec/src/zerovec/mod.rs +++ b/utils/zerovec/src/zerovec/mod.rs @@ -141,16 +141,32 @@ impl EyepatchHackVector { fn as_slice<'a>(&'a self) -> &'a [U] { unsafe { &*self.buf } } + + /// Return this type as a vector + /// + /// Data MUST be known to be owned beforehand + /// + /// Because this borrows self, this is effectively creating two owners to the same + /// data, make sure that `self` is cleaned up after this + /// + /// (this does not simply take `self` since then it wouldn't be usable from the Drop impl) + unsafe fn get_vec(&self) -> Vec { + debug_assert!(self.capacity != 0); + let slice: &[U] = self.as_slice(); + let len = slice.len(); + // Safety: we are assuming owned, and in owned cases + // this always represents a valid vector + Vec::from_raw_parts(self.buf as *mut U, len, self.capacity) + } } impl Drop for EyepatchHackVector { #[inline] fn drop(&mut self) { if self.capacity != 0 { - let slice: &[U] = self.as_slice(); - let len = slice.len(); unsafe { - let _: Vec = Vec::from_raw_parts(self.buf as *mut U, len, self.capacity); + // we don't need to clean up self here since we're already in a Drop impl + let _ = self.get_vec(); } } } @@ -837,15 +853,11 @@ where pub fn into_cow(self) -> Cow<'a, [T::ULE]> { if self.is_owned() { let vec = unsafe { - // If self is owned, then we know - // for a fact that it came from the parts of a Vec - // (via Self::new_owned()) - let len = (&*self.vector.buf).len(); - let ptr = self.vector.buf as *mut T::ULE; - let capacity = self.vector.capacity; - mem::forget(self); - Vec::from_raw_parts(ptr, len, capacity) + // safe to call: we know it's owned, + // and we mem::forget self immediately afterwards + self.vector.get_vec() }; + mem::forget(self); Cow::Owned(vec) } else { // We can extend the lifetime of the slice to 'a