Replace owning_ref with a safer datastructure

compiler/rustc_codegen_ssa/src/back/metadata.rs

@@ -13,8 +13,7 @@ use object::{
 use snap::write::FrameEncoder;
 
 use rustc_data_structures::memmap::Mmap;
-use rustc_data_structures::owning_ref::OwningRef;
-use rustc_data_structures::rustc_erase_owner;
+use rustc_data_structures::owned_slice::OwnedSlice;
 use rustc_data_structures::sync::MetadataRef;
 use rustc_metadata::EncodedMetadata;
 use rustc_session::cstore::MetadataLoader;
@@ -44,8 +43,11 @@ fn load_metadata_with(
         File::open(path).map_err(|e| format!("failed to open file '{}': {}", path.display(), e))?;
     let data = unsafe { Mmap::map(file) }
         .map_err(|e| format!("failed to mmap file '{}': {}", path.display(), e))?;
-    let metadata = OwningRef::new(data).try_map(f)?;
-    return Ok(rustc_erase_owner!(metadata.map_owner_box()));
+
+    let metadata: MetadataRef = OwnedSlice::new(Box::new(data));
+    let metadata = metadata.try_map(f)?;
+
+    Ok(metadata)
 }
 
 impl MetadataLoader for DefaultMetadataLoader {

compiler/rustc_data_structures/src/lib.rs

@@ -26,6 +26,7 @@
 #![feature(test)]
 #![feature(thread_id_value)]
 #![feature(vec_into_raw_parts)]
+#![feature(ptr_sub_ptr)]
 #![allow(rustc::default_hash_types)]
 #![allow(rustc::potential_query_instability)]
 
@@ -76,7 +77,6 @@ pub mod jobserver;
 pub mod macros;
 pub mod map_in_place;
 pub mod obligation_forest;
-pub mod owning_ref;
 pub mod sip128;
 pub mod small_c_str;
 pub mod small_str;
@@ -103,6 +103,7 @@ pub mod vec_map;
 pub mod work_queue;
 pub use atomic_ref::AtomicRef;
 pub mod frozen;
+pub mod owned_slice;
 pub mod sso;
 pub mod steal;
 pub mod tagged_ptr;

compiler/rustc_data_structures/src/memmap.rs

@@ -2,9 +2,10 @@ use std::fs::File;
 use std::io;
 use std::ops::Deref;
 
-use crate::owning_ref::StableAddress;
+// FIXME(nilstrieb): This was created to implement owning_ref::StableAddress.
+// Since owning_ref has been deleted, this can be removed as well.
 
-/// A trivial wrapper for [`memmap2::Mmap`] that implements [`StableAddress`].
+/// A trivial wrapper for [`memmap2::Mmap`] that implements owning_ref::StableAddress.
 #[cfg(not(target_arch = "wasm32"))]
 pub struct Mmap(memmap2::Mmap);
 
@@ -39,9 +40,3 @@ impl Deref for Mmap {
         &*self.0
     }
 }
-
-// SAFETY: On architectures other than WASM, mmap is used as backing storage. The address of this
-// memory map is stable. On WASM, `Vec<u8>` is used as backing storage. The `Mmap` type doesn't
-// export any function that can cause the `Vec` to be re-allocated. As such the address of the
-// bytes inside this `Vec` is stable.
-unsafe impl StableAddress for Mmap {}

compiler/rustc_data_structures/src/owned_slice.rs

@@ -0,0 +1,134 @@
+//! A type that stores a boxed type that derefs to a slice, and a custom index into that slice.
+//!
+//! The type can be mapped to have a different index, allowing you to take subslices while still
+//! preserving the original type.
+//!
+//! This is a specialized version of `owning_ref`, because `owning_ref` has various soundness
+//! issues that arise from its generality, that this one does not have.
+
+// FIXME(nilstrieb): This could probably use some more trait implementations,
+// though they are currently not required in the compiler.
+
+use std::marker::PhantomData;
+use std::ops::Deref;
+
+/// An owned subslice of an owned value.
+/// The owned value must be behind a [`Deref`] indirection, for example a [`Box`] or an [`Lrc`].
+///
+/// The `OwnWrap` is an indirection that derefs to the inner owner (`Own`). The inner owner
+/// then derefs to the actual slice, of which this type references a subslice.
+///
+/// Can be further subsliced using [`Self::map`].
+///
+/// [`Lrc`]: crate::sync::Lrc
+pub struct OwnedSlice<OwnWrap, T> {
+    /// The wrapper around the owned value. Derefs to the owned value, which then derefs to the slice.
+    owned: OwnWrap,
+    /// The start value of the subslice.
+    start: usize,
+    /// The length of the subslice.
+    len: usize,
+    /// +--------------------------+
+    /// | We conceptually own a T. |
+    /// +----+  +------------------+
+    ///       \/
+    /// boo! ⊂(´･◡･⊂ )∘˚˳°
+    _boo: PhantomData<T>,
+}
+
+impl<OwnWrap, Own: ?Sized, T> OwnedSlice<OwnWrap, T>
+where
+    OwnWrap: Deref<Target = Own>,
+    Own: Deref<Target = [T]> + 'static,
+{
+    /// Create a new `OwnedSlice<OwnWrap, T>`. Sets the subslice to contain the full slice that `OwnWrap`
+    /// nestedly derefs to.
+    pub fn new(owned: OwnWrap) -> Self {
+        let len = owned.len();
+        Self { owned, start: 0, len, _boo: PhantomData }
+    }
+
+    /// Change the slice to a smaller subslice, while retaining ownership over the full value.
+    ///
+    /// # Panics
+    /// Panics if the subslice is out of bounds of the smaller subslice.
+    pub fn map<F>(self, f: F) -> Self
+    where
+        F: FnOnce(&[T]) -> &[T],
+    {
+        self.try_map::<_, !>(|slice| Ok(f(slice))).unwrap()
+    }
+
+    /// Map the slice to a subslice, while retaining ownership over the full value.
+    /// The function may be fallible.
+    ///
+    /// # Panics
+    /// Panics if the returned subslice is out of bounds of the base slice.
+    pub fn try_map<F, E>(self, f: F) -> Result<Self, E>
+    where
+        F: FnOnce(&[T]) -> Result<&[T], E>,
+    {
+        let base_slice = self.base_slice();
+        let std::ops::Range { start: base_ptr, end: base_end_ptr } = base_slice.as_ptr_range();
+        let base_len = base_slice.len();
+
+        let slice = &base_slice[self.start..][..self.len];
+        let slice = f(slice)?;
+        let (slice_ptr, len) = (slice.as_ptr(), slice.len());
+
+        let start = if len == 0 {
+            // For empty slices, we don't really care where the start is. Also, the start of the subslice could
+            // be a one-past-the-end pointer, which we cannot allow in the code below, but is ok here.
+            0
+        } else {
+            // Assert that the start pointer is in bounds, I.E. points to the same allocated object.
+            // If the slice is empty or contains a zero-sized type, the start and end pointers of the
+            // base slice will always be the same, meaning this check will always fail.
+            assert!(base_ptr <= slice_ptr);
+            assert!(slice_ptr < base_end_ptr);
+
+            // SAFETY: We have checked that the `slice_ptr` is bigger than the `base_ptr`.
+            // We have also checked that it's in bounds of the allocated object.
+            let diff_in_bytes = unsafe { slice_ptr.cast::<u8>().sub_ptr(base_ptr.cast::<u8>()) };
+
+            // The subslice might not actually be a difference of sizeof(T), but truncating it should be fine.
+            diff_in_bytes / std::mem::size_of::<T>()
+        };
+
+        // Assert that the length is not out of bounds. This is not nessecary for soundness, but helps detect errors
+        // early, instead of panicking in the deref.
+        assert!((start + len) <= base_len);
+
+        Ok(Self { owned: self.owned, start, len, _boo: PhantomData })
+    }
+
+    fn base_slice(&self) -> &[T] {
+        &*self.owned
+    }
+}
+
+impl<OwnWrap, Own: ?Sized, T> Deref for OwnedSlice<OwnWrap, T>
+where
+    OwnWrap: Deref<Target = Own>,
+    Own: Deref<Target = [T]> + 'static,
+{
+    type Target = [T];
+
+    fn deref(&self) -> &Self::Target {
+        let base_slice = self.base_slice();
+        &base_slice[self.start..][..self.len]
+    }
+}
+
+impl<OwnWrap, Own: ?Sized, T> std::borrow::Borrow<[T]> for OwnedSlice<OwnWrap, T>
+where
+    OwnWrap: Deref<Target = Own>,
+    Own: Deref<Target = [T]> + 'static,
+{
+    fn borrow(&self) -> &[T] {
+        &*self
+    }
+}
+
+#[cfg(test)]
+mod tests;

compiler/rustc_data_structures/src/owned_slice/tests.rs

@@ -0,0 +1,71 @@
+use std::ops::Deref;
+
+use super::OwnedSlice;
+
+#[test]
+fn create_deref() {
+    let owned_slice = OwnedSlice::new(Box::new(vec![1, 2, 3]));
+
+    let slice = &*owned_slice;
+
+    assert_eq!(slice, &[1, 2, 3]);
+}
+
+#[test]
+fn map() {
+    let owned_slice: OwnedSlice<Box<dyn Deref<Target = [u8]>>, _> =
+        OwnedSlice::new(Box::new(vec![1, 2, 3, 4, 5]));
+
+    let owned_slice = owned_slice.map(|slice| slice.split_at(2).1);
+    let slice = &*owned_slice;
+
+    assert_eq!(slice, &[3, 4, 5]);
+}
+
+#[test]
+fn empty_slice() {
+    let owned_slice = OwnedSlice::new(Box::new(vec![1, 2, 3, 4, 5]));
+
+    let owned_slice = owned_slice.map(|slice| &slice[0..0]);
+
+    let slice = &*owned_slice;
+
+    assert_eq!(slice, &[]);
+}
+
+#[test]
+#[should_panic]
+fn out_of_bounds() {
+    static X: [u8; 5] = [1, 2, 3, 4, 5];
+
+    let owned_slice = OwnedSlice::new(Box::new(vec![1u8, 2, 3]));
+    let owned_slice = owned_slice.map(|_| &X[..]);
+    let slice = &*owned_slice;
+
+    assert_eq!(slice, &[1, 2, 3, 4, 5]);
+}
+
+#[test]
+#[should_panic]
+fn no_zsts_allowed() {
+    let other = Box::leak(Box::new(vec![(); 5]));
+    ignore_leak(other);
+
+    let owned_slice = OwnedSlice::new(Box::new(vec![(); 5]));
+    let owned_slice = owned_slice.map(|_| &other[..]);
+    let slice = &*owned_slice;
+
+    assert_eq!(slice, other);
+}
+
+/// It's ok for this to leak, we need a 'static reference.
+fn ignore_leak<T>(_ptr: *const T) {
+    #[cfg(miri)]
+    extern "Rust" {
+        fn miri_static_root(ptr: *const u8);
+    }
+    #[cfg(miri)]
+    unsafe {
+        miri_static_root(_ptr.cast())
+    };
+}