From 325c31e578210d0c72d8d5f1612074ddcbe514bb Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Wed, 15 Apr 2020 21:25:12 -0700 Subject: [PATCH] Address some of the reviewers comments. --- src/eval.rs | 2 +- src/machine.rs | 24 ++++---- src/shims/foreign_items/posix.rs | 57 ++++++++----------- src/shims/threads.rs | 14 +++-- src/threads.rs | 43 ++++++++++++-- tests/compile-fail/thread-spawn.rs | 9 +++ tests/run-pass/concurrency/locks.rs | 2 + tests/run-pass/concurrency/locks.stderr | 2 + tests/run-pass/concurrency/locks.stdout | 3 - tests/run-pass/concurrency/simple.rs | 2 + tests/run-pass/concurrency/simple.stderr | 2 + tests/run-pass/concurrency/simple.stdout | 10 ---- tests/run-pass/concurrency/thread_locals.rs | 2 + .../run-pass/concurrency/thread_locals.stderr | 2 + .../run-pass/concurrency/thread_locals.stdout | 1 - 15 files changed, 101 insertions(+), 74 deletions(-) create mode 100644 tests/compile-fail/thread-spawn.rs create mode 100644 tests/run-pass/concurrency/locks.stderr delete mode 100644 tests/run-pass/concurrency/locks.stdout create mode 100644 tests/run-pass/concurrency/simple.stderr delete mode 100644 tests/run-pass/concurrency/simple.stdout create mode 100644 tests/run-pass/concurrency/thread_locals.stderr delete mode 100644 tests/run-pass/concurrency/thread_locals.stdout diff --git a/src/eval.rs b/src/eval.rs index b0a59c64d1e..d83039e4756 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -206,7 +206,7 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> let res: InterpResult<'_, i64> = (|| { // Main loop. while ecx.schedule()? { - assert!(ecx.step()?); + assert!(ecx.step()?, "Bug: a terminated thread was scheduled for execution."); ecx.process_diagnostics(); } // Read the return code pointer *before* we run TLS destructors, to assert diff --git a/src/machine.rs b/src/machine.rs index a5183d3e816..0920364a44a 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -428,41 +428,37 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { mir::interpret::ConstValue::Scalar(Scalar::Ptr(ptr)) => { let alloc_id = ptr.alloc_id; let alloc = ecx.tcx.alloc_map.lock().get(alloc_id); + let tcx = ecx.tcx; + let is_thread_local = |def_id| { + tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::THREAD_LOCAL) + }; match alloc { - Some(GlobalAlloc::Static(def_id)) - if ecx - .tcx - .codegen_fn_attrs(def_id) - .flags - .contains(CodegenFnAttrFlags::THREAD_LOCAL) => - { - // We have a thread-local static. + Some(GlobalAlloc::Static(def_id)) if is_thread_local(def_id) => { let new_alloc_id = if let Some(new_alloc_id) = ecx.get_thread_local_alloc_id(alloc_id) { new_alloc_id } else { - if ecx.tcx.is_foreign_item(def_id) { + if tcx.is_foreign_item(def_id) { throw_unsup_format!( "Foreign thread-local statics are not supported." ) } - let instance = Instance::mono(ecx.tcx.tcx, def_id); + let instance = Instance::mono(tcx.tcx, def_id); let gid = GlobalId { instance, promoted: None }; - let raw_const = ecx - .tcx + let raw_const = tcx .const_eval_raw(ty::ParamEnv::reveal_all().and(gid)) .map_err(|err| { // no need to report anything, the const_eval call takes care of that // for statics - assert!(ecx.tcx.is_static(def_id)); + assert!(tcx.is_static(def_id)); match err { ErrorHandled::Reported => err_inval!(ReferencedConstant), ErrorHandled::TooGeneric => err_inval!(TooGeneric), } })?; let id = raw_const.alloc_id; - let mut alloc_map = ecx.tcx.alloc_map.lock(); + let mut alloc_map = tcx.alloc_map.lock(); let allocation = alloc_map.unwrap_memory(id); let new_alloc_id = alloc_map.create_memory_alloc(allocation); ecx.set_thread_local_alloc_id(alloc_id, new_alloc_id); diff --git a/src/shims/foreign_items/posix.rs b/src/shims/foreign_items/posix.rs index 4cd3b849911..5bb556aaa50 100644 --- a/src/shims/foreign_items/posix.rs +++ b/src/shims/foreign_items/posix.rs @@ -293,26 +293,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.write_scalar(Scalar::from_i32(result), dest)?; } - // Miscellaneous - "isatty" => { - let _fd = this.read_scalar(args[0])?.to_i32()?; - // "returns 1 if fd is an open file descriptor referring to a terminal; otherwise 0 is returned, and errno is set to indicate the error" - // FIXME: we just say nothing is a terminal. - let enotty = this.eval_libc("ENOTTY")?; - this.set_last_error(enotty)?; - this.write_null(dest)?; - } - "pthread_atfork" => { - let _prepare = this.read_scalar(args[0])?.not_undef()?; - let _parent = this.read_scalar(args[1])?.not_undef()?; - let _child = this.read_scalar(args[1])?.not_undef()?; - // We do not support forking, so there is nothing to do here. - this.write_null(dest)?; - } - "sched_yield" => { - this.write_null(dest)?; - } - // Threading "pthread_create" => { assert_eq!(args.len(), 4); @@ -339,20 +319,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.write_scalar(Scalar::from_i32(result), dest)?; } - "pthread_attr_getguardsize" => { - assert_eq!(args.len(), 2); - - let guard_size = this.deref_operand(args[1])?; - let guard_size_type = args[1].layout.ty - .builtin_deref(true) - .ok_or_else(|| err_ub_format!( - "wrong signature used for `pthread_attr_getguardsize`: first argument must be a raw pointer." - ))? - .ty; - let guard_size_layout = this.layout_of(guard_size_type)?; - this.write_scalar(Scalar::from_uint(crate::PAGE_SIZE, guard_size_layout.size), guard_size.into())?; - - // Return success (`0`). + // Miscellaneous + "isatty" => { + let _fd = this.read_scalar(args[0])?.to_i32()?; + // "returns 1 if fd is an open file descriptor referring to a terminal; otherwise 0 is returned, and errno is set to indicate the error" + // FIXME: we just say nothing is a terminal. + let enotty = this.eval_libc("ENOTTY")?; + this.set_last_error(enotty)?; + this.write_null(dest)?; + } + "pthread_atfork" => { + let _prepare = this.read_scalar(args[0])?.not_undef()?; + let _parent = this.read_scalar(args[1])?.not_undef()?; + let _child = this.read_scalar(args[1])?.not_undef()?; + // We do not support forking, so there is nothing to do here. this.write_null(dest)?; } @@ -369,6 +349,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx => { this.write_null(dest)?; } + "pthread_attr_getguardsize" if this.frame().instance.to_string().starts_with("std::sys::unix::") + => { + let guard_size = this.deref_operand(args[1])?; + let guard_size_layout = this.libc_ty_layout("size_t")?; + this.write_scalar(Scalar::from_uint(crate::PAGE_SIZE, guard_size_layout.size), guard_size.into())?; + + // Return success (`0`). + this.write_null(dest)?; + } | "signal" | "sigaction" diff --git a/src/shims/threads.rs b/src/shims/threads.rs index fc733d7f5c9..d8ba11d267f 100644 --- a/src/shims/threads.rs +++ b/src/shims/threads.rs @@ -11,13 +11,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx start_routine: OpTy<'tcx, Tag>, arg: OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, i32> { - println!( - "WARNING: The thread support is experimental. \ - For example, Miri does not detect data races yet." - ); - let this = self.eval_context_mut(); + this.tcx.sess.warn( + "The thread support is experimental. \ + For example, Miri does not detect data races yet.", + ); + let new_thread_id = this.create_thread()?; let old_thread_id = this.set_active_thread(new_thread_id)?; @@ -57,6 +57,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(0) } + fn pthread_join( &mut self, thread: OpTy<'tcx, Tag>, @@ -73,6 +74,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(0) } + fn pthread_detach(&mut self, thread: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); @@ -81,12 +83,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(0) } + fn pthread_self(&mut self, dest: PlaceTy<'tcx, Tag>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let thread_id = this.get_active_thread()?; this.write_scalar(Scalar::from_uint(thread_id.index() as u128, dest.layout.size), dest) } + fn prctl( &mut self, option: OpTy<'tcx, Tag>, diff --git a/src/threads.rs b/src/threads.rs index 170fb0c4767..c623fcae817 100644 --- a/src/threads.rs +++ b/src/threads.rs @@ -2,6 +2,7 @@ use std::cell::RefCell; use std::convert::TryFrom; +use std::num::NonZeroU32; use log::trace; @@ -42,21 +43,18 @@ impl ThreadId { } /// An identifier of a set of blocked threads. -/// -/// Note: 0 is not a valid identifier. #[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] -pub struct BlockSetId(u32); +pub struct BlockSetId(NonZeroU32); impl From for BlockSetId { fn from(id: u32) -> Self { - assert_ne!(id, 0, "0 is not a valid blockset id"); - Self(id) + Self(NonZeroU32::new(id).expect("0 is not a valid blockset id")) } } impl BlockSetId { pub fn to_u32_scalar<'tcx>(&self) -> Scalar { - Scalar::from_u32(self.0) + Scalar::from_u32(self.0.get()) } } @@ -150,6 +148,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { pub fn get_thread_local_alloc_id(&self, static_alloc_id: AllocId) -> Option { self.thread_local_alloc_ids.borrow().get(&(static_alloc_id, self.active_thread)).cloned() } + /// Set the allocation id as the allocation id of the given thread local /// static for the active thread. pub fn set_thread_local_alloc_id(&self, static_alloc_id: AllocId, new_alloc_id: AllocId) { @@ -161,20 +160,24 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { "Bug: a thread local initialized twice for the same thread." ); } + /// Borrow the stack of the active thread. fn active_thread_stack(&self) -> &[Frame<'mir, 'tcx, Tag, FrameData<'tcx>>] { &self.threads[self.active_thread].stack } + /// Mutably borrow the stack of the active thread. fn active_thread_stack_mut(&mut self) -> &mut Vec>> { &mut self.threads[self.active_thread].stack } + /// Create a new thread and returns its id. fn create_thread(&mut self) -> ThreadId { let new_thread_id = ThreadId::new(self.threads.len()); self.threads.push(Default::default()); new_thread_id } + /// Set an active thread and return the id of the thread that was active before. fn set_active_thread_id(&mut self, id: ThreadId) -> ThreadId { let active_thread_id = self.active_thread; @@ -182,19 +185,23 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { assert!(self.active_thread.index() < self.threads.len()); active_thread_id } + /// Get the id of the currently active thread. fn get_active_thread_id(&self) -> ThreadId { self.active_thread } + /// Get the borrow of the currently active thread. fn active_thread_mut(&mut self) -> &mut Thread<'mir, 'tcx> { &mut self.threads[self.active_thread] } + /// Mark the thread as detached, which means that no other thread will try /// to join it and the thread is responsible for cleaning up. fn detach_thread(&mut self, id: ThreadId) { self.threads[id].detached = true; } + /// Mark that the active thread tries to join the thread with `joined_thread_id`. fn join_thread(&mut self, joined_thread_id: ThreadId) { assert!(!self.threads[joined_thread_id].detached, "Bug: trying to join a detached thread."); @@ -215,23 +222,32 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { ); } } + /// Set the name of the active thread. fn set_thread_name(&mut self, new_thread_name: Vec) { self.active_thread_mut().thread_name = Some(new_thread_name); } + /// Get ids and states of all threads ever allocated. fn get_all_thread_ids_with_states(&self) -> Vec<(ThreadId, ThreadState)> { self.threads.iter_enumerated().map(|(id, thread)| (id, thread.state)).collect() } + + /// Allocate a new blockset id. fn create_blockset(&mut self) -> BlockSetId { self.blockset_counter = self.blockset_counter.checked_add(1).unwrap(); self.blockset_counter.into() } + + /// Block the currently active thread and put it into the given blockset. fn block_active_thread(&mut self, set: BlockSetId) { let state = &mut self.active_thread_mut().state; assert_eq!(*state, ThreadState::Enabled); *state = ThreadState::Blocked(set); } + + /// Unblock any one thread from the given blockset if it contains at least + /// one. Return the id of the unblocked thread. fn unblock_random_thread(&mut self, set: BlockSetId) -> Option { for (id, thread) in self.threads.iter_enumerated_mut() { if thread.state == ThreadState::Blocked(set) { @@ -242,6 +258,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { } None } + /// Decide which thread to run next. /// /// Returns `false` if all threads terminated. @@ -278,60 +295,74 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_ref(); this.machine.threads.get_thread_local_alloc_id(static_alloc_id) } + fn set_thread_local_alloc_id(&self, static_alloc_id: AllocId, thread_local_alloc_id: AllocId) { let this = self.eval_context_ref(); this.machine.threads.set_thread_local_alloc_id(static_alloc_id, thread_local_alloc_id) } + fn create_thread(&mut self) -> InterpResult<'tcx, ThreadId> { let this = self.eval_context_mut(); Ok(this.machine.threads.create_thread()) } + fn detach_thread(&mut self, thread_id: ThreadId) -> InterpResult<'tcx> { let this = self.eval_context_mut(); this.machine.threads.detach_thread(thread_id); Ok(()) } + fn join_thread(&mut self, joined_thread_id: ThreadId) -> InterpResult<'tcx> { let this = self.eval_context_mut(); this.machine.threads.join_thread(joined_thread_id); Ok(()) } + fn set_active_thread(&mut self, thread_id: ThreadId) -> InterpResult<'tcx, ThreadId> { let this = self.eval_context_mut(); Ok(this.machine.threads.set_active_thread_id(thread_id)) } + fn get_active_thread(&self) -> InterpResult<'tcx, ThreadId> { let this = self.eval_context_ref(); Ok(this.machine.threads.get_active_thread_id()) } + fn active_thread_stack(&self) -> &[Frame<'mir, 'tcx, Tag, FrameData<'tcx>>] { let this = self.eval_context_ref(); this.machine.threads.active_thread_stack() } + fn active_thread_stack_mut(&mut self) -> &mut Vec>> { let this = self.eval_context_mut(); this.machine.threads.active_thread_stack_mut() } + fn set_active_thread_name(&mut self, new_thread_name: Vec) -> InterpResult<'tcx, ()> { let this = self.eval_context_mut(); Ok(this.machine.threads.set_thread_name(new_thread_name)) } + fn get_all_thread_ids_with_states(&mut self) -> Vec<(ThreadId, ThreadState)> { let this = self.eval_context_mut(); this.machine.threads.get_all_thread_ids_with_states() } + fn create_blockset(&mut self) -> InterpResult<'tcx, BlockSetId> { let this = self.eval_context_mut(); Ok(this.machine.threads.create_blockset()) } + fn block_active_thread(&mut self, set: BlockSetId) -> InterpResult<'tcx> { let this = self.eval_context_mut(); Ok(this.machine.threads.block_active_thread(set)) } + fn unblock_random_thread(&mut self, set: BlockSetId) -> InterpResult<'tcx, Option> { let this = self.eval_context_mut(); Ok(this.machine.threads.unblock_random_thread(set)) } + /// Decide which thread to run next. /// /// Returns `false` if all threads terminated. diff --git a/tests/compile-fail/thread-spawn.rs b/tests/compile-fail/thread-spawn.rs new file mode 100644 index 00000000000..4b9073f3a73 --- /dev/null +++ b/tests/compile-fail/thread-spawn.rs @@ -0,0 +1,9 @@ +// ignore-linux +// ignore-macos +use std::thread; + +// error-pattern: Miri does not support threading + +fn main() { + thread::spawn(|| {}); +} diff --git a/tests/run-pass/concurrency/locks.rs b/tests/run-pass/concurrency/locks.rs index 575aeadd7fe..49935db91bd 100644 --- a/tests/run-pass/concurrency/locks.rs +++ b/tests/run-pass/concurrency/locks.rs @@ -1,3 +1,5 @@ +// ignore-windows + //! This test just calls the relevant APIs to check if Miri crashes. use std::sync::{Arc, Mutex}; diff --git a/tests/run-pass/concurrency/locks.stderr b/tests/run-pass/concurrency/locks.stderr new file mode 100644 index 00000000000..20a2bf3eeb8 --- /dev/null +++ b/tests/run-pass/concurrency/locks.stderr @@ -0,0 +1,2 @@ +warning: The thread support is experimental. For example, Miri does not detect data races yet. + diff --git a/tests/run-pass/concurrency/locks.stdout b/tests/run-pass/concurrency/locks.stdout deleted file mode 100644 index 2486b320db1..00000000000 --- a/tests/run-pass/concurrency/locks.stdout +++ /dev/null @@ -1,3 +0,0 @@ -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. diff --git a/tests/run-pass/concurrency/simple.rs b/tests/run-pass/concurrency/simple.rs index 5c295d1702d..5adc521f59c 100644 --- a/tests/run-pass/concurrency/simple.rs +++ b/tests/run-pass/concurrency/simple.rs @@ -1,3 +1,5 @@ +// ignore-windows + use std::thread; fn create_and_detach() { diff --git a/tests/run-pass/concurrency/simple.stderr b/tests/run-pass/concurrency/simple.stderr new file mode 100644 index 00000000000..20a2bf3eeb8 --- /dev/null +++ b/tests/run-pass/concurrency/simple.stderr @@ -0,0 +1,2 @@ +warning: The thread support is experimental. For example, Miri does not detect data races yet. + diff --git a/tests/run-pass/concurrency/simple.stdout b/tests/run-pass/concurrency/simple.stdout deleted file mode 100644 index 0506b7bdf83..00000000000 --- a/tests/run-pass/concurrency/simple.stdout +++ /dev/null @@ -1,10 +0,0 @@ -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. -WARNING: The thread support is experimental. For example, Miri does not detect data races yet. diff --git a/tests/run-pass/concurrency/thread_locals.rs b/tests/run-pass/concurrency/thread_locals.rs index 50aa6fee2f8..1805a1da3d0 100644 --- a/tests/run-pass/concurrency/thread_locals.rs +++ b/tests/run-pass/concurrency/thread_locals.rs @@ -1,3 +1,5 @@ +// ignore-windows + #![feature(thread_local)] use std::thread; diff --git a/tests/run-pass/concurrency/thread_locals.stderr b/tests/run-pass/concurrency/thread_locals.stderr new file mode 100644 index 00000000000..20a2bf3eeb8 --- /dev/null +++ b/tests/run-pass/concurrency/thread_locals.stderr @@ -0,0 +1,2 @@ +warning: The thread support is experimental. For example, Miri does not detect data races yet. + diff --git a/tests/run-pass/concurrency/thread_locals.stdout b/tests/run-pass/concurrency/thread_locals.stdout deleted file mode 100644 index 9a53b4a5c91..00000000000 --- a/tests/run-pass/concurrency/thread_locals.stdout +++ /dev/null @@ -1 +0,0 @@ -WARNING: The thread support is experimental. For example, Miri does not detect data races yet.