interpret: when passing an argument fails, point at that argument

Author: RalfJung

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -88,7 +88,7 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>(
         &ret.clone().into(),
         ReturnContinuation::Stop { cleanup: false },
     )?;
-    ecx.storage_live_for_always_live_locals()?;
+    ecx.push_stack_frame_done()?;
 
     // The main interpreter loop.
     while ecx.step()? {

compiler/rustc_const_eval/src/interpret/call.rs

@@ -404,166 +404,158 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
 
         // Push the "raw" frame -- this leaves locals uninitialized.
         self.push_stack_frame_raw(instance, body, destination, cont)?;
+        let preamble_span = self.frame().loc.unwrap_right(); // the span used for preamble errors
+
+        trace!(
+            "caller ABI: {:#?}, args: {:#?}",
+            caller_fn_abi,
+            args.iter()
+                .map(|arg| (
+                    arg.layout().ty,
+                    match arg {
+                        FnArg::Copy(op) => format!("copy({op:?})"),
+                        FnArg::InPlace(mplace) => format!("in-place({mplace:?})"),
+                    }
+                ))
+                .collect::<Vec<_>>()
+        );
+        trace!(
+            "spread_arg: {:?}, locals: {:#?}",
+            body.spread_arg,
+            body.args_iter()
+                .map(|local| (local, self.layout_of_local(self.frame(), local, None).unwrap().ty,))
+                .collect::<Vec<_>>()
+        );
 
-        // If an error is raised here, pop the frame again to get an accurate backtrace.
-        // To this end, we wrap it all in a `try` block.
-        let res: InterpResult<'tcx> = try {
-            trace!(
-                "caller ABI: {:#?}, args: {:#?}",
-                caller_fn_abi,
-                args.iter()
-                    .map(|arg| (
-                        arg.layout().ty,
-                        match arg {
-                            FnArg::Copy(op) => format!("copy({op:?})"),
-                            FnArg::InPlace(mplace) => format!("in-place({mplace:?})"),
-                        }
-                    ))
-                    .collect::<Vec<_>>()
-            );
-            trace!(
-                "spread_arg: {:?}, locals: {:#?}",
-                body.spread_arg,
-                body.args_iter()
-                    .map(|local| (
-                        local,
-                        self.layout_of_local(self.frame(), local, None).unwrap().ty,
-                    ))
-                    .collect::<Vec<_>>()
-            );
-
-            // In principle, we have two iterators: Where the arguments come from, and where
-            // they go to.
-
-            // 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; 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(),
-                "mismatch between caller ABI and caller arguments",
-            );
-            let mut caller_args = args
-                .iter()
-                .zip(caller_fn_abi.args.iter())
-                .filter(|arg_and_abi| !arg_and_abi.1.is_ignore());
-
-            // Now we have to spread them out across the callee's locals,
-            // taking into account the `spread_arg`. If we could write
-            // this is a single iterator (that handles `spread_arg`), then
-            // `pass_argument` would be the loop body. It takes care to
-            // not advance `caller_iter` for ignored arguments.
-            let mut callee_args_abis = callee_fn_abi.args.iter().enumerate();
-            // Determine whether there is a special VaList argument. This is always the
-            // last argument, and since arguments start at index 1 that's `arg_count`.
-            let va_list_arg =
-                callee_fn_abi.c_variadic.then(|| mir::Local::from_usize(body.arg_count));
-            for local in body.args_iter() {
-                // Construct the destination place for this argument. At this point all
-                // locals are still dead, so we cannot construct a `PlaceTy`.
-                let dest = mir::Place::from(local);
-                // `layout_of_local` does more than just the instantiation we need to get the
-                // type, but the result gets cached so this avoids calling the instantiation
-                // query *again* the next time this local is accessed.
-                let ty = self.layout_of_local(self.frame(), local, None)?.ty;
-                if Some(local) == va_list_arg {
-                    // This is the last callee-side argument of a variadic function.
-                    // This argument is a VaList holding the remaining caller-side arguments.
-                    self.storage_live(local)?;
-
-                    let place = self.eval_place(dest)?;
-                    let mplace = self.force_allocation(&place)?;
-
-                    // Consume the remaining arguments by putting them into the variable argument
-                    // list.
-                    let varargs = self.allocate_varargs(
-                        &mut caller_args,
-                        // "Ignored" arguments aren't actually passed, so the callee should also
-                        // ignore them. (`pass_argument` does this for regular arguments.)
-                        (&mut callee_args_abis).filter(|(_, abi)| !abi.is_ignore()),
-                    )?;
-                    // When the frame is dropped, these variable arguments are deallocated.
-                    self.frame_mut().va_list = varargs.clone();
-                    let key = self.va_list_ptr(varargs.into());
-
-                    // Zero the VaList, so it is fully initialized.
-                    self.write_bytes_ptr(
-                        mplace.ptr(),
-                        (0..mplace.layout.size.bytes()).map(|_| 0u8),
-                    )?;
+        // In principle, we have two iterators: Where the arguments come from, and where
+        // they go to.
 
-                    // Store the "key" pointer in the right field.
-                    let key_mplace = self.va_list_key_field(&mplace)?;
-                    self.write_pointer(key, &key_mplace)?;
-                } else if Some(local) == body.spread_arg {
-                    // Make the local live once, then fill in the value field by field.
-                    self.storage_live(local)?;
-                    // Must be a tuple
-                    let ty::Tuple(fields) = ty.kind() else {
-                        span_bug!(self.cur_span(), "non-tuple type for `spread_arg`: {ty}")
-                    };
-                    for (i, field_ty) in fields.iter().enumerate() {
-                        let dest = dest.project_deeper(
-                            &[mir::ProjectionElem::Field(FieldIdx::from_usize(i), field_ty)],
-                            *self.tcx,
-                        );
-                        let (idx, callee_abi) = callee_args_abis.next().unwrap();
-                        self.pass_argument(
-                            &mut caller_args,
-                            callee_abi,
-                            idx,
-                            &dest,
-                            field_ty,
-                            /* already_live */ true,
-                        )?;
-                    }
-                } else {
-                    // Normal argument. Cannot mark it as live yet, it might be unsized!
+        // 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; 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(),
+            "mismatch between caller ABI and caller arguments",
+        );
+        let mut caller_args = args
+            .iter()
+            .zip(caller_fn_abi.args.iter())
+            .filter(|arg_and_abi| !arg_and_abi.1.is_ignore());
+
+        // Now we have to spread them out across the callee's locals,
+        // taking into account the `spread_arg`. If we could write
+        // this is a single iterator (that handles `spread_arg`), then
+        // `pass_argument` would be the loop body. It takes care to
+        // not advance `caller_iter` for ignored arguments.
+        let mut callee_args_abis = callee_fn_abi.args.iter().enumerate();
+        // Determine whether there is a special VaList argument. This is always the
+        // last argument, and since arguments start at index 1 that's `arg_count`.
+        let va_list_arg = callee_fn_abi.c_variadic.then(|| mir::Local::from_usize(body.arg_count));
+        for local in body.args_iter() {
+            // Update the span that we show in case of an error to point to this argument.
+            self.frame_mut().loc = Right(body.local_decls[local].source_info.span);
+            // Construct the destination place for this argument. At this point all
+            // locals are still dead, so we cannot construct a `PlaceTy`.
+            let dest = mir::Place::from(local);
+            // `layout_of_local` does more than just the instantiation we need to get the
+            // type, but the result gets cached so this avoids calling the instantiation
+            // query *again* the next time this local is accessed.
+            let ty = self.layout_of_local(self.frame(), local, None)?.ty;
+            if Some(local) == va_list_arg {
+                // This is the last callee-side argument of a variadic function.
+                // This argument is a VaList holding the remaining caller-side arguments.
+                self.storage_live(local)?;
+
+                let place = self.eval_place(dest)?;
+                let mplace = self.force_allocation(&place)?;
+
+                // Consume the remaining arguments by putting them into the variable argument
+                // list.
+                let varargs = self.allocate_varargs(
+                    &mut caller_args,
+                    // "Ignored" arguments aren't actually passed, so the callee should also
+                    // ignore them. (`pass_argument` does this for regular arguments.)
+                    (&mut callee_args_abis).filter(|(_, abi)| !abi.is_ignore()),
+                )?;
+                // When the frame is dropped, these variable arguments are deallocated.
+                self.frame_mut().va_list = varargs.clone();
+                let key = self.va_list_ptr(varargs.into());
+
+                // Zero the VaList, so it is fully initialized.
+                self.write_bytes_ptr(mplace.ptr(), (0..mplace.layout.size.bytes()).map(|_| 0u8))?;
+
+                // Store the "key" pointer in the right field.
+                let key_mplace = self.va_list_key_field(&mplace)?;
+                self.write_pointer(key, &key_mplace)?;
+            } else if Some(local) == body.spread_arg {
+                // Make the local live once, then fill in the value field by field.
+                self.storage_live(local)?;
+                // Must be a tuple
+                let ty::Tuple(fields) = ty.kind() else {
+                    span_bug!(self.cur_span(), "non-tuple type for `spread_arg`: {ty}")
+                };
+                for (i, field_ty) in fields.iter().enumerate() {
+                    let dest = dest.project_deeper(
+                        &[mir::ProjectionElem::Field(FieldIdx::from_usize(i), field_ty)],
+                        *self.tcx,
+                    );
                     let (idx, callee_abi) = callee_args_abis.next().unwrap();
                     self.pass_argument(
                         &mut caller_args,
                         callee_abi,
                         idx,
                         &dest,
-                        ty,
-                        /* already_live */ false,
+                        field_ty,
+                        /* already_live */ true,
                     )?;
                 }
+            } else {
+                // Normal argument. Cannot mark it as live yet, it might be unsized!
+                let (idx, callee_abi) = callee_args_abis.next().unwrap();
+                self.pass_argument(
+                    &mut caller_args,
+                    callee_abi,
+                    idx,
+                    &dest,
+                    ty,
+                    /* already_live */ false,
+                )?;
             }
-            // If the callee needs a caller location, pretend we consume one more argument from the ABI.
-            if instance.def.requires_caller_location(*self.tcx) {
-                callee_args_abis.next().unwrap();
-            }
-            // Now we should have no more caller args or callee arg ABIs.
-            assert!(
-                callee_args_abis.next().is_none(),
-                "mismatch between callee ABI and callee body arguments"
-            );
-            if caller_args.next().is_some() {
-                throw_ub_format!("calling a function with more arguments than it expected");
-            }
-            // Don't forget to check the return type!
-            if !self.check_argument_compat(&caller_fn_abi.ret, &callee_fn_abi.ret)? {
-                throw_ub!(AbiMismatchReturn {
-                    caller_ty: caller_fn_abi.ret.layout.ty,
-                    callee_ty: callee_fn_abi.ret.layout.ty
-                });
-            }
+        }
 
-            // Protect return place for in-place return value passing.
-            // We only need to protect anything if this is actually an in-memory place.
-            if let Some(mplace) = destination_mplace {
-                M::protect_in_place_function_argument(self, &mplace)?;
-            }
+        // Don't forget to check the return type!
+        self.frame_mut().loc = Right(body.local_decls[mir::RETURN_PLACE].source_info.span);
+        if !self.check_argument_compat(&caller_fn_abi.ret, &callee_fn_abi.ret)? {
+            throw_ub!(AbiMismatchReturn {
+                caller_ty: caller_fn_abi.ret.layout.ty,
+                callee_ty: callee_fn_abi.ret.layout.ty
+            });
+        }
+        // Protect return place for in-place return value passing.
+        // We only need to protect anything if this is actually an in-memory place.
+        if let Some(mplace) = destination_mplace {
+            M::protect_in_place_function_argument(self, &mplace)?;
+        }
 
-            // Don't forget to mark "initially live" locals as live.
-            self.storage_live_for_always_live_locals()?;
-        };
-        res.inspect_err_kind(|_| {
-            // Don't show the incomplete stack frame in the error stacktrace.
-            self.stack_mut().pop();
-        })
+        // For the final checks, use same span as preamble since it is unclear what else to do.
+        self.frame_mut().loc = Right(preamble_span);
+        // If the callee needs a caller location, pretend we consume one more argument from the ABI.
+        if instance.def.requires_caller_location(*self.tcx) {
+            callee_args_abis.next().unwrap();
+        }
+        // Now we should have no more caller args or callee arg ABIs.
+        assert!(
+            callee_args_abis.next().is_none(),
+            "mismatch between callee ABI and callee body arguments"
+        );
+        if caller_args.next().is_some() {
+            throw_ub_format!("calling a function with more arguments than it expected");
+        }
+
+        // Done!
+        self.push_stack_frame_done()
     }
 
     /// Initiate a call to this function -- pushing the stack frame and initializing the arguments.

