Rollup merge of #124715 - RalfJung:interpret-noreturn, r=compiler-errors

interpret, miri: uniform treatments of intrinsics/functions with and without return block

A long time ago we didn't have a `dest: &MPlaceTy<'tcx, Self::Provenance>` for diverging functions, and since `dest` is used so often we special-cased these non-returning intrinsics and functions so that we'd have `dest` available everywhere else. But this has changed a while ago, now only the return block `ret` is optional, and there's a convenient `return_to_block` function for dealing with the `None` case.

So there no longer is any reason to treat diverging intrinsics/functions any different from those that do return.

Author: matthiaskrgr

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -467,19 +467,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
         let intrinsic_name = ecx.tcx.item_name(instance.def_id());
 
         // CTFE-specific intrinsics.
-        let Some(ret) = target else {
-            // Handle diverging intrinsics. We can't handle any of them (that are not already
-            // handled above), but check if there is a fallback body.
-            if ecx.tcx.intrinsic(instance.def_id()).unwrap().must_be_overridden {
-                throw_unsup_format!(
-                    "intrinsic `{intrinsic_name}` is not supported at compile-time"
-                );
-            }
-            return Ok(Some(ty::Instance {
-                def: ty::InstanceDef::Item(instance.def_id()),
-                args: instance.args,
-            }));
-        };
         match intrinsic_name {
             sym::ptr_guaranteed_cmp => {
                 let a = ecx.read_scalar(&args[0])?;
@@ -559,7 +546,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
             }
         }
 
-        ecx.go_to_block(ret);
+        // Intrinsic is done, jump to next block.
+        ecx.return_to_block(target)?;
         Ok(None)
     }
 

compiler/rustc_const_eval/src/interpret/intrinsics.rs

@@ -113,10 +113,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     ) -> InterpResult<'tcx, bool> {
         let instance_args = instance.args;
         let intrinsic_name = self.tcx.item_name(instance.def_id());
-        let Some(ret) = ret else {
-            // We don't support any intrinsic without return place.
-            return Ok(false);
-        };
 
         match intrinsic_name {
             sym::caller_location => {
@@ -376,7 +372,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     };
 
                     M::panic_nounwind(self, &msg)?;
-                    // Skip the `go_to_block` at the end.
+                    // Skip the `return_to_block` at the end (we panicked, we do not return).
                     return Ok(true);
                 }
             }
@@ -437,11 +433,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 self.write_scalar(Scalar::from_target_usize(align.bytes(), self), dest)?;
             }
 
+            // Unsupported intrinsic: skip the return_to_block below.
             _ => return Ok(false),
         }
 
         trace!("{:?}", self.dump_place(&dest.clone().into()));
-        self.go_to_block(ret);
+        self.return_to_block(ret)?;
         Ok(true)
     }
 

src/tools/miri/src/lib.rs

@@ -95,6 +95,7 @@ pub use rustc_const_eval::interpret::*;
 #[doc(no_inline)]
 pub use rustc_const_eval::interpret::{self, AllocMap, PlaceTy, Provenance as _};
 
+pub use crate::shims::EmulateItemResult;
 pub use crate::shims::env::{EnvVars, EvalContextExt as _};
 pub use crate::shims::foreign_items::{DynSym, EvalContextExt as _};
 pub use crate::shims::intrinsics::EvalContextExt as _;

src/tools/miri/src/shims/alloc.rs

@@ -4,7 +4,6 @@ use rustc_ast::expand::allocator::AllocatorKind;
 use rustc_target::abi::{Align, Size};
 
 use crate::*;
-use shims::foreign_items::EmulateForeignItemResult;
 
 /// Check some basic requirements for this allocation request:
 /// non-zero size, power-of-two alignment.
@@ -55,12 +54,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     fn emulate_allocator(
         &mut self,
         default: impl FnOnce(&mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx>,
-    ) -> InterpResult<'tcx, EmulateForeignItemResult> {
+    ) -> InterpResult<'tcx, EmulateItemResult> {
         let this = self.eval_context_mut();
 
         let Some(allocator_kind) = this.tcx.allocator_kind(()) else {
             // in real code, this symbol does not exist without an allocator
-            return Ok(EmulateForeignItemResult::NotSupported);
+            return Ok(EmulateItemResult::NotSupported);
         };
 
         match allocator_kind {
@@ -70,11 +69,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 // and not execute any Miri shim. Somewhat unintuitively doing so is done
                 // by returning `NotSupported`, which triggers the `lookup_exported_symbol`
                 // fallback case in `emulate_foreign_item`.
-                return Ok(EmulateForeignItemResult::NotSupported);
+                return Ok(EmulateItemResult::NotSupported);
             }
             AllocatorKind::Default => {
                 default(this)?;
-                Ok(EmulateForeignItemResult::NeedsJumping)
+                Ok(EmulateItemResult::NeedsJumping)
             }
         }
     }

src/tools/miri/src/shims/foreign_items.rs

@@ -28,16 +28,6 @@ impl DynSym {
     }
 }
 
-/// Returned by `emulate_foreign_item_inner`.
-pub enum EmulateForeignItemResult {
-    /// The caller is expected to jump to the return block.
-    NeedsJumping,
-    /// Jumping has already been taken care of.
-    AlreadyJumped,
-    /// The item is not supported.
-    NotSupported,
-}
-
 impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     /// Emulates calling a foreign item, failing if the item is not supported.
@@ -58,84 +48,47 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let this = self.eval_context_mut();
         let tcx = this.tcx.tcx;
 
-        // First: functions that diverge.
-        let ret = match ret {
-            None =>
-                match link_name.as_str() {
-                    "miri_start_unwind" => {
-                        // `check_shim` happens inside `handle_miri_start_unwind`.
-                        this.handle_miri_start_unwind(abi, link_name, args, unwind)?;
-                        return Ok(None);
-                    }
-                    // This matches calls to the foreign item `panic_impl`.
-                    // The implementation is provided by the function with the `#[panic_handler]` attribute.
-                    "panic_impl" => {
-                        // We don't use `check_shim` here because we are just forwarding to the lang
-                        // item. Argument count checking will be performed when the returned `Body` is
-                        // called.
-                        this.check_abi_and_shim_symbol_clash(abi, Abi::Rust, link_name)?;
-                        let panic_impl_id = tcx.lang_items().panic_impl().unwrap();
-                        let panic_impl_instance = ty::Instance::mono(tcx, panic_impl_id);
-                        return Ok(Some((
-                            this.load_mir(panic_impl_instance.def, None)?,
-                            panic_impl_instance,
-                        )));
-                    }
-                    "__rust_alloc_error_handler" => {
-                        // Forward to the right symbol that implements this function.
-                        let Some(handler_kind) = this.tcx.alloc_error_handler_kind(()) else {
-                            // in real code, this symbol does not exist without an allocator
-                            throw_unsup_format!(
-                                "`__rust_alloc_error_handler` cannot be called when no alloc error handler is set"
-                            );
-                        };
-                        let name = alloc_error_handler_name(handler_kind);
-                        let handler = this
-                            .lookup_exported_symbol(Symbol::intern(name))?
-                            .expect("missing alloc error handler symbol");
-                        return Ok(Some(handler));
-                    }
-                    #[rustfmt::skip]
-                    | "exit"
-                    | "ExitProcess"
-                    => {
-                        let exp_abi = if link_name.as_str() == "exit" {
-                            Abi::C { unwind: false }
-                        } else {
-                            Abi::System { unwind: false }
-                        };
-                        let [code] = this.check_shim(abi, exp_abi, link_name, args)?;
-                        // it's really u32 for ExitProcess, but we have to put it into the `Exit` variant anyway
-                        let code = this.read_scalar(code)?.to_i32()?;
-                        throw_machine_stop!(TerminationInfo::Exit { code: code.into(), leak_check: false });
-                    }
-                    "abort" => {
-                        let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
-                        throw_machine_stop!(TerminationInfo::Abort(
-                            "the program aborted execution".to_owned()
-                        ))
-                    }
-                    _ => {
-                        if let Some(body) = this.lookup_exported_symbol(link_name)? {
-                            return Ok(Some(body));
-                        }
-                        this.handle_unsupported(format!(
-                            "can't call (diverging) foreign function: {link_name}"
-                        ))?;
-                        return Ok(None);
-                    }
-                },
-            Some(p) => p,
-        };
+        // Some shims forward to other MIR bodies.
+        match link_name.as_str() {
+            // This matches calls to the foreign item `panic_impl`.
+            // The implementation is provided by the function with the `#[panic_handler]` attribute.
+            "panic_impl" => {
+                // We don't use `check_shim` here because we are just forwarding to the lang
+                // item. Argument count checking will be performed when the returned `Body` is
+                // called.
+                this.check_abi_and_shim_symbol_clash(abi, Abi::Rust, link_name)?;
+                let panic_impl_id = tcx.lang_items().panic_impl().unwrap();
+                let panic_impl_instance = ty::Instance::mono(tcx, panic_impl_id);
+                return Ok(Some((
+                    this.load_mir(panic_impl_instance.def, None)?,
+                    panic_impl_instance,
+                )));
+            }
+            "__rust_alloc_error_handler" => {
+                // Forward to the right symbol that implements this function.
+                let Some(handler_kind) = this.tcx.alloc_error_handler_kind(()) else {
+                    // in real code, this symbol does not exist without an allocator
+                    throw_unsup_format!(
+                        "`__rust_alloc_error_handler` cannot be called when no alloc error handler is set"
+                    );
+                };
+                let name = alloc_error_handler_name(handler_kind);
+                let handler = this
+                    .lookup_exported_symbol(Symbol::intern(name))?
+                    .expect("missing alloc error handler symbol");
+                return Ok(Some(handler));
+            }
+            _ => {}
+        }
 
