Skip to content

Commit dc847bf

Browse files
committed
address review comments
1 parent 9bf68cd commit dc847bf

File tree

7 files changed

+131
-57
lines changed

7 files changed

+131
-57
lines changed

src/concurrency/sync.rs

+7-7
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
269269
let this = self.eval_context_mut();
270270
if this.mutex_is_locked(mutex) {
271271
assert_ne!(this.mutex_get_owner(mutex), this.active_thread());
272-
this.mutex_enqueue_and_block(mutex, Some(retval), dest);
272+
this.mutex_enqueue_and_block(mutex, Some((retval, dest)));
273273
} else {
274274
// We can have it right now!
275275
this.mutex_lock(mutex);
@@ -390,13 +390,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
390390
}
391391

392392
/// Put the thread into the queue waiting for the mutex.
393-
/// Once the Mutex becomes available, `retval` will be written to `dest`.
393+
///
394+
/// Once the Mutex becomes available and if it exists, `retval_dest.0` will
395+
/// be written to `retval_dest.1`.
394396
#[inline]
395397
fn mutex_enqueue_and_block(
396398
&mut self,
397399
id: MutexId,
398-
retval: Option<Scalar>,
399-
dest: MPlaceTy<'tcx>,
400+
retval_dest: Option<(Scalar, MPlaceTy<'tcx>)>,
400401
) {
401402
let this = self.eval_context_mut();
402403
assert!(this.mutex_is_locked(id), "queing on unlocked mutex");
@@ -408,14 +409,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
408409
callback!(
409410
@capture<'tcx> {
410411
id: MutexId,
411-
retval: Option<Scalar>,
412-
dest: MPlaceTy<'tcx>,
412+
retval_dest: Option<(Scalar, MPlaceTy<'tcx>)>,
413413
}
414414
@unblock = |this| {
415415
assert!(!this.mutex_is_locked(id));
416416
this.mutex_lock(id);
417417

418-
if let Some(retval) = retval {
418+
if let Some((retval, dest)) = retval_dest {
419419
this.write_scalar(retval, &dest)?;
420420
}
421421

src/provenance_gc.rs

+11
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,17 @@ impl VisitProvenance for crate::MiriInterpCx<'_> {
162162
}
163163
}
164164

165+
impl<A, B> VisitProvenance for (A, B)
166+
where
167+
A: VisitProvenance,
168+
B: VisitProvenance,
169+
{
170+
fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
171+
self.0.visit_provenance(visit);
172+
self.1.visit_provenance(visit);
173+
}
174+
}
175+
165176
pub struct LiveAllocs<'a, 'tcx> {
166177
collected: FxHashSet<AllocId>,
167178
ecx: &'a MiriInterpCx<'tcx>,

src/shims/unix/macos/foreign_items.rs

+6-48
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use rustc_span::Symbol;
22
use rustc_target::spec::abi::Abi;
33

4+
use super::sync::EvalContextExt as _;
45
use crate::shims::unix::*;
56
use crate::*;
67

@@ -176,66 +177,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
176177

177178
"os_unfair_lock_lock" => {
178179
let [lock_op] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
179-
let id =
180-
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)?;
181-
182-
if this.mutex_is_locked(id) {
183-
if this.mutex_get_owner(id) == this.active_thread() {
184-
throw_machine_stop!(TerminationInfo::Abort(
185-
"attempted to lock an os_unfair_lock that is already locked by the current thread".to_owned()
186-
));
187-
}
188-
189-
this.mutex_enqueue_and_block(id, None, dest.clone());
190-
} else {
191-
this.mutex_lock(id);
192-
}
180+
this.os_unfair_lock_lock(lock_op)?;
193181
}
194182
"os_unfair_lock_trylock" => {
195183
let [lock_op] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
196-
let id =
197-
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)?;
198-
199-
if this.mutex_is_locked(id) {
200-
this.write_scalar(Scalar::from_bool(false), dest)?;
201-
} else {
202-
this.mutex_lock(id);
203-
this.write_scalar(Scalar::from_bool(true), dest)?;
204-
}
184+
this.os_unfair_lock_trylock(lock_op, dest)?;
205185
}
206186
"os_unfair_lock_unlock" => {
207187
let [lock_op] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
208-
let id =
209-
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)?;
210-
211-
if this.mutex_unlock(id)?.is_none() {
212-
throw_machine_stop!(TerminationInfo::Abort(
213-
"attempted to unlock an os_unfair_lock not owned by the current thread"
214-
.to_owned()
215-
));
216-
}
188+
this.os_unfair_lock_unlock(lock_op)?;
217189
}
218190
"os_unfair_lock_assert_owner" => {
219191
let [lock_op] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
220-
let id =
221-
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)?;
222-
223-
if !this.mutex_is_locked(id) || this.mutex_get_owner(id) != this.active_thread() {
224-
throw_machine_stop!(TerminationInfo::Abort(
225-
"called os_unfair_lock_assert_owner on an os_unfair_lock not owned by the current thread".to_owned()
226-
));
227-
}
192+
this.os_unfair_lock_assert_owner(lock_op)?;
228193
}
229194
"os_unfair_lock_assert_not_owner" => {
230195
let [lock_op] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
231-
let id =
232-
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)?;
233-
234-
if this.mutex_is_locked(id) && this.mutex_get_owner(id) == this.active_thread() {
235-
throw_machine_stop!(TerminationInfo::Abort(
236-
"called os_unfair_lock_assert_not_owner on an os_unfair_lock owned by the current thread".to_owned()
237-
));
238-
}
196+
this.os_unfair_lock_assert_not_owner(lock_op)?;
239197
}
240198

