Enforce that we don't generate code for comptime fns

[oli-obk]

... by ICEing if you try to compute a symbol for them. There are various other ways we can assert it, but they were all a bit icky or already unreachable.

r? @fmease (since you asked about it in #159727 (comment))

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @lcnr

[oli-obk]

@bors try @rust-timer queue

[RalfJung]

Sounds like Miri's find_mir_or_eval_fn should also ICE in that case?

[oli-obk]

I'm not worried about miri being able to reach this. There are enough safeguards to fail compilation before that happens. This was just about codegen picking what to codegen via various independent means, and I overlooked one.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2fa05b3 (2fa05b353b7076021011f04d6af42b83619e91ce)
Base parent: 165cce8 (165cce8d820b229af8f6a8226cf0b910b57600ff)

[rust-timer]

Finished benchmarking commit (2fa05b3): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -3.7%, secondary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-8.5%	[-8.5%, -8.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.7%	[-8.5%, 1.1%]	2

Cycles

This perf run didn't have relevant results for this metric.

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 485.902s -> 487.995s (0.43%)
Artifact size: 387.66 MiB -> 387.64 MiB (-0.01%)

[fmease]

From what I can gather, you modified this function so (among other things) tcx.is_reachable_non_generic in RootCollector::is_root returns false for comptime functions when collecting mono items (collect_roots < collect_crate_mono_items).

However, under MonoItemCollectionStrategy::Eager (-Clink-dead-code) is_root doesn't consider is_reachable_non_generic, meaning the following two snippets of code both ICE in debug mode if -Clink-dead-code is passed

#![feature(rustc_attrs)] // for `rustc_comptime`
#![crate_type = "lib"]

#[rustc_comptime]
pub fn f() { std::mem::offset_of!(((),), 0); }
//        => std::intrinsics::offset_of::<((),)>(0, 0)

#![feature(rustc_attrs)] // for `rustc_comptime`
#![crate_type = "lib"]

#[rustc_comptime]
pub fn f() { size_of::<()>(); }

with

thread 'rustc' (60874) panicked at compiler/rustc_symbol_mangling/src/lib.rs:246:9:
assertion failed: tcx.constness(instance.def_id()) != hir::Constness::Const { always: true }

The code above doesn't ICE in release mode.

However, calling these comptime intrinsics directly if -Clink-dead-code is passed also ICEs in release mode but it requires feature core_intrinsics of course & they already ICE on nightly, so this might be considered WONTFIX?:

#![feature(rustc_attrs)] // for `rustc_comptime`
#![feature(core_intrinsics)] // well...
#![crate_type = "lib"]

#[rustc_comptime] pub fn f() { std::intrinsics::size_of::<()>(); }

error: internal compiler error: compiler/rustc_codegen_ssa/src/mir/intrinsic.rs:140:21: Nullary intrinsic size_of must be called in a const block. If you are seeing this message from code outside the standard library, the unstable implementation details of the relevant intrinsic may have changed. Consider using stable APIs instead. If you are adding a new nullary intrinsic that is inherently a runtime intrinsic, update this check.
 --> <anon>:1:32
  |
1 | #[rustc_comptime] pub fn f() { std::intrinsics::size_of::<()>(); }
  |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

#![feature(rustc_attrs)] // for `rustc_comptime`
#![feature(core_intrinsics)] // well...
#![crate_type = "lib"]

#[rustc_comptime] pub fn f() { std::intrinsics::offset_of::<((),)>(0, 0); }

error: internal compiler error: compiler/rustc_codegen_ssa/src/mir/block.rs:1075:37: intrinsic offset_of must be overridden by codegen backend, but isn't
 --> <anon>:1:32
  |
1 | #[rustc_comptime] pub fn f() { std::intrinsics::offset_of::<((),)>(0, 0); }
  |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[fmease]

If you consider this acceptable for the time being (after all, use of -Clink-dead-code seems discouraged & it seems to require #![feature(rustc_attrs)] unless there's a different way to repro it w/o nightly features?), feel free to r=me w/ or w/o a FIXME.

I'm not super accustomed to this part of the compiler, so the fact that the current approach ICEs under -Clink-dead-code naively leads me to believe that there might be better places to put the Const { always: true } check(s)...

[oli-obk]

I initially had the check only in monomorphization, but that didn't work out fully (got linker errors 😨 ). So I decided to just add the ICE and avoid it, which immediately pointed me to the reachable_non_generics query. That eagerly collects the symbols (for stable sorting), so I can't prevent it just at the call sites.

Managed to confuse myself a few times in between, but I ended up removing all "redundant" (or not redundant, as you figured out) checks from mono collection. So we may need both after all. While I never understood the reason for link-dead-code's existence, I'll try to not break it more than it is considering the many issues about it.

[fmease]

Thanks, I love it!

r=me w/ or w/o a FIXME if you don't consider #159777 (comment) to be a concern for the time being.

[oli-obk]

The extra check only runs in eager mode, so we don't pay the cost for it except in link-dead-code mode, which... is already expensive as it is.

@bors r=fmease

[rust-bors]

📌 Commit 4609fbc has been approved by fmease

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

Reason for tree closure: spurious failures