Auto merge of #106180 - RalfJung:dereferenceable-generators, r=nbdd0121

make &mut !Unpin not dereferenceable, and Box<!Unpin> not noalias

See rust-lang/unsafe-code-guidelines#381 and [this LLVM discussion](https://discourse.llvm.org/t/interaction-of-noalias-and-dereferenceable/66979). The exact semantics of how `noalias` and `dereferenceable` interact are unclear, and `@comex` found a case of LLVM actually exploiting that ambiguity for optimizations. I think for now we should treat LLVM `dereferenceable` as implying a "fake read" to happen immediately at the top of the function (standing in for the spurious reads that LLVM might introduce), and that fake read is subject to all the usual `noalias` restrictions. This means we cannot put `dereferenceable` on `&mut !Unpin` references as those references can alias with other references that are being read and written inside the function (e.g. for self-referential generators), meaning the fake read introduces aliasing conflicts with those other accesses.

For `&` this is already not a problem due to #98017 which removed the `dereferenceable` attribute for other reasons.

Regular `&mut Unpin` references are unaffected, so I hope the impact of this is going to be tiny.

The first commit does some refactoring of the `PointerKind` enum since I found the old code very confusing each time I had to touch it. It doesn't change behavior.

Fixes rust-lang/miri#2714

EDIT: Turns out our `Box<!Unpin>` treatment was incorrect, too, so the PR also fixes that now (in codegen and Miri): we do not put `noalias` on these boxes any more.

Author: bors

compiler/rustc_abi/src/lib.rs

