diff --git a/src/machine.rs b/src/machine.rs index d418409df067..5dfe99627437 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 86a7a58ac4aa..d7474dbf87ef 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 4a0d5fc22ad6..d92945974007 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 6942acc5e2b0..cefe334574b4 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 8520dcd073a7..1e710a25edc9 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 000000000000..1b20ce8bfb38 --- /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); +} }