Merge pull request rust-lang#4747 from RalfJung/empty-stack-error

show span when there is an error invoking a global ctor/dtor or the thread main fn

Author: RalfJung

src/tools/miri/README.md

@@ -624,6 +624,7 @@ Definite bugs found:
 * [Mockall reading uninitialized memory when mocking `std::io::Read::read`, even if all expectations are satisfied](https://github.com/asomers/mockall/issues/647) (caught by Miri running Tokio's test suite)
 * [`ReentrantLock` not correctly dealing with reuse of addresses for TLS storage of different threads](https://github.com/rust-lang/rust/pull/141248)
 * [Rare Deadlock in the thread (un)parking example code](https://github.com/rust-lang/rust/issues/145816)
+* [`winit` registering a global constructor with the wrong ABI on Windows](https://github.com/rust-windowing/winit/issues/4435)
 
 Violations of [Stacked Borrows] found that are likely bugs (but Stacked Borrows is currently just an experiment):
 

src/tools/miri/src/concurrency/thread.rs

@@ -14,7 +14,7 @@ use rustc_hir::def_id::DefId;
 use rustc_index::{Idx, IndexVec};
 use rustc_middle::mir::Mutability;
 use rustc_middle::ty::layout::TyAndLayout;
-use rustc_span::Span;
+use rustc_span::{DUMMY_SP, Span};
 use rustc_target::spec::Os;
 
 use crate::concurrency::GlobalDataRaceHandler;
@@ -174,6 +174,10 @@ pub struct Thread<'tcx> {
     /// The virtual call stack.
     stack: Vec<Frame<'tcx, Provenance, FrameExtra<'tcx>>>,
 
+    /// A span that explains where the thread (or more specifically, its current root
+    /// frame) "comes from".
+    pub(crate) origin_span: Span,
+
     /// The function to call when the stack ran empty, to figure out what to do next.
     /// Conceptually, this is the interpreter implementation of the things that happen 'after' the
     /// Rust language entry point for this thread returns (usually implemented by the C or OS runtime).
@@ -303,6 +307,7 @@ impl<'tcx> Thread<'tcx> {
             state: ThreadState::Enabled,
             thread_name: name.map(|name| Vec::from(name.as_bytes())),
             stack: Vec::new(),
+            origin_span: DUMMY_SP,
             top_user_relevant_frame: None,
             join_status: ThreadJoinStatus::Joinable,
             unwind_payloads: Vec::new(),
@@ -318,6 +323,7 @@ impl VisitProvenance for Thread<'_> {
             unwind_payloads: panic_payload,
             last_error,
             stack,
+            origin_span: _,
             top_user_relevant_frame: _,
             state: _,
             thread_name: _,
@@ -584,6 +590,10 @@ impl<'tcx> ThreadManager<'tcx> {
         &self.threads[self.active_thread]
     }
 
+    pub fn thread_ref(&self, thread_id: ThreadId) -> &Thread<'tcx> {
+        &self.threads[thread_id]
+    }
+
     /// Mark the thread as detached, which means that no other thread will try
     /// to join it and the thread is responsible for cleaning up.
     ///
@@ -704,8 +714,9 @@ trait EvalContextPrivExt<'tcx>: MiriInterpCxExt<'tcx> {
     #[inline]
     fn run_on_stack_empty(&mut self) -> InterpResult<'tcx, Poll<()>> {
         let this = self.eval_context_mut();
-        let mut callback = this
-            .active_thread_mut()
+        let active_thread = this.active_thread_mut();
+        active_thread.origin_span = DUMMY_SP; // reset, the old value no longer applied
+        let mut callback = active_thread
             .on_stack_empty
             .take()
             .expect("`on_stack_empty` not set up, or already running");
@@ -891,11 +902,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let this = self.eval_context_mut();
 
         // Create the new thread
+        let current_span = this.machine.current_user_relevant_span();
         let new_thread_id = this.machine.threads.create_thread({
             let mut state = tls::TlsDtorsState::default();
             Box::new(move |m| state.on_stack_empty(m))
         });
-        let current_span = this.machine.current_user_relevant_span();
         match &mut this.machine.data_race {
             GlobalDataRaceHandler::None => {}
             GlobalDataRaceHandler::Vclocks(data_race) =>
@@ -934,12 +945,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // it.
         let ret_place = this.allocate(ret_layout, MiriMemoryKind::Machine.into())?;
 
-        this.call_function(
+        this.call_thread_root_function(
             instance,
             start_abi,
             &[func_arg],
             Some(&ret_place),
-            ReturnContinuation::Stop { cleanup: true },
+            current_span,
         )?;
 
         // Restore the old active thread frame.

src/tools/miri/src/diagnostics.rs

@@ -444,7 +444,11 @@ pub fn report_result<'tcx>(
     write!(primary_msg, "{}", format_interp_error(ecx.tcx.dcx(), res)).unwrap();
 
     if labels.is_empty() {
-        labels.push(format!("{} occurred here", title.unwrap_or("error")));
+        labels.push(format!(
+            "{} occurred {}",
+            title.unwrap_or("error"),
+            if stacktrace.is_empty() { "due to this code" } else { "here" }
+        ));
     }
 
     report_msg(
@@ -552,7 +556,14 @@ pub fn report_msg<'tcx>(
     thread: Option<ThreadId>,
     machine: &MiriMachine<'tcx>,
 ) {
-    let span = stacktrace.first().map_or(DUMMY_SP, |fi| fi.span);
+    let span = match stacktrace.first() {
+        Some(fi) => fi.span,
+        None =>
+            match thread {
+                Some(thread_id) => machine.threads.thread_ref(thread_id).origin_span,
+                None => DUMMY_SP,
+            },
+    };
     let sess = machine.tcx.sess;
     let level = match diag_level {
         DiagLevel::Error => Level::Error,
@@ -620,6 +631,12 @@ pub fn report_msg<'tcx>(
                 err.note(format!("{frame_info} at {span}"));
             }
         }
+    } else if stacktrace.len() == 0 && !span.is_dummy() {
+        err.note(format!(
+            "this {} occurred while pushing a call frame onto an empty stack",
+            level.to_str()
+        ));
+        err.note("the span indicates which code caused the function to be called, but may not be the literal call site");
     }
 
     err.emit();

src/tools/miri/src/helpers.rs

@@ -472,6 +472,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         )
     }
 
+    /// Call a function in an "empty" thread.
+    fn call_thread_root_function(
+        &mut self,
+        f: ty::Instance<'tcx>,
+        caller_abi: ExternAbi,
+        args: &[ImmTy<'tcx>],
+        dest: Option<&MPlaceTy<'tcx>>,
+        span: Span,
+    ) -> InterpResult<'tcx> {
+        let this = self.eval_context_mut();
+        assert!(this.active_thread_stack().is_empty());
+        assert!(this.active_thread_ref().origin_span.is_dummy());
+        this.active_thread_mut().origin_span = span;
+        this.call_function(f, caller_abi, args, dest, ReturnContinuation::Stop { cleanup: true })
+    }
+
     /// Visits the memory covered by `place`, sensitive to freezing: the 2nd parameter
     /// of `action` will be true if this is frozen, false if this is in an `UnsafeCell`.
     /// The range is relative to `place`.
@@ -995,11 +1011,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         interp_ok(())
     }
 
-    /// Lookup an array of immediates from any linker sections matching the provided predicate.
+    /// Lookup an array of immediates from any linker sections matching the provided predicate,
+    /// with the spans of where they were found.
     fn lookup_link_section(
         &mut self,
         include_name: impl Fn(&str) -> bool,
-    ) -> InterpResult<'tcx, Vec<ImmTy<'tcx>>> {
+    ) -> InterpResult<'tcx, Vec<(ImmTy<'tcx>, Span)>> {
         let this = self.eval_context_mut();
         let tcx = this.tcx.tcx;
 
@@ -1012,19 +1029,20 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             };
             if include_name(link_section.as_str()) {
                 let instance = ty::Instance::mono(tcx, def_id);
+                let span = tcx.def_span(def_id);
                 let const_val = this.eval_global(instance).unwrap_or_else(|err| {
                     panic!(
                         "failed to evaluate static in required link_section: {def_id:?}\n{err:?}"
                     )
                 });
                 match const_val.layout.ty.kind() {
                     ty::FnPtr(..) => {
-                        array.push(this.read_immediate(&const_val)?);
+                        array.push((this.read_immediate(&const_val)?, span));
                     }
                     ty::Array(elem_ty, _) if matches!(elem_ty.kind(), ty::FnPtr(..)) => {
                         let mut elems = this.project_array_fields(&const_val)?;
                         while let Some((_idx, elem)) = elems.next(this)? {
-                            array.push(this.read_immediate(&elem)?);
+                            array.push((this.read_immediate(&elem)?, span));
                         }
                     }
                     _ =>

src/tools/miri/src/lib.rs

@@ -39,6 +39,7 @@
     clippy::needless_question_mark,
     clippy::needless_lifetimes,
     clippy::too_long_first_doc_paragraph,
+    clippy::len_zero,
     // We don't use translatable diagnostics
     rustc::diagnostic_outside_of_impl,
     // We are not implementing queries here so it's fine

src/tools/miri/src/shims/global_ctor.rs

@@ -3,6 +3,7 @@
 use std::task::Poll;
 
 use rustc_abi::ExternAbi;
+use rustc_span::Span;
 use rustc_target::spec::BinaryFormat;
 
 use crate::*;
@@ -15,7 +16,7 @@ enum GlobalCtorStatePriv<'tcx> {
     #[default]
     Init,
     /// The list of constructor functions that we still have to call.
-    Ctors(Vec<ImmTy<'tcx>>),
+    Ctors(Vec<(ImmTy<'tcx>, Span)>),
     Done,
 }
 
@@ -67,19 +68,19 @@ impl<'tcx> GlobalCtorState<'tcx> {
                     break 'new_state Ctors(ctors);
                 }
                 Ctors(ctors) => {
-                    if let Some(ctor) = ctors.pop() {
+                    if let Some((ctor, span)) = ctors.pop() {
                         let this = this.eval_context_mut();
 
                         let ctor = ctor.to_scalar().to_pointer(this)?;
                         let thread_callback = this.get_ptr_fn(ctor)?.as_instance()?;
 
                         // The signature of this function is `unsafe extern "C" fn()`.
-                        this.call_function(
+                        this.call_thread_root_function(
                             thread_callback,
                             ExternAbi::C { unwind: false },
                             &[],
                             None,
-                            ReturnContinuation::Stop { cleanup: true },
+                            span,
                         )?;
 
                         return interp_ok(Poll::Pending); // we stay in this state (but `ctors` got shorter)