Rollup merge of #120683 - RalfJung:symbolic-alignment-ice, r=oli-obk

miri: fix ICE with symbolic alignment check on extern static

Fixes rust-lang/miri#3288. Also fixes [this example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=38ee338ff10726be72bdd6efa3386763).

This could almost be a Miri PR, except for that typo fix in the validator. I started this as a rustc patch since I thought I need rustc changes, and now it'd be too annoying to turn this into a Miri PR...

r? `@oli-obk`

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -993,10 +993,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             // Complain about any other kind of error -- those are bad because we'd like to
             // report them in a way that shows *where* in the value the issue lies.
             Err(err) => {
-                bug!(
-                    "Unexpected Undefined Behavior error during validation: {}",
-                    self.format_error(err)
-                );
+                bug!("Unexpected error during validation: {}", self.format_error(err));
             }
         }
     }

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

@@ -383,7 +383,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             // will never cause UB on the pointer itself.
             let (_, _, kind) = this.get_alloc_info(*alloc_id);
             if matches!(kind, AllocKind::LiveData) {
-                let alloc_extra = this.get_alloc_extra(*alloc_id).unwrap();
+                let alloc_extra = this.get_alloc_extra(*alloc_id)?; // can still fail for `extern static`
                 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/machine.rs

@@ -2,7 +2,7 @@
 //! `Machine` trait.
 
 use std::borrow::Cow;
-use std::cell::{Cell, RefCell};
+use std::cell::RefCell;
 use std::collections::hash_map::Entry;
 use std::fmt;
 use std::path::Path;
@@ -336,20 +336,11 @@ pub struct AllocExtra<'tcx> {
     /// if this allocation is leakable. The backtrace is not
     /// pruned yet; that should be done before printing it.
     pub backtrace: Option<Vec<FrameInfo<'tcx>>>,
-    /// An offset inside this allocation that was deemed aligned even for symbolic alignment checks.
-    /// Invariant: the promised alignment will never be less than the native alignment of this allocation.
-    pub symbolic_alignment: Cell<Option<(Size, Align)>>,
 }
 
 impl VisitProvenance for AllocExtra<'_> {
     fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
-        let AllocExtra {
-            borrow_tracker,
-            data_race,
-            weak_memory,
-            backtrace: _,
-            symbolic_alignment: _,
-        } = self;
+        let AllocExtra { borrow_tracker, data_race, weak_memory, backtrace: _ } = self;
 
         borrow_tracker.visit_provenance(visit);
         data_race.visit_provenance(visit);
@@ -572,6 +563,14 @@ pub struct MiriMachine<'mir, 'tcx> {
     /// that is fixed per stack frame; this lets us have sometimes different results for the
     /// same const while ensuring consistent results within a single call.
     const_cache: RefCell<FxHashMap<(mir::Const<'tcx>, usize), OpTy<'tcx, Provenance>>>,
+
+    /// For each allocation, an offset inside that allocation that was deemed aligned even for
+    /// symbolic alignment checks. This cannot be stored in `AllocExtra` since it needs to be
+    /// tracked for vtables and function allocations as well as regular allocations.
+    ///
+    /// Invariant: the promised alignment will never be less than the native alignment of the
+    /// allocation.
+    pub(crate) symbolic_alignment: RefCell<FxHashMap<AllocId, (Size, Align)>>,
 }
 
 impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
@@ -698,6 +697,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
             collect_leak_backtraces: config.collect_leak_backtraces,
             allocation_spans: RefCell::new(FxHashMap::default()),
             const_cache: RefCell::new(FxHashMap::default()),
+            symbolic_alignment: RefCell::new(FxHashMap::default()),
         }
     }
 
@@ -810,6 +810,7 @@ impl VisitProvenance for MiriMachine<'_, '_> {
             collect_leak_backtraces: _,
             allocation_spans: _,
             const_cache: _,
+            symbolic_alignment: _,
         } = self;
 
         threads.visit_provenance(visit);
@@ -893,9 +894,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
             return None;
         }
         // Let's see which alignment we have been promised for this allocation.
-        let alloc_info = ecx.get_alloc_extra(alloc_id).unwrap(); // cannot fail since the allocation is live
-        let (promised_offset, promised_align) =
-            alloc_info.symbolic_alignment.get().unwrap_or((Size::ZERO, alloc_align));
+        let (promised_offset, promised_align) = ecx
+            .machine
+            .symbolic_alignment
+            .borrow()
+            .get(&alloc_id)
+            .copied()
+            .unwrap_or((Size::ZERO, alloc_align));
         if promised_align < align {
             // Definitely not enough.
             Some(Misalignment { has: promised_align, required: align })
@@ -1132,7 +1137,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
                 data_race: race_alloc,
                 weak_memory: buffer_alloc,
                 backtrace,
-                symbolic_alignment: Cell::new(None),
             },
             |ptr| ecx.global_base_pointer(ptr),
         )?;