241199
_ => return Ok(EmulateItemResult::NotSupported),

src/shims/unix/macos/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
pub mod foreign_items;
2+
pub mod sync;

src/shims/unix/macos/sync.rs

+102
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
//! Contains macOS-specific synchronization functions.
2+
//!
3+
//! For `os_unfair_lock`, see the documentation [here]
4+
//! (https://developer.apple.com/documentation/os/synchronization?language=objc)
5+
//! and in case of underspecification its implementation [here]
6+
//! (https://github.com/apple-oss-distributions/libplatform/blob/a00a4cc36da2110578bcf3b8eeeeb93dcc7f4e11/src/os/lock.c#L645)
7+
8+
use crate::*;
9+
10+
impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {}
11+
trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
12+
fn os_unfair_lock_getid(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx, MutexId> {
13+
let this = self.eval_context_mut();
14+
// os_unfair_lock holds a 32-bit value, is initialized with zero and
15+
// must be assumed to be opaque. Therefore, we can just store our
16+
// internal mutex ID in the structure without anyone noticing.
17+
this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)
18+
}
19+
}
20+
21+
impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
22+
pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
23+
fn os_unfair_lock_lock(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
24+
let this = self.eval_context_mut();
25+
26+
let id = this.os_unfair_lock_getid(lock_op)?;
27+
if this.mutex_is_locked(id) {
28+
if this.mutex_get_owner(id) == this.active_thread() {
29+
// Matching the current macOS implementation: abort on reentrant locking.
30+
throw_machine_stop!(TerminationInfo::Abort(
31+
"attempted to lock an os_unfair_lock that is already locked by the current thread".to_owned()
32+
));
33+
}
34+
35+
this.mutex_enqueue_and_block(id, None);
36+
} else {
37+
this.mutex_lock(id);
38+
}
39+
40+
Ok(())
41+
}
42+
43+
fn os_unfair_lock_trylock(
44+
&mut self,
45+
lock_op: &OpTy<'tcx>,
46+
dest: &MPlaceTy<'tcx>,
47+
) -> InterpResult<'tcx> {
48+
let this = self.eval_context_mut();
49+
50+
let id = this.os_unfair_lock_getid(lock_op)?;
51+
if this.mutex_is_locked(id) {
52+
// Contrary to the blocking lock function, this does not check for
53+
// reentrancy.
54+
this.write_scalar(Scalar::from_bool(false), dest)?;
55+
} else {
56+
this.mutex_lock(id);
57+
this.write_scalar(Scalar::from_bool(true), dest)?;
58+
}
59+
60+
Ok(())
61+
}
62+
63+
fn os_unfair_lock_unlock(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
64+
let this = self.eval_context_mut();
65+
66+
let id = this.os_unfair_lock_getid(lock_op)?;
67+
if this.mutex_unlock(id)?.is_none() {
68+
// Matching the current macOS implementation: abort.
69+
throw_machine_stop!(TerminationInfo::Abort(
70+
"attempted to unlock an os_unfair_lock not owned by the current thread".to_owned()
71+
));
72+
}
73+
74+
Ok(())
75+
}
76+
77+
fn os_unfair_lock_assert_owner(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
78+
let this = self.eval_context_mut();
79+
80+
let id = this.os_unfair_lock_getid(lock_op)?;
81+
if !this.mutex_is_locked(id) || this.mutex_get_owner(id) != this.active_thread() {
82+
throw_machine_stop!(TerminationInfo::Abort(
83+
"called os_unfair_lock_assert_owner on an os_unfair_lock not owned by the current thread".to_owned()
84+
));
85+
}
86+
87+
Ok(())
88+
}
89+
90+
fn os_unfair_lock_assert_not_owner(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
91+
let this = self.eval_context_mut();
92+
93+
let id = this.os_unfair_lock_getid(lock_op)?;
94+
if this.mutex_is_locked(id) && this.mutex_get_owner(id) == this.active_thread() {
95+
throw_machine_stop!(TerminationInfo::Abort(
96+
"called os_unfair_lock_assert_not_owner on an os_unfair_lock owned by the current thread".to_owned()
97+
));
98+
}
99+
100+
Ok(())
101+
}
102+
}

src/shims/unix/sync.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
473473
let ret = if this.mutex_is_locked(id) {
474474
let owner_thread = this.mutex_get_owner(id);
475475
if owner_thread != this.active_thread() {
476-
this.mutex_enqueue_and_block(id, Some(Scalar::from_i32(0)), dest.clone());
476+
this.mutex_enqueue_and_block(id, Some((Scalar::from_i32(0), dest.clone())));
477477
return Ok(());
478478
} else {
479479
// Trying to acquire the same mutex again.

tests/pass-dep/concurrency/apple-os-unfair-lock.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,10 @@ fn main() {
1414
libc::os_unfair_lock_assert_not_owner(lock.get());
1515
}
1616

17-
// `os_unfair_lock`s can be moved and leaked:
17+
// `os_unfair_lock`s can be moved and leaked, even when locked,
18+
// as long as no operation is occurring on the lock:
1819
let lock = lock;
1920
let locked = unsafe { libc::os_unfair_lock_trylock(lock.get()) };
2021
assert!(locked);
22+
let _lock = lock;
2123
}

0 commit comments

Comments
 (0)