compiler/rustc_const_eval/src/interpret/stack.rs

@@ -381,7 +381,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         let locals = IndexVec::from_elem(dead_local, &body.local_decls);
         let pre_frame = Frame {
             body,
-            loc: Right(body.span), // Span used for errors caused during preamble.
+            loc: Right(self.tcx.def_span(body.source.def_id())), // Span used for errors caused during preamble.
             return_cont,
             return_place: return_place.clone(),
             locals,
@@ -408,7 +408,6 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
 
         // Finish things up.
         M::after_stack_push(self)?;
-        self.frame_mut().loc = Left(mir::Location::START);
         // `tracing_separate_thread` is used to instruct the tracing_chrome [tracing::Layer] in Miri
         // to put the "frame" span on a separate trace thread/line than other spans, to make the
         // visualization in <https://ui.perfetto.dev> easier to interpret. It is set to a value of
@@ -466,9 +465,11 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         }
     }
 
-    /// In the current stack frame, mark all locals as live that are not arguments and don't have
-    /// `Storage*` annotations (this includes the return place).
-    pub(crate) fn storage_live_for_always_live_locals(&mut self) -> InterpResult<'tcx> {
+    /// Call this after `push_stack_frame_raw` and when all the other setup that needs to be done
+    /// is completed.
+    pub(crate) fn push_stack_frame_done(&mut self) -> InterpResult<'tcx> {
+        // Mark all locals as live that are not arguments and don't have `Storage*` annotations
+        // (this includes the return place, but not the arguments).
         self.storage_live(mir::RETURN_PLACE)?;
 
         let body = self.body();
@@ -478,6 +479,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 self.storage_live(local)?;
             }
         }
+
+        // Get ready to execute the first instruction in the stack frame.
+        self.frame_mut().loc = Left(mir::Location::START);
+
         interp_ok(())
     }
 

compiler/rustc_const_eval/src/lib.rs

@@ -5,7 +5,6 @@
 #![feature(never_type)]
 #![feature(slice_ptr_get)]
 #![feature(trait_alias)]
-#![feature(try_blocks)]
 #![feature(unqualified_local_imports)]
 #![feature(yeet_expr)]
 #![warn(unqualified_local_imports)]

src/tools/miri/tests/fail-dep/concurrency/libc_pthread_create_too_few_args.rs

@@ -5,6 +5,7 @@
 use std::{mem, ptr};
 
 extern "C" fn thread_start() -> *mut libc::c_void {
+    //~^ERROR: calling a function with more arguments than it expected
     panic!()
 }
 
@@ -16,7 +17,6 @@ fn main() {
             mem::transmute(thread_start);
         assert_eq!(
             libc::pthread_create(&mut native, ptr::null(), thread_start, ptr::null_mut()),
-            //~^ERROR: calling a function with more arguments than it expected
             0
         );
         assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0);