Skip to content

Implement rwlocks on Windows #1461

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

Merged
merged 5 commits into from
Jun 28, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion src/shims/tls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
fn schedule_windows_tls_dtors(&mut self) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let active_thread = this.get_active_thread();
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows not supported");
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
// Windows has a special magic linker section that is run on certain events.
// Instead of searching for that section and supporting arbitrary hooks in there
// (that would be basically https://github.com/rust-lang/miri/issues/450),
Expand Down
33 changes: 29 additions & 4 deletions src/shims/windows/dlsym.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@ use rustc_middle::mir;

use crate::*;
use helpers::check_arg_count;
use shims::windows::sync::EvalContextExt as _;

#[derive(Debug, Copy, Clone)]
pub enum Dlsym {
AcquireSRWLockExclusive,
ReleaseSRWLockExclusive,
TryAcquireSRWLockExclusive,
AcquireSRWLockShared,
ReleaseSRWLockShared,
TryAcquireSRWLockShared,
}

impl Dlsym {
Expand All @@ -15,7 +20,11 @@ impl Dlsym {
pub fn from_str(name: &str) -> InterpResult<'static, Option<Dlsym>> {
Ok(match name {
"AcquireSRWLockExclusive" => Some(Dlsym::AcquireSRWLockExclusive),
"ReleaseSRWLockExclusive" => Some(Dlsym::ReleaseSRWLockExclusive),
"TryAcquireSRWLockExclusive" => Some(Dlsym::TryAcquireSRWLockExclusive),
"AcquireSRWLockShared" => Some(Dlsym::AcquireSRWLockShared),
"ReleaseSRWLockShared" => Some(Dlsym::ReleaseSRWLockShared),
"TryAcquireSRWLockShared" => Some(Dlsym::TryAcquireSRWLockShared),
"SetThreadStackGuarantee" => None,
"GetSystemTimePreciseAsFileTime" => None,
_ => throw_unsup_format!("unsupported Windows dlsym: {}", name),
Expand All @@ -38,13 +47,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
match dlsym {
Dlsym::AcquireSRWLockExclusive => {
let &[ptr] = check_arg_count(args)?;
let lock = this.deref_operand(ptr)?; // points to ptr-sized data
throw_unsup_format!("AcquireSRWLockExclusive is not actually implemented");
this.AcquireSRWLockExclusive(ptr)?;
}
Dlsym::ReleaseSRWLockExclusive => {
let &[ptr] = check_arg_count(args)?;
this.ReleaseSRWLockExclusive(ptr)?;
}
Dlsym::TryAcquireSRWLockExclusive => {
let &[ptr] = check_arg_count(args)?;
let ret = this.TryAcquireSRWLockExclusive(ptr)?;
this.write_scalar(Scalar::from_u8(ret), dest)?;
}
Dlsym::AcquireSRWLockShared => {
let &[ptr] = check_arg_count(args)?;
let lock = this.deref_operand(ptr)?; // points to ptr-sized data
throw_unsup_format!("AcquireSRWLockExclusive is not actually implemented");
this.AcquireSRWLockShared(ptr)?;
}
Dlsym::ReleaseSRWLockShared => {
let &[ptr] = check_arg_count(args)?;
this.ReleaseSRWLockShared(ptr)?;
}
Dlsym::TryAcquireSRWLockShared => {
let &[ptr] = check_arg_count(args)?;
let ret = this.TryAcquireSRWLockShared(ptr)?;
this.write_scalar(Scalar::from_u8(ret), dest)?;
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/shims/windows/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
// HANDLE = isize
// DWORD = ULONG = u32
// BOOL = i32
// BOOLEAN = u8
match link_name {
// Environment related shims
"GetEnvironmentVariableW" => {
Expand Down Expand Up @@ -301,7 +302,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
#[allow(non_snake_case)]
let &[_lpCriticalSection] = check_arg_count(args)?;
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows not supported");
// There is only one thread, so this always succeeds and returns TRUE
// There is only one thread, so this always succeeds and returns TRUE.
this.write_scalar(Scalar::from_i32(1), dest)?;
}

Expand Down
161 changes: 161 additions & 0 deletions src/shims/windows/sync.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
use rustc_target::abi::Size;

use crate::*;

// Locks are pointer-sized pieces of data, initialized to 0.
// We use them to count readers, with usize::MAX representing the write-locked state.

fn deref_lock<'mir, 'tcx: 'mir>(
ecx: &mut MiriEvalContext<'mir, 'tcx>,
lock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx, MPlaceTy<'tcx, Tag>> {
// `lock` is a pointer to `void*`; cast it to a pointer to `usize`.
let lock = ecx.deref_operand(lock_op)?;
let usize = ecx.machine.layouts.usize;
assert_eq!(lock.layout.size, usize.size);
Ok(lock.offset(Size::ZERO, MemPlaceMeta::None, usize, ecx)?)
}

impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
#[allow(non_snake_case)]
fn AcquireSRWLockExclusive(
Copy link
Contributor

Choose a reason for hiding this comment

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

This function could just invoke its Try* equivalent and cause the deadlock error in case the result is 0. That should help deduplicating the logic

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you looked at an old intermediate commit -- this changed quite a bit for the final PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I entirely re-implemented rwlocks later and figured I would keep the history. Sorry for the confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

😆 I didn't get that far yet

Copy link
Contributor

Choose a reason for hiding this comment

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

so the thread id assertions are gone because the logic would now support threading? (I know we don't support thread starting yet at all, but the lock logic works now correctly for threads?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. It's the same logic as what we use on POSIX targets.

&mut self,
lock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");

let lock = deref_lock(this, lock_op)?;
let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
if lock_val == 0 {
// Currently not locked. Lock it.
let new_val = Scalar::from_machine_usize(this.machine_usize_max(), this);
this.write_scalar(new_val, lock.into())?;
} else {
// Lock is already held. This is a deadlock.
throw_machine_stop!(TerminationInfo::Deadlock);
}

Ok(())
}

#[allow(non_snake_case)]
fn TryAcquireSRWLockExclusive(
&mut self,
lock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx, u8> {
let this = self.eval_context_mut();
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");

let lock = deref_lock(this, lock_op)?;
let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
if lock_val == 0 {
// Currently not locked. Lock it.
let new_val = this.machine_usize_max();
this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
Ok(1)
} else {
// Lock is already held.
Ok(0)
}
}

#[allow(non_snake_case)]
fn ReleaseSRWLockExclusive(
&mut self,
lock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");

let lock = deref_lock(this, lock_op)?;
let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
if lock_val == this.machine_usize_max() {
// Currently locked. Unlock it.
let new_val = 0;
this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
} else {
// Lock is not locked.
throw_ub_format!("calling ReleaseSRWLockExclusive on an SRWLock that is not exclusively locked");
}

Ok(())
}

#[allow(non_snake_case)]
fn AcquireSRWLockShared(
&mut self,
lock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");

let lock = deref_lock(this, lock_op)?;
let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
if lock_val == this.machine_usize_max() {
// Currently write locked. This is a deadlock.
throw_machine_stop!(TerminationInfo::Deadlock);
} else {
// Bump up read counter (cannot overflow as we just checkd against usize::MAX);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Bump up read counter (cannot overflow as we just checkd against usize::MAX);
// Bump up read counter (cannot overflow as we just checked against usize::MAX);

let new_val = lock_val+1;
// Make sure this does not reach the "write locked" flag.
Copy link
Contributor

Choose a reason for hiding this comment

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

nice.

if new_val == this.machine_usize_max() {
throw_unsup_format!("SRWLock read-acquired too many times");
}
this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
}

Ok(())
}

#[allow(non_snake_case)]
fn TryAcquireSRWLockShared(
&mut self,
lock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx, u8> {
let this = self.eval_context_mut();
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");

let lock = deref_lock(this, lock_op)?;
let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
if lock_val == this.machine_usize_max() {
// Currently write locked.
Ok(0)
} else {
// Bump up read counter (cannot overflow as we just checkd against usize::MAX);
let new_val = lock_val+1;
// Make sure this does not reach the "write locked" flag.
if new_val == this.machine_usize_max() {
throw_unsup_format!("SRWLock read-acquired too many times");
}
this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
Ok(1)
}
}

#[allow(non_snake_case)]
fn ReleaseSRWLockShared(
&mut self,
lock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");

let lock = deref_lock(this, lock_op)?;
let lock_val = this.read_scalar(lock.into())?.to_machine_usize(this)?;
if lock_val == this.machine_usize_max() {
// Currently write locked. This is a UB.
throw_ub_format!("calling ReleaseSRWLockShared on write-locked SRWLock");
} else if lock_val == 0 {
// Currently not locked at all.
throw_ub_format!("calling ReleaseSRWLockShared on unlocked SRWLock");
} else {
// Decrement read counter (cannot overflow as we just checkd against 0);
let new_val = lock_val-1;
this.write_scalar(Scalar::from_machine_usize(new_val, this), lock.into())?;
}

Ok(())
}
}