interning: decide about mutability purely based on the kind of interning, not the types we see

Author: RalfJung

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -5,21 +5,20 @@
 //!
 //! In principle, this is not very complicated: we recursively walk the final value, follow all the
 //! pointers, and move all reachable allocations to the global `tcx` memory. The only complication
-//! is picking the right mutability for the allocations in a `static` initializer: we want to make
-//! as many allocations as possible immutable so LLVM can put them into read-only memory. At the
-//! same time, we need to make memory that could be mutated by the program mutable to avoid
-//! incorrect compilations. To achieve this, we do a type-based traversal of the final value,
-//! tracking mutable and shared references and `UnsafeCell` to determine the current mutability.
-//! (In principle, we could skip this type-based part for `const` and promoteds, as they need to be
-//! always immutable. At least for `const` however we use this opportunity to reject any `const`
-//! that contains allocations whose mutability we cannot identify.)
+//! is picking the right mutability: the outermost allocation generally has a clear mutability, but
+//! what about the other allocations it points to that have also been created with this value? We
+//! don't want to do guesswork here. The rules are: `static`, `const`, and promoted can only create
+//! immutable allocations that way. `static mut` can be initialized with expressions like `&mut 42`,
+//! so all inner allocations are marked mutable. Some of them could potentially be made immutable,
+//! but that would require relying on type information, and given how many ways Rust has to lie
+//! about type information, we want to avoid doing that.
 
 use super::validity::RefTracking;
 use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir as hir;
 use rustc_middle::mir::interpret::InterpResult;
-use rustc_middle::ty::{self, layout::TyAndLayout, Ty};
+use rustc_middle::ty::{self, layout::TyAndLayout};
 
 use rustc_ast::Mutability;
 
@@ -56,6 +55,8 @@ struct InternVisitor<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx, const_ev
     /// This field stores whether we are *currently* inside an `UnsafeCell`. This can affect
     /// the intern mode of references we encounter.
     inside_unsafe_cell: bool,
+    /// The mutability with which to intern the pointers we find.
+    intern_mutability: Mutability,
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Hash, Eq)]
@@ -82,10 +83,9 @@ fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx, const_eval:
     ecx: &'rt mut InterpCx<'mir, 'tcx, M>,
     leftover_allocations: &'rt mut FxIndexSet<AllocId>,
     alloc_id: AllocId,
