Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

miri: fix ICE with symbolic alignment check on extern static #120683

Merged
merged 1 commit into from
Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -992,10 +992,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));
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/borrow_tracker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might have been an ICE waiting to happen, but it didn't seem worth trying to craft a testcase.

let alloc_borrow_tracker = &alloc_extra.borrow_tracker.as_ref().unwrap();
alloc_borrow_tracker.release_protector(&this.machine, borrow_tracker, *tag)?;
}
Expand Down
34 changes: 19 additions & 15 deletions src/tools/miri/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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()),
}
}

Expand Down Expand Up @@ -810,6 +810,7 @@ impl VisitProvenance for MiriMachine<'_, '_> {
collect_leak_backtraces: _,
allocation_spans: _,
const_cache: _,
symbolic_alignment: _,
} = self;

threads.visit_provenance(visit);
Expand Down Expand Up @@ -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 })
Expand Down Expand Up @@ -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),
)?;
Expand Down
1 change: 1 addition & 0 deletions src/tools/miri/src/provenance_gc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions src/tools/miri/src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
@@ -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

23 changes: 22 additions & 1 deletion src/tools/miri/tests/pass/align_offset_symbolic.rs
Original file line number Diff line number Diff line change
@@ -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]);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}