@@ -1439,21 +1439,12 @@ impl<V: Idx> fmt::Debug for LayoutS<V> {
 
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum PointerKind {
-    /// Most general case, we know no restrictions to tell LLVM.
-    SharedMutable,
-
-    /// `&T` where `T` contains no `UnsafeCell`, is `dereferenceable`, `noalias` and `readonly`.
-    Frozen,
-
-    /// `&mut T` which is `dereferenceable` and `noalias` but not `readonly`.
-    UniqueBorrowed,
-
-    /// `&mut !Unpin`, which is `dereferenceable` but neither `noalias` nor `readonly`.
-    UniqueBorrowedPinned,
-
-    /// `Box<T>`, which is `noalias` (even on return types, unlike the above) but neither `readonly`
-    /// nor `dereferenceable`.
-    UniqueOwned,
+    /// Shared reference. `frozen` indicates the absence of any `UnsafeCell`.
+    SharedRef { frozen: bool },
+    /// Mutable reference. `unpin` indicates the absence of any pinned data.
+    MutableRef { unpin: bool },
+    /// Box. `unpin` indicates the absence of any pinned data.
+    Box { unpin: bool },
 }
 
 /// Note that this information is advisory only, and backends are free to ignore it.

compiler/rustc_middle/src/ty/layout.rs

@@ -818,125 +818,114 @@ where
         let tcx = cx.tcx();
         let param_env = cx.param_env();
 
-        let pointee_info =
-            match *this.ty.kind() {
-                ty::RawPtr(mt) if offset.bytes() == 0 => {
-                    tcx.layout_of(param_env.and(mt.ty)).ok().map(|layout| PointeeInfo {
-                        size: layout.size,
-                        align: layout.align.abi,
-                        safe: None,
-                    })
-                }
-                ty::FnPtr(fn_sig) if offset.bytes() == 0 => {
-                    tcx.layout_of(param_env.and(tcx.mk_fn_ptr(fn_sig))).ok().map(|layout| {
-                        PointeeInfo { size: layout.size, align: layout.align.abi, safe: None }
-                    })
-                }
-                ty::Ref(_, ty, mt) if offset.bytes() == 0 => {
-                    let kind = if tcx.sess.opts.optimize == OptLevel::No {
-                        // Use conservative pointer kind if not optimizing. This saves us the
-                        // Freeze/Unpin queries, and can save time in the codegen backend (noalias
-                        // attributes in LLVM have compile-time cost even in unoptimized builds).
-                        PointerKind::SharedMutable
-                    } else {
-                        match mt {
-                            hir::Mutability::Not => {
-                                if ty.is_freeze(tcx, cx.param_env()) {
-                                    PointerKind::Frozen
-                                } else {
-                                    PointerKind::SharedMutable
-                                }
-                            }
-                            hir::Mutability::Mut => {
-                                // References to self-referential structures should not be considered
-                                // noalias, as another pointer to the structure can be obtained, that
-                                // is not based-on the original reference. We consider all !Unpin
-                                // types to be potentially self-referential here.
-                                if ty.is_unpin(tcx, cx.param_env()) {
-                                    PointerKind::UniqueBorrowed
-                                } else {
-                                    PointerKind::UniqueBorrowedPinned
-                                }
-                            }
-                        }
-                    };
+        let pointee_info = match *this.ty.kind() {
+            ty::RawPtr(mt) if offset.bytes() == 0 => {
+                tcx.layout_of(param_env.and(mt.ty)).ok().map(|layout| PointeeInfo {
+                    size: layout.size,
+                    align: layout.align.abi,
+                    safe: None,
+                })
+            }
+            ty::FnPtr(fn_sig) if offset.bytes() == 0 => {
+                tcx.layout_of(param_env.and(tcx.mk_fn_ptr(fn_sig))).ok().map(|layout| PointeeInfo {
+                    size: layout.size,
+                    align: layout.align.abi,
+                    safe: None,
+                })
+            }
+            ty::Ref(_, ty, mt) if offset.bytes() == 0 => {
+                // Use conservative pointer kind if not optimizing. This saves us the
+                // Freeze/Unpin queries, and can save time in the codegen backend (noalias
+                // attributes in LLVM have compile-time cost even in unoptimized builds).
+                let optimize = tcx.sess.opts.optimize != OptLevel::No;
+                let kind = match mt {
+                    hir::Mutability::Not => PointerKind::SharedRef {
+                        frozen: optimize && ty.is_freeze(tcx, cx.param_env()),
+                    },
+                    hir::Mutability::Mut => PointerKind::MutableRef {
+                        unpin: optimize && ty.is_unpin(tcx, cx.param_env()),
+                    },
+                };
 
-                    tcx.layout_of(param_env.and(ty)).ok().map(|layout| PointeeInfo {
-                        size: layout.size,
-                        align: layout.align.abi,
-                        safe: Some(kind),
-                    })
-                }
+                tcx.layout_of(param_env.and(ty)).ok().map(|layout| PointeeInfo {
+                    size: layout.size,
+                    align: layout.align.abi,
+                    safe: Some(kind),
+                })
+            }
 
-                _ => {
-                    let mut data_variant = match this.variants {
-                        // Within the discriminant field, only the niche itself is
-                        // always initialized, so we only check for a pointer at its
-                        // offset.
-                        //
-                        // If the niche is a pointer, it's either valid (according
-                        // to its type), or null (which the niche field's scalar
-                        // validity range encodes). This allows using
-                        // `dereferenceable_or_null` for e.g., `Option<&T>`, and
-                        // this will continue to work as long as we don't start
-                        // using more niches than just null (e.g., the first page of
-                        // the address space, or unaligned pointers).
-                        Variants::Multiple {
-                            tag_encoding: TagEncoding::Niche { untagged_variant, .. },
-                            tag_field,
-                            ..
-                        } if this.fields.offset(tag_field) == offset => {
-                            Some(this.for_variant(cx, untagged_variant))
-                        }
-                        _ => Some(this),
-                    };
+            _ => {
+                let mut data_variant = match this.variants {
+                    // Within the discriminant field, only the niche itself is
+                    // always initialized, so we only check for a pointer at its
+                    // offset.
+                    //
+                    // If the niche is a pointer, it's either valid (according
+                    // to its type), or null (which the niche field's scalar
+                    // validity range encodes). This allows using
+                    // `dereferenceable_or_null` for e.g., `Option<&T>`, and
+                    // this will continue to work as long as we don't start
+                    // using more niches than just null (e.g., the first page of
+                    // the address space, or unaligned pointers).
+                    Variants::Multiple {
+                        tag_encoding: TagEncoding::Niche { untagged_variant, .. },
+                        tag_field,
+                        ..
+                    } if this.fields.offset(tag_field) == offset => {
+                        Some(this.for_variant(cx, untagged_variant))
+                    }
+                    _ => Some(this),
+                };
 
-                    if let Some(variant) = data_variant {
-                        // We're not interested in any unions.
-                        if let FieldsShape::Union(_) = variant.fields {
-                            data_variant = None;
-                        }
+                if let Some(variant) = data_variant {
+                    // We're not interested in any unions.
+                    if let FieldsShape::Union(_) = variant.fields {
+                        data_variant = None;
                     }
+                }
 
-                    let mut result = None;
-
-                    if let Some(variant) = data_variant {
-                        // FIXME(erikdesjardins): handle non-default addrspace ptr sizes
-                        // (requires passing in the expected address space from the caller)
-                        let ptr_end = offset + Pointer(AddressSpace::DATA).size(cx);
-                        for i in 0..variant.fields.count() {
-                            let field_start = variant.fields.offset(i);
-                            if field_start <= offset {
-                                let field = variant.field(cx, i);
-                                result = field.to_result().ok().and_then(|field| {
-                                    if ptr_end <= field_start + field.size {
-                                        // We found the right field, look inside it.
-                                        let field_info =
-                                            field.pointee_info_at(cx, offset - field_start);
-                                        field_info
-                                    } else {
-                                        None
-                                    }
-                                });
-                                if result.is_some() {
-                                    break;
+                let mut result = None;
+
+                if let Some(variant) = data_variant {
+                    // FIXME(erikdesjardins): handle non-default addrspace ptr sizes
+                    // (requires passing in the expected address space from the caller)
+                    let ptr_end = offset + Pointer(AddressSpace::DATA).size(cx);
+                    for i in 0..variant.fields.count() {
+                        let field_start = variant.fields.offset(i);
+                        if field_start <= offset {
+                            let field = variant.field(cx, i);
+                            result = field.to_result().ok().and_then(|field| {
+                                if ptr_end <= field_start + field.size {
+                                    // We found the right field, look inside it.
+                                    let field_info =
+                                        field.pointee_info_at(cx, offset - field_start);
+                                    field_info
+                                } else {
+                                    None
                                 }
+                            });
+                            if result.is_some() {
+                                break;
                             }
                         }
                     }
+                }
 
-                    // FIXME(eddyb) This should be for `ptr::Unique<T>`, not `Box<T>`.
-                    if let Some(ref mut pointee) = result {
-                        if let ty::Adt(def, _) = this.ty.kind() {
-                            if def.is_box() && offset.bytes() == 0 {
-                                pointee.safe = Some(PointerKind::UniqueOwned);
-                            }
+                // FIXME(eddyb) This should be for `ptr::Unique<T>`, not `Box<T>`.
+                if let Some(ref mut pointee) = result {
+                    if let ty::Adt(def, _) = this.ty.kind() {
+                        if def.is_box() && offset.bytes() == 0 {
+                            let optimize = tcx.sess.opts.optimize != OptLevel::No;
+                            pointee.safe = Some(PointerKind::Box {
+                                unpin: optimize && this.ty.boxed_ty().is_unpin(tcx, cx.param_env()),
+                            });
                         }
                     }
-
-                    result
                 }
-            };
+
+                result
+            }
+        };
 
         debug!(
             "pointee_info_at (offset={:?}, type kind: {:?}) => {:?}",

compiler/rustc_ty_utils/src/abi.rs

@@ -254,15 +254,18 @@ fn adjust_for_rust_scalar<'tcx>(
         if let Some(kind) = pointee.safe {
             attrs.pointee_align = Some(pointee.align);
 
-            // `Box` (`UniqueBorrowed`) are not necessarily dereferenceable
-            // for the entire duration of the function as they can be deallocated
-            // at any time. Same for shared mutable references. If LLVM had a
-            // way to say "dereferenceable on entry" we could use it here.
+            // `Box` are not necessarily dereferenceable for the entire duration of the function as
+            // they can be deallocated at any time. Same for non-frozen shared references (see
+            // <https://github.com/rust-lang/rust/pull/98017>), and for mutable references to
+            // potentially self-referential types (see
+            // <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>). If LLVM had a way
+            // to say "dereferenceable on entry" we could use it here.
             attrs.pointee_size = match kind {
-                PointerKind::UniqueBorrowed
-                | PointerKind::UniqueBorrowedPinned
-                | PointerKind::Frozen => pointee.size,
-                PointerKind::SharedMutable | PointerKind::UniqueOwned => Size::ZERO,
+                PointerKind::Box { .. }
+                | PointerKind::SharedRef { frozen: false }
+                | PointerKind::MutableRef { unpin: false } => Size::ZERO,
+                PointerKind::SharedRef { frozen: true }
+                | PointerKind::MutableRef { unpin: true } => pointee.size,
             };
 
             // The aliasing rules for `Box<T>` are still not decided, but currently we emit
@@ -275,26 +278,24 @@ fn adjust_for_rust_scalar<'tcx>(
             // versions at all anymore. We still support turning it off using -Zmutable-noalias.
             let noalias_mut_ref = cx.tcx.sess.opts.unstable_opts.mutable_noalias;
 
-            // `&mut` pointer parameters never alias other parameters,
-            // or mutable global data
+            // `&T` where `T` contains no `UnsafeCell<U>` is immutable, and can be marked as both
+            // `readonly` and `noalias`, as LLVM's definition of `noalias` is based solely on memory
+            // dependencies rather than pointer equality. However this only applies to arguments,
+            // not return values.
             //
-            // `&T` where `T` contains no `UnsafeCell<U>` is immutable,
-            // and can be marked as both `readonly` and `noalias`, as
-            // LLVM's definition of `noalias` is based solely on memory
-            // dependencies rather than pointer equality
+            // `&mut T` and `Box<T>` where `T: Unpin` are unique and hence `noalias`.
             let no_alias = match kind {
-                PointerKind::SharedMutable | PointerKind::UniqueBorrowedPinned => false,
-                PointerKind::UniqueBorrowed => noalias_mut_ref,
-                PointerKind::UniqueOwned => noalias_for_box,
-                PointerKind::Frozen => true,
+                PointerKind::SharedRef { frozen } => frozen,
+                PointerKind::MutableRef { unpin } => unpin && noalias_mut_ref,
+                PointerKind::Box { unpin } => unpin && noalias_for_box,
             };
             // We can never add `noalias` in return position; that LLVM attribute has some very surprising semantics
             // (see <https://github.com/rust-lang/unsafe-code-guidelines/issues/385#issuecomment-1368055745>).
             if no_alias && !is_return {
                 attrs.set(ArgAttribute::NoAlias);
             }
 
-            if kind == PointerKind::Frozen && !is_return {
+            if matches!(kind, PointerKind::SharedRef { frozen: true }) && !is_return {
                 attrs.set(ArgAttribute::ReadOnly);
             }
         }

src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs

@@ -81,21 +81,18 @@ impl NewPermission {
                         protector: None,
                     }
                 } else if pointee.is_unpin(*cx.tcx, cx.param_env()) {
-                    // A regular full mutable reference.
+                    // A regular full mutable reference. On `FnEntry` this is `noalias` and `dereferenceable`.
                     NewPermission::Uniform {
                         perm: Permission::Unique,
                         access: Some(AccessKind::Write),
                         protector,
                     }
                 } else {
+                    // `!Unpin` dereferences do not get `noalias` nor `dereferenceable`.
                     NewPermission::Uniform {
                         perm: Permission::SharedReadWrite,
-                        // FIXME: We emit `dereferenceable` for `!Unpin` mutable references, so we
-                        // should do fake accesses here. But then we run into
-                        // <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>, so for now
-                        // we don't do that.
                         access: None,
-                        protector,
+                        protector: None,
                     }
                 }
             }
@@ -109,6 +106,7 @@ impl NewPermission {
                 }
             }
             ty::Ref(_, _pointee, Mutability::Not) => {
+                // Shared references. If frozen, these get `noalias` and `dereferenceable`; otherwise neither.
                 NewPermission::FreezeSensitive {
                     freeze_perm: Permission::SharedReadOnly,
                     freeze_access: Some(AccessKind::Read),
@@ -137,6 +135,32 @@ impl NewPermission {
         }
     }
 
+    fn from_box_ty<'tcx>(
+        ty: Ty<'tcx>,
+        kind: RetagKind,
+        cx: &crate::MiriInterpCx<'_, 'tcx>,
+    ) -> Self {
+        // `ty` is not the `Box` but the field of the Box with this pointer (due to allocator handling).
+        let pointee = ty.builtin_deref(true).unwrap().ty;
+        if pointee.is_unpin(*cx.tcx, cx.param_env()) {
+            // A regular box. On `FnEntry` this is `noalias`, but not `dereferenceable` (hence only
+            // a weak protector).
+            NewPermission::Uniform {
+                perm: Permission::Unique,
+                access: Some(AccessKind::Write),
+                protector: (kind == RetagKind::FnEntry)
+                    .then_some(ProtectorKind::WeakProtector),
+            }
+        } else {
+            // `!Unpin` boxes do not get `noalias` nor `dereferenceable`.
+            NewPermission::Uniform {
+                perm: Permission::SharedReadWrite,
+                access: None,
+                protector: None,
+            }
+        }
+    }
+
     fn protector(&self) -> Option<ProtectorKind> {
         match self {
             NewPermission::Uniform { protector, .. } => *protector,
@@ -916,12 +940,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
             fn visit_box(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
                 // Boxes get a weak protectors, since they may be deallocated.
-                let new_perm = NewPermission::Uniform {
-                    perm: Permission::Unique,
-                    access: Some(AccessKind::Write),
-                    protector: (self.kind == RetagKind::FnEntry)
-                        .then_some(ProtectorKind::WeakProtector),
-                };
+                let new_perm = NewPermission::from_box_ty(place.layout.ty, self.kind, self.ecx);
                 self.retag_ptr_inplace(place, new_perm, self.retag_cause)
             }