From 0a4e8caa8c303e8a8b5459bb79c7474eb53619ae Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 26 Jul 2020 11:15:01 +0200 Subject: [PATCH 1/8] adjust to canonical_alloc_id removal --- src/diagnostics.rs | 4 ++-- src/intptrcast.rs | 3 +-- src/machine.rs | 38 ++++++++++---------------------------- src/thread.rs | 41 +---------------------------------------- 4 files changed, 14 insertions(+), 72 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 009f8aa29c..3c64862445 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -94,8 +94,8 @@ pub fn report_error<'tcx, 'mir>( let helps = match e.kind { Unsupported(UnsupportedOpInfo::NoMirFor(..)) => vec![format!("make sure to use a Miri sysroot, which you can prepare with `cargo miri setup`")], - Unsupported(UnsupportedOpInfo::ReadBytesAsPointer) => - panic!("`ReadBytesAsPointer` cannot be raised by Miri"), + Unsupported(UnsupportedOpInfo::ReadBytesAsPointer | UnsupportedOpInfo::ThreadLocalStatic(_) | UnsupportedOpInfo::ReadExternStatic(_)) => + panic!("Error should never be raised by Miri: {:?}", e.kind), Unsupported(_) => vec![format!("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support")], UndefinedBehavior(UndefinedBehaviorInfo::AlignmentCheckFailed { .. }) => diff --git a/src/intptrcast.rs b/src/intptrcast.rs index c908bdf24e..188ff94861 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -6,7 +6,6 @@ use log::trace; use rand::Rng; use rustc_data_structures::fx::FxHashMap; -use rustc_mir::interpret::{AllocCheck, AllocId, InterpResult, Memory, Machine, Pointer, PointerArithmetic}; use rustc_target::abi::{Size, HasDataLayout}; use crate::*; @@ -79,7 +78,7 @@ impl<'mir, 'tcx> GlobalState { ) -> InterpResult<'tcx, u64> { let mut global_state = memory.extra.intptrcast.borrow_mut(); let global_state = &mut *global_state; - let id = Evaluator::canonical_alloc_id(memory, ptr.alloc_id); + let id = ptr.alloc_id; // There is nothing wrong with a raw pointer being cast to an integer only after // it became dangling. Hence `MaybeDead`. diff --git a/src/machine.rs b/src/machine.rs index e9217896ef..d418409df0 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -426,44 +426,26 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { Ok(()) } - fn thread_local_alloc_id( + fn thread_local_static_alloc_id( ecx: &mut InterpCx<'mir, 'tcx, Self>, def_id: DefId, ) -> InterpResult<'tcx, AllocId> { ecx.get_or_create_thread_local_alloc_id(def_id) } - fn adjust_global_const( - ecx: &InterpCx<'mir, 'tcx, Self>, - mut val: mir::interpret::ConstValue<'tcx>, - ) -> InterpResult<'tcx, mir::interpret::ConstValue<'tcx>> { - // FIXME: Remove this, do The Right Thing in `thread_local_alloc_id` instead. - ecx.remap_thread_local_alloc_ids(&mut val)?; - Ok(val) - } - - fn canonical_alloc_id(mem: &Memory<'mir, 'tcx, Self>, id: AllocId) -> AllocId { - let tcx = mem.tcx; - // Figure out if this is an extern static, and if yes, which one. - let def_id = match tcx.get_global_alloc(id) { - Some(GlobalAlloc::Static(def_id)) if tcx.is_foreign_item(def_id) => def_id, - _ => { - // No need to canonicalize anything. - return id; - } - }; - let attrs = tcx.get_attrs(def_id); + fn extern_static_alloc_id( + memory: &Memory<'mir, 'tcx, Self>, + def_id: DefId, + ) -> InterpResult<'tcx, AllocId> { + let attrs = memory.tcx.get_attrs(def_id); let link_name = match attr::first_attr_value_str_by_name(&attrs, sym::link_name) { Some(name) => name, - None => tcx.item_name(def_id), + None => memory.tcx.item_name(def_id), }; - // Check if we know this one. - if let Some(canonical_id) = mem.extra.extern_statics.get(&link_name) { - trace!("canonical_alloc_id: {:?} ({}) -> {:?}", id, link_name, canonical_id); - *canonical_id + if let Some(&id) = memory.extra.extern_statics.get(&link_name) { + Ok(id) } else { - // Return original id; `Memory::get_static_alloc` will throw an error. - id + throw_unsup_format!("`extern` static {:?} is not supported by Miri", def_id) } } diff --git a/src/thread.rs b/src/thread.rs index 42a4dbded5..0a83b71665 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -11,11 +11,7 @@ use log::trace; use rustc_data_structures::fx::FxHashMap; use rustc_hir::def_id::DefId; use rustc_index::vec::{Idx, IndexVec}; -use rustc_middle::{ - middle::codegen_fn_attrs::CodegenFnAttrFlags, - mir, - ty::{self, Instance}, -}; +use rustc_middle::ty::{self, Instance}; use crate::sync::SynchronizationState; use crate::*; @@ -499,41 +495,6 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { // Public interface to thread management. impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { - /// A workaround for thread-local statics until - /// https://github.com/rust-lang/rust/issues/70685 is fixed: change the - /// thread-local allocation id with a freshly generated allocation id for - /// the currently active thread. - fn remap_thread_local_alloc_ids( - &self, - val: &mut mir::interpret::ConstValue<'tcx>, - ) -> InterpResult<'tcx> { - let this = self.eval_context_ref(); - match *val { - mir::interpret::ConstValue::Scalar(Scalar::Ptr(ref mut ptr)) => { - let alloc_id = ptr.alloc_id; - let alloc = this.tcx.get_global_alloc(alloc_id); - let tcx = this.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 is_thread_local(def_id) => { - ptr.alloc_id = this.get_or_create_thread_local_alloc_id(def_id)?; - } - _ => {} - } - } - _ => { - // FIXME: Handling only `Scalar` seems to work for now, but at - // least in principle thread-locals could be in any constant, so - // we should also consider other cases. However, once - // https://github.com/rust-lang/rust/issues/70685 gets fixed, - // this code will have to be rewritten anyway. - } - } - Ok(()) - } - /// Get a thread-specific allocation id for the given thread-local static. /// If needed, allocate a new one. /// From 7b07fc385c1b9e61f8388c5540d344ecf25bb932 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 26 Jul 2020 12:11:27 +0200 Subject: [PATCH 2/8] get_or_create_thread_local_alloc_id: share code with Memory::get_global_alloc --- src/thread.rs | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/src/thread.rs b/src/thread.rs index 0a83b71665..8d493ac8f3 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -11,7 +11,6 @@ use log::trace; use rustc_data_structures::fx::FxHashMap; use rustc_hir::def_id::DefId; use rustc_index::vec::{Idx, IndexVec}; -use rustc_middle::ty::{self, Instance}; use crate::sync::SynchronizationState; use crate::*; @@ -497,9 +496,6 @@ impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mi pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { /// Get a thread-specific allocation id for the given thread-local static. /// If needed, allocate a new one. - /// - /// FIXME: This method should be replaced as soon as - /// https://github.com/rust-lang/rust/issues/70685 gets fixed. fn get_or_create_thread_local_alloc_id(&self, def_id: DefId) -> InterpResult<'tcx, AllocId> { let this = self.eval_context_ref(); let tcx = this.tcx; @@ -511,29 +507,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // We need to allocate a thread-specific allocation id for this // thread-local static. // - // At first, we invoke the `const_eval_raw` query and extract the - // allocation from it. Unfortunately, we have to duplicate the code - // from `Memory::get_global_alloc` that does this. - // + // At first, we compute the initial value for this static. // Then we store the retrieved allocation back into the `alloc_map` // to get a fresh allocation id, which we can use as a // thread-specific allocation id for the thread-local static. + // On first access to that allocation, it will be copied over to the machine memory. if tcx.is_foreign_item(def_id) { throw_unsup_format!("foreign thread-local statics are not supported"); } - // Invoke the `const_eval_raw` query. - let instance = Instance::mono(tcx.tcx, def_id); - let gid = GlobalId { instance, promoted: None }; - 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!(tcx.is_static(def_id)); - err - })?; - let id = raw_const.alloc_id; - // Extract the allocation from the query result. - let allocation = tcx.global_alloc(id).unwrap_memory(); + let allocation = interpret::get_static(*tcx, def_id)?; // Create a new allocation id for the same allocation in this hacky // way. Internally, `alloc_map` deduplicates allocations, but this // is fine because Miri will make a copy before a first mutable From 390899e8b9c9b3b415a630e663418f1ad7e10c4d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 26 Jul 2020 15:55:15 +0200 Subject: [PATCH 3/8] test referencing unsupported extern static --- tests/compile-fail/extern_static.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 tests/compile-fail/extern_static.rs diff --git a/tests/compile-fail/extern_static.rs b/tests/compile-fail/extern_static.rs new file mode 100644 index 0000000000..650dfd0ac7 --- /dev/null +++ b/tests/compile-fail/extern_static.rs @@ -0,0 +1,10 @@ +#![feature(raw_ref_op)] +//! Even referencing an unknown `extern static` already triggers an error. + +extern "C" { + static mut FOO: i32; +} + +fn main() { + let _val = unsafe { &raw const FOO }; //~ ERROR is not supported by Miri +} From 2a42f8e93c3be903cbfd940cbee3299506e184c7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 26 Jul 2020 15:53:02 +0200 Subject: [PATCH 4/8] fix and test order of TLS dtors and thread joining --- src/shims/tls.rs | 4 +-- src/thread.rs | 26 ++++++++++++++----- tests/run-pass/concurrency/tls_lib_drop.rs | 8 +++--- .../run-pass/concurrency/tls_lib_drop.stdout | 6 ++--- 4 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/shims/tls.rs b/src/shims/tls.rs index 8d05442ad6..4a0d5fc22a 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -328,9 +328,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// schedules them one by one each time it is called and reenables the /// thread so that it can be executed normally by the main execution loop. /// - /// FIXME: we do not support yet deallocation of thread local statics. - /// Issue: https://github.com/rust-lang/miri/issues/1369 - /// /// Note: we consistently run TLS destructors for all threads, including the /// main thread. However, it is not clear that we should run the TLS /// destructors for the main thread. See issue: @@ -367,6 +364,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // All dtors done! this.machine.tls.delete_all_thread_tls(active_thread); + this.thread_terminated(); Ok(()) } diff --git a/src/thread.rs b/src/thread.rs index 8d493ac8f3..8520dcd073 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -410,6 +410,20 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { None } + /// Handles thread termination of the active thread: wakes up threads joining on this one, + /// and deallocated thread-local statics. + /// + /// This is called from `tls.rs` after handling the TLS dtors. + fn thread_terminated(&mut self) { + for (i, thread) in self.threads.iter_enumerated_mut() { + // Check if we need to unblock any threads. + if thread.state == ThreadState::BlockedOnJoin(self.active_thread) { + trace!("unblocking {:?} because {:?} terminated", i, self.active_thread); + thread.state = ThreadState::Enabled; + } + } + } + /// Decide which action to take next and on which thread. /// /// The currently implemented scheduling policy is the one that is commonly @@ -421,13 +435,6 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { // checks whether the thread has popped all its stack and if yes, sets // the thread state to terminated). if self.threads[self.active_thread].check_terminated() { - // Check if we need to unblock any threads. - for (i, thread) in self.threads.iter_enumerated_mut() { - if thread.state == ThreadState::BlockedOnJoin(self.active_thread) { - trace!("unblocking {:?} because {:?} terminated", i, self.active_thread); - thread.state = ThreadState::Enabled; - } - } return Ok(SchedulingAction::ExecuteDtors); } if self.threads[MAIN_THREAD].state == ThreadState::Terminated { @@ -660,4 +667,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); this.machine.threads.schedule() } + + #[inline] + fn thread_terminated(&mut self) { + self.eval_context_mut().machine.threads.thread_terminated() + } } diff --git a/tests/run-pass/concurrency/tls_lib_drop.rs b/tests/run-pass/concurrency/tls_lib_drop.rs index de2566de85..46f59ef620 100644 --- a/tests/run-pass/concurrency/tls_lib_drop.rs +++ b/tests/run-pass/concurrency/tls_lib_drop.rs @@ -9,7 +9,8 @@ struct TestCell { impl Drop for TestCell { fn drop(&mut self) { - println!("Dropping: {}", self.value.borrow()) + for _ in 0..10 { thread::yield_now(); } + println!("Dropping: {} (should be before 'Continue main 1').", self.value.borrow()) } } @@ -28,7 +29,7 @@ fn check_destructors() { }) .join() .unwrap(); - println!("Continue main.") + println!("Continue main 1.") } struct JoinCell { @@ -37,8 +38,9 @@ struct JoinCell { impl Drop for JoinCell { fn drop(&mut self) { + for _ in 0..10 { thread::yield_now(); } let join_handle = self.value.borrow_mut().take().unwrap(); - println!("Joining: {}", join_handle.join().unwrap()); + println!("Joining: {} (should be before 'Continue main 2').", join_handle.join().unwrap()); } } diff --git a/tests/run-pass/concurrency/tls_lib_drop.stdout b/tests/run-pass/concurrency/tls_lib_drop.stdout index d622c0ccce..484979b04c 100644 --- a/tests/run-pass/concurrency/tls_lib_drop.stdout +++ b/tests/run-pass/concurrency/tls_lib_drop.stdout @@ -1,4 +1,4 @@ -Dropping: 5 -Continue main. +Dropping: 5 (should be before 'Continue main 1'). +Continue main 1. +Joining: 7 (should be before 'Continue main 2'). Continue main 2. -Joining: 7 From c77540ce13890ba5f16f276967e86f8d7fb8f78e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 27 Jul 2020 12:53:39 +0200 Subject: [PATCH 5/8] deallocate thread-local statics when the thread dies --- src/machine.rs | 16 ++++-- src/shims/env.rs | 4 +- src/shims/tls.rs | 2 +- src/stacked_borrows.rs | 4 +- src/thread.rs | 56 ++++++++++++------- .../thread_local_static_dealloc.rs | 13 +++++ 6 files changed, 63 insertions(+), 32 deletions(-) create mode 100644 tests/compile-fail/concurrency/thread_local_static_dealloc.rs diff --git a/src/machine.rs b/src/machine.rs index d418409df0..5dfe996274 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -64,7 +64,10 @@ pub enum MiriMemoryKind { Global, /// Memory for extern statics. /// This memory may leak. - ExternGlobal, + ExternStatic, + /// Memory for thread-local statics. + /// This memory may leak. + Tls, } impl Into> for MiriMemoryKind { @@ -80,7 +83,7 @@ impl MayLeak for MiriMemoryKind { use self::MiriMemoryKind::*; match self { Rust | C | WinHeap | Env => false, - Machine | Global | ExternGlobal => true, + Machine | Global | ExternStatic | Tls => true, } } } @@ -94,8 +97,9 @@ impl fmt::Display for MiriMemoryKind { WinHeap => write!(f, "Windows heap"), Machine => write!(f, "machine-managed memory"), Env => write!(f, "environment variable"), - Global => write!(f, "global"), - ExternGlobal => write!(f, "extern global"), + Global => write!(f, "global (static or const)"), + ExternStatic => write!(f, "extern static"), + Tls => write!(f, "thread-local static"), } } } @@ -175,7 +179,7 @@ impl MemoryExtra { // "__cxa_thread_atexit_impl" // This should be all-zero, pointer-sized. let layout = this.machine.layouts.usize; - let place = this.allocate(layout, MiriMemoryKind::ExternGlobal.into()); + let place = this.allocate(layout, MiriMemoryKind::ExternStatic.into()); this.write_scalar(Scalar::from_machine_usize(0, this), place.into())?; Self::add_extern_static(this, "__cxa_thread_atexit_impl", place.ptr); // "environ" @@ -185,7 +189,7 @@ impl MemoryExtra { // "_tls_used" // This is some obscure hack that is part of the Windows TLS story. It's a `u8`. let layout = this.machine.layouts.u8; - let place = this.allocate(layout, MiriMemoryKind::ExternGlobal.into()); + let place = this.allocate(layout, MiriMemoryKind::ExternStatic.into()); this.write_scalar(Scalar::from_u8(0), place.into())?; Self::add_extern_static(this, "_tls_used", place.ptr); } diff --git a/src/shims/env.rs b/src/shims/env.rs index 86a7a58ac4..d7474dbf87 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -383,9 +383,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.memory.deallocate(this.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::Env.into())?; } else { // No `environ` allocated yet, let's do that. - // This is memory backing an extern static, hence `ExternGlobal`, not `Env`. + // This is memory backing an extern static, hence `ExternStatic`, not `Env`. let layout = this.machine.layouts.usize; - let place = this.allocate(layout, MiriMemoryKind::ExternGlobal.into()); + let place = this.allocate(layout, MiriMemoryKind::ExternStatic.into()); this.machine.env_vars.environ = Some(place); } diff --git a/src/shims/tls.rs b/src/shims/tls.rs index 4a0d5fc22a..d929459740 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -364,7 +364,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // All dtors done! this.machine.tls.delete_all_thread_tls(active_thread); - this.thread_terminated(); + this.thread_terminated()?; Ok(()) } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 6942acc5e2..cefe334574 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -469,10 +469,10 @@ impl Stacks { // `Global` memory can be referenced by global pointers from `tcx`. // Thus we call `global_base_ptr` such that the global pointers get the same tag // as what we use here. - // `ExternGlobal` is used for extern statics, and thus must also be listed here. + // `ExternStatic` is used for extern statics, and thus must also be listed here. // `Env` we list because we can get away with precise tracking there. // The base pointer is not unique, so the base permission is `SharedReadWrite`. - MemoryKind::Machine(MiriMemoryKind::Global | MiriMemoryKind::ExternGlobal | MiriMemoryKind::Env) => + MemoryKind::Machine(MiriMemoryKind::Global | MiriMemoryKind::ExternStatic | MiriMemoryKind::Tls | MiriMemoryKind::Env) => (extra.borrow_mut().global_base_ptr(id), Permission::SharedReadWrite), // Everything else we handle entirely untagged for now. // FIXME: experiment with more precise tracking. diff --git a/src/thread.rs b/src/thread.rs index 8520dcd073..1e710a25ed 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -410,18 +410,31 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { None } - /// Handles thread termination of the active thread: wakes up threads joining on this one, - /// and deallocated thread-local statics. - /// - /// This is called from `tls.rs` after handling the TLS dtors. - fn thread_terminated(&mut self) { + /// Wakes up threads joining on the active one and deallocates thread-local statics. + /// The `AllocId` that can now be freed is returned. + fn thread_terminated(&mut self) -> Vec { + let mut free_tls_statics = Vec::new(); + { + let mut thread_local_statics = self.thread_local_alloc_ids.borrow_mut(); + thread_local_statics.retain(|&(_def_id, thread), &mut alloc_id| { + if thread != self.active_thread { + // Keep this static around. + return true; + } + // Delete this static from the map and from memory. + // We cannot free directly here as we cannot use `?` in this context. + free_tls_statics.push(alloc_id); + return false; + }); + } + // Check if we need to unblock any threads. for (i, thread) in self.threads.iter_enumerated_mut() { - // Check if we need to unblock any threads. if thread.state == ThreadState::BlockedOnJoin(self.active_thread) { trace!("unblocking {:?} because {:?} terminated", i, self.active_thread); thread.state = ThreadState::Enabled; } } + return free_tls_statics; } /// Decide which action to take next and on which thread. @@ -503,8 +516,8 @@ impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mi pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { /// Get a thread-specific allocation id for the given thread-local static. /// If needed, allocate a new one. - fn get_or_create_thread_local_alloc_id(&self, def_id: DefId) -> InterpResult<'tcx, AllocId> { - let this = self.eval_context_ref(); + fn get_or_create_thread_local_alloc_id(&mut self, def_id: DefId) -> InterpResult<'tcx, AllocId> { + let this = self.eval_context_mut(); let tcx = this.tcx; if let Some(new_alloc_id) = this.machine.threads.get_thread_local_alloc_id(def_id) { // We already have a thread-specific allocation id for this @@ -513,21 +526,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } else { // We need to allocate a thread-specific allocation id for this // thread-local static. - // - // At first, we compute the initial value for this static. - // Then we store the retrieved allocation back into the `alloc_map` - // to get a fresh allocation id, which we can use as a - // thread-specific allocation id for the thread-local static. - // On first access to that allocation, it will be copied over to the machine memory. + // First, we compute the initial value for this static. if tcx.is_foreign_item(def_id) { throw_unsup_format!("foreign thread-local statics are not supported"); } let allocation = interpret::get_static(*tcx, def_id)?; - // Create a new allocation id for the same allocation in this hacky - // way. Internally, `alloc_map` deduplicates allocations, but this - // is fine because Miri will make a copy before a first mutable - // access. - let new_alloc_id = tcx.create_memory_alloc(allocation); + // Create a fresh allocation with this content. + let new_alloc_id = this.memory.allocate_with(allocation.clone(), MiriMemoryKind::Tls.into()).alloc_id; this.machine.threads.set_thread_local_alloc_id(def_id, new_alloc_id); Ok(new_alloc_id) } @@ -668,8 +673,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.machine.threads.schedule() } + /// Handles thread termination of the active thread: wakes up threads joining on this one, + /// and deallocated thread-local statics. + /// + /// This is called from `tls.rs` after handling the TLS dtors. #[inline] - fn thread_terminated(&mut self) { - self.eval_context_mut().machine.threads.thread_terminated() + fn thread_terminated(&mut self) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + for alloc_id in this.machine.threads.thread_terminated() { + let ptr = this.memory.global_base_pointer(alloc_id.into())?; + this.memory.deallocate(ptr, None, MiriMemoryKind::Tls.into())?; + } + Ok(()) } } diff --git a/tests/compile-fail/concurrency/thread_local_static_dealloc.rs b/tests/compile-fail/concurrency/thread_local_static_dealloc.rs new file mode 100644 index 0000000000..1b20ce8bfb --- /dev/null +++ b/tests/compile-fail/concurrency/thread_local_static_dealloc.rs @@ -0,0 +1,13 @@ +// ignore-windows: Concurrency on Windows is not supported yet. + +//! Ensure that thread-local statics get deallocated when the thread dies. + +#![feature(thread_local)] + +#[thread_local] +static mut TLS: u8 = 0; + +fn main() { unsafe { + let dangling_ptr = std::thread::spawn(|| &TLS as *const u8 as usize).join().unwrap(); + let _val = *(dangling_ptr as *const u8); +} } From 6fbaa72642dacf92746c695046c8dfe6834ef18f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 27 Jul 2020 13:10:11 +0200 Subject: [PATCH 6/8] fix diagnostics printing when triggered during TLS dtor scheduling --- src/diagnostics.rs | 24 ++++++++++++++++++------ src/eval.rs | 4 ++-- src/shims/tls.rs | 3 +++ 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 3c64862445..2e8809cd73 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -162,7 +162,15 @@ fn report_msg<'tcx>( } else { tcx.sess.diagnostic().span_note_diag(span, title) }; - err.span_label(span, span_msg); + // Show main message. + if span != DUMMY_SP { + err.span_label(span, span_msg); + } else { + // Make sure we show the message even when it is a dummy span. + err.note(&span_msg); + err.note("(no span available)"); + } + // Show help messages. if !helps.is_empty() { // Add visual separator before backtrace. helps.last_mut().unwrap().push_str("\n"); @@ -198,7 +206,7 @@ pub fn register_diagnostic(e: NonHaltingDiagnostic) { /// after a step was taken. pub struct TopFrameInfo<'tcx> { stack_size: usize, - instance: ty::Instance<'tcx>, + instance: Option>, span: Span, } @@ -209,11 +217,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx DIAGNOSTICS.with(|diagnostics| assert!(diagnostics.borrow().is_empty())); let this = self.eval_context_ref(); + if this.active_thread_stack().is_empty() { + // Diagnostics can happen even with the emoty stack (e.g. deallocation thread-local statics). + return TopFrameInfo { stack_size: 0, instance: None, span: DUMMY_SP }; + } let frame = this.frame(); TopFrameInfo { stack_size: this.active_thread_stack().len(), - instance: frame.instance, + instance: Some(frame.instance), span: frame.current_source_info().map_or(DUMMY_SP, |si| si.span), } } @@ -237,15 +249,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if stacktrace.len() < info.stack_size { assert!(stacktrace.len() == info.stack_size-1, "we should never pop more than one frame at once"); let frame_info = FrameInfo { - instance: info.instance, + instance: info.instance.unwrap(), span: info.span, lint_root: None, }; stacktrace.insert(0, frame_info); - } else { + } else if let Some(instance) = info.instance { // Adjust topmost frame. stacktrace[0].span = info.span; - assert_eq!(stacktrace[0].instance, info.instance, "we should not pop and push a frame in one step"); + assert_eq!(stacktrace[0].instance, instance, "we should not pop and push a frame in one step"); } // Show diagnostics. diff --git a/src/eval.rs b/src/eval.rs index 8561edcc05..cc5a6eb21f 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -211,11 +211,10 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> let res: InterpResult<'_, i64> = (|| { // Main loop. loop { + let info = ecx.preprocess_diagnostics(); match ecx.schedule()? { SchedulingAction::ExecuteStep => { - let info = ecx.preprocess_diagnostics(); assert!(ecx.step()?, "a terminated thread was scheduled for execution"); - ecx.process_diagnostics(info); } SchedulingAction::ExecuteTimeoutCallback => { assert!(ecx.machine.communicate, @@ -233,6 +232,7 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> break; } } + ecx.process_diagnostics(info); } let return_code = ecx.read_scalar(ret_place.into())?.check_init()?.to_machine_isize(&ecx)?; Ok(return_code) diff --git a/src/shims/tls.rs b/src/shims/tls.rs index d929459740..2ba0782f70 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -348,6 +348,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return Ok(()) } } + // The remaining dtors make some progress each time around the scheduler loop, + // until they return `false` to indicate that they are done. + // The macOS thread wide destructor runs "before any TLS slots get // freed", so do that first. if this.schedule_macos_tls_dtor()? { From bec7aab7fd089d89e940029038669f851d497caa Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 27 Jul 2020 14:10:31 +0200 Subject: [PATCH 7/8] Typos Co-authored-by: Oliver Scherer --- src/diagnostics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 2e8809cd73..1b41ba4418 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -218,7 +218,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_ref(); if this.active_thread_stack().is_empty() { - // Diagnostics can happen even with the emoty stack (e.g. deallocation thread-local statics). + // Diagnostics can happen even with the empty stack (e.g. deallocation of thread-local statics). return TopFrameInfo { stack_size: 0, instance: None, span: DUMMY_SP }; } let frame = this.frame(); From cae90b6d293dc0e9bb3275457a94323bd14d51a1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 27 Jul 2020 23:40:27 +0200 Subject: [PATCH 8/8] rustup and test fixes --- rust-version | 2 +- tests/compile-fail/concurrency/thread_local_static_dealloc.rs | 2 +- tests/run-pass/panic/catch_panic.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rust-version b/rust-version index 211af60b88..3f188639fa 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -13f9aa190957b993a268fd4a046fce76ca8814ee +efc02b03d18b0cbaa55b1e421d792f70a39230b2 diff --git a/tests/compile-fail/concurrency/thread_local_static_dealloc.rs b/tests/compile-fail/concurrency/thread_local_static_dealloc.rs index 1b20ce8bfb..73e4ab5965 100644 --- a/tests/compile-fail/concurrency/thread_local_static_dealloc.rs +++ b/tests/compile-fail/concurrency/thread_local_static_dealloc.rs @@ -9,5 +9,5 @@ static mut TLS: u8 = 0; fn main() { unsafe { let dangling_ptr = std::thread::spawn(|| &TLS as *const u8 as usize).join().unwrap(); - let _val = *(dangling_ptr as *const u8); + let _val = *(dangling_ptr as *const u8); //~ ERROR dereferenced after this allocation got freed } } diff --git a/tests/run-pass/panic/catch_panic.rs b/tests/run-pass/panic/catch_panic.rs index ac41de586e..811c370d81 100644 --- a/tests/run-pass/panic/catch_panic.rs +++ b/tests/run-pass/panic/catch_panic.rs @@ -1,4 +1,4 @@ -// normalize-stderr-test "[^ ]*libcore/[a-z/]+.rs[0-9:]*" -> "$$LOC" +// normalize-stderr-test "[^ ]*libcore/[a-z_/]+.rs[0-9:]*" -> "$$LOC" #![feature(never_type)] #![allow(unconditional_panic)]