src/tools/miri/src/provenance_gc.rs

@@ -196,6 +196,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         let this = self.eval_context_mut();
         let allocs = LiveAllocs { ecx: this, collected: allocs };
         this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.is_live(*id));
+        this.machine.symbolic_alignment.borrow_mut().retain(|id, _| allocs.is_live(*id));
         this.machine.intptrcast.borrow_mut().remove_unreachable_allocs(&allocs);
         if let Some(borrow_tracker) = &this.machine.borrow_tracker {
             borrow_tracker.borrow_mut().remove_unreachable_allocs(&allocs);

src/tools/miri/src/shims/foreign_items.rs

@@ -585,17 +585,17 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 }
                 if let Ok((alloc_id, offset, ..)) = this.ptr_try_get_alloc_id(ptr) {
                     let (_size, alloc_align, _kind) = this.get_alloc_info(alloc_id);
-                    // Not `get_alloc_extra_mut`, need to handle read-only allocations!
-                    let alloc_extra = this.get_alloc_extra(alloc_id)?;
                     // If the newly promised alignment is bigger than the native alignment of this
                     // allocation, and bigger than the previously promised alignment, then set it.
                     if align > alloc_align
-                        && !alloc_extra
+                        && !this
+                            .machine
                             .symbolic_alignment
-                            .get()
-                            .is_some_and(|(_, old_align)| align <= old_align)
+                            .get_mut()
+                            .get(&alloc_id)
+                            .is_some_and(|&(_, old_align)| align <= old_align)
                     {
-                        alloc_extra.symbolic_alignment.set(Some((offset, align)));
+                        this.machine.symbolic_alignment.get_mut().insert(alloc_id, (offset, align));
                     }
                 }
             }

src/tools/miri/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.rs

@@ -0,0 +1,12 @@
+//@compile-flags: -Zmiri-symbolic-alignment-check
+
+extern "C" {
+    static _dispatch_queue_attr_concurrent: [u8; 0];
+}
+
+static DISPATCH_QUEUE_CONCURRENT: &'static [u8; 0] =
+    unsafe { &_dispatch_queue_attr_concurrent };
+
+fn main() {
+    let _val = *DISPATCH_QUEUE_CONCURRENT; //~ERROR: is not supported
+}

src/tools/miri/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.stderr

@@ -0,0 +1,14 @@
+error: unsupported operation: `extern` static `_dispatch_queue_attr_concurrent` from crate `issue_miri_3288_ice_symbolic_alignment_extern_static` is not supported by Miri
+  --> $DIR/issue-miri-3288-ice-symbolic-alignment-extern-static.rs:LL:CC
+   |
+LL |     let _val = *DISPATCH_QUEUE_CONCURRENT;
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^ `extern` static `_dispatch_queue_attr_concurrent` from crate `issue_miri_3288_ice_symbolic_alignment_extern_static` is not supported by Miri
+   |
+   = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/issue-miri-3288-ice-symbolic-alignment-extern-static.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/pass/align_offset_symbolic.rs

@@ -1,6 +1,8 @@
 //@compile-flags: -Zmiri-symbolic-alignment-check
 #![feature(strict_provenance)]
 
+use std::mem;
+
 fn test_align_to() {
     const N: usize = 4;
     let d = Box::new([0u32; N]);
@@ -68,7 +70,7 @@ fn test_u64_array() {
     #[repr(align(8))]
     struct AlignToU64<T>(T);
 
-    const BYTE_LEN: usize = std::mem::size_of::<[u64; 4]>();
+    const BYTE_LEN: usize = mem::size_of::<[u64; 4]>();
     type Data = AlignToU64<[u8; BYTE_LEN]>;
 
     fn example(data: &Data) {
@@ -101,10 +103,29 @@ fn huge_align() {
     let _ = std::ptr::invalid::<HugeSize>(SIZE).align_offset(SIZE);
 }
 
+// This shows that we cannot store the promised alignment info in `AllocExtra`,
+// since vtables do not have an `AllocExtra`.
+fn vtable() {
+    #[cfg(target_pointer_width = "64")]
+    type TWOPTR = u128;
+    #[cfg(target_pointer_width = "32")]
+    type TWOPTR = u64;
+
+    let ptr: &dyn Send = &0;
+    let parts: (*const (), *const u8) = unsafe { mem::transmute(ptr) };
+    let vtable = parts.1 ;
+    let offset = vtable.align_offset(mem::align_of::<TWOPTR>());
+    let _vtable_aligned = vtable.wrapping_add(offset) as *const [TWOPTR; 0];
+    // FIXME: we can't actually do the access since vtable pointers act like zero-sized allocations.
+    // Enable the next line once https://github.com/rust-lang/rust/issues/117945 is implemented.
+    //let _place = unsafe { &*vtable_aligned };
+}
+
 fn main() {
     test_align_to();
     test_from_utf8();
     test_u64_array();
     test_cstr();
     huge_align();
+    vtable();
 }