-    mode: InternMode,
-    ty: Option<Ty<'tcx>>,
+    mutability: Mutability,
 ) -> Option<IsStaticOrFn> {
-    trace!("intern_shallow {:?} with {:?}", alloc_id, mode);
+    trace!("intern_shallow {:?}", alloc_id);
     // remove allocation
     let tcx = ecx.tcx;
     let Some((kind, mut alloc)) = ecx.memory.alloc_map.remove(&alloc_id) else {
@@ -112,28 +112,15 @@ fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx, const_eval:
     // Set allocation mutability as appropriate. This is used by LLVM to put things into
     // read-only memory, and also by Miri when evaluating other globals that
     // access this one.
-    if let InternMode::Static(mutability) = mode {
-        // For this, we need to take into account `UnsafeCell`. When `ty` is `None`, we assume
-        // no interior mutability.
-        let frozen = ty.map_or(true, |ty| ty.is_freeze(*ecx.tcx, ecx.param_env));
-        // For statics, allocation mutability is the combination of place mutability and
-        // type mutability.
-        // The entire allocation needs to be mutable if it contains an `UnsafeCell` anywhere.
-        let immutable = mutability == Mutability::Not && frozen;
-        if immutable {
+    match mutability {
+        Mutability::Not => {
             alloc.mutability = Mutability::Not;
-        } else {
-            // Just making sure we are not "upgrading" an immutable allocation to mutable.
+        }
+        Mutability::Mut => {
+            // This must be already mutable, we won't "un-freeze" allocations ever.
             assert_eq!(alloc.mutability, Mutability::Mut);
         }
-    } else {
-        // No matter what, *constants are never mutable*. Mutating them is UB.
-        // See const_eval::machine::MemoryExtra::can_access_statics for why
-        // immutability is so important.
-
-        // Validation will ensure that there is no `UnsafeCell` on an immutable allocation.
-        alloc.mutability = Mutability::Not;
-    };
+    }
     // link the alloc id to the actual allocation
     leftover_allocations.extend(alloc.provenance().ptrs().iter().map(|&(_, alloc_id)| alloc_id));
     let alloc = tcx.mk_const_alloc(alloc);
@@ -144,13 +131,8 @@ fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx, const_eval:
 impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx, const_eval::MemoryKind>>
     InternVisitor<'rt, 'mir, 'tcx, M>
 {
-    fn intern_shallow(
-        &mut self,
-        alloc_id: AllocId,
-        mode: InternMode,
-        ty: Option<Ty<'tcx>>,
-    ) -> Option<IsStaticOrFn> {
-        intern_shallow(self.ecx, self.leftover_allocations, alloc_id, mode, ty)
+    fn intern_shallow(&mut self, alloc_id: AllocId) -> Option<IsStaticOrFn> {
+        intern_shallow(self.ecx, self.leftover_allocations, alloc_id, self.intern_mutability)
     }
 }
 
@@ -181,7 +163,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory
                 if let Some(alloc_id) = ptr.provenance {
                     // Explicitly choose const mode here, since vtables are immutable, even
                     // if the reference of the fat pointer is mutable.
-                    self.intern_shallow(alloc_id, InternMode::Const, None);
+                    self.intern_shallow(alloc_id);
                 } else {
                     // Validation will error (with a better message) on an invalid vtable pointer.
                     // Let validation show the error message, but make sure it *does* error.
@@ -234,7 +216,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory
                         InternMode::Const
                     }
                 };
-                match self.intern_shallow(alloc_id, ref_mode, Some(referenced_ty)) {
+                match self.intern_shallow(alloc_id) {
                     // No need to recurse, these are interned already and statics may have
                     // cycles, so we don't want to recurse there
                     Some(IsStaticOrFn) => {}
@@ -332,12 +314,36 @@ pub fn intern_const_alloc_recursive<
     intern_kind: InternKind,
     ret: &MPlaceTy<'tcx>,
 ) -> Result<(), ErrorGuaranteed> {
-    let tcx = ecx.tcx;
     let base_intern_mode = match intern_kind {
         InternKind::Static(mutbl) => InternMode::Static(mutbl),
         // `Constant` includes array lengths.
         InternKind::Constant | InternKind::Promoted => InternMode::Const,
     };
+    // We are interning recursively, and for mutability we are distinguishing the "root" allocation
+    // that we are starting in, and all other allocations that we are encountering recursively.
+    let (base_mutability, inner_mutability) = match intern_kind {
+        InternKind::Constant | InternKind::Promoted => {
+            // Completely immutable.
+            (Mutability::Not, Mutability::Not)
+        }
+        InternKind::Static(Mutability::Not) => {
+            (
+                // Outermost allocation is mutable if `!Freeze`.
+                if ret.layout.ty.is_freeze(*ecx.tcx, ecx.param_env) {
+                    Mutability::Not
+                } else {
+                    Mutability::Mut
+                },
+                // Inner allocations are never mutable.
+                Mutability::Not,
+            )
+        }
+        InternKind::Static(Mutability::Mut) => {
+            // Just make everything mutable. We accept code like
+            // `static mut X = &mut 42`, so even inner allocations need to be mutable.
+            (Mutability::Mut, Mutability::Mut)
+        }
+    };
 
     // Type based interning.
     // `ref_tracking` tracks typed references we have already interned and still need to crawl for
@@ -354,18 +360,22 @@ pub fn intern_const_alloc_recursive<
         // The outermost allocation must exist, because we allocated it with
         // `Memory::allocate`.
         ret.ptr().provenance.unwrap(),
-        base_intern_mode,
-        Some(ret.layout.ty),
+        base_mutability,
     );
 
     ref_tracking.track((ret.clone(), base_intern_mode), || ());
 
+    // We do a type-based traversal to find more allocations to intern. The interner is currently
+    // mid-refactoring; eventually the type-based traversal will be replaced but a simple traversal
+    // of all provenance we see in the allocations, but for now we avoid changing rustc error
+    // messages or accepting extra programs by keeping the old type-based interner around.
     while let Some(((mplace, mode), _)) = ref_tracking.todo.pop() {
         let res = InternVisitor {
             ref_tracking: &mut ref_tracking,
             ecx,
             mode,
             leftover_allocations,
+            intern_mutability: inner_mutability,
             inside_unsafe_cell: false,
         }
         .visit_value(&mplace);
@@ -394,9 +404,9 @@ pub fn intern_const_alloc_recursive<
     debug!("dead_alloc_map: {:#?}", ecx.memory.dead_alloc_map);
     while let Some(alloc_id) = todo.pop() {
         if let Some((_, mut alloc)) = ecx.memory.alloc_map.remove(&alloc_id) {
-            // We can't call the `intern_shallow` method here, as its logic is tailored to safe
-            // references and a `leftover_allocations` set (where we only have a todo-list here).
-            // So we hand-roll the interning logic here again.
+            // This still needs to be interned. We keep the old logic around here to avoid changing
+            // rustc behavior; eventually this should all be removed in favor of ignroing all types
+            // and interning everything with a simple `intern_shallow` loop.
             match intern_kind {
                 // Statics may point to mutable allocations.
                 // Even for immutable statics it would be ok to have mutable allocations behind
@@ -410,11 +420,7 @@ pub fn intern_const_alloc_recursive<
                 // mutability), mutating through them would be UB.
                 // There's no way we can check whether the user is using raw pointers correctly,
                 // so all we can do is mark this as immutable here.
-                InternKind::Promoted => {
-                    // See const_eval::machine::MemoryExtra::can_access_statics for why
-                    // immutability is so important.
-                    alloc.mutability = Mutability::Not;
-                }
+                InternKind::Promoted => {}
                 // If it's a constant, we should not have any "leftovers" as everything
                 // is tracked by const-checking.
                 // FIXME: downgrade this to a warning? It rejects some legitimate consts,
@@ -425,12 +431,19 @@ pub fn intern_const_alloc_recursive<
                 // drop glue, such as the example above.
                 InternKind::Constant => {
                     ecx.tcx.sess.emit_err(UnsupportedUntypedPointer { span: ecx.tcx.span });
-                    // For better errors later, mark the allocation as immutable.
+                }
+            }
+            match inner_mutability {
+                Mutability::Not => {
                     alloc.mutability = Mutability::Not;
                 }
+                Mutability::Mut => {
+                    // This must be already mutable, we won't "un-freeze" allocations ever.
+                    assert_eq!(alloc.mutability, Mutability::Mut);
+                }
             }
-            let alloc = tcx.mk_const_alloc(alloc);
-            tcx.set_alloc_id_memory(alloc_id, alloc);
+            let alloc = ecx.tcx.mk_const_alloc(alloc);
+            ecx.tcx.set_alloc_id_memory(alloc_id, alloc);
             for &(_, alloc_id) in alloc.inner().provenance().ptrs().iter() {
                 if leftover_allocations.insert(alloc_id) {
                     todo.push(alloc_id);