various cleanups based on review

Author: RalfJung

compiler/rustc_const_eval/src/interpret/call.rs

@@ -418,7 +418,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             // The "where they come from" part is easy, we expect the caller to do any special handling
             // that might be required here (e.g. for untupling).
             // If `with_caller_location` is set we pretend there is an extra argument (that
-            // we will not pass).
+            // we will not pass; our `caller_location` intrinsic implementation walks the stack instead).
             assert_eq!(
                 args.len() + if with_caller_location { 1 } else { 0 },
                 caller_fn_abi.args.len(),
@@ -502,14 +502,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             // Don't forget to mark "initially live" locals as live.
             self.storage_live_for_always_live_locals()?;
         };
-        match res {
-            Err(err) => {
-                // Don't show the incomplete stack frame in the error stacktrace.
-                self.stack_mut().pop();
-                Err(err)
-            }
-            Ok(()) => Ok(()),
-        }
+        res.inspect_err(|_| {
+            // Don't show the incomplete stack frame in the error stacktrace.
+            self.stack_mut().pop();
+        })
     }
 
     /// Initiate a call to this function -- pushing the stack frame and initializing the arguments.
@@ -907,7 +903,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 .local_to_op(mir::RETURN_PLACE, None)
                 .expect("return place should always be live");
             let dest = self.frame().return_place.clone();
