Auto merge of rust-lang#97740 - RalfJung:ctfe-cycle-spans, r=lcnr

bors · bors · commit 282445a288b3 · 2022-06-09T01:52:15.000Z
use precise spans for recursive const evaluation This fixes rust-lang#73283 by using a `TyCtxtAt` with a more precise span when the interpreter recursively calls itself. Hopefully such calls are sufficiently rare that this does not cost us too much performance. (In theory, cycles can also arise through layout computation, as layout can depend on consts -- but layout computation happens all the time so we'd have to do something to not make this terrible for performance.)
diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs
@@ -126,7 +126,9 @@ pub struct Frame<'mir, 'tcx, Tag: Provenance = AllocId, Extra = ()> {
     /// 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>,
+    ///
+    /// Needs to be public because ConstProp does unspeakable things to it.
+    pub loc: Result<mir::Location, Span>,
 }
 
 /// What we store about a frame in an interpreter backtrace.
@@ -320,6 +322,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> LayoutOfHelpers<'tcx> for InterpC
 
     #[inline]
     fn layout_tcx_at_span(&self) -> Span {
+        // Using the cheap root span for performance.
         self.tcx.span
     }
 
@@ -923,7 +926,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             self.param_env
         };
         let param_env = param_env.with_const();
-        let val = self.tcx.eval_to_allocation_raw(param_env.and(gid))?;
+        // Use a precise span for better cycle errors.
+        let val = self.tcx.at(self.cur_span()).eval_to_allocation_raw(param_env.and(gid))?;
         self.raw_const_to_mplace(val)
     }
 
diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs
@@ -169,7 +169,7 @@ impl<'mir, 'tcx: 'mir, 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(),
-                    _ => bug!("already checked for nullary intrinsics"),
+                    _ => bug!(),
                 };
                 let val =
                     self.tcx.const_eval_global_id(self.param_env, gid, Some(self.tcx.span))?;
@@ -215,7 +215,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     sym::add_with_overflow => BinOp::Add,
                     sym::sub_with_overflow => BinOp::Sub,
                     sym::mul_with_overflow => BinOp::Mul,
-                    _ => bug!("Already checked for int ops"),
+                    _ => bug!(),
                 };
                 self.binop_with_overflow(bin_op, &lhs, &rhs, dest)?;
             }
@@ -251,7 +251,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     sym::unchecked_mul => BinOp::Mul,
                     sym::unchecked_div => BinOp::Div,
                     sym::unchecked_rem => BinOp::Rem,
-                    _ => bug!("Already checked for int ops"),
+                    _ => bug!(),
                 };
                 let (val, overflowed, _ty) = self.overflowing_binary_op(bin_op, &l, &r)?;
                 if overflowed {
diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics/caller_location.rs b/compiler/rustc_const_eval/src/interpret/intrinsics/caller_location.rs
@@ -70,7 +70,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
         }
 
-        bug!("no non-`#[track_caller]` frame found")
+        span_bug!(self.cur_span(), "no non-`#[track_caller]` frame found")
     }
 
     /// Allocate a `const core::panic::Location` with the provided filename and line/column numbers.
diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs
@@ -504,7 +504,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     throw_unsup!(ReadExternStatic(def_id));
                 }
 
-                (self.tcx.eval_static_initializer(def_id)?, Some(def_id))
+                // Use a precise span for better cycle errors.
+                (self.tcx.at(self.cur_span()).eval_static_initializer(def_id)?, Some(def_id))
             }
         };
         M::before_access_global(*self.tcx, &self.machine, id, alloc, def_id, is_write)?;
diff --git a/compiler/rustc_const_eval/src/interpret/operator.rs b/compiler/rustc_const_eval/src/interpret/operator.rs
@@ -154,14 +154,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 let result = match bin_op {
                     Shl => l.checked_shl(r).unwrap(),
                     Shr => l.checked_shr(r).unwrap(),
-                    _ => bug!("it has already been checked that this is a shift op"),
+                    _ => bug!(),
                 };
                 result as u128
             } else {
                 match bin_op {
                     Shl => l.checked_shl(r).unwrap(),
                     Shr => l.checked_shr(r).unwrap(),
-                    _ => bug!("it has already been checked that this is a shift op"),
+                    _ => bug!(),
                 }
             };
             let truncated = self.truncate(result, left_layout);
diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs
@@ -55,33 +55,32 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         };
         let basic_block = &self.body().basic_blocks()[loc.block];
 
-        let old_frames = self.frame_idx();
-
         if let Some(stmt) = basic_block.statements.get(loc.statement_index) {
-            assert_eq!(old_frames, self.frame_idx());
+            let old_frames = self.frame_idx();
             self.statement(stmt)?;
+            // Make sure we are not updating `statement_index` of the wrong frame.
+            assert_eq!(old_frames, self.frame_idx());
+            // Advance the program counter.
+            self.frame_mut().loc.as_mut().unwrap().statement_index += 1;
             return Ok(true);
         }
 
         M::before_terminator(self)?;
 
         let terminator = basic_block.terminator();
-        assert_eq!(old_frames, self.frame_idx());
         self.terminator(terminator)?;
         Ok(true)
     }
 
     /// Runs the interpretation logic for the given `mir::Statement` at the current frame and
-    /// statement counter. This also moves the statement counter forward.
+    /// statement counter.
+    ///
+    /// This does NOT move the statement counter forward, the caller has to do that!
     pub fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> {
         info!("{:?}", stmt);
 
         use rustc_middle::mir::StatementKind::*;
 
-        // Some statements (e.g., box) push new stack frames.
-        // We have to record the stack frame number *before* executing the statement.
-        let frame_idx = self.frame_idx();
-
         match &stmt.kind {
             Assign(box (place, rvalue)) => self.eval_rvalue_into_place(rvalue, *place)?,
 
@@ -144,7 +143,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             Nop => {}
         }
 
-        self.stack_mut()[frame_idx].loc.as_mut().unwrap().statement_index += 1;
         Ok(())
     }
 
@@ -300,6 +298,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         Ok(())
     }
 
+    /// Evaluate the given terminator. Will also adjust the stack frame and statement position accordingly.
     fn terminator(&mut self, terminator: &mir::Terminator<'tcx>) -> InterpResult<'tcx> {
         info!("{:?}", terminator.kind);
 
diff --git a/compiler/rustc_middle/src/mir/interpret/queries.rs b/compiler/rustc_middle/src/mir/interpret/queries.rs
@@ -3,9 +3,9 @@ use super::{ErrorHandled, EvalToConstValueResult, GlobalId};
 use crate::mir;
 use crate::ty::fold::TypeFoldable;
 use crate::ty::subst::InternalSubsts;
-use crate::ty::{self, TyCtxt};
+use crate::ty::{self, query::TyCtxtAt, TyCtxt};
 use rustc_hir::def_id::DefId;
-use rustc_span::Span;
+use rustc_span::{Span, DUMMY_SP};
 
 impl<'tcx> TyCtxt<'tcx> {
     /// Evaluates a constant without providing any substitutions. This is useful to evaluate consts
@@ -86,14 +86,25 @@ impl<'tcx> TyCtxt<'tcx> {
         }
     }
 
+    /// Evaluate a static's initializer, returning the allocation of the initializer's memory.
+    #[inline(always)]
+    pub fn eval_static_initializer(
+        self,
+        def_id: DefId,
+    ) -> Result<mir::ConstAllocation<'tcx>, ErrorHandled> {
+        self.at(DUMMY_SP).eval_static_initializer(def_id)
+    }
+}
+
+impl<'tcx> TyCtxtAt<'tcx> {
     /// Evaluate a static's initializer, returning the allocation of the initializer's memory.
     pub fn eval_static_initializer(
         self,
         def_id: DefId,
     ) -> Result<mir::ConstAllocation<'tcx>, ErrorHandled> {
         trace!("eval_static_initializer: Need to compute {:?}", def_id);
         assert!(self.is_static(def_id));
-        let instance = ty::Instance::mono(self, def_id);
+        let instance = ty::Instance::mono(*self, def_id);
         let gid = GlobalId { instance, promoted: None };
         self.eval_to_allocation(gid, ty::ParamEnv::reveal_all())
     }
@@ -109,7 +120,9 @@ impl<'tcx> TyCtxt<'tcx> {
         let raw_const = self.eval_to_allocation_raw(param_env.and(gid))?;
         Ok(self.global_alloc(raw_const.alloc_id).unwrap_memory())
     }
+}
 
