Auto merge of rust-lang#123781 - RalfJung:miri-fn-identity, r=oli-obk

Miri function identity hack: account for possible inlining

Having a non-lifetime generic is not the only reason a function can be duplicated. Another possibility is that the function may be eligible for cross-crate inlining. So also take into account the inlining attribute in this Miri hack for function pointer identity.

That said, `cross_crate_inlinable` will still sometimes return true even for `inline(never)` functions:
- when they are `DefKind::Ctor(..) | DefKind::Closure` -- I assume those cannot be `InlineAttr::Never` anyway?
- when `cross_crate_inline_threshold == InliningThreshold::Always`

so maybe this is still not quite the right criterion to use for function pointer identity.

Author: bors

compiler/rustc_codegen_cranelift/src/constant.rs

@@ -155,7 +155,7 @@ pub(crate) fn codegen_const_value<'tcx>(
                             fx.bcx.ins().global_value(fx.pointer_type, local_data_id)
                         }
                     }
-                    GlobalAlloc::Function(instance) => {
+                    GlobalAlloc::Function { instance, .. } => {
                         let func_id = crate::abi::import_function(fx.tcx, fx.module, instance);
                         let local_func_id =
                             fx.module.declare_func_in_func(func_id, &mut fx.bcx.func);
@@ -351,7 +351,9 @@ fn define_all_allocs(tcx: TyCtxt<'_>, module: &mut dyn Module, cx: &mut Constant
             TodoItem::Alloc(alloc_id) => {
                 let alloc = match tcx.global_alloc(alloc_id) {
                     GlobalAlloc::Memory(alloc) => alloc,
-                    GlobalAlloc::Function(_) | GlobalAlloc::Static(_) | GlobalAlloc::VTable(..) => {
+                    GlobalAlloc::Function { .. }
+                    | GlobalAlloc::Static(_)
+                    | GlobalAlloc::VTable(..) => {
                         unreachable!()
                     }
                 };
@@ -415,7 +417,7 @@ fn define_all_allocs(tcx: TyCtxt<'_>, module: &mut dyn Module, cx: &mut Constant
 
             let reloc_target_alloc = tcx.global_alloc(alloc_id);
             let data_id = match reloc_target_alloc {
-                GlobalAlloc::Function(instance) => {
+                GlobalAlloc::Function { instance, .. } => {
                     assert_eq!(addend, 0);
                     let func_id =
                         crate::abi::import_function(tcx, module, instance.polymorphize(tcx));

compiler/rustc_codegen_gcc/src/common.rs

@@ -220,7 +220,7 @@ impl<'gcc, 'tcx> ConstMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
                         }
                         value
                     }
-                    GlobalAlloc::Function(fn_instance) => self.get_fn_addr(fn_instance),
+                    GlobalAlloc::Function { instance, .. } => self.get_fn_addr(instance),
                     GlobalAlloc::VTable(ty, trait_ref) => {
                         let alloc = self
                             .tcx

compiler/rustc_codegen_llvm/src/common.rs

@@ -289,8 +289,8 @@ impl<'ll, 'tcx> ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
                             (value, AddressSpace::DATA)
                         }
                     }
-                    GlobalAlloc::Function(fn_instance) => (
-                        self.get_fn_addr(fn_instance.polymorphize(self.tcx)),
+                    GlobalAlloc::Function { instance, .. } => (
+                        self.get_fn_addr(instance.polymorphize(self.tcx)),
                         self.data_layout().instruction_address_space,
                     ),
                     GlobalAlloc::VTable(ty, trait_ref) => {

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -308,7 +308,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         let Some((alloc_kind, mut alloc)) = self.memory.alloc_map.remove(&alloc_id) else {
             // Deallocating global memory -- always an error
             return Err(match self.tcx.try_get_global_alloc(alloc_id) {
-                Some(GlobalAlloc::Function(..)) => {
+                Some(GlobalAlloc::Function { .. }) => {
                     err_ub_custom!(
                         fluent::const_eval_invalid_dealloc,
                         alloc_id = alloc_id,
@@ -555,7 +555,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 // Memory of a constant or promoted or anonymous memory referenced by a static.
                 (mem, None)
             }
-            Some(GlobalAlloc::Function(..)) => throw_ub!(DerefFunctionPointer(id)),
+            Some(GlobalAlloc::Function { .. }) => throw_ub!(DerefFunctionPointer(id)),
             Some(GlobalAlloc::VTable(..)) => throw_ub!(DerefVTablePointer(id)),
             None => throw_ub!(PointerUseAfterFree(id, CheckInAllocMsg::MemoryAccessTest)),
             Some(GlobalAlloc::Static(def_id)) => {
@@ -828,7 +828,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 let alloc = alloc.inner();
                 (alloc.size(), alloc.align, AllocKind::LiveData)
             }
-            Some(GlobalAlloc::Function(_)) => bug!("We already checked function pointers above"),
+            Some(GlobalAlloc::Function { .. }) => {
+                bug!("We already checked function pointers above")
+            }
             Some(GlobalAlloc::VTable(..)) => {
                 // No data to be accessed here. But vtables are pointer-aligned.
                 return (Size::ZERO, self.tcx.data_layout.pointer_align.abi, AllocKind::VTable);
@@ -865,7 +867,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             Some(FnVal::Other(*extra))
         } else {
             match self.tcx.try_get_global_alloc(id) {
-                Some(GlobalAlloc::Function(instance)) => Some(FnVal::Instance(instance)),
+                Some(GlobalAlloc::Function { instance, .. }) => Some(FnVal::Instance(instance)),
                 _ => None,
             }
         }
@@ -1056,8 +1058,8 @@ impl<'a, 'tcx, M: Machine<'tcx>> std::fmt::Debug for DumpAllocs<'a, 'tcx, M> {
                                 alloc.inner(),
                             )?;
                         }
-                        Some(GlobalAlloc::Function(func)) => {
-                            write!(fmt, " (fn: {func})")?;
+                        Some(GlobalAlloc::Function { instance, .. }) => {
+                            write!(fmt, " (fn: {instance})")?;
                         }
                         Some(GlobalAlloc::VTable(ty, Some(trait_ref))) => {
                             write!(fmt, " (vtable: impl {trait_ref} for {ty})")?;

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -745,7 +745,7 @@ fn mutability<'tcx>(ecx: &InterpCx<'tcx, impl Machine<'tcx>>, alloc_id: AllocId)
             }
         }
         GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
-        GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
+        GlobalAlloc::Function { .. } | GlobalAlloc::VTable(..) => {
             // These are immutable, we better don't allow mutable pointers here.
             Mutability::Not
         }

compiler/rustc_middle/src/mir/interpret/mod.rs

@@ -18,6 +18,7 @@ use smallvec::{smallvec, SmallVec};
 use tracing::{debug, trace};
 
 use rustc_ast::LitKind;
+use rustc_attr::InlineAttr;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::sync::{HashMapExt, Lock};
 use rustc_errors::ErrorGuaranteed;
@@ -134,10 +135,11 @@ pub fn specialized_encode_alloc_id<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>>(
             AllocDiscriminant::Alloc.encode(encoder);
             alloc.encode(encoder);
         }
-        GlobalAlloc::Function(fn_instance) => {
-            trace!("encoding {:?} with {:#?}", alloc_id, fn_instance);
+        GlobalAlloc::Function { instance, unique } => {
+            trace!("encoding {:?} with {:#?}", alloc_id, instance);
             AllocDiscriminant::Fn.encode(encoder);
-            fn_instance.encode(encoder);
+            instance.encode(encoder);
+            unique.encode(encoder);
         }
         GlobalAlloc::VTable(ty, poly_trait_ref) => {
             trace!("encoding {:?} with {ty:#?}, {poly_trait_ref:#?}", alloc_id);
@@ -285,7 +287,12 @@ impl<'s> AllocDecodingSession<'s> {
                     trace!("creating fn alloc ID");
                     let instance = ty::Instance::decode(decoder);
                     trace!("decoded fn alloc instance: {:?}", instance);
-                    let alloc_id = decoder.interner().reserve_and_set_fn_alloc(instance);
+                    let unique = bool::decode(decoder);
+                    // Here we cannot call `reserve_and_set_fn_alloc` as that would use a query, which
+                    // is not possible in this context. That's why the allocation stores
+                    // whether it is unique or not.
+                    let alloc_id =
+                        decoder.interner().reserve_and_set_fn_alloc_internal(instance, unique);
                     alloc_id
                 }
                 AllocDiscriminant::VTable => {
@@ -323,7 +330,12 @@ impl<'s> AllocDecodingSession<'s> {
 #[derive(Debug, Clone, Eq, PartialEq, Hash, TyDecodable, TyEncodable, HashStable)]
 pub enum GlobalAlloc<'tcx> {
     /// The alloc ID is used as a function pointer.
-    Function(Instance<'tcx>),
+    Function {
+        instance: Instance<'tcx>,
+        /// Stores whether this instance is unique, i.e. all pointers to this function use the same
+        /// alloc ID.
+        unique: bool,
+    },
     /// This alloc ID points to a symbolic (not-reified) vtable.
     VTable(Ty<'tcx>, Option<ty::PolyExistentialTraitRef<'tcx>>),
     /// The alloc ID points to a "lazy" static variable that did not get computed (yet).
@@ -349,7 +361,7 @@ impl<'tcx> GlobalAlloc<'tcx> {
     #[inline]
     pub fn unwrap_fn(&self) -> Instance<'tcx> {
         match *self {
-            GlobalAlloc::Function(instance) => instance,
+            GlobalAlloc::Function { instance, .. } => instance,
             _ => bug!("expected function, got {:?}", self),
         }
     }
@@ -368,7 +380,7 @@ impl<'tcx> GlobalAlloc<'tcx> {
     #[inline]
     pub fn address_space(&self, cx: &impl HasDataLayout) -> AddressSpace {
         match self {
-            GlobalAlloc::Function(..) => cx.data_layout().instruction_address_space,
+            GlobalAlloc::Function { .. } => cx.data_layout().instruction_address_space,
             GlobalAlloc::Static(..) | GlobalAlloc::Memory(..) | GlobalAlloc::VTable(..) => {
                 AddressSpace::DATA
             }
@@ -426,7 +438,7 @@ impl<'tcx> TyCtxt<'tcx> {
     fn reserve_and_set_dedup(self, alloc: GlobalAlloc<'tcx>) -> AllocId {
         let mut alloc_map = self.alloc_map.lock();
         match alloc {
-            GlobalAlloc::Function(..) | GlobalAlloc::Static(..) | GlobalAlloc::VTable(..) => {}
+            GlobalAlloc::Function { .. } | GlobalAlloc::Static(..) | GlobalAlloc::VTable(..) => {}
             GlobalAlloc::Memory(..) => bug!("Trying to dedup-reserve memory with real data!"),
         }
         if let Some(&alloc_id) = alloc_map.dedup.get(&alloc) {
@@ -445,30 +457,45 @@ impl<'tcx> TyCtxt<'tcx> {
         self.reserve_and_set_dedup(GlobalAlloc::Static(static_id))
     }
 
+    /// Generates an `AllocId` for a function. The caller must already have decided whether this
+    /// function obtains a unique AllocId or gets de-duplicated via the cache.
+    fn reserve_and_set_fn_alloc_internal(self, instance: Instance<'tcx>, unique: bool) -> AllocId {
+        let alloc = GlobalAlloc::Function { instance, unique };
+        if unique {
+            // Deduplicate.
+            self.reserve_and_set_dedup(alloc)
+        } else {
+            // Get a fresh ID.
+            let mut alloc_map = self.alloc_map.lock();
+            let id = alloc_map.reserve();
+            alloc_map.alloc_map.insert(id, alloc);
+            id
+        }
+    }
+
     /// Generates an `AllocId` for a function. Depending on the function type,
     /// this might get deduplicated or assigned a new ID each time.
     pub fn reserve_and_set_fn_alloc(self, instance: Instance<'tcx>) -> AllocId {
         // Functions cannot be identified by pointers, as asm-equal functions can get deduplicated
         // by the linker (we set the "unnamed_addr" attribute for LLVM) and functions can be
-        // duplicated across crates.
-        // We thus generate a new `AllocId` for every mention of a function. This means that
-        // `main as fn() == main as fn()` is false, while `let x = main as fn(); x == x` is true.
-        // However, formatting code relies on function identity (see #58320), so we only do
-        // this for generic functions. Lifetime parameters are ignored.
+        // duplicated across crates. We thus generate a new `AllocId` for every mention of a
+        // function. This means that `main as fn() == main as fn()` is false, while `let x = main as
+        // fn(); x == x` is true. However, as a quality-of-life feature it can be useful to identify
+        // certain functions uniquely, e.g. for backtraces. So we identify whether codegen will
+        // actually emit duplicate functions. It does that when they have non-lifetime generics, or
+        // when they can be inlined. All other functions are given a unique address.
+        // This is not a stable guarantee! The `inline` attribute is a hint and cannot be relied
+        // upon for anything. But if we don't do this, backtraces look terrible.
         let is_generic = instance
             .args
             .into_iter()
             .any(|kind| !matches!(kind.unpack(), GenericArgKind::Lifetime(_)));
-        if is_generic {
-            // Get a fresh ID.
-            let mut alloc_map = self.alloc_map.lock();
-            let id = alloc_map.reserve();
-            alloc_map.alloc_map.insert(id, GlobalAlloc::Function(instance));
-            id
-        } else {
-            // Deduplicate.
-            self.reserve_and_set_dedup(GlobalAlloc::Function(instance))
-        }
+        let can_be_inlined = match self.codegen_fn_attrs(instance.def_id()).inline {
+            InlineAttr::Never => false,
+            _ => true,
+        };
+        let unique = !is_generic && !can_be_inlined;
+        self.reserve_and_set_fn_alloc_internal(instance, unique)
     }
 
     /// Generates an `AllocId` for a (symbolic, not-reified) vtable. Will get deduplicated.

compiler/rustc_middle/src/mir/pretty.rs

@@ -1449,7 +1449,7 @@ pub fn write_allocations<'tcx>(
             // This can't really happen unless there are bugs, but it doesn't cost us anything to
             // gracefully handle it and allow buggy rustc to be debugged via allocation printing.
             None => write!(w, " (deallocated)")?,
-            Some(GlobalAlloc::Function(inst)) => write!(w, " (fn: {inst})")?,
+            Some(GlobalAlloc::Function { instance, .. }) => write!(w, " (fn: {instance})")?,
             Some(GlobalAlloc::VTable(ty, Some(trait_ref))) => {
                 write!(w, " (vtable: impl {trait_ref} for {ty})")?
             }

compiler/rustc_middle/src/ty/print/pretty.rs

@@ -1667,7 +1667,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write {
                                 Some(GlobalAlloc::Static(def_id)) => {
                                     p!(write("<static({:?})>", def_id))
                                 }
-                                Some(GlobalAlloc::Function(_)) => p!("<function>"),
+                                Some(GlobalAlloc::Function { .. }) => p!("<function>"),
                                 Some(GlobalAlloc::VTable(..)) => p!("<vtable>"),
                                 None => p!("<dangling pointer>"),
                             }
@@ -1679,7 +1679,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write {
             ty::FnPtr(_) => {
                 // FIXME: We should probably have a helper method to share code with the "Byte strings"
                 // printing above (which also has to handle pointers to all sorts of things).
-                if let Some(GlobalAlloc::Function(instance)) =
+                if let Some(GlobalAlloc::Function { instance, .. }) =
                     self.tcx().try_get_global_alloc(prov.alloc_id())
                 {
                     self.typed_value(

compiler/rustc_monomorphize/src/collector.rs

@@ -1160,10 +1160,10 @@ fn collect_alloc<'tcx>(tcx: TyCtxt<'tcx>, alloc_id: AllocId, output: &mut MonoIt
                 });
             }
         }
-        GlobalAlloc::Function(fn_instance) => {
-            if should_codegen_locally(tcx, fn_instance) {
-                trace!("collecting {:?} with {:#?}", alloc_id, fn_instance);
-                output.push(create_fn_mono_item(tcx, fn_instance, DUMMY_SP));
+        GlobalAlloc::Function { instance, .. } => {
+            if should_codegen_locally(tcx, instance) {
+                trace!("collecting {:?} with {:#?}", alloc_id, instance);
+                output.push(create_fn_mono_item(tcx, instance, DUMMY_SP));
             }
         }
         GlobalAlloc::VTable(ty, trait_ref) => {

compiler/rustc_passes/src/reachable.rs

@@ -310,7 +310,7 @@ impl<'tcx> ReachableContext<'tcx> {
                 GlobalAlloc::Static(def_id) => {
                     self.propagate_item(Res::Def(self.tcx.def_kind(def_id), def_id))
                 }
-                GlobalAlloc::Function(instance) => {
+                GlobalAlloc::Function { instance, .. } => {
                     // Manually visit to actually see the instance's `DefId`. Type visitors won't see it
                     self.propagate_item(Res::Def(
                         self.tcx.def_kind(instance.def_id()),

compiler/rustc_smir/src/rustc_smir/convert/mir.rs

@@ -709,7 +709,7 @@ impl<'tcx> Stable<'tcx> for mir::interpret::GlobalAlloc<'tcx> {
 
     fn stable(&self, tables: &mut Tables<'_>) -> Self::T {
         match self {
-            mir::interpret::GlobalAlloc::Function(instance) => {
+            mir::interpret::GlobalAlloc::Function { instance, .. } => {
                 GlobalAlloc::Function(instance.stable(tables))
             }
             mir::interpret::GlobalAlloc::VTable(ty, trait_ref) => {

src/tools/miri/src/shims/backtrace.rs

@@ -119,7 +119,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let (alloc_id, offset, _prov) = this.ptr_get_alloc_id(ptr)?;
 
         // This has to be an actual global fn ptr, not a dlsym function.
-        let fn_instance = if let Some(GlobalAlloc::Function(instance)) =
+        let fn_instance = if let Some(GlobalAlloc::Function { instance, .. }) =
             this.tcx.try_get_global_alloc(alloc_id)
         {
             instance

src/tools/miri/tests/pass/function_pointers.rs

@@ -23,6 +23,7 @@ fn h(i: i32, j: i32) -> i32 {
     j * i * 7
 }
 
+#[inline(never)]
 fn i() -> i32 {
     73
 }
@@ -77,7 +78,7 @@ fn main() {
     assert_eq!(indirect_mut3(h), 210);
     assert_eq!(indirect_once3(h), 210);
     // Check that `i` always has the same address. This is not guaranteed
-    // but Miri currently uses a fixed address for monomorphic functions.
+    // but Miri currently uses a fixed address for non-inlineable monomorphic functions.
     assert!(return_fn_ptr(i) == i);
     assert!(return_fn_ptr(i) as unsafe fn() -> i32 == i as fn() -> i32 as unsafe fn() -> i32);
     // Miri gives different addresses to different reifications of a generic function.

src/tools/miri/tests/pass/issues/issue-91636.rs

@@ -10,6 +10,7 @@ impl Function {
     }
 }
 
+#[inline(never)]
 fn dummy(_: &str) {}
 
 fn main() {