-            let err = if self.stack().len() == 1 {
+            let res = if self.stack().len() == 1 {
                 // The initializer of constants and statics will get validated separately
                 // after the constant has been fully evaluated. While we could fall back to the default
                 // code path, that will cause -Zenforce-validity to cycle on static initializers.
@@ -924,7 +920,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             // We delay actually short-circuiting on this error until *after* the stack frame is
             // popped, since we want this error to be attributed to the caller, whose type defines
             // this transmute.
-            err
+            res
         } else {
             Ok(())
         };
@@ -953,22 +949,23 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         // Normal return, figure out where to jump.
         if unwinding {
             // Follow the unwind edge.
-            let unwind = match stack_pop_info.return_to_block {
-                StackPopCleanup::Goto { unwind, .. } => unwind,
+            match stack_pop_info.return_to_block {
+                StackPopCleanup::Goto { unwind, .. } => {
+                    // This must be the very last thing that happens, since it can in fact push a new stack frame.
+                    self.unwind_to_block(unwind)
+                }
                 StackPopCleanup::Root { .. } => {
                     panic!("encountered StackPopCleanup::Root when unwinding!")
                 }
-            };
-            // This must be the very last thing that happens, since it can in fact push a new stack frame.
-            self.unwind_to_block(unwind)
+            }
         } else {
             // Follow the normal return edge.
             match stack_pop_info.return_to_block {
                 StackPopCleanup::Goto { ret, .. } => self.return_to_block(ret),
                 StackPopCleanup::Root { .. } => {
                     assert!(
                         self.stack().is_empty(),
-                        "only the topmost frame can have StackPopCleanup::Root"
+                        "only the bottommost frame can have StackPopCleanup::Root"
                     );
                     Ok(())
                 }

compiler/rustc_const_eval/src/interpret/stack.rs

@@ -264,7 +264,7 @@ impl<'tcx, Prov: Provenance, Extra> Frame<'tcx, Prov, Extra> {
     /// this frame (can happen e.g. during frame initialization, and during unwinding on
     /// frames without cleanup code).
     ///
-    /// Used by priroda.
+    /// Used by [priroda](https://github.com/oli-obk/priroda).
     pub fn current_loc(&self) -> Either<mir::Location, Span> {
         self.loc
     }
@@ -340,6 +340,8 @@ impl<'tcx, Prov: Provenance, Extra> Frame<'tcx, Prov, Extra> {
 impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     /// Very low-level helper that pushes a stack frame without initializing
     /// the arguments or local variables.
+    ///
+    /// The high-level version of this is `init_stack_frame`.
     #[instrument(skip(self, body, return_place, return_to_block), level = "debug")]
     pub(crate) fn push_stack_frame_raw(
         &mut self,
@@ -392,13 +394,16 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         Ok(())
     }
 
-    /// Pops a stack frame from the stack and returns some information about it.
+    /// Low-level helper that pops a stack frame from the stack and returns some information about
+    /// it.
     ///
     /// This also deallocates locals, if necessary.
     ///
     /// [`M::before_stack_pop`] should be called before calling this function.
     /// [`M::after_stack_pop`] is called by this function automatically.
     ///
+    /// The high-level version of this is `return_from_current_stack_frame`.
+    ///
     /// [`M::before_stack_pop`]: Machine::before_stack_pop
     /// [`M::after_stack_pop`]: Machine::after_stack_pop
     pub(super) fn pop_stack_frame_raw(

compiler/rustc_const_eval/src/interpret/step.rs

@@ -369,44 +369,38 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     }
 
     /// Evaluate the arguments of a function call
-    fn eval_fn_call_arguments(
+    fn eval_fn_call_argument(
         &self,
-        ops: &[Spanned<mir::Operand<'tcx>>],
-    ) -> InterpResult<'tcx, Vec<FnArg<'tcx, M::Provenance>>> {
-        ops.iter()
-            .map(|op| {
-                let arg = match &op.node {
-                    mir::Operand::Copy(_) | mir::Operand::Constant(_) => {
-                        // Make a regular copy.
-                        let op = self.eval_operand(&op.node, None)?;
+        op: &mir::Operand<'tcx>,
+    ) -> InterpResult<'tcx, FnArg<'tcx, M::Provenance>> {
+        Ok(match op {
+            mir::Operand::Copy(_) | mir::Operand::Constant(_) => {
+                // Make a regular copy.
+                let op = self.eval_operand(op, None)?;
+                FnArg::Copy(op)
+            }
+            mir::Operand::Move(place) => {
+                // If this place lives in memory, preserve its location.
+                // We call `place_to_op` which will be an `MPlaceTy` whenever there exists
+                // an mplace for this place. (This is in contrast to `PlaceTy::as_mplace_or_local`
+                // which can return a local even if that has an mplace.)
+                let place = self.eval_place(*place)?;
+                let op = self.place_to_op(&place)?;
+
+                match op.as_mplace_or_imm() {
+                    Either::Left(mplace) => FnArg::InPlace(mplace),
+                    Either::Right(_imm) => {
+                        // This argument doesn't live in memory, so there's no place
+                        // to make inaccessible during the call.
+                        // We rely on there not being any stray `PlaceTy` that would let the
+                        // caller directly access this local!
+                        // This is also crucial for tail calls, where we want the `FnArg` to
+                        // stay valid when the old stack frame gets popped.
                         FnArg::Copy(op)
                     }
-                    mir::Operand::Move(place) => {
-                        // If this place lives in memory, preserve its location.
-                        // We call `place_to_op` which will be an `MPlaceTy` whenever there exists
-                        // an mplace for this place. (This is in contrast to `PlaceTy::as_mplace_or_local`
-                        // which can return a local even if that has an mplace.)
-                        let place = self.eval_place(*place)?;
-                        let op = self.place_to_op(&place)?;
-
-                        match op.as_mplace_or_imm() {
-                            Either::Left(mplace) => FnArg::InPlace(mplace),
-                            Either::Right(_imm) => {
-                                // This argument doesn't live in memory, so there's no place
-                                // to make inaccessible during the call.
-                                // We rely on there not being any stray `PlaceTy` that would let the
-                                // caller directly access this local!
-                                // This is also crucial for tail calls, where we want the `FnArg` to
-                                // stay valid when the old stack frame gets popped.
-                                FnArg::Copy(op)
-                            }
-                        }
-                    }
-                };
-
-                Ok(arg)
-            })
-            .collect()
+                }
+            }
+        })
     }
 
     /// Shared part of `Call` and `TailCall` implementation — finding and evaluating all the
@@ -418,7 +412,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         args: &[Spanned<mir::Operand<'tcx>>],
     ) -> InterpResult<'tcx, EvaluatedCalleeAndArgs<'tcx, M>> {
         let func = self.eval_operand(func, None)?;
-        let args = self.eval_fn_call_arguments(args)?;
+        let args = args
+            .iter()
+            .map(|arg| self.eval_fn_call_argument(&arg.node))
+            .collect::<InterpResult<'tcx, Vec<_>>>()?;
 
         let fn_sig_binder = func.layout.ty.fn_sig(*self.tcx);
         let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, fn_sig_binder);