+impl<'tcx> TyCtxt<'tcx> {
     /// Destructure a type-level constant ADT or array into its variant index and its field values.
     /// Panics if the destructuring fails, use `try_destructure_const` for fallible version.
     pub fn destructure_const(
diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs
@@ -437,10 +437,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         source_info.scope.lint_root(self.source_scopes)
     }
 
-    fn use_ecx<F, T>(&mut self, f: F) -> Option<T>
+    fn use_ecx<F, T>(&mut self, source_info: SourceInfo, f: F) -> Option<T>
     where
         F: FnOnce(&mut Self) -> InterpResult<'tcx, T>,
     {
+        // Overwrite the PC -- whatever the interpreter does to it does not make any sense anyway.
+        self.ecx.frame_mut().loc = Err(source_info.span);
         match f(self) {
             Ok(val) => Some(val),
             Err(error) => {
@@ -501,17 +503,17 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
     }
 
     /// Returns the value, if any, of evaluating `place`.
-    fn eval_place(&mut self, place: Place<'tcx>) -> Option<OpTy<'tcx>> {
+    fn eval_place(&mut self, place: Place<'tcx>, source_info: SourceInfo) -> Option<OpTy<'tcx>> {
         trace!("eval_place(place={:?})", place);
-        self.use_ecx(|this| this.ecx.eval_place_to_op(place, None))
+        self.use_ecx(source_info, |this| this.ecx.eval_place_to_op(place, None))
     }
 
     /// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant`
     /// or `eval_place`, depending on the variant of `Operand` used.
     fn eval_operand(&mut self, op: &Operand<'tcx>, source_info: SourceInfo) -> Option<OpTy<'tcx>> {
         match *op {
             Operand::Constant(ref c) => self.eval_constant(c, source_info),
-            Operand::Move(place) | Operand::Copy(place) => self.eval_place(place),
+            Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, source_info),
         }
     }
 
@@ -537,7 +539,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         arg: &Operand<'tcx>,
         source_info: SourceInfo,
     ) -> Option<()> {
-        if let (val, true) = self.use_ecx(|this| {
+        if let (val, true) = self.use_ecx(source_info, |this| {
             let val = this.ecx.read_immediate(&this.ecx.eval_operand(arg, None)?)?;
             let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, &val)?;
             Ok((val, overflow))
@@ -564,8 +566,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         right: &Operand<'tcx>,
         source_info: SourceInfo,
     ) -> Option<()> {
-        let r = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?));
-        let l = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?));
+        let r = self.use_ecx(source_info, |this| {
+            this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?)
+        });
+        let l = self.use_ecx(source_info, |this| {
+            this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?)
+        });
         // Check for exceeding shifts *even if* we cannot evaluate the LHS.
         if op == BinOp::Shr || op == BinOp::Shl {
             let r = r?;
@@ -602,7 +608,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
 
         if let (Some(l), Some(r)) = (&l, &r) {
             // The remaining operators are handled through `overflowing_binary_op`.
-            if self.use_ecx(|this| {
+            if self.use_ecx(source_info, |this| {
                 let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
                 Ok(overflow)
             })? {
@@ -690,7 +696,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
             return None;
         }
 
-        self.use_ecx(|this| this.ecx.eval_rvalue_into_place(rvalue, place))
+        self.use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, place))
     }
 }
 
@@ -890,7 +896,10 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> {
                 StatementKind::SetDiscriminant { ref place, .. } => {
                     match self.ecx.machine.can_const_prop[place.local] {
                         ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => {
-                            if self.use_ecx(|this| this.ecx.statement(statement)).is_some() {
+                            if self
+                                .use_ecx(source_info, |this| this.ecx.statement(statement))
+                                .is_some()
+                            {
                                 trace!("propped discriminant into {:?}", place);
                             } else {
                                 Self::remove_const(&mut self.ecx, place.local);
diff --git a/src/test/ui/consts/recursive-zst-static.default.stderr b/src/test/ui/consts/recursive-zst-static.default.stderr
@@ -5,10 +5,10 @@ LL | static FOO: () = FOO;
    | ^^^^^^^^^^^^^^^^^^^^^
    |
 note: ...which requires const-evaluating + checking `FOO`...
-  --> $DIR/recursive-zst-static.rs:10:1
+  --> $DIR/recursive-zst-static.rs:10:18
    |
 LL | static FOO: () = FOO;
-   | ^^^^^^^^^^^^^^^^^^^^^
+   |                  ^^^
    = note: ...which again requires const-evaluating + checking `FOO`, completing the cycle
    = note: cycle used when running analysis passes on this crate
 
diff --git a/src/test/ui/consts/recursive-zst-static.unleash.stderr b/src/test/ui/consts/recursive-zst-static.unleash.stderr
@@ -5,10 +5,10 @@ LL | static FOO: () = FOO;
    | ^^^^^^^^^^^^^^^^^^^^^
    |
 note: ...which requires const-evaluating + checking `FOO`...
-  --> $DIR/recursive-zst-static.rs:10:1
+  --> $DIR/recursive-zst-static.rs:10:18
    |
 LL | static FOO: () = FOO;
-   | ^^^^^^^^^^^^^^^^^^^^^
+   |                  ^^^
    = note: ...which again requires const-evaluating + checking `FOO`, completing the cycle
    = note: cycle used when running analysis passes on this crate
 
diff --git a/src/test/ui/consts/write-to-static-mut-in-static.stderr b/src/test/ui/consts/write-to-static-mut-in-static.stderr
@@ -11,10 +11,10 @@ LL | pub static mut C: u32 = unsafe { C = 1; 0 };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
 note: ...which requires const-evaluating + checking `C`...
-  --> $DIR/write-to-static-mut-in-static.rs:5:1
+  --> $DIR/write-to-static-mut-in-static.rs:5:34
    |
 LL | pub static mut C: u32 = unsafe { C = 1; 0 };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                                  ^^^^^
    = note: ...which again requires const-evaluating + checking `C`, completing the cycle
    = note: cycle used when running analysis passes on this crate
 
diff --git a/src/test/ui/recursion/recursive-static-definition.stderr b/src/test/ui/recursion/recursive-static-definition.stderr
@@ -5,10 +5,10 @@ LL | pub static FOO: u32 = FOO;
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
 note: ...which requires const-evaluating + checking `FOO`...
-  --> $DIR/recursive-static-definition.rs:1:1
+  --> $DIR/recursive-static-definition.rs:1:23
    |
 LL | pub static FOO: u32 = FOO;
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                       ^^^
    = note: ...which again requires const-evaluating + checking `FOO`, completing the cycle
    = note: cycle used when running analysis passes on this crate
 

Original file line number	Diff line number	Diff line change
`@@ -70,7 +70,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {`
`70`	`70`	`}`
`71`	`71`	`}`
`72`	`72`
`73`		- bug!("no non-`#[track_caller]` frame found")
	`73`	+ span_bug!(self.cur_span(), "no non-`#[track_caller]` frame found")
`74`	`74`	`}`
`75`	`75`
`76`	`76`	/// Allocate a `const core::panic::Location` with the provided filename and line/column numbers.
Original file line number	Diff line number	Diff line change
`@@ -504,7 +504,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {`
`504`	`504`	`throw_unsup!(ReadExternStatic(def_id));`
`505`	`505`	`}`
`506`	`506`
`507`		`- (self.tcx.eval_static_initializer(def_id)?, Some(def_id))`
	`507`	`+ // Use a precise span for better cycle errors.`
	`508`	`+ (self.tcx.at(self.cur_span()).eval_static_initializer(def_id)?, Some(def_id))`
`508`	`509`	`}`
`509`	`510`	`};`
`510`	`511`	`M::before_access_global(*self.tcx, &self.machine, id, alloc, def_id, is_write)?;`
Original file line number	Diff line number	Diff line change
`@@ -5,10 +5,10 @@ LL \| static FOO: () = FOO;`
`5`	`5`	`\| ^^^^^^^^^^^^^^^^^^^^^`
`6`	`6`	`\|`
`7`	`7`	note: ...which requires const-evaluating + checking `FOO`...
`8`		`- --> $DIR/recursive-zst-static.rs:10:1`
	`8`	`+ --> $DIR/recursive-zst-static.rs:10:18`
`9`	`9`	`\|`
`10`	`10`	`LL \| static FOO: () = FOO;`
`11`		`- \| ^^^^^^^^^^^^^^^^^^^^^`
	`11`	`+ \| ^^^`
`12`	`12`	= note: ...which again requires const-evaluating + checking `FOO`, completing the cycle
`13`	`13`	`= note: cycle used when running analysis passes on this crate`
`14`	`14`