Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 3a3cf59

Browse files
rust-timerrust-timer
authoredFeb 6, 2024
Unrolled build for rust-lang#120683
Rollup merge of rust-lang#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`
2 parents 4a2fe44 + 25635b9 commit 3a3cf59

File tree

8 files changed

+76
-27
lines changed

8 files changed

+76
-27
lines changed
 

‎compiler/rustc_const_eval/src/interpret/validity.rs

+1-4
Original file line numberDiff line numberDiff line change
@@ -993,10 +993,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
993993
// Complain about any other kind of error -- those are bad because we'd like to
994994
// report them in a way that shows *where* in the value the issue lies.
995995
Err(err) => {
996-
bug!(
997-
"Unexpected Undefined Behavior error during validation: {}",
998-
self.format_error(err)
999-
);
996+
bug!("Unexpected error during validation: {}", self.format_error(err));
1000997
}
1001998
}
1002999
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
383383
// will never cause UB on the pointer itself.
384384
let (_, _, kind) = this.get_alloc_info(*alloc_id);
385385
if matches!(kind, AllocKind::LiveData) {
386-
let alloc_extra = this.get_alloc_extra(*alloc_id).unwrap();
386+
let alloc_extra = this.get_alloc_extra(*alloc_id)?; // can still fail for `extern static`
387387
let alloc_borrow_tracker = &alloc_extra.borrow_tracker.as_ref().unwrap();
388388
alloc_borrow_tracker.release_protector(&this.machine, borrow_tracker, *tag)?;
389389
}

‎src/tools/miri/src/machine.rs

+19-15
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//! `Machine` trait.
33
44
use std::borrow::Cow;
5-
use std::cell::{Cell, RefCell};
5+
use std::cell::RefCell;
66
use std::collections::hash_map::Entry;
77
use std::fmt;
88
use std::path::Path;
@@ -336,20 +336,11 @@ pub struct AllocExtra<'tcx> {
336336
/// if this allocation is leakable. The backtrace is not
337337
/// pruned yet; that should be done before printing it.
338338
pub backtrace: Option<Vec<FrameInfo<'tcx>>>,
339-
/// An offset inside this allocation that was deemed aligned even for symbolic alignment checks.
340-
/// Invariant: the promised alignment will never be less than the native alignment of this allocation.
341-
pub symbolic_alignment: Cell<Option<(Size, Align)>>,
342339
}
343340

344341
impl VisitProvenance for AllocExtra<'_> {
345342
fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
346-
let AllocExtra {
347-
borrow_tracker,
348-
data_race,
349-
weak_memory,
350-
backtrace: _,
351-
symbolic_alignment: _,
352-
} = self;
343+
let AllocExtra { borrow_tracker, data_race, weak_memory, backtrace: _ } = self;
353344

354345
borrow_tracker.visit_provenance(visit);
355346
data_race.visit_provenance(visit);
@@ -572,6 +563,14 @@ pub struct MiriMachine<'mir, 'tcx> {
572563
/// that is fixed per stack frame; this lets us have sometimes different results for the
573564
/// same const while ensuring consistent results within a single call.
574565
const_cache: RefCell<FxHashMap<(mir::Const<'tcx>, usize), OpTy<'tcx, Provenance>>>,
566+
567+
/// For each allocation, an offset inside that allocation that was deemed aligned even for
568+
/// symbolic alignment checks. This cannot be stored in `AllocExtra` since it needs to be
569+
/// tracked for vtables and function allocations as well as regular allocations.
570+
///
571+
/// Invariant: the promised alignment will never be less than the native alignment of the
572+
/// allocation.
573+
pub(crate) symbolic_alignment: RefCell<FxHashMap<AllocId, (Size, Align)>>,
575574
}
576575

577576
impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
@@ -698,6 +697,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
698697
collect_leak_backtraces: config.collect_leak_backtraces,
699698
allocation_spans: RefCell::new(FxHashMap::default()),
700699
const_cache: RefCell::new(FxHashMap::default()),
700+
symbolic_alignment: RefCell::new(FxHashMap::default()),
701701
}
702702
}
703703

@@ -810,6 +810,7 @@ impl VisitProvenance for MiriMachine<'_, '_> {
810810
collect_leak_backtraces: _,
811811
allocation_spans: _,
812812
const_cache: _,
813+
symbolic_alignment: _,
813814
} = self;
814815

