Miri: Skip over GlobalAllocs when GC'ing base_addr

Author: saethlin

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -58,9 +58,12 @@ impl<T: fmt::Display> fmt::Display for MemoryKind<T> {
 }
 
 /// The return value of `get_alloc_info` indicates the "kind" of the allocation.
+#[derive(Clone, Copy)]
 pub enum AllocKind {
     /// A regular live data allocation.
     LiveData,
+    /// A regular live data allocation that will never be deallocated.
+    GlobalLiveData,
     /// A function allocation (that fn ptrs point to).
     Function,
     /// A (symbolic) vtable allocation.
@@ -69,6 +72,24 @@ pub enum AllocKind {
     Dead,
 }
 
+impl AllocKind {
+    pub fn is_global(self) -> bool {
+        use AllocKind::*;
+        match self {
+            GlobalLiveData | Function | VTable => true,
+            LiveData | Dead => false,
+        }
+    }
+
+    pub fn is_live_data(self) -> bool {
+        use AllocKind::*;
+        match self {
+            GlobalLiveData | LiveData => true,
+            Function | VTable | Dead => false,
+        }
+    }
+}
+
 /// The value of a function pointer.
 #[derive(Debug, Copy, Clone)]
 pub enum FnVal<'tcx, Other> {
@@ -733,13 +754,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     .expect("statics should not have generic parameters");
                 let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap();
                 assert!(layout.is_sized());
-                (layout.size, layout.align.abi, AllocKind::LiveData)
+                (layout.size, layout.align.abi, AllocKind::GlobalLiveData)
             }
             Some(GlobalAlloc::Memory(alloc)) => {
                 // Need to duplicate the logic here, because the global allocations have
                 // different associated types than the interpreter-local ones.
                 let alloc = alloc.inner();
-                (alloc.size(), alloc.align, AllocKind::LiveData)
+                (alloc.size(), alloc.align, AllocKind::GlobalLiveData)
             }
             Some(GlobalAlloc::Function(_)) => bug!("We already checked function pointers above"),
             Some(GlobalAlloc::VTable(..)) => {

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

@@ -382,7 +382,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             // pointer is readable, and the implicit read access inserted
             // will never cause UB on the pointer itself.
             let (_, _, kind) = this.get_alloc_info(*alloc_id);
-            if matches!(kind, AllocKind::LiveData) {
+            if kind.is_live_data() {
                 let alloc_extra = this.get_alloc_extra(*alloc_id).unwrap();
                 let alloc_borrow_tracker = &alloc_extra.borrow_tracker.as_ref().unwrap();
                 alloc_borrow_tracker.release_protector(&this.machine, borrow_tracker, *tag)?;

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

@@ -648,32 +648,27 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
             };
 
             let (_size, _align, alloc_kind) = this.get_alloc_info(alloc_id);
-            match alloc_kind {
-                AllocKind::LiveData => {
-                    // This should have alloc_extra data, but `get_alloc_extra` can still fail
-                    // if converting this alloc_id from a global to a local one
-                    // uncovers a non-supported `extern static`.
-                    let extra = this.get_alloc_extra(alloc_id)?;
-                    let mut stacked_borrows = extra
-                        .borrow_tracker_sb()
-                        .borrow_mut();
-                    // Note that we create a *second* `DiagnosticCxBuilder` below for the actual retag.
-                    // FIXME: can this be done cleaner?
-                    let dcx = DiagnosticCxBuilder::retag(
-                        &this.machine,
-                        retag_info,
-                        new_tag,
-                        orig_tag,
-                        alloc_range(base_offset, size),
-                    );
-                    let mut dcx = dcx.build(&mut stacked_borrows.history, base_offset);
-                    dcx.log_creation();
-                    if new_perm.protector().is_some() {
-                        dcx.log_protector();
-                    }
-                },
-                AllocKind::Function | AllocKind::VTable | AllocKind::Dead => {
-                    // No stacked borrows on these allocations.
+            if alloc_kind.is_live_data() {
+                // This should have alloc_extra data, but `get_alloc_extra` can still fail
+                // if converting this alloc_id from a global to a local one
+                // uncovers a non-supported `extern static`.
+                let extra = this.get_alloc_extra(alloc_id)?;
+                let mut stacked_borrows = extra
+                    .borrow_tracker_sb()
+                    .borrow_mut();
+                // Note that we create a *second* `DiagnosticCxBuilder` below for the actual retag.
+                // FIXME: can this be done cleaner?
+                let dcx = DiagnosticCxBuilder::retag(
+                    &this.machine,
+                    retag_info,
+                    new_tag,
+                    orig_tag,
+                    alloc_range(base_offset, size),
+                );
+                let mut dcx = dcx.build(&mut stacked_borrows.history, base_offset);
+                dcx.log_creation();
+                if new_perm.protector().is_some() {
+                    dcx.log_protector();
                 }
             }
             Ok(())
@@ -1012,18 +1007,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         // This is okay because accessing them is UB anyway, no need for any Stacked Borrows checks.
         // NOT using `get_alloc_extra_mut` since this might be a read-only allocation!
         let (_size, _align, kind) = this.get_alloc_info(alloc_id);
-        match kind {
-            AllocKind::LiveData => {
-                // This should have alloc_extra data, but `get_alloc_extra` can still fail
-                // if converting this alloc_id from a global to a local one
-                // uncovers a non-supported `extern static`.
-                let alloc_extra = this.get_alloc_extra(alloc_id)?;
-                trace!("Stacked Borrows tag {tag:?} exposed in {alloc_id:?}");
-                alloc_extra.borrow_tracker_sb().borrow_mut().exposed_tags.insert(tag);
-            }
-            AllocKind::Function | AllocKind::VTable | AllocKind::Dead => {
-                // No stacked borrows on these allocations.
-            }
+        if kind.is_live_data() {
+            // This should have alloc_extra data, but `get_alloc_extra` can still fail
+            // if converting this alloc_id from a global to a local one
+            // uncovers a non-supported `extern static`.
+            let alloc_extra = this.get_alloc_extra(alloc_id)?;
+            trace!("Stacked Borrows tag {tag:?} exposed in {alloc_id:?}");
+            alloc_extra.borrow_tracker_sb().borrow_mut().exposed_tags.insert(tag);
         }
         Ok(())
     }

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

@@ -285,7 +285,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         }
 
         let alloc_kind = this.get_alloc_info(alloc_id).2;
-        if !matches!(alloc_kind, AllocKind::LiveData) {
+        if !alloc_kind.is_live_data() {
             assert_eq!(ptr_size, Size::ZERO); // we did the deref check above, size has to be 0 here
             // There's not actually any bytes here where accesses could even be tracked.
             // Just produce the new provenance, nothing else to do.
@@ -538,18 +538,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         // This is okay because accessing them is UB anyway, no need for any Tree Borrows checks.
         // NOT using `get_alloc_extra_mut` since this might be a read-only allocation!
         let (_size, _align, kind) = this.get_alloc_info(alloc_id);
-        match kind {
-            AllocKind::LiveData => {
-                // This should have alloc_extra data, but `get_alloc_extra` can still fail
-                // if converting this alloc_id from a global to a local one
-                // uncovers a non-supported `extern static`.
-                let alloc_extra = this.get_alloc_extra(alloc_id)?;
-                trace!("Tree Borrows tag {tag:?} exposed in {alloc_id:?}");
-                alloc_extra.borrow_tracker_tb().borrow_mut().expose_tag(tag);
-            }
-            AllocKind::Function | AllocKind::VTable | AllocKind::Dead => {
-                // No tree borrows on these allocations.
-            }
+        if kind.is_live_data() {
+            // This should have alloc_extra data, but `get_alloc_extra` can still fail
+            // if converting this alloc_id from a global to a local one
+            // uncovers a non-supported `extern static`.
+            let alloc_extra = this.get_alloc_extra(alloc_id)?;
+            trace!("Tree Borrows tag {tag:?} exposed in {alloc_id:?}");
+            alloc_extra.borrow_tracker_tb().borrow_mut().expose_tag(tag);
         }
         Ok(())
     }

src/tools/miri/src/intptrcast.rs

@@ -35,7 +35,7 @@ pub struct GlobalStateInner {
     /// `AllocExtra` because function pointers also have a base address, and
     /// they do not have an `AllocExtra`.
     /// This is the inverse of `int_to_ptr_map`.
-    base_addr: FxHashMap<AllocId, u64>,
+    base_addr: FxHashMap<AllocId, (u64, bool)>,
     /// Whether an allocation has been exposed or not. This cannot be put
     /// into `AllocExtra` for the same reason as `base_addr`.
     exposed: FxHashSet<AllocId>,
@@ -78,7 +78,7 @@ impl GlobalStateInner {
     pub fn remove_unreachable_allocs(&mut self, allocs: &LiveAllocs<'_, '_, '_>) {
         // `exposed` and `int_to_ptr_map` are cleared immediately when an allocation
         // is freed, so `base_addr` is the only one we have to clean up based on the GC.
-        self.base_addr.retain(|id, _| allocs.is_live(*id));
+        self.base_addr.retain(|id, (_addr, is_global)| *is_global || allocs.is_live(*id));
     }
 }
 
@@ -138,7 +138,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let global_state = &mut *global_state;
 
         Ok(match global_state.base_addr.entry(alloc_id) {
-            Entry::Occupied(entry) => *entry.get(),
+            Entry::Occupied(entry) => entry.get().0,
             Entry::Vacant(entry) => {
                 let (size, align, kind) = ecx.get_alloc_info(alloc_id);
                 // This is either called immediately after allocation (and then cached), or when
@@ -161,7 +161,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     .checked_add(slack)
                     .ok_or_else(|| err_exhaust!(AddressSpaceFull))?;
                 let base_addr = align_addr(base_addr, align.bytes());
-                entry.insert(base_addr);
+                entry.insert((base_addr, kind.is_global()));
                 trace!(
                     "Assigning base address {:#x} to allocation {:?} (size: {}, align: {}, slack: {})",
                     base_addr,
@@ -312,7 +312,7 @@ impl GlobalStateInner {
         // returns a dead allocation.
         // To avoid a linear scan we first look up the address in `base_addr`, and then find it in
         // `int_to_ptr_map`.
-        let addr = *self.base_addr.get(&dead_id).unwrap();
+        let addr = self.base_addr.get(&dead_id).unwrap().0;
         let pos = self.int_to_ptr_map.binary_search_by_key(&addr, |(addr, _)| *addr).unwrap();
         let removed = self.int_to_ptr_map.remove(pos);
         assert_eq!(removed, (addr, dead_id)); // double-check that we removed the right thing