Auto merge of #1489 - RalfJung:tls-alloc-ids, r=oli-obk

Adjust for new rustc alloc ID handling and deallocate thread-local statics

Miri side of rust-lang/rust#74775.

Fixes #1369
Fixes #1488

Author: bors

rust-version

@@ -1 +1 @@
-13f9aa190957b993a268fd4a046fce76ca8814ee
+efc02b03d18b0cbaa55b1e421d792f70a39230b2

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 { .. }) =>
@@ -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<ty::Instance<'tcx>>,
     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 empty stack (e.g. deallocation of 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.

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)

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`.

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<MemoryKind<MiriMemoryKind>> 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);
             }
@@ -426,44 +430,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)
         }
     }
 

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);
         }
 

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:
@@ -351,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()? {
@@ -367,6 +367,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(())
     }

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.