815816
threads.visit_provenance(visit);
@@ -893,9 +894,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
893894
return None;
894895
}
895896
// Let's see which alignment we have been promised for this allocation.
896-
let alloc_info = ecx.get_alloc_extra(alloc_id).unwrap(); // cannot fail since the allocation is live
897-
let (promised_offset, promised_align) =
898-
alloc_info.symbolic_alignment.get().unwrap_or((Size::ZERO, alloc_align));
897+
let (promised_offset, promised_align) = ecx
898+
.machine
899+
.symbolic_alignment
900+
.borrow()
901+
.get(&alloc_id)
902+
.copied()
903+
.unwrap_or((Size::ZERO, alloc_align));
899904
if promised_align < align {
900905
// Definitely not enough.
901906
Some(Misalignment { has: promised_align, required: align })
@@ -1132,7 +1137,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
11321137
data_race: race_alloc,
11331138
weak_memory: buffer_alloc,
11341139
backtrace,
1135-
symbolic_alignment: Cell::new(None),
11361140
},
11371141
|ptr| ecx.global_base_pointer(ptr),
11381142
)?;

‎src/tools/miri/src/provenance_gc.rs

+1
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
196196
let this = self.eval_context_mut();
197197
let allocs = LiveAllocs { ecx: this, collected: allocs };
198198
this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.is_live(*id));
199+
this.machine.symbolic_alignment.borrow_mut().retain(|id, _| allocs.is_live(*id));
199200
this.machine.intptrcast.borrow_mut().remove_unreachable_allocs(&allocs);
200201
if let Some(borrow_tracker) = &this.machine.borrow_tracker {
201202
borrow_tracker.borrow_mut().remove_unreachable_allocs(&allocs);

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -585,17 +585,17 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
585585
}
586586
if let Ok((alloc_id, offset, ..)) = this.ptr_try_get_alloc_id(ptr) {
587587
let (_size, alloc_align, _kind) = this.get_alloc_info(alloc_id);
588-
// Not `get_alloc_extra_mut`, need to handle read-only allocations!
589-
let alloc_extra = this.get_alloc_extra(alloc_id)?;
590588
// If the newly promised alignment is bigger than the native alignment of this
591589
// allocation, and bigger than the previously promised alignment, then set it.
592590
if align > alloc_align
593-
&& !alloc_extra
591+
&& !this
592+
.machine
594593
.symbolic_alignment
595-
.get()
596-
.is_some_and(|(_, old_align)| align <= old_align)
594+
.get_mut()
595+
.get(&alloc_id)
596+
.is_some_and(|&(_, old_align)| align <= old_align)
597597
{
598-
alloc_extra.symbolic_alignment.set(Some((offset, align)));
598+
this.machine.symbolic_alignment.get_mut().insert(alloc_id, (offset, align));
599599
}
600600
}
601601
}

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

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
//@compile-flags: -Zmiri-symbolic-alignment-check
2+
3+
extern "C" {
4+
static _dispatch_queue_attr_concurrent: [u8; 0];
5+
}
6+
7+
static DISPATCH_QUEUE_CONCURRENT: &'static [u8; 0] =
8+
unsafe { &_dispatch_queue_attr_concurrent };
9+
10+
fn main() {
11+
let _val = *DISPATCH_QUEUE_CONCURRENT; //~ERROR: is not supported
12+
}

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

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

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

+22-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
//@compile-flags: -Zmiri-symbolic-alignment-check
22
#![feature(strict_provenance)]
33

4+
use std::mem;
5+
46
fn test_align_to() {
57
const N: usize = 4;
68
let d = Box::new([0u32; N]);
@@ -68,7 +70,7 @@ fn test_u64_array() {
6870
#[repr(align(8))]
6971
struct AlignToU64<T>(T);
7072

71-
const BYTE_LEN: usize = std::mem::size_of::<[u64; 4]>();
73+
const BYTE_LEN: usize = mem::size_of::<[u64; 4]>();
7274
type Data = AlignToU64<[u8; BYTE_LEN]>;
7375

7476
fn example(data: &Data) {
@@ -101,10 +103,29 @@ fn huge_align() {
101103
let _ = std::ptr::invalid::<HugeSize>(SIZE).align_offset(SIZE);
102104
}
103105

106+
// This shows that we cannot store the promised alignment info in `AllocExtra`,
107+
// since vtables do not have an `AllocExtra`.
108+
fn vtable() {
109+
#[cfg(target_pointer_width = "64")]
110+
type TWOPTR = u128;
111+
#[cfg(target_pointer_width = "32")]
112+
type TWOPTR = u64;
113+
114+
let ptr: &dyn Send = &0;
115+
let parts: (*const (), *const u8) = unsafe { mem::transmute(ptr) };
116+
let vtable = parts.1 ;
117+
let offset = vtable.align_offset(mem::align_of::<TWOPTR>());
118+
let _vtable_aligned = vtable.wrapping_add(offset) as *const [TWOPTR; 0];
119+
// FIXME: we can't actually do the access since vtable pointers act like zero-sized allocations.
120+
// Enable the next line once https://github.com/rust-lang/rust/issues/117945 is implemented.
121+
//let _place = unsafe { &*vtable_aligned };
122+
}
123+
104124
fn main() {
105125
test_align_to();
106126
test_from_utf8();
107127
test_u64_array();
108128
test_cstr();
109129
huge_align();
130+
vtable();
110131
}

0 commit comments

Comments
 (0)