-        // Second: functions that return immediately.
-        match this.emulate_foreign_item_inner(link_name, abi, args, dest)? {
-            EmulateForeignItemResult::NeedsJumping => {
+        // The rest either implements the logic, or falls back to `lookup_exported_symbol`.
+        match this.emulate_foreign_item_inner(link_name, abi, args, dest, unwind)? {
+            EmulateItemResult::NeedsJumping => {
                 trace!("{:?}", this.dump_place(&dest.clone().into()));
-                this.go_to_block(ret);
+                this.return_to_block(ret)?;
             }
-            EmulateForeignItemResult::AlreadyJumped => (),
-            EmulateForeignItemResult::NotSupported => {
+            EmulateItemResult::AlreadyJumped => (),
+            EmulateItemResult::NotSupported => {
                 if let Some(body) = this.lookup_exported_symbol(link_name)? {
                     return Ok(Some(body));
                 }
@@ -243,7 +196,8 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         abi: Abi,
         args: &[OpTy<'tcx, Provenance>],
         dest: &MPlaceTy<'tcx, Provenance>,
-    ) -> InterpResult<'tcx, EmulateForeignItemResult> {
+        unwind: mir::UnwindAction,
+    ) -> InterpResult<'tcx, EmulateItemResult> {
         let this = self.eval_context_mut();
 
         // First deal with any external C functions in linked .so file.
@@ -254,7 +208,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             // by the specified `.so` file; we should continue and check if it corresponds to
             // a provided shim.
             if this.call_external_c_fct(link_name, dest, args)? {
-                return Ok(EmulateForeignItemResult::NeedsJumping);
+                return Ok(EmulateItemResult::NeedsJumping);
             }
         }
 
@@ -298,6 +252,11 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         // shim, add it to the corresponding submodule.
         match link_name.as_str() {
             // Miri-specific extern functions
+            "miri_start_unwind" => {
+                // `check_shim` happens inside `handle_miri_start_unwind`.
+                this.handle_miri_start_unwind(abi, link_name, args, unwind)?;
+                return Ok(EmulateItemResult::AlreadyJumped);
+            }
             "miri_run_provenance_gc" => {
                 let [] = this.check_shim(abi, Abi::Rust, link_name, args)?;
                 this.run_provenance_gc();
@@ -362,29 +321,24 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 // Return value: 0 on success, otherwise the size it would have needed.
                 this.write_int(if success { 0 } else { needed_size }, dest)?;
             }
-
             // Obtains the size of a Miri backtrace. See the README for details.
             "miri_backtrace_size" => {
                 this.handle_miri_backtrace_size(abi, link_name, args, dest)?;
             }
-
             // Obtains a Miri backtrace. See the README for details.
             "miri_get_backtrace" => {
                 // `check_shim` happens inside `handle_miri_get_backtrace`.
                 this.handle_miri_get_backtrace(abi, link_name, args, dest)?;
             }
-
             // Resolves a Miri backtrace frame. See the README for details.
             "miri_resolve_frame" => {
                 // `check_shim` happens inside `handle_miri_resolve_frame`.
                 this.handle_miri_resolve_frame(abi, link_name, args, dest)?;
             }
-
             // Writes the function and file names of a Miri backtrace frame into a user provided buffer. See the README for details.
             "miri_resolve_frame_names" => {
                 this.handle_miri_resolve_frame_names(abi, link_name, args)?;
             }
-
             // Writes some bytes to the interpreter's stdout/stderr. See the
             // README for details.
             "miri_write_to_stdout" | "miri_write_to_stderr" => {
@@ -398,7 +352,6 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     _ => unreachable!(),
                 };
             }
-
             // Promises that a pointer has a given symbolic alignment.
             "miri_promise_symbolic_alignment" => {
                 use rustc_target::abi::AlignFromBytesError;
@@ -442,6 +395,25 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 }
             }
 
+            // Aborting the process.
+            "exit" | "ExitProcess" => {
+                let exp_abi = if link_name.as_str() == "exit" {
+                    Abi::C { unwind: false }
+                } else {
+                    Abi::System { unwind: false }
+                };
+                let [code] = this.check_shim(abi, exp_abi, link_name, args)?;
+                // it's really u32 for ExitProcess, but we have to put it into the `Exit` variant anyway
+                let code = this.read_scalar(code)?.to_i32()?;
+                throw_machine_stop!(TerminationInfo::Exit { code: code.into(), leak_check: false });
+            }
+            "abort" => {
+                let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
+                throw_machine_stop!(TerminationInfo::Abort(
+                    "the program aborted execution".to_owned()
+                ))
+            }
+
             // Standard C allocation
             "malloc" => {
                 let [size] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
@@ -504,7 +476,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     "__rust_alloc" => return this.emulate_allocator(default),
                     "miri_alloc" => {
                         default(this)?;
-                        return Ok(EmulateForeignItemResult::NeedsJumping);
+                        return Ok(EmulateItemResult::NeedsJumping);
                     }
                     _ => unreachable!(),
                 }
@@ -564,7 +536,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     }
                     "miri_dealloc" => {
                         default(this)?;
-                        return Ok(EmulateForeignItemResult::NeedsJumping);
+                        return Ok(EmulateItemResult::NeedsJumping);
                     }
                     _ => unreachable!(),
                 }
