From dca00ab85ec970432ebaf1cf59b0e795db2d65cd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 27 Jun 2020 13:50:52 +0200 Subject: [PATCH 1/5] introduce platform-specific module hierarchy for dlsym (similar to foreign_items) --- src/shims/dlsym.rs | 43 ++++----------- src/shims/posix/dlsym.rs | 39 +++++++++++++ src/shims/posix/linux/dlsym.rs | 34 ++++++++++++ src/shims/posix/linux/mod.rs | 1 + src/shims/posix/macos/dlsym.rs | 49 +++++++++++++++++ src/shims/posix/macos/mod.rs | 1 + src/shims/posix/mod.rs | 1 + src/shims/windows/dlsym.rs | 55 +++++++++++++++++++ src/shims/windows/mod.rs | 3 + src/shims/windows/sync.rs | 0 .../run-pass/concurrency/sync_singlethread.rs | 6 +- 11 files changed, 194 insertions(+), 38 deletions(-) create mode 100644 src/shims/posix/dlsym.rs create mode 100644 src/shims/posix/linux/dlsym.rs create mode 100644 src/shims/posix/macos/dlsym.rs create mode 100644 src/shims/windows/dlsym.rs create mode 100644 src/shims/windows/sync.rs diff --git a/src/shims/dlsym.rs b/src/shims/dlsym.rs index 87c7f447ac03..9b15cb9ac9a3 100644 --- a/src/shims/dlsym.rs +++ b/src/shims/dlsym.rs @@ -1,34 +1,24 @@ use rustc_middle::mir; use crate::*; -use helpers::check_arg_count; +use shims::posix::dlsym as posix; +use shims::windows::dlsym as windows; #[derive(Debug, Copy, Clone)] +#[allow(non_camel_case_types)] pub enum Dlsym { - GetEntropy, + Posix(posix::Dlsym), + Windows(windows::Dlsym), } impl Dlsym { // Returns an error for unsupported symbols, and None if this symbol // should become a NULL pointer (pretend it does not exist). pub fn from_str(name: &[u8], target_os: &str) -> InterpResult<'static, Option> { - use self::Dlsym::*; - let name = String::from_utf8_lossy(name); + let name = &*String::from_utf8_lossy(name); Ok(match target_os { - "linux" => match &*name { - "__pthread_get_minstack" => None, - _ => throw_unsup_format!("unsupported Linux dlsym: {}", name), - } - "macos" => match &*name { - "getentropy" => Some(GetEntropy), - _ => throw_unsup_format!("unsupported macOS dlsym: {}", name), - } - "windows" => match &*name { - "SetThreadStackGuarantee" => None, - "AcquireSRWLockExclusive" => None, - "GetSystemTimePreciseAsFileTime" => None, - _ => throw_unsup_format!("unsupported Windows dlsym: {}", name), - } + "linux" | "macos" => posix::Dlsym::from_str(name, target_os)?.map(Dlsym::Posix), + "windows" => windows::Dlsym::from_str(name)?.map(Dlsym::Windows), os => bug!("dlsym not implemented for target_os {}", os), }) } @@ -42,23 +32,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx args: &[OpTy<'tcx, Tag>], ret: Option<(PlaceTy<'tcx, Tag>, mir::BasicBlock)>, ) -> InterpResult<'tcx> { - use self::Dlsym::*; - let this = self.eval_context_mut(); - let (dest, ret) = ret.expect("we don't support any diverging dlsym"); - match dlsym { - GetEntropy => { - let &[ptr, len] = check_arg_count(args)?; - let ptr = this.read_scalar(ptr)?.not_undef()?; - let len = this.read_scalar(len)?.to_machine_usize(this)?; - this.gen_random(ptr, len)?; - this.write_null(dest)?; - } + Dlsym::Posix(dlsym) => posix::EvalContextExt::call_dlsym(this, dlsym, args, ret), + Dlsym::Windows(dlsym) => windows::EvalContextExt::call_dlsym(this, dlsym, args, ret), } - - this.dump_place(*dest); - this.go_to_block(ret); - Ok(()) } } diff --git a/src/shims/posix/dlsym.rs b/src/shims/posix/dlsym.rs new file mode 100644 index 000000000000..52d9844bed51 --- /dev/null +++ b/src/shims/posix/dlsym.rs @@ -0,0 +1,39 @@ +use rustc_middle::mir; + +use crate::*; +use shims::posix::linux::dlsym as linux; +use shims::posix::macos::dlsym as macos; + +#[derive(Debug, Copy, Clone)] +pub enum Dlsym { + Linux(linux::Dlsym), + MacOs(macos::Dlsym), +} + +impl Dlsym { + // Returns an error for unsupported symbols, and None if this symbol + // should become a NULL pointer (pretend it does not exist). + pub fn from_str(name: &str, target_os: &str) -> InterpResult<'static, Option> { + Ok(match target_os { + "linux" => linux::Dlsym::from_str(name)?.map(Dlsym::Linux), + "macos" => macos::Dlsym::from_str(name)?.map(Dlsym::MacOs), + _ => unreachable!(), + }) + } +} + +impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + fn call_dlsym( + &mut self, + dlsym: Dlsym, + args: &[OpTy<'tcx, Tag>], + ret: Option<(PlaceTy<'tcx, Tag>, mir::BasicBlock)>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + match dlsym { + Dlsym::Linux(dlsym) => linux::EvalContextExt::call_dlsym(this, dlsym, args, ret), + Dlsym::MacOs(dlsym) => macos::EvalContextExt::call_dlsym(this, dlsym, args, ret), + } + } +} diff --git a/src/shims/posix/linux/dlsym.rs b/src/shims/posix/linux/dlsym.rs new file mode 100644 index 000000000000..9be300edf495 --- /dev/null +++ b/src/shims/posix/linux/dlsym.rs @@ -0,0 +1,34 @@ +use rustc_middle::mir; + +use crate::*; + +#[derive(Debug, Copy, Clone)] +pub enum Dlsym { +} + +impl Dlsym { + // Returns an error for unsupported symbols, and None if this symbol + // should become a NULL pointer (pretend it does not exist). + pub fn from_str(name: &str) -> InterpResult<'static, Option> { + Ok(match &*name { + "__pthread_get_minstack" => None, + _ => throw_unsup_format!("unsupported Linux dlsym: {}", name), + }) + } +} + +impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + fn call_dlsym( + &mut self, + dlsym: Dlsym, + _args: &[OpTy<'tcx, Tag>], + ret: Option<(PlaceTy<'tcx, Tag>, mir::BasicBlock)>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let (_dest, _ret) = ret.expect("we don't support any diverging dlsym"); + assert!(this.tcx.sess.target.target.target_os == "linux"); + + match dlsym {} + } +} diff --git a/src/shims/posix/linux/mod.rs b/src/shims/posix/linux/mod.rs index 09c6507b24f8..cadd6a8ea384 100644 --- a/src/shims/posix/linux/mod.rs +++ b/src/shims/posix/linux/mod.rs @@ -1 +1,2 @@ pub mod foreign_items; +pub mod dlsym; diff --git a/src/shims/posix/macos/dlsym.rs b/src/shims/posix/macos/dlsym.rs new file mode 100644 index 000000000000..8256c10b0d39 --- /dev/null +++ b/src/shims/posix/macos/dlsym.rs @@ -0,0 +1,49 @@ +use rustc_middle::mir; + +use crate::*; +use helpers::check_arg_count; + +#[derive(Debug, Copy, Clone)] +#[allow(non_camel_case_types)] +pub enum Dlsym { + getentropy, +} + +impl Dlsym { + // Returns an error for unsupported symbols, and None if this symbol + // should become a NULL pointer (pretend it does not exist). + pub fn from_str(name: &str) -> InterpResult<'static, Option> { + Ok(match name { + "getentropy" => Some(Dlsym::getentropy), + _ => throw_unsup_format!("unsupported macOS dlsym: {}", name), + }) + } +} + +impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + fn call_dlsym( + &mut self, + dlsym: Dlsym, + args: &[OpTy<'tcx, Tag>], + ret: Option<(PlaceTy<'tcx, Tag>, mir::BasicBlock)>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let (dest, ret) = ret.expect("we don't support any diverging dlsym"); + assert!(this.tcx.sess.target.target.target_os == "macos"); + + match dlsym { + Dlsym::getentropy => { + let &[ptr, len] = check_arg_count(args)?; + let ptr = this.read_scalar(ptr)?.not_undef()?; + let len = this.read_scalar(len)?.to_machine_usize(this)?; + this.gen_random(ptr, len)?; + this.write_null(dest)?; + } + } + + this.dump_place(*dest); + this.go_to_block(ret); + Ok(()) + } +} diff --git a/src/shims/posix/macos/mod.rs b/src/shims/posix/macos/mod.rs index 09c6507b24f8..cadd6a8ea384 100644 --- a/src/shims/posix/macos/mod.rs +++ b/src/shims/posix/macos/mod.rs @@ -1 +1,2 @@ pub mod foreign_items; +pub mod dlsym; diff --git a/src/shims/posix/mod.rs b/src/shims/posix/mod.rs index 2f505cfb9c0b..9916c65be0fb 100644 --- a/src/shims/posix/mod.rs +++ b/src/shims/posix/mod.rs @@ -1,4 +1,5 @@ pub mod foreign_items; +pub mod dlsym; mod fs; mod sync; diff --git a/src/shims/windows/dlsym.rs b/src/shims/windows/dlsym.rs new file mode 100644 index 000000000000..34ed6ca150e0 --- /dev/null +++ b/src/shims/windows/dlsym.rs @@ -0,0 +1,55 @@ +use rustc_middle::mir; + +use crate::*; +use helpers::check_arg_count; + +#[derive(Debug, Copy, Clone)] +pub enum Dlsym { + AcquireSRWLockExclusive, + AcquireSRWLockShared, +} + +impl Dlsym { + // Returns an error for unsupported symbols, and None if this symbol + // should become a NULL pointer (pretend it does not exist). + pub fn from_str(name: &str) -> InterpResult<'static, Option> { + Ok(match name { + "AcquireSRWLockExclusive" => Some(Dlsym::AcquireSRWLockExclusive), + "AcquireSRWLockShared" => Some(Dlsym::AcquireSRWLockShared), + "SetThreadStackGuarantee" => None, + "GetSystemTimePreciseAsFileTime" => None, + _ => throw_unsup_format!("unsupported Windows dlsym: {}", name), + }) + } +} + +impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + fn call_dlsym( + &mut self, + dlsym: Dlsym, + args: &[OpTy<'tcx, Tag>], + ret: Option<(PlaceTy<'tcx, Tag>, mir::BasicBlock)>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let (dest, ret) = ret.expect("we don't support any diverging dlsym"); + assert!(this.tcx.sess.target.target.target_os == "windows"); + + 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"); + } + 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.dump_place(*dest); + this.go_to_block(ret); + Ok(()) + } +} diff --git a/src/shims/windows/mod.rs b/src/shims/windows/mod.rs index 09c6507b24f8..04f9ace8e799 100644 --- a/src/shims/windows/mod.rs +++ b/src/shims/windows/mod.rs @@ -1 +1,4 @@ pub mod foreign_items; +pub mod dlsym; + +mod sync; diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/run-pass/concurrency/sync_singlethread.rs b/tests/run-pass/concurrency/sync_singlethread.rs index 8b8594d4df69..749db855e296 100644 --- a/tests/run-pass/concurrency/sync_singlethread.rs +++ b/tests/run-pass/concurrency/sync_singlethread.rs @@ -6,10 +6,7 @@ use std::hint; fn main() { test_mutex_stdlib(); - #[cfg(not(target_os = "windows"))] // TODO: implement RwLock on Windows - { - test_rwlock_stdlib(); - } + test_rwlock_stdlib(); test_spin_loop_hint(); test_thread_yield_now(); } @@ -24,7 +21,6 @@ fn test_mutex_stdlib() { drop(m); } -#[cfg(not(target_os = "windows"))] fn test_rwlock_stdlib() { use std::sync::RwLock; let rw = RwLock::new(0); From 8e9296994837c82c39b2c4cee7623d9181e4bc80 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 27 Jun 2020 14:35:58 +0200 Subject: [PATCH 2/5] implement Windows SRWLock shims --- src/shims/tls.rs | 2 +- src/shims/windows/dlsym.rs | 33 +++++- src/shims/windows/foreign_items.rs | 3 +- src/shims/windows/sync.rs | 161 +++++++++++++++++++++++++++++ 4 files changed, 193 insertions(+), 6 deletions(-) diff --git a/src/shims/tls.rs b/src/shims/tls.rs index 704598ef2c6c..0cd9ef056505 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -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), diff --git a/src/shims/windows/dlsym.rs b/src/shims/windows/dlsym.rs index 34ed6ca150e0..737fd4314f63 100644 --- a/src/shims/windows/dlsym.rs +++ b/src/shims/windows/dlsym.rs @@ -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 { @@ -15,7 +20,11 @@ impl Dlsym { pub fn from_str(name: &str) -> InterpResult<'static, Option> { 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), @@ -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)?; } } diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index 2a30a2348997..ddb70b752e79 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -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" => { @@ -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)?; } diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index e69de29bb2d1..ef40eb089110 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -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( + &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); + 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(()) + } + + #[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(()) + } +} From e54619b5e18ce9781faed975f5101db621608ea0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 27 Jun 2020 14:43:37 +0200 Subject: [PATCH 3/5] with this, we support panics on Windows --- src/shims/foreign_items.rs | 1 - src/shims/mod.rs | 8 -------- src/shims/panic.rs | 8 -------- tests/compile-fail/abort-terminator.rs | 1 - tests/compile-fail/panic/double_panic.rs | 1 - tests/compile-fail/panic/windows1.rs | 9 --------- tests/compile-fail/panic/windows2.rs | 9 --------- tests/compile-fail/panic/windows3.rs | 10 ---------- tests/run-pass/panic/catch_panic.rs | 2 +- tests/run-pass/panic/div-by-zero-2.rs | 1 - tests/run-pass/panic/div-by-zero-2.stderr | 2 +- tests/run-pass/panic/overflowing-lsh-neg.rs | 1 - tests/run-pass/panic/overflowing-lsh-neg.stderr | 2 +- tests/run-pass/panic/overflowing-rsh-1.rs | 1 - tests/run-pass/panic/overflowing-rsh-1.stderr | 2 +- tests/run-pass/panic/overflowing-rsh-2.rs | 1 - tests/run-pass/panic/overflowing-rsh-2.stderr | 2 +- tests/run-pass/panic/panic1.rs | 1 - tests/run-pass/panic/panic1.stderr | 2 +- tests/run-pass/panic/panic2.rs | 1 - tests/run-pass/panic/panic2.stderr | 2 +- tests/run-pass/panic/panic3.rs | 1 - tests/run-pass/panic/panic3.stderr | 2 +- tests/run-pass/panic/panic4.rs | 1 - tests/run-pass/panic/panic4.stderr | 2 +- tests/run-pass/panic/std-panic-locations.rs | 1 - tests/run-pass/transmute_fat2.rs | 1 - tests/run-pass/transmute_fat2.stderr | 2 +- 28 files changed, 10 insertions(+), 67 deletions(-) delete mode 100644 tests/compile-fail/panic/windows1.rs delete mode 100644 tests/compile-fail/panic/windows2.rs delete mode 100644 tests/compile-fail/panic/windows3.rs diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 14c5aac4899a..a7495beef72e 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -129,7 +129,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // This matches calls to the foreign item `panic_impl`. // The implementation is provided by the function with the `#[panic_handler]` attribute. "panic_impl" => { - this.check_panic_supported()?; let panic_impl_id = tcx.lang_items().panic_impl().unwrap(); let panic_impl_instance = ty::Instance::mono(tcx, panic_impl_id); return Ok(Some(&*this.load_mir(panic_impl_instance.def, None)?)); diff --git a/src/shims/mod.rs b/src/shims/mod.rs index 37e7b8c40462..56754a9ebde5 100644 --- a/src/shims/mod.rs +++ b/src/shims/mod.rs @@ -52,14 +52,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return this.emulate_foreign_item(instance.def_id(), args, ret, unwind); } - // Better error message for panics on Windows. - let def_id = instance.def_id(); - if Some(def_id) == this.tcx.lang_items().begin_panic_fn() || - Some(def_id) == this.tcx.lang_items().panic_impl() - { - this.check_panic_supported()?; - } - // Otherwise, load the MIR. Ok(Some(&*this.load_mir(instance.def, None)?)) } diff --git a/src/shims/panic.rs b/src/shims/panic.rs index 43f90f1b04f7..8e291c201215 100644 --- a/src/shims/panic.rs +++ b/src/shims/panic.rs @@ -34,14 +34,6 @@ pub struct CatchUnwindData<'tcx> { impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { - /// Check if panicking is supported on this target, and give a good error otherwise. - fn check_panic_supported(&self) -> InterpResult<'tcx> { - match self.eval_context_ref().tcx.sess.target.target.target_os.as_str() { - "linux" | "macos" => Ok(()), - _ => throw_unsup_format!("panicking is not supported on this target"), - } - } - /// Handles the special `miri_start_panic` intrinsic, which is called /// by libpanic_unwind to delegate the actual unwinding process to Miri. fn handle_miri_start_panic( diff --git a/tests/compile-fail/abort-terminator.rs b/tests/compile-fail/abort-terminator.rs index af1a155435fb..1bfa289a52b4 100644 --- a/tests/compile-fail/abort-terminator.rs +++ b/tests/compile-fail/abort-terminator.rs @@ -1,5 +1,4 @@ // error-pattern: the evaluated program aborted -// ignore-windows (panics dont work on Windows) #![feature(unwind_attributes)] #[unwind(aborts)] diff --git a/tests/compile-fail/panic/double_panic.rs b/tests/compile-fail/panic/double_panic.rs index 3085d0b00657..80d74f026232 100644 --- a/tests/compile-fail/panic/double_panic.rs +++ b/tests/compile-fail/panic/double_panic.rs @@ -1,5 +1,4 @@ // error-pattern: the evaluated program aborted -// ignore-windows (panics dont work on Windows) struct Foo; impl Drop for Foo { diff --git a/tests/compile-fail/panic/windows1.rs b/tests/compile-fail/panic/windows1.rs deleted file mode 100644 index 142ba85c42c7..000000000000 --- a/tests/compile-fail/panic/windows1.rs +++ /dev/null @@ -1,9 +0,0 @@ -// ignore-linux -// ignore-macos - -// Test that panics on Windows give a reasonable error message. - -// error-pattern: panicking is not supported on this target -fn main() { - core::panic!("this is {}", "Windows"); -} diff --git a/tests/compile-fail/panic/windows2.rs b/tests/compile-fail/panic/windows2.rs deleted file mode 100644 index da2cfb59362e..000000000000 --- a/tests/compile-fail/panic/windows2.rs +++ /dev/null @@ -1,9 +0,0 @@ -// ignore-linux -// ignore-macos - -// Test that panics on Windows give a reasonable error message. - -// error-pattern: panicking is not supported on this target -fn main() { - std::panic!("this is Windows"); -} diff --git a/tests/compile-fail/panic/windows3.rs b/tests/compile-fail/panic/windows3.rs deleted file mode 100644 index a2e7bf5a7d43..000000000000 --- a/tests/compile-fail/panic/windows3.rs +++ /dev/null @@ -1,10 +0,0 @@ -// ignore-linux -// ignore-macos - -// Test that panics on Windows give a reasonable error message. - -// error-pattern: panicking is not supported on this target -#[allow(unconditional_panic)] -fn main() { - let _val = 1/0; -} diff --git a/tests/run-pass/panic/catch_panic.rs b/tests/run-pass/panic/catch_panic.rs index 288ae1965a69..ac41de586e8a 100644 --- a/tests/run-pass/panic/catch_panic.rs +++ b/tests/run-pass/panic/catch_panic.rs @@ -1,7 +1,7 @@ -// ignore-windows: Unwind panicking does not currently work on Windows // normalize-stderr-test "[^ ]*libcore/[a-z/]+.rs[0-9:]*" -> "$$LOC" #![feature(never_type)] #![allow(unconditional_panic)] + use std::panic::{catch_unwind, AssertUnwindSafe}; use std::cell::Cell; diff --git a/tests/run-pass/panic/div-by-zero-2.rs b/tests/run-pass/panic/div-by-zero-2.rs index cfacc9db0d66..fac5415696fc 100644 --- a/tests/run-pass/panic/div-by-zero-2.rs +++ b/tests/run-pass/panic/div-by-zero-2.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows #![allow(unconditional_panic)] fn main() { diff --git a/tests/run-pass/panic/div-by-zero-2.stderr b/tests/run-pass/panic/div-by-zero-2.stderr index 77dca2aac1e2..d255811be2a9 100644 --- a/tests/run-pass/panic/div-by-zero-2.stderr +++ b/tests/run-pass/panic/div-by-zero-2.stderr @@ -1 +1 @@ -thread 'main' panicked at 'attempt to divide by zero', $DIR/div-by-zero-2.rs:5:14 +thread 'main' panicked at 'attempt to divide by zero', $DIR/div-by-zero-2.rs:4:14 diff --git a/tests/run-pass/panic/overflowing-lsh-neg.rs b/tests/run-pass/panic/overflowing-lsh-neg.rs index ee15ca0284ef..bf5eed1c550f 100644 --- a/tests/run-pass/panic/overflowing-lsh-neg.rs +++ b/tests/run-pass/panic/overflowing-lsh-neg.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows #![allow(arithmetic_overflow)] fn main() { diff --git a/tests/run-pass/panic/overflowing-lsh-neg.stderr b/tests/run-pass/panic/overflowing-lsh-neg.stderr index e1e7daa119ab..04d98a0a2f15 100644 --- a/tests/run-pass/panic/overflowing-lsh-neg.stderr +++ b/tests/run-pass/panic/overflowing-lsh-neg.stderr @@ -1 +1 @@ -thread 'main' panicked at 'attempt to shift left with overflow', $DIR/overflowing-lsh-neg.rs:5:14 +thread 'main' panicked at 'attempt to shift left with overflow', $DIR/overflowing-lsh-neg.rs:4:14 diff --git a/tests/run-pass/panic/overflowing-rsh-1.rs b/tests/run-pass/panic/overflowing-rsh-1.rs index 36ab948a5efa..4c0106f0fb1f 100644 --- a/tests/run-pass/panic/overflowing-rsh-1.rs +++ b/tests/run-pass/panic/overflowing-rsh-1.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows #![allow(arithmetic_overflow)] fn main() { diff --git a/tests/run-pass/panic/overflowing-rsh-1.stderr b/tests/run-pass/panic/overflowing-rsh-1.stderr index 20a45739ae2e..a9a72f46222d 100644 --- a/tests/run-pass/panic/overflowing-rsh-1.stderr +++ b/tests/run-pass/panic/overflowing-rsh-1.stderr @@ -1 +1 @@ -thread 'main' panicked at 'attempt to shift right with overflow', $DIR/overflowing-rsh-1.rs:5:14 +thread 'main' panicked at 'attempt to shift right with overflow', $DIR/overflowing-rsh-1.rs:4:14 diff --git a/tests/run-pass/panic/overflowing-rsh-2.rs b/tests/run-pass/panic/overflowing-rsh-2.rs index 27cc65fa7685..19d16e7bc84a 100644 --- a/tests/run-pass/panic/overflowing-rsh-2.rs +++ b/tests/run-pass/panic/overflowing-rsh-2.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows #![allow(arithmetic_overflow)] fn main() { diff --git a/tests/run-pass/panic/overflowing-rsh-2.stderr b/tests/run-pass/panic/overflowing-rsh-2.stderr index 3381116ae6c8..24b61194565d 100644 --- a/tests/run-pass/panic/overflowing-rsh-2.stderr +++ b/tests/run-pass/panic/overflowing-rsh-2.stderr @@ -1 +1 @@ -thread 'main' panicked at 'attempt to shift right with overflow', $DIR/overflowing-rsh-2.rs:6:14 +thread 'main' panicked at 'attempt to shift right with overflow', $DIR/overflowing-rsh-2.rs:5:14 diff --git a/tests/run-pass/panic/panic1.rs b/tests/run-pass/panic/panic1.rs index 61321c658166..9d9ad28df5a7 100644 --- a/tests/run-pass/panic/panic1.rs +++ b/tests/run-pass/panic/panic1.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows fn main() { std::panic!("panicking from libstd"); } diff --git a/tests/run-pass/panic/panic1.stderr b/tests/run-pass/panic/panic1.stderr index 305fc1a1a6e6..954b8799a082 100644 --- a/tests/run-pass/panic/panic1.stderr +++ b/tests/run-pass/panic/panic1.stderr @@ -1 +1 @@ -thread 'main' panicked at 'panicking from libstd', $DIR/panic1.rs:3:5 +thread 'main' panicked at 'panicking from libstd', $DIR/panic1.rs:2:5 diff --git a/tests/run-pass/panic/panic2.rs b/tests/run-pass/panic/panic2.rs index d6ab864795ea..d90e3f2e0ac1 100644 --- a/tests/run-pass/panic/panic2.rs +++ b/tests/run-pass/panic/panic2.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows fn main() { std::panic!("{}-panicking from libstd", 42); } diff --git a/tests/run-pass/panic/panic2.stderr b/tests/run-pass/panic/panic2.stderr index cd40559c81ef..e90e3502cbfb 100644 --- a/tests/run-pass/panic/panic2.stderr +++ b/tests/run-pass/panic/panic2.stderr @@ -1 +1 @@ -thread 'main' panicked at '42-panicking from libstd', $DIR/panic2.rs:3:5 +thread 'main' panicked at '42-panicking from libstd', $DIR/panic2.rs:2:5 diff --git a/tests/run-pass/panic/panic3.rs b/tests/run-pass/panic/panic3.rs index 10a42c7e6c00..418ee4f8411e 100644 --- a/tests/run-pass/panic/panic3.rs +++ b/tests/run-pass/panic/panic3.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows fn main() { core::panic!("panicking from libcore"); } diff --git a/tests/run-pass/panic/panic3.stderr b/tests/run-pass/panic/panic3.stderr index e3aa902f0cbc..0a3c191b282e 100644 --- a/tests/run-pass/panic/panic3.stderr +++ b/tests/run-pass/panic/panic3.stderr @@ -1 +1 @@ -thread 'main' panicked at 'panicking from libcore', $DIR/panic3.rs:3:5 +thread 'main' panicked at 'panicking from libcore', $DIR/panic3.rs:2:5 diff --git a/tests/run-pass/panic/panic4.rs b/tests/run-pass/panic/panic4.rs index 06e2dd008fff..0fcc53813b5d 100644 --- a/tests/run-pass/panic/panic4.rs +++ b/tests/run-pass/panic/panic4.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows fn main() { core::panic!("{}-panicking from libcore", 42); } diff --git a/tests/run-pass/panic/panic4.stderr b/tests/run-pass/panic/panic4.stderr index 1a242a868cae..946059b1e49f 100644 --- a/tests/run-pass/panic/panic4.stderr +++ b/tests/run-pass/panic/panic4.stderr @@ -1 +1 @@ -thread 'main' panicked at '42-panicking from libcore', $DIR/panic4.rs:3:5 +thread 'main' panicked at '42-panicking from libcore', $DIR/panic4.rs:2:5 diff --git a/tests/run-pass/panic/std-panic-locations.rs b/tests/run-pass/panic/std-panic-locations.rs index d5f38fc2672e..ac2e8d5305df 100644 --- a/tests/run-pass/panic/std-panic-locations.rs +++ b/tests/run-pass/panic/std-panic-locations.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows #![feature(option_expect_none, option_unwrap_none)] //! Test that panic locations for `#[track_caller]` functions in std have the correct //! location reported. diff --git a/tests/run-pass/transmute_fat2.rs b/tests/run-pass/transmute_fat2.rs index c667aab6bb5f..3dff2cc1e0c9 100644 --- a/tests/run-pass/transmute_fat2.rs +++ b/tests/run-pass/transmute_fat2.rs @@ -1,4 +1,3 @@ -// ignore-windows: Unwind panicking does not currently work on Windows fn main() { #[cfg(target_pointer_width="64")] let bad = unsafe { diff --git a/tests/run-pass/transmute_fat2.stderr b/tests/run-pass/transmute_fat2.stderr index 2539e58814d6..08849a5b517a 100644 --- a/tests/run-pass/transmute_fat2.stderr +++ b/tests/run-pass/transmute_fat2.stderr @@ -1 +1 @@ -thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', $DIR/transmute_fat2.rs:12:5 +thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', $DIR/transmute_fat2.rs:11:5 From a9dc2796cac8d49f67f6055d3fe3561a13f604b7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 28 Jun 2020 09:23:01 +0200 Subject: [PATCH 4/5] Move get/set_at_offset helpers to global helpers file --- src/helpers.rs | 31 +++++++++++++++ src/shims/posix/sync.rs | 84 +++++++---------------------------------- 2 files changed, 45 insertions(+), 70 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 4e5e0dcfca25..c1eaf4eb4865 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -467,6 +467,37 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } } + + fn read_scalar_at_offset( + &self, + op: OpTy<'tcx, Tag>, + offset: u64, + layout: TyAndLayout<'tcx>, + ) -> InterpResult<'tcx, ScalarMaybeUninit> { + let this = self.eval_context_ref(); + let op_place = this.deref_operand(op)?; + let offset = Size::from_bytes(offset); + // Ensure that the following read at an offset is within bounds + assert!(op_place.layout.size >= offset + layout.size); + let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?; + this.read_scalar(value_place.into()) + } + + fn write_scalar_at_offset( + &mut self, + op: OpTy<'tcx, Tag>, + offset: u64, + value: impl Into>, + layout: TyAndLayout<'tcx>, + ) -> InterpResult<'tcx, ()> { + let this = self.eval_context_mut(); + let op_place = this.deref_operand(op)?; + let offset = Size::from_bytes(offset); + // Ensure that the following read at an offset is within bounds + assert!(op_place.layout.size >= offset + layout.size); + let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?; + this.write_scalar(value, value_place.into()) + } } /// Check that the number of args is what we expect. diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index a61c80d5118c..bc4be56557a4 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -2,57 +2,11 @@ use std::convert::TryInto; use std::time::{Duration, SystemTime}; use std::ops::Not; -use rustc_middle::ty::{layout::TyAndLayout, TyKind, TypeAndMut}; -use rustc_target::abi::{LayoutOf, Size}; - use crate::*; use stacked_borrows::Tag; use thread::Time; -fn assert_ptr_target_min_size<'mir, 'tcx: 'mir>( - ecx: &MiriEvalContext<'mir, 'tcx>, - operand: OpTy<'tcx, Tag>, - min_size: u64, -) -> InterpResult<'tcx, ()> { - let target_ty = match operand.layout.ty.kind { - TyKind::RawPtr(TypeAndMut { ty, mutbl: _ }) => ty, - _ => panic!("Argument to pthread function was not a raw pointer"), - }; - let target_layout = ecx.layout_of(target_ty)?; - assert!(target_layout.size.bytes() >= min_size); - Ok(()) -} - -fn get_at_offset<'mir, 'tcx: 'mir>( - ecx: &MiriEvalContext<'mir, 'tcx>, - op: OpTy<'tcx, Tag>, - offset: u64, - layout: TyAndLayout<'tcx>, - min_size: u64, -) -> InterpResult<'tcx, ScalarMaybeUninit> { - // Ensure that the following read at an offset to the attr pointer is within bounds - assert_ptr_target_min_size(ecx, op, min_size)?; - let op_place = ecx.deref_operand(op)?; - let value_place = op_place.offset(Size::from_bytes(offset), MemPlaceMeta::None, layout, ecx)?; - ecx.read_scalar(value_place.into()) -} - -fn set_at_offset<'mir, 'tcx: 'mir>( - ecx: &mut MiriEvalContext<'mir, 'tcx>, - op: OpTy<'tcx, Tag>, - offset: u64, - value: impl Into>, - layout: TyAndLayout<'tcx>, - min_size: u64, -) -> InterpResult<'tcx, ()> { - // Ensure that the following write at an offset to the attr pointer is within bounds - assert_ptr_target_min_size(ecx, op, min_size)?; - let op_place = ecx.deref_operand(op)?; - let value_place = op_place.offset(Size::from_bytes(offset), MemPlaceMeta::None, layout, ecx)?; - ecx.write_scalar(value.into(), value_place.into()) -} - // pthread_mutexattr_t is either 4 or 8 bytes, depending on the platform. // Our chosen memory layout for emulation (does not have to match the platform layout!): @@ -66,8 +20,6 @@ fn set_at_offset<'mir, 'tcx: 'mir>( /// in `pthread_mutexattr_settype` function. const PTHREAD_MUTEX_NORMAL_FLAG: i32 = 0x8000000; -const PTHREAD_MUTEXATTR_T_MIN_SIZE: u64 = 4; - fn is_mutex_kind_default<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, kind: Scalar, @@ -88,7 +40,7 @@ fn mutexattr_get_kind<'mir, 'tcx: 'mir>( ecx: &MiriEvalContext<'mir, 'tcx>, attr_op: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, ScalarMaybeUninit> { - get_at_offset(ecx, attr_op, 0, ecx.machine.layouts.i32, PTHREAD_MUTEXATTR_T_MIN_SIZE) + ecx.read_scalar_at_offset(attr_op, 0, ecx.machine.layouts.i32) } fn mutexattr_set_kind<'mir, 'tcx: 'mir>( @@ -96,7 +48,7 @@ fn mutexattr_set_kind<'mir, 'tcx: 'mir>( attr_op: OpTy<'tcx, Tag>, kind: impl Into>, ) -> InterpResult<'tcx, ()> { - set_at_offset(ecx, attr_op, 0, kind, ecx.machine.layouts.i32, PTHREAD_MUTEXATTR_T_MIN_SIZE) + ecx.write_scalar_at_offset(attr_op, 0, kind, ecx.machine.layouts.i32) } // pthread_mutex_t is between 24 and 48 bytes, depending on the platform. @@ -108,14 +60,12 @@ fn mutexattr_set_kind<'mir, 'tcx: 'mir>( // bytes 12-15 or 16-19 (depending on platform): mutex kind, as an i32 // (the kind has to be at its offset for compatibility with static initializer macros) -const PTHREAD_MUTEX_T_MIN_SIZE: u64 = 24; - fn mutex_get_kind<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, mutex_op: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, ScalarMaybeUninit> { let offset = if ecx.pointer_size().bytes() == 8 { 16 } else { 12 }; - get_at_offset(ecx, mutex_op, offset, ecx.machine.layouts.i32, PTHREAD_MUTEX_T_MIN_SIZE) + ecx.read_scalar_at_offset(mutex_op, offset, ecx.machine.layouts.i32) } fn mutex_set_kind<'mir, 'tcx: 'mir>( @@ -124,14 +74,14 @@ fn mutex_set_kind<'mir, 'tcx: 'mir>( kind: impl Into>, ) -> InterpResult<'tcx, ()> { let offset = if ecx.pointer_size().bytes() == 8 { 16 } else { 12 }; - set_at_offset(ecx, mutex_op, offset, kind, ecx.machine.layouts.i32, PTHREAD_MUTEX_T_MIN_SIZE) + ecx.write_scalar_at_offset(mutex_op, offset, kind, ecx.machine.layouts.i32) } fn mutex_get_id<'mir, 'tcx: 'mir>( ecx: &MiriEvalContext<'mir, 'tcx>, mutex_op: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, ScalarMaybeUninit> { - get_at_offset(ecx, mutex_op, 4, ecx.machine.layouts.u32, PTHREAD_MUTEX_T_MIN_SIZE) + ecx.read_scalar_at_offset(mutex_op, 4, ecx.machine.layouts.u32) } fn mutex_set_id<'mir, 'tcx: 'mir>( @@ -139,7 +89,7 @@ fn mutex_set_id<'mir, 'tcx: 'mir>( mutex_op: OpTy<'tcx, Tag>, id: impl Into>, ) -> InterpResult<'tcx, ()> { - set_at_offset(ecx, mutex_op, 4, id, ecx.machine.layouts.u32, PTHREAD_MUTEX_T_MIN_SIZE) + ecx.write_scalar_at_offset(mutex_op, 4, id, ecx.machine.layouts.u32) } fn mutex_get_or_create_id<'mir, 'tcx: 'mir>( @@ -165,13 +115,11 @@ fn mutex_get_or_create_id<'mir, 'tcx: 'mir>( // (need to avoid this because it is set by static initializer macros) // bytes 4-7: rwlock id as u32 or 0 if id is not assigned yet. -const PTHREAD_RWLOCK_T_MIN_SIZE: u64 = 32; - fn rwlock_get_id<'mir, 'tcx: 'mir>( ecx: &MiriEvalContext<'mir, 'tcx>, rwlock_op: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, ScalarMaybeUninit> { - get_at_offset(ecx, rwlock_op, 4, ecx.machine.layouts.u32, PTHREAD_RWLOCK_T_MIN_SIZE) + ecx.read_scalar_at_offset(rwlock_op, 4, ecx.machine.layouts.u32) } fn rwlock_set_id<'mir, 'tcx: 'mir>( @@ -179,7 +127,7 @@ fn rwlock_set_id<'mir, 'tcx: 'mir>( rwlock_op: OpTy<'tcx, Tag>, id: impl Into>, ) -> InterpResult<'tcx, ()> { - set_at_offset(ecx, rwlock_op, 4, id, ecx.machine.layouts.u32, PTHREAD_RWLOCK_T_MIN_SIZE) + ecx.write_scalar_at_offset(rwlock_op, 4, id, ecx.machine.layouts.u32) } fn rwlock_get_or_create_id<'mir, 'tcx: 'mir>( @@ -204,13 +152,11 @@ fn rwlock_get_or_create_id<'mir, 'tcx: 'mir>( // store an i32 in the first four bytes equal to the corresponding libc clock id constant // (e.g. CLOCK_REALTIME). -const PTHREAD_CONDATTR_T_MIN_SIZE: u64 = 4; - fn condattr_get_clock_id<'mir, 'tcx: 'mir>( ecx: &MiriEvalContext<'mir, 'tcx>, attr_op: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, ScalarMaybeUninit> { - get_at_offset(ecx, attr_op, 0, ecx.machine.layouts.i32, PTHREAD_CONDATTR_T_MIN_SIZE) + ecx.read_scalar_at_offset(attr_op, 0, ecx.machine.layouts.i32) } fn condattr_set_clock_id<'mir, 'tcx: 'mir>( @@ -218,7 +164,7 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>( attr_op: OpTy<'tcx, Tag>, clock_id: impl Into>, ) -> InterpResult<'tcx, ()> { - set_at_offset(ecx, attr_op, 0, clock_id, ecx.machine.layouts.i32, PTHREAD_CONDATTR_T_MIN_SIZE) + ecx.write_scalar_at_offset(attr_op, 0, clock_id, ecx.machine.layouts.i32) } // pthread_cond_t @@ -230,13 +176,11 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>( // bytes 4-7: the conditional variable id as u32 or 0 if id is not assigned yet. // bytes 8-11: the clock id constant as i32 -const PTHREAD_COND_T_MIN_SIZE: u64 = 12; - fn cond_get_id<'mir, 'tcx: 'mir>( ecx: &MiriEvalContext<'mir, 'tcx>, cond_op: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, ScalarMaybeUninit> { - get_at_offset(ecx, cond_op, 4, ecx.machine.layouts.u32, PTHREAD_COND_T_MIN_SIZE) + ecx.read_scalar_at_offset(cond_op, 4, ecx.machine.layouts.u32) } fn cond_set_id<'mir, 'tcx: 'mir>( @@ -244,7 +188,7 @@ fn cond_set_id<'mir, 'tcx: 'mir>( cond_op: OpTy<'tcx, Tag>, id: impl Into>, ) -> InterpResult<'tcx, ()> { - set_at_offset(ecx, cond_op, 4, id, ecx.machine.layouts.u32, PTHREAD_COND_T_MIN_SIZE) + ecx.write_scalar_at_offset(cond_op, 4, id, ecx.machine.layouts.u32) } fn cond_get_or_create_id<'mir, 'tcx: 'mir>( @@ -267,7 +211,7 @@ fn cond_get_clock_id<'mir, 'tcx: 'mir>( ecx: &MiriEvalContext<'mir, 'tcx>, cond_op: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, ScalarMaybeUninit> { - get_at_offset(ecx, cond_op, 8, ecx.machine.layouts.i32, PTHREAD_COND_T_MIN_SIZE) + ecx.read_scalar_at_offset(cond_op, 8, ecx.machine.layouts.i32) } fn cond_set_clock_id<'mir, 'tcx: 'mir>( @@ -275,7 +219,7 @@ fn cond_set_clock_id<'mir, 'tcx: 'mir>( cond_op: OpTy<'tcx, Tag>, clock_id: impl Into>, ) -> InterpResult<'tcx, ()> { - set_at_offset(ecx, cond_op, 8, clock_id, ecx.machine.layouts.i32, PTHREAD_COND_T_MIN_SIZE) + ecx.write_scalar_at_offset(cond_op, 8, clock_id, ecx.machine.layouts.i32) } /// Try to reacquire the mutex associated with the condition variable after we From 3a5bcb97edd5bab769341aacde18a05c015aa396 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 28 Jun 2020 09:47:20 +0200 Subject: [PATCH 5/5] move rwlock dequeuing to shared code, and use that code for Windows rwlocks --- src/shims/posix/sync.rs | 22 +-- src/shims/windows/foreign_items.rs | 6 +- src/shims/windows/sync.rs | 126 ++++++------- src/sync.rs | 165 +++++++++++------- .../compile-fail/concurrency/thread-spawn.rs | 2 +- 5 files changed, 160 insertions(+), 161 deletions(-) diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index bc4be56557a4..cce0ddc930df 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -1,6 +1,5 @@ use std::convert::TryInto; use std::time::{Duration, SystemTime}; -use std::ops::Not; use crate::*; use stacked_borrows::Tag; @@ -548,27 +547,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let active_thread = this.get_active_thread(); if this.rwlock_reader_unlock(id, active_thread) { - // The thread was a reader. - if this.rwlock_is_locked(id).not() { - // No more readers owning the lock. Give it to a writer if there - // is any. - this.rwlock_dequeue_and_lock_writer(id); - } Ok(0) - } else if Some(active_thread) == this.rwlock_writer_unlock(id) { - // The thread was a writer. - // - // We are prioritizing writers here against the readers. As a - // result, not only readers can starve writers, but also writers can - // starve readers. - if this.rwlock_dequeue_and_lock_writer(id) { - // Someone got the write lock, nice. - } else { - // Give the lock to all readers. - while this.rwlock_dequeue_and_lock_reader(id) { - // Rinse and repeat. - } - } + } else if this.rwlock_writer_unlock(id, active_thread) { Ok(0) } else { throw_ub_format!("unlocked an rwlock that was not locked by the active thread"); diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index ddb70b752e79..e8937bbb3085 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -257,7 +257,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Better error for attempts to create a thread "CreateThread" => { - throw_unsup_format!("Miri does not support threading"); + throw_unsup_format!("Miri does not support concurrency on Windows"); } // Incomplete shims that we "stub out" just to get pre-main initialization code to work. @@ -292,7 +292,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if this.frame().instance.to_string().starts_with("std::sys::windows::") => { #[allow(non_snake_case)] let &[_lpCriticalSection] = check_arg_count(args)?; - 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"); // Nothing to do, not even a return value. // (Windows locks are reentrant, and we have only 1 thread, // so not doing any futher checks here is at least not incorrect.) @@ -301,7 +301,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if this.frame().instance.to_string().starts_with("std::sys::windows::") => { #[allow(non_snake_case)] let &[_lpCriticalSection] = check_arg_count(args)?; - 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"); // There is only one thread, so this always succeeds and returns TRUE. this.write_scalar(Scalar::from_i32(1), dest)?; } diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index ef40eb089110..7bad3c08a598 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -1,19 +1,22 @@ -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. +// We use the first 4 bytes to store the RwLockId. -fn deref_lock<'mir, 'tcx: 'mir>( +fn srwlock_get_or_create_id<'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)?) +) -> InterpResult<'tcx, RwLockId> { + let id = ecx.read_scalar_at_offset(lock_op, 0, ecx.machine.layouts.u32)?.to_u32()?; + if id == 0 { + // 0 is a default value and also not a valid rwlock id. Need to allocate + // a new rwlock. + let id = ecx.rwlock_create(); + ecx.write_scalar_at_offset(lock_op, 0, id.to_u32_scalar(), ecx.machine.layouts.u32)?; + Ok(id) + } else { + Ok(RwLockId::from_u32(id)) + } } impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} @@ -24,17 +27,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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 id = srwlock_get_or_create_id(this, lock_op)?; + let active_thread = this.get_active_thread(); - 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())?; + if this.rwlock_is_locked(id) { + // Note: this will deadlock if the lock is already locked by this + // thread in any way. + // + // FIXME: Detect and report the deadlock proactively. (We currently + // report the deadlock only when no thread can continue execution, + // but we could detect that this lock is already locked and report + // an error.) + this.rwlock_enqueue_and_block_writer(id, active_thread); } else { - // Lock is already held. This is a deadlock. - throw_machine_stop!(TerminationInfo::Deadlock); + this.rwlock_writer_lock(id, active_thread); } Ok(()) @@ -46,18 +52,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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 id = srwlock_get_or_create_id(this, lock_op)?; + let active_thread = this.get_active_thread(); - 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 { + if this.rwlock_is_locked(id) { // Lock is already held. Ok(0) + } else { + this.rwlock_writer_lock(id, active_thread); + Ok(1) } } @@ -67,17 +70,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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 id = srwlock_get_or_create_id(this, lock_op)?; + let active_thread = this.get_active_thread(); - 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"); + if !this.rwlock_writer_unlock(id, active_thread) { + // The docs do not say anything about this case, but it seems better to not allow it. + throw_ub_format!("calling ReleaseSRWLockExclusive on an SRWLock that is not exclusively locked by the current thread"); } Ok(()) @@ -89,21 +87,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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 id = srwlock_get_or_create_id(this, lock_op)?; + let active_thread = this.get_active_thread(); - 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); + if this.rwlock_is_write_locked(id) { + this.rwlock_enqueue_and_block_reader(id, active_thread); } 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())?; + this.rwlock_reader_lock(id, active_thread); } Ok(()) @@ -115,21 +105,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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 id = srwlock_get_or_create_id(this, lock_op)?; + let active_thread = this.get_active_thread(); - 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. + if this.rwlock_is_write_locked(id) { 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())?; + this.rwlock_reader_lock(id, active_thread); Ok(1) } } @@ -140,20 +122,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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 id = srwlock_get_or_create_id(this, lock_op)?; + let active_thread = this.get_active_thread(); - 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())?; + if !this.rwlock_reader_unlock(id, active_thread) { + // The docs do not say anything about this case, but it seems better to not allow it. + throw_ub_format!("calling ReleaseSRWLockShared on an SRWLock that is not locked by the current thread"); } Ok(()) diff --git a/src/sync.rs b/src/sync.rs index 0d4b4d6b7c1c..7e3c27b386df 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -3,6 +3,8 @@ use std::convert::TryFrom; use std::num::NonZeroU32; use std::ops::Not; +use log::trace; + use rustc_index::vec::{Idx, IndexVec}; use crate::*; @@ -102,6 +104,52 @@ pub(super) struct SynchronizationState { condvars: IndexVec, } +// Private extension trait for local helper methods +impl<'mir, 'tcx: 'mir> EvalContextExtPriv<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} +trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + /// Take a reader out of the queue waiting for the lock. + /// Returns `true` if some thread got the rwlock. + #[inline] + fn rwlock_dequeue_and_lock_reader(&mut self, id: RwLockId) -> bool { + let this = self.eval_context_mut(); + if let Some(reader) = this.machine.threads.sync.rwlocks[id].reader_queue.pop_front() { + this.unblock_thread(reader); + this.rwlock_reader_lock(id, reader); + true + } else { + false + } + } + + /// Take the writer out of the queue waiting for the lock. + /// Returns `true` if some thread got the rwlock. + #[inline] + fn rwlock_dequeue_and_lock_writer(&mut self, id: RwLockId) -> bool { + let this = self.eval_context_mut(); + if let Some(writer) = this.machine.threads.sync.rwlocks[id].writer_queue.pop_front() { + this.unblock_thread(writer); + this.rwlock_writer_lock(id, writer); + true + } else { + false + } + } + + /// Take a thread out of the queue waiting for the mutex, and lock + /// the mutex for it. Returns `true` if some thread has the mutex now. + #[inline] + fn mutex_dequeue_and_lock(&mut self, id: MutexId) -> bool { + let this = self.eval_context_mut(); + if let Some(thread) = this.machine.threads.sync.mutexes[id].queue.pop_front() { + this.unblock_thread(thread); + this.mutex_lock(id, thread); + true + } else { + false + } + } +} + // Public interface to synchronization primitives. Please note that in most // cases, the function calls are infallible and it is the client's (shim // implementation's) responsibility to detect and deal with erroneous @@ -124,8 +172,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx #[inline] /// Check if locked. - fn mutex_is_locked(&mut self, id: MutexId) -> bool { - let this = self.eval_context_mut(); + fn mutex_is_locked(&self, id: MutexId) -> bool { + let this = self.eval_context_ref(); this.machine.threads.sync.mutexes[id].owner.is_some() } @@ -174,7 +222,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } Some(old_lock_count) } else { - // Mutex is unlocked. + // Mutex is not locked. None } } @@ -188,20 +236,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.block_thread(thread); } - #[inline] - /// Take a thread out of the queue waiting for the mutex, and lock - /// the mutex for it. Returns `true` if some thread has the mutex now. - fn mutex_dequeue_and_lock(&mut self, id: MutexId) -> bool { - let this = self.eval_context_mut(); - if let Some(thread) = this.machine.threads.sync.mutexes[id].queue.pop_front() { - this.unblock_thread(thread); - this.mutex_lock(id, thread); - true - } else { - false - } - } - #[inline] /// Create state for a new read write lock. fn rwlock_create(&mut self) -> RwLockId { @@ -211,17 +245,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx #[inline] /// Check if locked. - fn rwlock_is_locked(&mut self, id: RwLockId) -> bool { - let this = self.eval_context_mut(); - this.machine.threads.sync.rwlocks[id].writer.is_some() - || this.machine.threads.sync.rwlocks[id].readers.is_empty().not() + fn rwlock_is_locked(&self, id: RwLockId) -> bool { + let this = self.eval_context_ref(); + let rwlock = &this.machine.threads.sync.rwlocks[id]; + trace!( + "rwlock_is_locked: {:?} writer is {:?} and there are {} reader threads (some of which could hold multiple read locks)", + id, rwlock.writer, rwlock.readers.len(), + ); + rwlock.writer.is_some()|| rwlock.readers.is_empty().not() } #[inline] /// Check if write locked. - fn rwlock_is_write_locked(&mut self, id: RwLockId) -> bool { - let this = self.eval_context_mut(); - this.machine.threads.sync.rwlocks[id].writer.is_some() + fn rwlock_is_write_locked(&self, id: RwLockId) -> bool { + let this = self.eval_context_ref(); + let rwlock = &this.machine.threads.sync.rwlocks[id]; + trace!("rwlock_is_write_locked: {:?} writer is {:?}", id, rwlock.writer); + rwlock.writer.is_some() } /// Read-lock the lock by adding the `reader` the list of threads that own @@ -229,12 +269,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn rwlock_reader_lock(&mut self, id: RwLockId, reader: ThreadId) { let this = self.eval_context_mut(); assert!(!this.rwlock_is_write_locked(id), "the lock is write locked"); + trace!("rwlock_reader_lock: {:?} now also held (one more time) by {:?}", id, reader); let count = this.machine.threads.sync.rwlocks[id].readers.entry(reader).or_insert(0); *count = count.checked_add(1).expect("the reader counter overflowed"); } - /// Try read-unlock the lock for `reader`. Returns `true` if succeeded, - /// `false` if this `reader` did not hold the lock. + /// Try read-unlock the lock for `reader` and potentially give the lock to a new owner. + /// Returns `true` if succeeded, `false` if this `reader` did not hold the lock. fn rwlock_reader_unlock(&mut self, id: RwLockId, reader: ThreadId) -> bool { let this = self.eval_context_mut(); match this.machine.threads.sync.rwlocks[id].readers.entry(reader) { @@ -243,12 +284,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx assert!(*count > 0, "rwlock locked with count == 0"); *count -= 1; if *count == 0 { + trace!("rwlock_reader_unlock: {:?} no longer held by {:?}", id, reader); entry.remove(); + } else { + trace!("rwlock_reader_unlock: {:?} held one less time by {:?}", id, reader); } - true } - Entry::Vacant(_) => false, + Entry::Vacant(_) => return false, // we did not even own this lock } + // The thread was a reader. If the lock is not held any more, give it to a writer. + if this.rwlock_is_locked(id).not() { + this.rwlock_dequeue_and_lock_writer(id); + } + true } #[inline] @@ -259,38 +307,49 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx reader: ThreadId, ) { let this = self.eval_context_mut(); - assert!(this.rwlock_is_write_locked(id), "queueing on not write locked lock"); + assert!(this.rwlock_is_write_locked(id), "read-queueing on not write locked rwlock"); this.machine.threads.sync.rwlocks[id].reader_queue.push_back(reader); this.block_thread(reader); } - #[inline] - /// Take a reader out the queue waiting for the lock. - /// Returns `true` if some thread got the rwlock. - fn rwlock_dequeue_and_lock_reader(&mut self, id: RwLockId) -> bool { - let this = self.eval_context_mut(); - if let Some(reader) = this.machine.threads.sync.rwlocks[id].reader_queue.pop_front() { - this.unblock_thread(reader); - this.rwlock_reader_lock(id, reader); - true - } else { - false - } - } - #[inline] /// Lock by setting the writer that owns the lock. fn rwlock_writer_lock(&mut self, id: RwLockId, writer: ThreadId) { let this = self.eval_context_mut(); assert!(!this.rwlock_is_locked(id), "the rwlock is already locked"); + trace!("rwlock_writer_lock: {:?} now held by {:?}", id, writer); this.machine.threads.sync.rwlocks[id].writer = Some(writer); } #[inline] /// Try to unlock by removing the writer. - fn rwlock_writer_unlock(&mut self, id: RwLockId) -> Option { + fn rwlock_writer_unlock(&mut self, id: RwLockId, expected_writer: ThreadId) -> bool { let this = self.eval_context_mut(); - this.machine.threads.sync.rwlocks[id].writer.take() + let rwlock = &mut this.machine.threads.sync.rwlocks[id]; + if let Some(current_writer) = rwlock.writer { + if current_writer != expected_writer { + // Only the owner can unlock the rwlock. + return false; + } + rwlock.writer = None; + trace!("rwlock_writer_unlock: {:?} unlocked by {:?}", id, expected_writer); + // The thread was a writer. + // + // We are prioritizing writers here against the readers. As a + // result, not only readers can starve writers, but also writers can + // starve readers. + if this.rwlock_dequeue_and_lock_writer(id) { + // Someone got the write lock, nice. + } else { + // Give the lock to all readers. + while this.rwlock_dequeue_and_lock_reader(id) { + // Rinse and repeat. + } + } + true + } else { + false + } } #[inline] @@ -301,25 +360,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx writer: ThreadId, ) { let this = self.eval_context_mut(); - assert!(this.rwlock_is_locked(id), "queueing on unlocked lock"); + assert!(this.rwlock_is_locked(id), "write-queueing on unlocked rwlock"); this.machine.threads.sync.rwlocks[id].writer_queue.push_back(writer); this.block_thread(writer); } - #[inline] - /// Take the writer out the queue waiting for the lock. - /// Returns `true` if some thread got the rwlock. - fn rwlock_dequeue_and_lock_writer(&mut self, id: RwLockId) -> bool { - let this = self.eval_context_mut(); - if let Some(writer) = this.machine.threads.sync.rwlocks[id].writer_queue.pop_front() { - this.unblock_thread(writer); - this.rwlock_writer_lock(id, writer); - true - } else { - false - } - } - #[inline] /// Create state for a new conditional variable. fn condvar_create(&mut self) -> CondvarId { diff --git a/tests/compile-fail/concurrency/thread-spawn.rs b/tests/compile-fail/concurrency/thread-spawn.rs index f0e4ab3817d3..27760eed8dba 100644 --- a/tests/compile-fail/concurrency/thread-spawn.rs +++ b/tests/compile-fail/concurrency/thread-spawn.rs @@ -3,7 +3,7 @@ use std::thread; -// error-pattern: Miri does not support threading +// error-pattern: Miri does not support concurrency on Windows fn main() { thread::spawn(|| {});