Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Mandragorian committed Aug 30, 2024
1 parent 18bad5a commit 2e1a917
Show file tree
Hide file tree
Showing 4 changed files with 183 additions and 101 deletions.
10 changes: 8 additions & 2 deletions src/concurrency/init_once.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
offset: u64,
) -> InterpResult<'tcx, InitOnceId> {
let this = self.eval_context_mut();
this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.init_onces)?
.ok_or_else(|| err_ub_format!("init_once has invalid ID").into())
this.get_or_create_id(
lock_op,
lock_layout,
offset,
|ecx| &mut ecx.machine.sync.init_onces,
|_| Ok(Default::default()),
)?
.ok_or_else(|| err_ub_format!("init_once has invalid ID").into())
}

#[inline]
Expand Down
92 changes: 79 additions & 13 deletions src/concurrency/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub struct AdditionalMutexData {
pub kind: MutexKind,

/// The address of the mutex.
pub address: Pointer,
pub address: u64,
}

/// The mutex state.
Expand Down Expand Up @@ -195,14 +195,15 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
/// Returns `None` if memory stores a non-zero invalid ID.
///
/// `get_objs` must return the `IndexVec` that stores all the objects of this type.
/// `create_obj` must create the new object if initialization is needed.
#[inline]
fn get_or_create_id<Id: SyncId + Idx, T: Default>(
fn get_or_create_id<Id: SyncId + Idx, T>(
&mut self,
lock_op: &OpTy<'tcx>,
lock_layout: TyAndLayout<'tcx>,
offset: u64,
get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec<Id, T>,
initialize_data: impl FnOnce() -> InterpResult<'tcx, Option<AdditionalMutexData>>,
create_obj: impl for<'a> FnOnce(&'a mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>,
) -> InterpResult<'tcx, Option<Id>> {
let this = self.eval_context_mut();
let value_place =
Expand All @@ -224,10 +225,9 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
Ok(if success.to_bool().expect("compare_exchange's second return value is a bool") {
// We set the in-memory ID to `next_index`, now also create this object in the machine
// state.
let new_index = get_objs(this).push(T::default());
let obj = create_obj(this)?;
let new_index = get_objs(this).push(obj);
assert_eq!(next_index, new_index);
let data = initialize_data()?;
get_objs(this)[new_index].data = data;
Some(new_index)
} else {
let id = Id::from_u32(old.to_u32().expect("layout is u32"));
Expand All @@ -240,6 +240,32 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
})
}

/// Eagerly creates a Miri sync structure.
///
/// `create_id` will store the index of the sync_structure in the memory pointed to by
/// `lock_op`, so that future calls to `get_or_create_id` will see it as initialized.
/// - `lock_op` must hold a pointer to the sync structure.
/// - `lock_layout` must be the memory layout of the sync structure.
/// - `offset` must be the offset inside the sync structure where its miri id will be stored.
/// - `get_objs` is described in `get_or_create_id`.
/// - `obj` must be the new sync object.
fn create_id<Id: SyncId + Idx, T>(
&mut self,
lock_op: &OpTy<'tcx>,
lock_layout: TyAndLayout<'tcx>,
offset: u64,
get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec<Id, T>,
obj: T,
) -> InterpResult<'tcx, Id> {
let this = self.eval_context_mut();
let value_place =
this.deref_pointer_and_offset(lock_op, offset, lock_layout, this.machine.layouts.u32)?;

let new_index = get_objs(this).push(obj);
this.write_scalar(Scalar::from_u32(new_index.to_u32()), &value_place)?;
Ok(new_index)
}

fn condvar_reacquire_mutex(
&mut self,
mutex: MutexId,
Expand All @@ -266,16 +292,44 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
// situations.
impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
/// Eagerly create and initialize a new mutex.
fn mutex_create(
&mut self,
lock_op: &OpTy<'tcx>,
lock_layout: TyAndLayout<'tcx>,
offset: u64,
data: Option<AdditionalMutexData>,
) -> InterpResult<'tcx, MutexId> {
let this = self.eval_context_mut();
this.create_id(
lock_op,
lock_layout,
offset,
|ecx| &mut ecx.machine.sync.mutexes,
Mutex { data, ..Default::default() },
)
}

/// Lazily create a new mutex.
/// `initialize_data` must return any additional data that a user wants to associate with the mutex.
fn mutex_get_or_create_id(
&mut self,
lock_op: &OpTy<'tcx>,
lock_layout: TyAndLayout<'tcx>,
offset: u64,
initialize_data: impl FnOnce() -> InterpResult<'tcx, Option<AdditionalMutexData>>,
initialize_data: impl for<'a> FnOnce(
&'a mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, Option<AdditionalMutexData>>,
) -> InterpResult<'tcx, MutexId> {
let this = self.eval_context_mut();
this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.mutexes)?
.ok_or_else(|| err_ub_format!("mutex has invalid ID").into(), initialize_data)
this.get_or_create_id(
lock_op,
lock_layout,
offset,
|ecx| &mut ecx.machine.sync.mutexes,
|ecx| initialize_data(ecx).map(|data| Mutex { data, ..Default::default() }),
)?
.ok_or_else(|| err_ub_format!("mutex has invalid ID").into())
}

/// Retrieve the additional data stored for a mutex.
Expand All @@ -294,8 +348,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
offset: u64,
) -> InterpResult<'tcx, RwLockId> {
let this = self.eval_context_mut();
this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.rwlocks)?
.ok_or_else(|| err_ub_format!("rwlock has invalid ID").into())
this.get_or_create_id(
lock_op,
lock_layout,
offset,
|ecx| &mut ecx.machine.sync.rwlocks,
|_| Ok(Default::default()),
)?
.ok_or_else(|| err_ub_format!("rwlock has invalid ID").into())
}

fn condvar_get_or_create_id(
Expand All @@ -305,8 +365,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
offset: u64,
) -> InterpResult<'tcx, CondvarId> {
let this = self.eval_context_mut();
this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.condvars)?
.ok_or_else(|| err_ub_format!("condvar has invalid ID").into())
this.get_or_create_id(
lock_op,
lock_layout,
offset,
|ecx| &mut ecx.machine.sync.condvars,
|_| Ok(Default::default()),
)?
.ok_or_else(|| err_ub_format!("condvar has invalid ID").into())
}

#[inline]
Expand Down
2 changes: 1 addition & 1 deletion src/shims/unix/macos/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
// os_unfair_lock holds a 32-bit value, is initialized with zero and
// must be assumed to be opaque. Therefore, we can just store our
// internal mutex ID in the structure without anyone noticing.
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0, || Ok(None))
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0, |_| Ok(None))
}
}

Expand Down
Loading

0 comments on commit 2e1a917

Please sign in to comment.