@@ -966,11 +938,11 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                         shims::windows::foreign_items::EvalContextExt::emulate_foreign_item_inner(
                             this, link_name, abi, args, dest,
                         ),
-                    _ => Ok(EmulateForeignItemResult::NotSupported),
+                    _ => Ok(EmulateItemResult::NotSupported),
                 },
         };
         // We only fall through to here if we did *not* hit the `_` arm above,
         // i.e., if we actually emulated the function with one of the shims.
-        Ok(EmulateForeignItemResult::NeedsJumping)
+        Ok(EmulateItemResult::NeedsJumping)
     }
 }

src/tools/miri/src/shims/intrinsics/atomic.rs

@@ -20,7 +20,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         intrinsic_name: &str,
         args: &[OpTy<'tcx, Provenance>],
         dest: &MPlaceTy<'tcx, Provenance>,
-    ) -> InterpResult<'tcx, bool> {
+    ) -> InterpResult<'tcx, EmulateItemResult> {
         let this = self.eval_context_mut();
 
         let intrinsic_structure: Vec<_> = intrinsic_name.split('_').collect();
@@ -114,9 +114,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 this.atomic_rmw_op(args, dest, AtomicOp::Max, rw_ord(ord)?)?;
             }
 
-            _ => return Ok(false),
+            _ => return Ok(EmulateItemResult::NotSupported),
         }
-        Ok(true)
+        Ok(EmulateItemResult::NeedsJumping)
     }
 }