Miri function identity hack: account for possible inlining

[RalfJung]

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.

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[RalfJung]

r? @oli-obk

[RalfJung]

This is likely blocked on #123780.

Actually that may not be true any more -- Miri should evaluate statics only once and gives at most one address to each AllocId, so the entire thing may be unnecessary now. I don't remember if USIZE_MARKER was already a static back when we added this hack in the interpreter.

[tmiasko]

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.

[RalfJung]

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

[saethlin]

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.

[bjorn3]

#[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.

[RalfJung]

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.

[RalfJung]

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...

[saethlin]

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?

[RalfJung]

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

[saethlin]

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.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[RalfJung]

🤔 the incremental test suite is failing...?

[RalfJung]

I have no idea how this can affect incremental compilation, but it seems quite concerning. Except for some backtraces printed by Miri, it should be completely fine to do the "new AllocId each time an item-to-fn-ptr coercion happens" for all functions. Bot now it seems like somehow something else started to rely on this not happening.

@oli-obk do you understand how this could be happening?

[michaelwoerister]

The error looks like a query is being executed during the deserialization of an incr. comp. on-disk cache entry, which is not allowed (see #91919).

[RalfJung]

Thanks! Maybe codegen_fn_attrs is a query, and reserve_and_set_fn_alloc gets called during deserialization?

(That incremental error message is really not giving any hint to the underlying cause, then.)

[michaelwoerister]

Yes, reserve_and_set_fn_alloc is called during decoding here:

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

Line 393 in 3288583

let alloc_id = decoder.interner().reserve_and_set_fn_alloc(instance);

[michaelwoerister]

Is there any way you can refactor this to not call the codegen_fn_attrs query?

[RalfJung]

I did not realize this logic is also called during decoding. I'll have to rethink this... cross-crate handling is largely a mystery to me and so far magically things worked despite me largely ignoring encoding/decoding concerns. ;)

[michaelwoerister]

I tried to make the error message more useful in #124252.

[RalfJung]

Yes that's a much nicer error. It even tells me which query was being incorrectly invoked. :)

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[RalfJung]

@oli-obk the latest version should hopefully fix the incremental issues. :)
@rustbot ready

[oli-obk]

Yea, that looks reliable to me

@bors r+

[bors]

📌 Commit 41b98da has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit 41b98da with merge 4892331...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 4892331 to master...

[rust-timer]

Finished benchmarking commit (4892331): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 1.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.6%	[0.7%, 2.5%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -0.9%, secondary 0.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[0.6%, 2.1%]	2
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	1
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 698.879s -> 699.885s (0.14%)
Artifact size: 328.27 MiB -> 328.23 MiB (-0.01%)