Rollup merge of #70590 - RalfJung:miri-backtrace, r=oli-obk

Miri: make backtrace function names and spans match up

Currently, Miri backtraces are a bit confusing:
```
error: Undefined Behavior: entering unreachable code
  --> tests/compile-fail/never_transmute_void.rs:10:11
   |
10 |     match v {} //~ ERROR  entering unreachable code
   |           ^ entering unreachable code
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
note: inside call to `f` at tests/compile-fail/never_transmute_void.rs:17:5
  --> tests/compile-fail/never_transmute_void.rs:17:5
   |
17 |     f(v); //~ inside call to `f`
   |     ^^^^
   = note: inside call to `main` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:67:34
   = note: inside call to closure at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:52:73
   = note: inside call to closure at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:130:5
```
When reading this like a normal backtrace, one would expect that e.g. the backrace involves the "main" function at "libstd/rt.rs:67:34". But that is not actually where we are in the main function, that is *where the main function is called*.

This is not how backtraces are usually rendered (including e.g. with `RUST_BACKTRACE=1`). Usually we print next to each function name where inside that function the frame is currently executing, not where the *parent* frame is executing. With this PR and the Miri side at rust-lang/miri#1283, the backtrace now looks as follows:
```
error: Undefined Behavior: entering unreachable code
  --> tests/compile-fail/never_transmute_void.rs:10:11
   |
10 |     match v {} //~ ERROR entering unreachable code
   |           ^ entering unreachable code
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: inside `f` at tests/compile-fail/never_transmute_void.rs:10:11
note: inside `main` at tests/compile-fail/never_transmute_void.rs:17:5
  --> tests/compile-fail/never_transmute_void.rs:17:5
   |
17 |     f(v); //~ inside `main`
   |     ^^^^
   = note: inside closure at /home/r/src/rust/rustc/src/libstd/rt.rs:67:34
   = note: inside closure at /home/r/src/rust/rustc/src/libstd/rt.rs:52:73
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<[closure@DefId(1:6034 ~ std[87db]::rt[0]::lang_start_internal[0]::{{closure}}[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/r/src/rust/rustc/src/libstd/sys_common/backtrace.rs:130:5
```
Now function name and printed line numbers match up in the notes.

This code is partially shared with const-eval, so the change also affects const-eval: instead of printing what is being called at some span, we print which function/constant this span is inside.

With this, we can also remove the `span` field from Miri's stack frames (which used to track the *caller span* of that frame, quite confusing), and then get of a whole lot of `span` arguments that ultimately just served to fill that field (and as a fallback for `caller_location`, which however was never actually used).

r? @oli-obk

Author: Dylan-DPC

src/librustc_middle/mir/interpret/error.rs

@@ -53,9 +53,8 @@ pub struct ConstEvalErr<'tcx> {
 
 #[derive(Debug)]
 pub struct FrameInfo<'tcx> {
-    /// This span is in the caller.
-    pub call_site: Span,
     pub instance: ty::Instance<'tcx>,
+    pub span: Span,
     pub lint_root: Option<hir::HirId>,
 }
 
