Auto merge of #75396 - RalfJung:miri-spans, r=oli-obk

Miri: improve spans of required_const failures In #75339 I added a loop evaluating all consts required by a function body. Unfortunately, if one of their evaluations fails, then the span used for that was that of the first statement in the function body, which happened to work form some existing test but is not sensible in general. This PR changes it to point to the whole function instead, which is at least not wrong. r? @oli-obk
rust-lang · Aug 12, 2020 · 576d27c · 576d27c
2 parents ef1d58e + fd32fe9
commit 576d27c
Show file tree

Hide file tree

Showing 11 changed files with 69 additions and 56 deletions.
diff --git a/src/librustc_mir/const_eval/machine.rs b/src/librustc_mir/const_eval/machine.rs
@@ -301,12 +301,16 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
         Ok(())
     }
 
-    fn after_stack_push(ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
-        // Enforce stack size limit.
-        if !ecx.tcx.sess.recursion_limit().value_within_limit(ecx.stack().len()) {
+    #[inline(always)]
+    fn init_frame_extra(
+        ecx: &mut InterpCx<'mir, 'tcx, Self>,
+        frame: Frame<'mir, 'tcx>,
+    ) -> InterpResult<'tcx, Frame<'mir, 'tcx>> {
+        // Enforce stack size limit. Add 1 because this is run before the new frame is pushed.
+        if !ecx.tcx.sess.recursion_limit().value_within_limit(ecx.stack().len() + 1) {
             throw_exhaust!(StackFrameLimitReached)
         } else {
-            Ok(())
+            Ok(frame)
         }
     }
 

diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs
@@ -16,7 +16,7 @@ use rustc_middle::ty::layout::{self, TyAndLayout};
 use rustc_middle::ty::{
     self, query::TyCtxtAt, subst::SubstsRef, ParamEnv, Ty, TyCtxt, TypeFoldable,
 };
-use rustc_span::{source_map::DUMMY_SP, Pos, Span};
+use rustc_span::{Pos, Span};
 use rustc_target::abi::{Align, HasDataLayout, LayoutOf, Size, TargetDataLayout};
 
 use super::{
@@ -83,9 +83,11 @@ pub struct Frame<'mir, 'tcx, Tag = (), Extra = ()> {
     ////////////////////////////////////////////////////////////////////////////////
     // Current position within the function
     ////////////////////////////////////////////////////////////////////////////////
-    /// If this is `None`, we are unwinding and this function doesn't need any clean-up.
-    /// Just continue the same as with `Resume`.
-    pub loc: Option<mir::Location>,
+    /// If this is `Err`, we are not currently executing any particular statement in
+    /// this frame (can happen e.g. during frame initialization, and during unwinding on
+    /// frames without cleanup code).
+    /// We basically abuse `Result` as `Either`.
+    pub(super) loc: Result<mir::Location, Span>,
 }
 
 /// What we store about a frame in an interpreter backtrace.
@@ -189,7 +191,14 @@ impl<'mir, 'tcx, Tag> Frame<'mir, 'tcx, Tag> {
 impl<'mir, 'tcx, Tag, Extra> Frame<'mir, 'tcx, Tag, Extra> {
     /// Return the `SourceInfo` of the current instruction.
     pub fn current_source_info(&self) -> Option<&mir::SourceInfo> {
-        self.loc.map(|loc| self.body.source_info(loc))
+        self.loc.ok().map(|loc| self.body.source_info(loc))
+    }
+
+    pub fn current_span(&self) -> Span {
+        match self.loc {
+            Ok(loc) => self.body.source_info(loc).span,
+            Err(span) => span,
+        }
     }
 }
 
@@ -324,11 +333,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
     #[inline(always)]
     pub fn cur_span(&self) -> Span {
-        self.stack()
-            .last()
-            .and_then(|f| f.current_source_info())
-            .map(|si| si.span)
-            .unwrap_or(self.tcx.span)
+        self.stack().last().map(|f| f.current_span()).unwrap_or(self.tcx.span)
     }
 
     #[inline(always)]
@@ -640,7 +645,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         // first push a stack frame so we have access to the local substs
         let pre_frame = Frame {
             body,
-            loc: Some(mir::Location::START),
+            loc: Err(body.span), // Span used for errors caused during preamble.
             return_to_block,
             return_place,
             // empty local array, we fill it in below, after we are inside the stack frame and
@@ -654,9 +659,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
         // Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
         for const_ in &body.required_consts {
+            let span = const_.span;
             let const_ =
                 self.subst_from_current_frame_and_normalize_erasing_regions(const_.literal);
-            self.const_to_op(const_, None)?;
+            self.const_to_op(const_, None).map_err(|err| {
+                // If there was an error, set the span of the current frame to this constant.
+                // Avoiding doing this when evaluation succeeds.
+                self.frame_mut().loc = Err(span);
+                err
+            })?;
         }
 
         // Locals are initially uninitialized.
@@ -683,8 +694,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         }
         // done
         self.frame_mut().locals = locals;
-
         M::after_stack_push(self)?;
+        self.frame_mut().loc = Ok(mir::Location::START);
         info!("ENTERING({}) {}", self.frame_idx(), self.frame().instance);
 
         Ok(())
@@ -693,7 +704,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// Jump to the given block.
     #[inline]
     pub fn go_to_block(&mut self, target: mir::BasicBlock) {
-        self.frame_mut().loc = Some(mir::Location { block: target, statement_index: 0 });
+        self.frame_mut().loc = Ok(mir::Location { block: target, statement_index: 0 });
     }
 
     /// *Return* to the given `target` basic block.
@@ -715,7 +726,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// If `target` is `None`, that indicates the function does not need cleanup during
     /// unwinding, and we will just keep propagating that upwards.
     pub fn unwind_to_block(&mut self, target: Option<mir::BasicBlock>) {
-        self.frame_mut().loc = target.map(|block| mir::Location { block, statement_index: 0 });
+        self.frame_mut().loc = match target {
+            Some(block) => Ok(mir::Location { block, statement_index: 0 }),
+            None => Err(self.frame_mut().body.span),
+        };
     }
 
     /// Pops the current frame from the stack, deallocating the
@@ -743,8 +757,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         assert_eq!(
             unwinding,
             match self.frame().loc {
-                None => true,
-                Some(loc) => self.body().basic_blocks()[loc.block].is_cleanup,
+                Ok(loc) => self.body().basic_blocks()[loc.block].is_cleanup,
+                Err(_) => true,
             }
         );
 
@@ -920,14 +934,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     pub fn generate_stacktrace(&self) -> Vec<FrameInfo<'tcx>> {
         let mut frames = Vec::new();
         for frame in self.stack().iter().rev() {
-            let source_info = frame.current_source_info();
-            let lint_root = source_info.and_then(|source_info| {
+            let lint_root = frame.current_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);
+            let span = frame.current_span();
 
             frames.push(FrameInfo { span, instance: frame.instance, lint_root });
         }

diff --git a/src/librustc_mir/interpret/intrinsics/caller_location.rs b/src/librustc_mir/interpret/intrinsics/caller_location.rs
@@ -30,8 +30,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             // Assert that there is always such a frame.
             .unwrap();
         // 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).
+        // (`loc` is `Err` when we are unwinding and the frame does not require cleanup).
         let loc = frame.loc.unwrap();
         // If this is a `Call` terminator, use the `fn_span` instead.
         let block = &frame.body.basic_blocks()[loc.block];

diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs
@@ -409,12 +409,4 @@ pub macro compile_time_machine(<$mir: lifetime, $tcx: lifetime>) {
     ) -> Self::PointerTag {
         ()
     }
-
-    #[inline(always)]
-    fn init_frame_extra(
-        _ecx: &mut InterpCx<$mir, $tcx, Self>,
-        frame: Frame<$mir, $tcx>,
-    ) -> InterpResult<$tcx, Frame<$mir, $tcx>> {
-        Ok(frame)
-    }
 }
diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs
@@ -47,8 +47,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         }
 
         let loc = match self.frame().loc {
-            Some(loc) => loc,
-            None => {
+            Ok(loc) => loc,
+            Err(_) => {
                 // We are unwinding and this fn has no cleanup code.
                 // Just go on unwinding.
                 trace!("unwinding: skipping frame");
@@ -283,7 +283,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
         self.eval_terminator(terminator)?;
         if !self.stack().is_empty() {
-            if let Some(loc) = self.frame().loc {
+            if let Ok(loc) = self.frame().loc {
                 info!("// executing {:?}", loc.block);
             }
         }

diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs
@@ -281,6 +281,14 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
         Ok(())
     }
 
+    #[inline(always)]
+    fn init_frame_extra(
+        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
+        frame: Frame<'mir, 'tcx>,
+    ) -> InterpResult<'tcx, Frame<'mir, 'tcx>> {
+        Ok(frame)
+    }
+
     #[inline(always)]
     fn stack(
         ecx: &'a InterpCx<'mir, 'tcx, Self>,

diff --git a/src/test/ui/consts/const-err-multi.stderr b/src/test/ui/consts/const-err-multi.stderr
@@ -24,17 +24,17 @@ error: any use of this value will cause an error
   --> $DIR/const-err-multi.rs:7:19
    |
 LL | pub const C: u8 = A as u8;
-   | ------------------^^^^^^^-
+   | ------------------^-------
    |                   |
    |                   referenced constant has errors
 
 error: any use of this value will cause an error
-  --> $DIR/const-err-multi.rs:9:19
+  --> $DIR/const-err-multi.rs:9:24
    |
 LL | pub const D: i8 = 50 - A;
-   | ------------------^^^^^^-
-   |                   |
-   |                   referenced constant has errors
+   | -----------------------^-
+   |                        |
+   |                        referenced constant has errors
 
 error: aborting due to 4 previous errors
 
diff --git a/src/test/ui/consts/const-eval/erroneous-const.rs b/src/test/ui/consts/const-eval/erroneous-const.rs
@@ -8,8 +8,8 @@ impl<T> PrintName<T> {
 }
 
 const fn no_codegen<T>() {
-    if false { //~ERROR evaluation of constant value failed
-        let _ = PrintName::<T>::VOID;
+    if false {
+        let _ = PrintName::<T>::VOID; //~ERROR evaluation of constant value failed
     }
 }
 

diff --git a/src/test/ui/consts/const-eval/erroneous-const.stderr b/src/test/ui/consts/const-eval/erroneous-const.stderr
@@ -25,12 +25,10 @@ LL | #![warn(const_err, unconditional_panic)]
    |         ^^^^^^^^^
 
 error[E0080]: evaluation of constant value failed
-  --> $DIR/erroneous-const.rs:11:5
+  --> $DIR/erroneous-const.rs:12:17
    |
-LL | /     if false {
-LL | |         let _ = PrintName::<T>::VOID;
-LL | |     }
-   | |_____^ referenced constant has errors
+LL |         let _ = PrintName::<T>::VOID;
+   |                 ^^^^^^^^^^^^^^^^^^^^ referenced constant has errors
 
 error[E0080]: could not evaluate static initializer
   --> $DIR/erroneous-const.rs:16:22

diff --git a/src/test/ui/consts/uninhabited-const-issue-61744.rs b/src/test/ui/consts/uninhabited-const-issue-61744.rs
@@ -1,11 +1,11 @@
 // build-fail
 
 pub const unsafe fn fake_type<T>() -> T {
-    hint_unreachable() //~ ERROR evaluation of constant value failed
+    hint_unreachable()
 }
 
 pub const unsafe fn hint_unreachable() -> ! {
-    fake_type()
+    fake_type() //~ ERROR evaluation of constant value failed
 }
 
 trait Const {

diff --git a/src/test/ui/consts/uninhabited-const-issue-61744.stderr b/src/test/ui/consts/uninhabited-const-issue-61744.stderr
@@ -1,11 +1,9 @@
 error[E0080]: evaluation of constant value failed
-  --> $DIR/uninhabited-const-issue-61744.rs:4:5
+  --> $DIR/uninhabited-const-issue-61744.rs:8:5
    |
 LL |     hint_unreachable()
-   |     ^^^^^^^^^^^^^^^^^^
+   |     ------------------
    |     |
-   |     reached the configured maximum number of stack frames
-   |     inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
    |     inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
    |     inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
    |     inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
@@ -72,8 +70,9 @@ LL |     hint_unreachable()
    |     inside `fake_type::<i32>` at $DIR/uninhabited-const-issue-61744.rs:4:5
 ...
 LL |     fake_type()
-   |     -----------
+   |     ^^^^^^^^^^^
    |     |
+   |     reached the configured maximum number of stack frames
    |     inside `hint_unreachable` at $DIR/uninhabited-const-issue-61744.rs:8:5
    |     inside `hint_unreachable` at $DIR/uninhabited-const-issue-61744.rs:8:5
    |     inside `hint_unreachable` at $DIR/uninhabited-const-issue-61744.rs:8:5