Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miri function identity hack: account for possible inlining #123781

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Miri function identity hack: account for possible inlining #123781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions compiler/rustc_codegen_cranelift/src/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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!()
}
};
Expand Down Expand Up @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_llvm/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
14 changes: 8 additions & 6 deletions compiler/rustc_const_eval/src/interpret/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)) => {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
}
}
Expand Down Expand Up @@ -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})")?;
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/validity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
73 changes: 50 additions & 23 deletions compiler/rustc_middle/src/mir/interpret/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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).
Expand All @@ -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),
}
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
};
Comment on lines +493 to +496
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codegen_fn_attrs(...).inline encodes the inline attribute, which in general is orthogonal to the question how many times item can be code generated:

  • Generic functions / closures can be marked with #[inline(never)].
  • #[no_mangle] functions can be marked #[inline(always)].
  • Cross crate inlining is separate from the attribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic functions are already handled above.

The direction we need is that if the item can be cross-crate inlined then we should give it a fresh address each time here, i.e. is_generic || can_be_inlined should be true. It's okay if we give it a fresh address too often. I am not sure if that property holds with my current definition, it's hard to say as everything is spread across so many parts of rustc.

Cc @saethlin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is and was entirely unclear to me if any parts of rustc rely on certain Instances getting LocalCopy or GloballyShared codegen. I've been meaning to try to clean this logic up at some point.

Cross-crate inlining should not be orthogonal from the attribute. It should return true for anything that is #[inline] or #[inline(always)], and use heuristics for all other cases.

I do not understand why we permit #[no_mangle] with #[inline(always)]. That seems like an incoherent request to me. #[no_mangle] is widely understood as requesting that the function be lowered as a single external symbol that can be linked against, and lowering it as LocalCopy directly contradicts that. We should probably produce a hard error for that attribute combination.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[no_mangle] + #[inline(always)] to me seems like a request to export a single copy for consumption by C code, while still codegening a separate local copy for each use from Rust with the request to inline it where possible.

Copy link
Member Author

@RalfJung RalfJung Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this is all off-topic for this PR. The key thing this PR relies on is that if a function is non-generic (only lifetimes) and inline(never) then it will never have more than one copy across the entire crate graph.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even that is a rather soft requirement -- the LLVM attribute we set makes it so that functions can still be (de)duplicated. It's just that making all functions duplicated in Miri makes backtraces look worse so it's kind of not worth it...

Ideally we'd have an attribute that makes a function guaranteed to have a unique address, and then we suppress the unnamed_addr attribute. Then we can decorate the backtrace functions with that.

But until then this PR at least surfaces more possible address mismatches than before. I am just a bit worried about generating an endless amount of AllocId if a function is cast to a pointer a lot...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just a bit worried about generating an endless amount of AllocId if a function is cast to a pointer a lot...

Shouldn't the GC prevent any issues with this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GC doesn't remove allocations, I don't think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it doesn't. I'll file a Miri issue as a possible extension, but for now this program:

fn main() {
    loop {
        oof as usize;
    }
}

fn oof() {}

seems to take a minute for its memory usage to grow to 1 GB. That is definitely not desirable but in general I think programs that do that number of casts will have other runtime or memory usage issues.

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.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})")?
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/ty/print/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>"),
}
Expand All @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_monomorphize/src/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,10 +1223,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) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/reachable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_smir/src/rustc_smir/convert/mir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/shims/backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/tools/miri/tests/pass/function_pointers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ fn h(i: i32, j: i32) -> i32 {
j * i * 7
}

#[inline(never)]
fn i() -> i32 {
73
}
Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions src/tools/miri/tests/pass/issues/issue-91636.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ impl Function {
}
}

#[inline(never)]
fn dummy(_: &str) {}

fn main() {
Expand Down
Loading