@@ -65,12 +64,12 @@ impl<'tcx> fmt::Display for FrameInfo<'tcx> {
             if tcx.def_key(self.instance.def_id()).disambiguated_data.data
                 == DefPathData::ClosureExpr
             {
-                write!(f, "inside call to closure")?;
+                write!(f, "inside closure")?;
             } else {
-                write!(f, "inside call to `{}`", self.instance)?;
+                write!(f, "inside `{}`", self.instance)?;
             }
-            if !self.call_site.is_dummy() {
-                let lo = tcx.sess.source_map().lookup_char_pos(self.call_site.lo());
+            if !self.span.is_dummy() {
+                let lo = tcx.sess.source_map().lookup_char_pos(self.span.lo());
                 write!(f, " at {}:{}:{}", lo.file.name, lo.line, lo.col.to_usize() + 1)?;
             }
             Ok(())
@@ -168,13 +167,10 @@ impl<'tcx> ConstEvalErr<'tcx> {
             if let Some(span_msg) = span_msg {
                 err.span_label(self.span, span_msg);
             }
-            // Add spans for the stacktrace.
-            // Skip the last, which is just the environment of the constant.  The stacktrace
-            // is sometimes empty because we create "fake" eval contexts in CTFE to do work
-            // on constant values.
-            if !self.stacktrace.is_empty() {
-                for frame_info in &self.stacktrace[..self.stacktrace.len() - 1] {
-                    err.span_label(frame_info.call_site, frame_info.to_string());
+            // Add spans for the stacktrace. Don't print a single-line backtrace though.
+            if self.stacktrace.len() > 1 {
+                for frame_info in &self.stacktrace {
+                    err.span_label(frame_info.span, frame_info.to_string());
                 }
             }
             // Let the caller finish the job.

src/librustc_mir/const_eval/error.rs

@@ -55,6 +55,6 @@ pub fn error_to_const_error<'mir, 'tcx, M: Machine<'mir, 'tcx>>(
     mut error: InterpErrorInfo<'tcx>,
 ) -> ConstEvalErr<'tcx> {
     error.print_backtrace();
-    let stacktrace = ecx.generate_stacktrace(None);
+    let stacktrace = ecx.generate_stacktrace();
     ConstEvalErr { error: error.kind, stacktrace, span: ecx.tcx.span }
 }

src/librustc_mir/const_eval/eval_queries.rs

@@ -46,7 +46,6 @@ fn eval_body_using_ecx<'mir, 'tcx>(
 
     ecx.push_stack_frame(
         cid.instance,
-        body.span,
         body,
         Some(ret.into()),
         StackPopCleanup::None { cleanup: false },

src/librustc_mir/const_eval/machine.rs

@@ -8,9 +8,9 @@ use std::hash::Hash;
 use rustc_data_structures::fx::FxHashMap;
 
 use rustc_ast::ast::Mutability;
+use rustc_hir::def_id::DefId;
 use rustc_middle::mir::AssertMessage;
 use rustc_span::symbol::Symbol;
-use rustc_span::{def_id::DefId, Span};
 
 use crate::interpret::{
     self, AllocId, Allocation, GlobalId, ImmTy, InterpCx, InterpResult, Memory, MemoryKind, OpTy,
@@ -64,7 +64,6 @@ impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter> {
     /// If this returns successfully (`Ok`), the function should just be evaluated normally.
     fn hook_panic_fn(
         &mut self,
-        span: Span,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx>],
     ) -> InterpResult<'tcx> {
@@ -77,7 +76,7 @@ impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter> {
 
             let msg_place = self.deref_operand(args[0])?;
             let msg = Symbol::intern(self.read_str(msg_place)?);
-            let span = self.find_closest_untracked_caller_location().unwrap_or(span);
+            let span = self.find_closest_untracked_caller_location();
             let (file, line, col) = self.location_triple_for_span(span);
             Err(ConstEvalErrKind::Panic { msg, file, line, col }.into())
         } else {
@@ -191,7 +190,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
 
     fn find_mir_or_eval_fn(
         ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        span: Span,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx>],
         ret: Option<(PlaceTy<'tcx>, mir::BasicBlock)>,
@@ -213,7 +211,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
             } else {
                 // Some functions we support even if they are non-const -- but avoid testing
                 // that for const fn!
-                ecx.hook_panic_fn(span, instance, args)?;
+                ecx.hook_panic_fn(instance, args)?;
                 // We certainly do *not* want to actually call the fn
                 // though, so be sure we return here.
                 throw_unsup_format!("calling non-const function `{}`", instance)
@@ -248,13 +246,12 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
 
     fn call_intrinsic(
         ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        span: Span,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx>],
         ret: Option<(PlaceTy<'tcx>, mir::BasicBlock)>,
         _unwind: Option<mir::BasicBlock>,
     ) -> InterpResult<'tcx> {
-        if ecx.emulate_intrinsic(span, instance, args, ret)? {
+        if ecx.emulate_intrinsic(instance, args, ret)? {
             return Ok(());
         }
         // An intrinsic that we do not support

src/librustc_mir/interpret/eval_context.rs

@@ -17,7 +17,7 @@ use rustc_middle::ty::layout::{self, Align, HasDataLayout, LayoutOf, Size, TyAnd
 use rustc_middle::ty::query::TyCtxtAt;
 use rustc_middle::ty::subst::SubstsRef;
 use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable};
-use rustc_span::source_map::{self, Span, DUMMY_SP};
+use rustc_span::source_map::DUMMY_SP;
 
 use super::{
     Immediate, MPlaceTy, Machine, MemPlace, MemPlaceMeta, Memory, OpTy, Operand, Place, PlaceTy,
@@ -57,9 +57,6 @@ pub struct Frame<'mir, 'tcx, Tag = (), Extra = ()> {
     /// The def_id and substs of the current function.
     pub instance: ty::Instance<'tcx>,
 
-    /// The span of the call site.
-    pub span: source_map::Span,
-
     /// Extra data for the machine.
     pub extra: Extra,
 
@@ -502,7 +499,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     pub fn push_stack_frame(
         &mut self,
         instance: ty::Instance<'tcx>,
-        span: Span,
         body: &'mir mir::Body<'tcx>,
         return_place: Option<PlaceTy<'tcx, M::PointerTag>>,
         return_to_block: StackPopCleanup,
@@ -522,7 +518,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             // empty local array, we fill it in below, after we are inside the stack frame and
             // all methods actually know about the frame
             locals: IndexVec::new(),
-            span,
             instance,
             stmt: 0,
             extra,
@@ -541,7 +536,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 // statics and constants don't have `Storage*` statements, no need to look for them
                 Some(DefKind::Static) | Some(DefKind::Const) | Some(DefKind::AssocConst) => {}
                 _ => {
-                    trace!("push_stack_frame: {:?}: num_bbs: {}", span, body.basic_blocks().len());
                     for block in body.basic_blocks() {
                         for stmt in block.statements.iter() {
                             use rustc_middle::mir::StatementKind::{StorageDead, StorageLive};
@@ -859,33 +853,21 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         }
     }
 
-    pub fn generate_stacktrace(&self, explicit_span: Option<Span>) -> Vec<FrameInfo<'tcx>> {
-        let mut last_span = None;
+    pub fn generate_stacktrace(&self) -> Vec<FrameInfo<'tcx>> {
         let mut frames = Vec::new();
         for frame in self.stack().iter().rev() {
-            // make sure we don't emit frames that are duplicates of the previous
-            if explicit_span == Some(frame.span) {
-                last_span = Some(frame.span);
-                continue;
-            }
-            if let Some(last) = last_span {
-                if last == frame.span {
-                    continue;
-                }
-            } else {
-                last_span = Some(frame.span);
-            }
-
-            let lint_root = frame.current_source_info().and_then(|source_info| {
+            let source_info = frame.current_source_info();
+            let lint_root = source_info.and_then(|source_info| {
                 match &frame.body.source_scopes[source_info.scope].local_data {
                     mir::ClearCrossCrate::Set(data) => Some(data.lint_root),
                     mir::ClearCrossCrate::Clear => None,
                 }
             });
+            let span = source_info.map_or(DUMMY_SP, |source_info| source_info.span);
 
-            frames.push(FrameInfo { call_site: frame.span, instance: frame.instance, lint_root });
+            frames.push(FrameInfo { span, instance: frame.instance, lint_root });
         }
-        trace!("generate stacktrace: {:#?}, {:?}", frames, explicit_span);
+        trace!("generate stacktrace: {:#?}", frames);
         frames
     }
 }
@@ -899,7 +881,6 @@ where
     fn hash_stable(&self, hcx: &mut StableHashingContext<'ctx>, hasher: &mut StableHasher) {
         self.body.hash_stable(hcx, hasher);
         self.instance.hash_stable(hcx, hasher);
-        self.span.hash_stable(hcx, hasher);
         self.return_to_block.hash_stable(hcx, hasher);
         self.return_place.as_ref().map(|r| &**r).hash_stable(hcx, hasher);
         self.locals.hash_stable(hcx, hasher);

src/librustc_mir/interpret/intrinsics.rs

@@ -15,7 +15,6 @@ use rustc_middle::ty::layout::{LayoutOf, Primitive, Size};
 use rustc_middle::ty::subst::SubstsRef;
 use rustc_middle::ty::TyCtxt;
 use rustc_span::symbol::{sym, Symbol};
-use rustc_span::Span;
 
 use super::{ImmTy, InterpCx, Machine, OpTy, PlaceTy};
 
@@ -78,7 +77,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// Returns `true` if emulation happened.
     pub fn emulate_intrinsic(
         &mut self,
-        span: Span,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, M::PointerTag>],
         ret: Option<(PlaceTy<'tcx, M::PointerTag>, mir::BasicBlock)>,
@@ -101,7 +99,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         // `src/librustc_middle/ty/constness.rs`
         match intrinsic_name {
             sym::caller_location => {
-                let span = self.find_closest_untracked_caller_location().unwrap_or(span);
+                let span = self.find_closest_untracked_caller_location();
                 let location = self.alloc_caller_location_for_span(span);
                 self.write_scalar(location.ptr, dest)?;
             }
@@ -118,7 +116,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     sym::needs_drop => self.tcx.types.bool,
                     sym::type_id => self.tcx.types.u64,
                     sym::type_name => self.tcx.mk_static_str(),
-                    _ => span_bug!(span, "Already checked for nullary intrinsics"),
+                    _ => bug!("already checked for nullary intrinsics"),
                 };
                 let val = self.const_eval(gid, ty)?;
                 self.copy_op(val, dest)?;

src/librustc_mir/interpret/intrinsics/caller_location.rs

@@ -12,18 +12,21 @@ use crate::interpret::{
 
 impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// Walks up the callstack from the intrinsic's callsite, searching for the first callsite in a
-    /// frame which is not `#[track_caller]`. If the first frame found lacks `#[track_caller]`, then
-    /// `None` is returned and the callsite of the function invocation itself should be used.
-    crate fn find_closest_untracked_caller_location(&self) -> Option<Span> {
-        let mut caller_span = None;
-        for next_caller in self.stack.iter().rev() {
-            if !next_caller.instance.def.requires_caller_location(*self.tcx) {
-                return caller_span;
-            }
-            caller_span = Some(next_caller.span);
-        }
-
-        caller_span
+    /// frame which is not `#[track_caller]`.
+    crate fn find_closest_untracked_caller_location(&self) -> Span {
+        self.stack
+            .iter()
+            .rev()
+            // Find first non-`#[track_caller]` frame.
+            .find(|frame| !frame.instance.def.requires_caller_location(*self.tcx))
+            // Assert that there is always such a frame.
+            .unwrap()
+            .current_source_info()
+            // Assert that the frame we look at is actually executing code currently
+            // (`current_source_info` is None when we are unwinding and the frame does
+            // not require cleanup).
+            .unwrap()
+            .span
     }
 
     /// Allocate a `const core::panic::Location` with the provided filename and line/column numbers.

src/librustc_mir/interpret/machine.rs

@@ -7,7 +7,7 @@ use std::hash::Hash;
 
 use rustc_middle::mir;
 use rustc_middle::ty::{self, Ty};
-use rustc_span::{def_id::DefId, Span};
+use rustc_span::def_id::DefId;
 
 use super::{
     AllocId, Allocation, AllocationExtra, Frame, ImmTy, InterpCx, InterpResult, Memory, MemoryKind,
@@ -135,7 +135,6 @@ pub trait Machine<'mir, 'tcx>: Sized {
     /// was used.
     fn find_mir_or_eval_fn(
         ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        span: Span,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, Self::PointerTag>],
         ret: Option<(PlaceTy<'tcx, Self::PointerTag>, mir::BasicBlock)>,
@@ -156,7 +155,6 @@ pub trait Machine<'mir, 'tcx>: Sized {
     /// responsibility to advance the instruction pointer as appropriate.
     fn call_intrinsic(
         ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        span: Span,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, Self::PointerTag>],
         ret: Option<(PlaceTy<'tcx, Self::PointerTag>, mir::BasicBlock)>,

src/librustc_mir/interpret/terminator.rs

@@ -4,7 +4,6 @@ use std::convert::TryFrom;
 use rustc_middle::ty::layout::{self, LayoutOf, TyAndLayout};
 use rustc_middle::ty::Instance;
 use rustc_middle::{mir, ty};
-use rustc_span::source_map::Span;
 use rustc_target::spec::abi::Abi;
 
 use super::{
@@ -71,14 +70,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     Some((dest, ret)) => Some((self.eval_place(dest)?, ret)),
                     None => None,
                 };
-                self.eval_fn_call(
-                    fn_val,
-                    terminator.source_info.span,
-                    abi,
-                    &args[..],
-                    ret,
-                    *cleanup,
-                )?;
+                self.eval_fn_call(fn_val, abi, &args[..], ret, *cleanup)?;
             }
 
             Drop { location, target, unwind } => {
@@ -88,7 +80,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 trace!("TerminatorKind::drop: {:?}, type {}", location, ty);
 
                 let instance = Instance::resolve_drop_in_place(*self.tcx, ty);
-                self.drop_in_place(place, instance, terminator.source_info.span, target, unwind)?;
+                self.drop_in_place(place, instance, target, unwind)?;
             }
 
             Assert { ref cond, expected, ref msg, target, cleanup } => {
@@ -196,7 +188,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     fn eval_fn_call(
         &mut self,
         fn_val: FnVal<'tcx, M::ExtraFnVal>,
-        span: Span,
         caller_abi: Abi,
         args: &[OpTy<'tcx, M::PointerTag>],
         ret: Option<(PlaceTy<'tcx, M::PointerTag>, mir::BasicBlock)>,
@@ -242,7 +233,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         match instance.def {
             ty::InstanceDef::Intrinsic(..) => {
                 assert!(caller_abi == Abi::RustIntrinsic || caller_abi == Abi::PlatformIntrinsic);
-                M::call_intrinsic(self, span, instance, args, ret, unwind)
+                M::call_intrinsic(self, instance, args, ret, unwind)
             }
             ty::InstanceDef::VtableShim(..)
             | ty::InstanceDef::ReifyShim(..)
@@ -252,14 +243,13 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             | ty::InstanceDef::CloneShim(..)
             | ty::InstanceDef::Item(_) => {
                 // We need MIR for this fn
-                let body = match M::find_mir_or_eval_fn(self, span, instance, args, ret, unwind)? {
+                let body = match M::find_mir_or_eval_fn(self, instance, args, ret, unwind)? {
                     Some(body) => body,
                     None => return Ok(()),
                 };
 
                 self.push_stack_frame(
                     instance,
-                    span,
                     body,
                     ret.map(|p| p.0),
                     StackPopCleanup::Goto { ret: ret.map(|p| p.1), unwind },
@@ -407,7 +397,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     OpTy::from(ImmTy { layout: this_receiver_ptr, imm: receiver_place.ptr.into() });
                 trace!("Patched self operand to {:#?}", args[0]);
                 // recurse with concrete function
-                self.eval_fn_call(drop_fn, span, caller_abi, &args, ret, unwind)
+                self.eval_fn_call(drop_fn, caller_abi, &args, ret, unwind)
             }
         }
     }
@@ -416,7 +406,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         &mut self,
         place: PlaceTy<'tcx, M::PointerTag>,
         instance: ty::Instance<'tcx>,
-        span: Span,
         target: mir::BasicBlock,
         unwind: Option<mir::BasicBlock>,
     ) -> InterpResult<'tcx> {
@@ -444,7 +433,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
         self.eval_fn_call(
             FnVal::Instance(instance),
-            span,
             Abi::Rust,
             &[arg.into()],
             Some((dest.into(), target)),