Skip to content

Commit 41b98da

Browse files
RalfJungRalfJung
committed
Miri function identity hack: account for possible inlining
1 parent 7d97c59 commit 41b98da

File tree

14 files changed

+80
-47
lines changed

14 files changed

+80
-47
lines changed

Diff for: ‎compiler/rustc_codegen_cranelift/src/constant.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ pub(crate) fn codegen_const_value<'tcx>(
155155
fx.bcx.ins().global_value(fx.pointer_type, local_data_id)
156156
}
157157
}
158-
GlobalAlloc::Function(instance) => {
158+
GlobalAlloc::Function { instance, .. } => {
159159
let func_id = crate::abi::import_function(fx.tcx, fx.module, instance);
160160
let local_func_id =
161161
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
351351
TodoItem::Alloc(alloc_id) => {
352352
let alloc = match tcx.global_alloc(alloc_id) {
353353
GlobalAlloc::Memory(alloc) => alloc,
354-
GlobalAlloc::Function(_) | GlobalAlloc::Static(_) | GlobalAlloc::VTable(..) => {
354+
GlobalAlloc::Function { .. }
355+
| GlobalAlloc::Static(_)
356+
| GlobalAlloc::VTable(..) => {
355357
unreachable!()
356358
}
357359
};
@@ -415,7 +417,7 @@ fn define_all_allocs(tcx: TyCtxt<'_>, module: &mut dyn Module, cx: &mut Constant
415417

416418
let reloc_target_alloc = tcx.global_alloc(alloc_id);
417419
let data_id = match reloc_target_alloc {
418-
GlobalAlloc::Function(instance) => {
420+
GlobalAlloc::Function { instance, .. } => {
419421
assert_eq!(addend, 0);
420422
let func_id =
421423
crate::abi::import_function(tcx, module, instance.polymorphize(tcx));

Diff for: ‎compiler/rustc_codegen_gcc/src/common.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ impl<'gcc, 'tcx> ConstMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
220220
}
221221
value
222222
}
223-
GlobalAlloc::Function(fn_instance) => self.get_fn_addr(fn_instance),
223+
GlobalAlloc::Function { instance, .. } => self.get_fn_addr(instance),
224224
GlobalAlloc::VTable(ty, trait_ref) => {
225225
let alloc = self
226226
.tcx

Diff for: ‎compiler/rustc_codegen_llvm/src/common.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -289,8 +289,8 @@ impl<'ll, 'tcx> ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
289289
(value, AddressSpace::DATA)
290290
}
291291
}
292-
GlobalAlloc::Function(fn_instance) => (
293-
self.get_fn_addr(fn_instance.polymorphize(self.tcx)),
292+
GlobalAlloc::Function { instance, .. } => (
293+
self.get_fn_addr(instance.polymorphize(self.tcx)),
294294
self.data_layout().instruction_address_space,
295295
),
296296
GlobalAlloc::VTable(ty, trait_ref) => {

Diff for: ‎compiler/rustc_const_eval/src/interpret/memory.rs

+8-6
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
308308
let Some((alloc_kind, mut alloc)) = self.memory.alloc_map.remove(&alloc_id) else {
309309
// Deallocating global memory -- always an error
310310
return Err(match self.tcx.try_get_global_alloc(alloc_id) {
311-
Some(GlobalAlloc::Function(..)) => {
311+
Some(GlobalAlloc::Function { .. }) => {
312312
err_ub_custom!(
313313
fluent::const_eval_invalid_dealloc,
314314
alloc_id = alloc_id,
@@ -555,7 +555,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
555555
// Memory of a constant or promoted or anonymous memory referenced by a static.
556556
(mem, None)
557557
}
558-
Some(GlobalAlloc::Function(..)) => throw_ub!(DerefFunctionPointer(id)),
558+
Some(GlobalAlloc::Function { .. }) => throw_ub!(DerefFunctionPointer(id)),
559559
Some(GlobalAlloc::VTable(..)) => throw_ub!(DerefVTablePointer(id)),
560560
None => throw_ub!(PointerUseAfterFree(id, CheckInAllocMsg::MemoryAccessTest)),
561561
Some(GlobalAlloc::Static(def_id)) => {
@@ -828,7 +828,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
828828
let alloc = alloc.inner();
829829
(alloc.size(), alloc.align, AllocKind::LiveData)
830830
}
831-
Some(GlobalAlloc::Function(_)) => bug!("We already checked function pointers above"),
831+
Some(GlobalAlloc::Function { .. }) => {
832+
bug!("We already checked function pointers above")
833+
}
832834
Some(GlobalAlloc::VTable(..)) => {
833835
// No data to be accessed here. But vtables are pointer-aligned.
834836
return (Size::ZERO, self.tcx.data_layout.pointer_align.abi, AllocKind::VTable);
@@ -865,7 +867,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
865867
Some(FnVal::Other(*extra))
866868
} else {
867869
match self.tcx.try_get_global_alloc(id) {
868-
Some(GlobalAlloc::Function(instance)) => Some(FnVal::Instance(instance)),
870+
Some(GlobalAlloc::Function { instance, .. }) => Some(FnVal::Instance(instance)),
869871
_ => None,
870872
}
871873
}
@@ -1056,8 +1058,8 @@ impl<'a, 'tcx, M: Machine<'tcx>> std::fmt::Debug for DumpAllocs<'a, 'tcx, M> {
10561058
alloc.inner(),
10571059
)?;
10581060
}
1059-
Some(GlobalAlloc::Function(func)) => {
1060-
write!(fmt, " (fn: {func})")?;
1061+
Some(GlobalAlloc::Function { instance, .. }) => {
1062+
write!(fmt, " (fn: {instance})")?;
10611063
}
10621064
Some(GlobalAlloc::VTable(ty, Some(trait_ref))) => {
10631065
write!(fmt, " (vtable: impl {trait_ref} for {ty})")?;

Diff for: ‎compiler/rustc_const_eval/src/interpret/validity.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -745,7 +745,7 @@ fn mutability<'tcx>(ecx: &InterpCx<'tcx, impl Machine<'tcx>>, alloc_id: AllocId)
745745
}
746746
}
747747
GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
748-
GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
748+
GlobalAlloc::Function { .. } | GlobalAlloc::VTable(..) => {
749749
// These are immutable, we better don't allow mutable pointers here.
750750
Mutability::Not
751751
}

Diff for: ‎compiler/rustc_middle/src/mir/interpret/mod.rs

+50-23
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ use smallvec::{smallvec, SmallVec};
1818
use tracing::{debug, trace};
1919

2020
use rustc_ast::LitKind;
21+
use rustc_attr::InlineAttr;
2122
use rustc_data_structures::fx::FxHashMap;
2223
use rustc_data_structures::sync::{HashMapExt, Lock};
2324
use rustc_errors::ErrorGuaranteed;
@@ -134,10 +135,11 @@ pub fn specialized_encode_alloc_id<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>>(
134135
AllocDiscriminant::Alloc.encode(encoder);
135136
alloc.encode(encoder);
136137
}
137-
GlobalAlloc::Function(fn_instance) => {
138-
trace!("encoding {:?} with {:#?}", alloc_id, fn_instance);
138+
GlobalAlloc::Function { instance, unique } => {
139+
trace!("encoding {:?} with {:#?}", alloc_id, instance);
139140
AllocDiscriminant::Fn.encode(encoder);
140-
fn_instance.encode(encoder);
141+
instance.encode(encoder);
142+
unique.encode(encoder);
141143
}
142144
GlobalAlloc::VTable(ty, poly_trait_ref) => {
143145
trace!("encoding {:?} with {ty:#?}, {poly_trait_ref:#?}", alloc_id);
@@ -285,7 +287,12 @@ impl<'s> AllocDecodingSession<'s> {
285287
trace!("creating fn alloc ID");
286288
let instance = ty::Instance::decode(decoder);
287289
trace!("decoded fn alloc instance: {:?}", instance);
288-
let alloc_id = decoder.interner().reserve_and_set_fn_alloc(instance);
290+
let unique = bool::decode(decoder);
291+
// Here we cannot call `reserve_and_set_fn_alloc` as that would use a query, which
292+
// is not possible in this context. That's why the allocation stores
293+
// whether it is unique or not.
294+
let alloc_id =
295+
decoder.interner().reserve_and_set_fn_alloc_internal(instance, unique);
289296
alloc_id
290297
}
291298
AllocDiscriminant::VTable => {
@@ -323,7 +330,12 @@ impl<'s> AllocDecodingSession<'s> {
323330
#[derive(Debug, Clone, Eq, PartialEq, Hash, TyDecodable, TyEncodable, HashStable)]
324331
pub enum GlobalAlloc<'tcx> {
325332
/// The alloc ID is used as a function pointer.
326-
Function(Instance<'tcx>),
333+
Function {
334+
instance: Instance<'tcx>,
335+
/// Stores whether this instance is unique, i.e. all pointers to this function use the same
336+
/// alloc ID.
337+
unique: bool,
338+
},
327339
/// This alloc ID points to a symbolic (not-reified) vtable.
328340
VTable(Ty<'tcx>, Option<ty::PolyExistentialTraitRef<'tcx>>),
329341
/// The alloc ID points to a "lazy" static variable that did not get computed (yet).
@@ -349,7 +361,7 @@ impl<'tcx> GlobalAlloc<'tcx> {
349361
#[inline]
350362
pub fn unwrap_fn(&self) -> Instance<'tcx> {
351363
match *self {
352-
GlobalAlloc::Function(instance) => instance,
364+
GlobalAlloc::Function { instance, .. } => instance,
353365
_ => bug!("expected function, got {:?}", self),
354366
}
355367
}
@@ -368,7 +380,7 @@ impl<'tcx> GlobalAlloc<'tcx> {
368380
#[inline]
369381
pub fn address_space(&self, cx: &impl HasDataLayout) -> AddressSpace {
370382
match self {
371-
GlobalAlloc::Function(..) => cx.data_layout().instruction_address_space,
383+
GlobalAlloc::Function { .. } => cx.data_layout().instruction_address_space,
372384
GlobalAlloc::Static(..) | GlobalAlloc::Memory(..) | GlobalAlloc::VTable(..) => {
373385
AddressSpace::DATA
374386
}
@@ -426,7 +438,7 @@ impl<'tcx> TyCtxt<'tcx> {
426438
fn reserve_and_set_dedup(self, alloc: GlobalAlloc<'tcx>) -> AllocId {
427439
let mut alloc_map = self.alloc_map.lock();
428440
match alloc {
429-
GlobalAlloc::Function(..) | GlobalAlloc::Static(..) | GlobalAlloc::VTable(..) => {}
441+
GlobalAlloc::Function { .. } | GlobalAlloc::Static(..) | GlobalAlloc::VTable(..) => {}
430442
GlobalAlloc::Memory(..) => bug!("Trying to dedup-reserve memory with real data!"),
431443
}
432444
if let Some(&alloc_id) = alloc_map.dedup.get(&alloc) {
@@ -445,30 +457,45 @@ impl<'tcx> TyCtxt<'tcx> {
445457
self.reserve_and_set_dedup(GlobalAlloc::Static(static_id))
446458
}
447459

460+
/// Generates an `AllocId` for a function. The caller must already have decided whether this
461+
/// function obtains a unique AllocId or gets de-duplicated via the cache.
462+
fn reserve_and_set_fn_alloc_internal(self, instance: Instance<'tcx>, unique: bool) -> AllocId {
463+
let alloc = GlobalAlloc::Function { instance, unique };
464+
if unique {
465+
// Deduplicate.
466+
self.reserve_and_set_dedup(alloc)
467+
} else {
468+
// Get a fresh ID.
469+
let mut alloc_map = self.alloc_map.lock();
470+
let id = alloc_map.reserve();
471+
alloc_map.alloc_map.insert(id, alloc);
472+
id
473+
}
474+
}
475+
448476
/// Generates an `AllocId` for a function. Depending on the function type,
449477
/// this might get deduplicated or assigned a new ID each time.
450478
pub fn reserve_and_set_fn_alloc(self, instance: Instance<'tcx>) -> AllocId {
451479
// Functions cannot be identified by pointers, as asm-equal functions can get deduplicated
452480
// by the linker (we set the "unnamed_addr" attribute for LLVM) and functions can be
453-
// duplicated across crates.
454-
// We thus generate a new `AllocId` for every mention of a function. This means that
455-
// `main as fn() == main as fn()` is false, while `let x = main as fn(); x == x` is true.
456-
// However, formatting code relies on function identity (see #58320), so we only do
457-
// this for generic functions. Lifetime parameters are ignored.
481+
// duplicated across crates. We thus generate a new `AllocId` for every mention of a
482+
// function. This means that `main as fn() == main as fn()` is false, while `let x = main as
483+
// fn(); x == x` is true. However, as a quality-of-life feature it can be useful to identify
484+
// certain functions uniquely, e.g. for backtraces. So we identify whether codegen will
485+
// actually emit duplicate functions. It does that when they have non-lifetime generics, or
486+
// when they can be inlined. All other functions are given a unique address.
487+
// This is not a stable guarantee! The `inline` attribute is a hint and cannot be relied
488+
// upon for anything. But if we don't do this, backtraces look terrible.
458489
let is_generic = instance
459490
.args
460491
.into_iter()
461492
.any(|kind| !matches!(kind.unpack(), GenericArgKind::Lifetime(_)));
462-
if is_generic {
463-
// Get a fresh ID.
464-
let mut alloc_map = self.alloc_map.lock();
465-
let id = alloc_map.reserve();
466-
alloc_map.alloc_map.insert(id, GlobalAlloc::Function(instance));
467-
id
468-
} else {
469-
// Deduplicate.
470-
self.reserve_and_set_dedup(GlobalAlloc::Function(instance))
471-
}
493+
let can_be_inlined = match self.codegen_fn_attrs(instance.def_id()).inline {
494+
InlineAttr::Never => false,
495+
_ => true,
496+
};
497+
let unique = !is_generic && !can_be_inlined;
498+
self.reserve_and_set_fn_alloc_internal(instance, unique)
472499
}
473500

474501
/// Generates an `AllocId` for a (symbolic, not-reified) vtable. Will get deduplicated.

Diff for: ‎compiler/rustc_middle/src/mir/pretty.rs

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

Diff for: ‎compiler/rustc_middle/src/ty/print/pretty.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -1667,7 +1667,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write {
16671667
Some(GlobalAlloc::Static(def_id)) => {
16681668
p!(write("<static({:?})>", def_id))
16691669
}
1670-
Some(GlobalAlloc::Function(_)) => p!("<function>"),
1670+
Some(GlobalAlloc::Function { .. }) => p!("<function>"),
16711671
Some(GlobalAlloc::VTable(..)) => p!("<vtable>"),
16721672
None => p!("<dangling pointer>"),
16731673
}
@@ -1679,7 +1679,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write {
16791679
ty::FnPtr(_) => {
16801680
// FIXME: We should probably have a helper method to share code with the "Byte strings"
16811681
// printing above (which also has to handle pointers to all sorts of things).
1682-
if let Some(GlobalAlloc::Function(instance)) =
1682+
if let Some(GlobalAlloc::Function { instance, .. }) =
16831683
self.tcx().try_get_global_alloc(prov.alloc_id())
16841684
{
16851685
self.typed_value(

Diff for: ‎compiler/rustc_monomorphize/src/collector.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -1223,10 +1223,10 @@ fn collect_alloc<'tcx>(tcx: TyCtxt<'tcx>, alloc_id: AllocId, output: &mut MonoIt
12231223
});
12241224
}
12251225
}
1226-
GlobalAlloc::Function(fn_instance) => {
1227-
if should_codegen_locally(tcx, fn_instance) {
1228-
trace!("collecting {:?} with {:#?}", alloc_id, fn_instance);
1229-
output.push(create_fn_mono_item(tcx, fn_instance, DUMMY_SP));
1226+
GlobalAlloc::Function { instance, .. } => {
1227+
if should_codegen_locally(tcx, instance) {
1228+
trace!("collecting {:?} with {:#?}", alloc_id, instance);
1229+
output.push(create_fn_mono_item(tcx, instance, DUMMY_SP));
12301230
}
12311231
}
12321232
GlobalAlloc::VTable(ty, trait_ref) => {

Diff for: ‎compiler/rustc_passes/src/reachable.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ impl<'tcx> ReachableContext<'tcx> {
310310
GlobalAlloc::Static(def_id) => {
311311
self.propagate_item(Res::Def(self.tcx.def_kind(def_id), def_id))
312312
}
313-
GlobalAlloc::Function(instance) => {
313+
GlobalAlloc::Function { instance, .. } => {
314314
// Manually visit to actually see the instance's `DefId`. Type visitors won't see it
315315
self.propagate_item(Res::Def(
316316
self.tcx.def_kind(instance.def_id()),

Diff for: ‎compiler/rustc_smir/src/rustc_smir/convert/mir.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -709,7 +709,7 @@ impl<'tcx> Stable<'tcx> for mir::interpret::GlobalAlloc<'tcx> {
709709

710710
fn stable(&self, tables: &mut Tables<'_>) -> Self::T {
711711
match self {
712-
mir::interpret::GlobalAlloc::Function(instance) => {
712+
mir::interpret::GlobalAlloc::Function { instance, .. } => {
713713
GlobalAlloc::Function(instance.stable(tables))
714714
}
715715
mir::interpret::GlobalAlloc::VTable(ty, trait_ref) => {

Diff for: ‎src/tools/miri/src/shims/backtrace.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
119119
let (alloc_id, offset, _prov) = this.ptr_get_alloc_id(ptr)?;
120120

121121
// This has to be an actual global fn ptr, not a dlsym function.
122-
let fn_instance = if let Some(GlobalAlloc::Function(instance)) =
122+
let fn_instance = if let Some(GlobalAlloc::Function { instance, .. }) =
123123
this.tcx.try_get_global_alloc(alloc_id)
124124
{
125125
instance

Diff for: ‎src/tools/miri/tests/pass/function_pointers.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ fn h(i: i32, j: i32) -> i32 {
2323
j * i * 7
2424
}
2525

26+
#[inline(never)]
2627
fn i() -> i32 {
2728
73
2829
}
@@ -77,7 +78,7 @@ fn main() {
7778
assert_eq!(indirect_mut3(h), 210);
7879
assert_eq!(indirect_once3(h), 210);
7980
// Check that `i` always has the same address. This is not guaranteed
80-
// but Miri currently uses a fixed address for monomorphic functions.
81+
// but Miri currently uses a fixed address for non-inlineable monomorphic functions.
8182
assert!(return_fn_ptr(i) == i);
8283
assert!(return_fn_ptr(i) as unsafe fn() -> i32 == i as fn() -> i32 as unsafe fn() -> i32);
8384
// Miri gives different addresses to different reifications of a generic function.

Diff for: ‎src/tools/miri/tests/pass/issues/issue-91636.rs

+1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ impl Function {
1010
}
1111
}
1212

13+
#[inline(never)]
1314
fn dummy(_: &str) {}
1415

1516
fn main() {

0 commit comments

Comments
 (0)