Fix cases where std accidentally relied on inline(never)

[saethlin]

This PR increases the power of -Zcross-crate-inline-threshold=always so that it applies through #[inline(never)]. Note that though this is called "cross-crate-inlining" in this case especially it is just lazy per-CGU codegen. The MIR inliner and LLVM still respect the attribute as much as they ever have.

Trying to bootstrap with the new -Zcross-crate-inline-threshold=always change revealed two bugs:

We have special intrinsics assert_inhabited, assert_zero_valid, and assert_mem_uniniitalized_valid which codegen backends will lower to nothing or a call to panic_nounwind. Since we may not have any call to panic_nounwind in MIR but emit one anyway, we need to specially tell MirUsedCollector about this situation.

#[lang = "start"] is special-cased already so that MirUsedCollector will collect it, but then when we make it cross-crate-inlinable it is only assigned to a CGU based on whether MirUsedCollector saw a call to it, which of course we didn't.

---

I started looking into this because #118683 revealed a case where we were accidentally relying on a function being #[inline(never)], and cranking up cross-crate-inlinability seems like a way to find other situations like that.

r? @nnethercote because I don't like what I'm doing to the CGU partitioning code here but I can't come up with something much better

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[nnethercote]

I left a couple of nits. I don't feel like I understand what's going on here well enough to confidently give an r+ :/

[nnethercote]

This comment should be updated to explain why start_fn gets special treatment.

[saethlin]

I don't feel like I understand what's going on here well enough to confidently give an r+

Do you mean that you don't understand this part of the compiler, you don't understand the motivation for the change, or you don't understand the way I've decided to change the handling of lang = "start" and panic_nounwind in this specific way? I'm happy to spend time discussing this if it's the second or third.

[nnethercote]

A little bit of all three!

[saethlin]

Okay! I'll try to start from the beginning and explain everything.

The standard library has a feature called panic_immediate_abort. The idea is that if you're running on a system that can't emit panic messages or you don't care about them, you want the panic runtime optimized away. "The panic runtime" basically means this function and all the others like it:

rust/library/core/src/panicking.rs

Lines 46 to 52 in 5b8bc56

	#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never), cold)]
	#[cfg_attr(feature = "panic_immediate_abort", inline)]
	#[track_caller]
	#[lang = "panic_fmt"] // needed for const-evaluated panics
	#[rustc_do_not_const_check] // hooked by const-eval
	#[rustc_const_unstable(feature = "core_panic", issue = "none")]
	pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {

and also all the formatting code they pull in.

Currently (I hope to change this eventually), we use cfg_attr so that by default the standard library puts #[inline(never)] and #[cold] on all the functions which are entrypoints to the panic runtime, because they are in fact cold, and also contain a lot of formatting code, so inlining them is undesirable. But if we don't want panic formatting in our build, we want calls to those functions optimized down to just a ud2 (or equivalent). So when -Zbuild-std-features=panic_immediate_abort is passed, we swap out those attributes for #[inline] instead.

The LLVM inlinehint that #[inline] produces is not very important here, LLVM is trivially able to figure out that these functions are just an unreachable terminator. But since we want these functions to be inlined into our executable from the standard library, we need cross-crate-inlining. We could require LTO, but it would be much nicer to not, and we can get the desired effect by swapping in just #[inline] on these functions.

---

With #[inline(never)] we eagerly codegen all the panic entrypoints when compiling the standard library. But when we swap to #[inline], we only codegen of functions which are determined to be reachable, as determined by MirUsedCollector and CGU partitioning.

This can go wrong if MirUsedCollector doesn't realize that a function is used. I recently submitted #118693 to fix a scenario much like this PR. In that other PR, since panic_misaligned_pointer_dereference is not actually called in MIR, we need to tell MirUsedCollector that a particular kind of MIR terminator causes that function to become reachable. This situation with panic_misaligned_pointer_dereference is what motivated me to create this PR. The root problem is that the compiler assumes that MirUsedCollector finds all the used MIR, so if it ever doesn't we have a lurking problem.

So. The problem pattern here is that we add #[inline] to a function then said function becomes an undefined symbol at link time, because said function requires special treatment in our reachability analysis that was missing. This PR is about finding and fixing those cases.

---

The best way to find these cases is to apply our #[inline] style codegen to everything. -Zcross-crate-inline-threshold=always exists to provide this, but when I initially implemented it I didn't realize that the codegen strategy and the inlining optimization are completely separate- we can have cross-crate codegen with inlining forbidden. So previously -Zcross-crate-inline-threshold=always meant to apply cross-crate codegen to all functions except those which are #[no_mangle] or #[inline(never)] and now it only #[no_mangle] functions are exempted.

So I made that change then tried to build the compiler with -Zcross-crate-inline-threshold=always x build --stage 2 and got some undefined symbol errors that I started to debug.

The first errors were for panic_nounwind. This symbol has exactly the same problem as panic_misaligned_pointer_dereference; codegen inserts calls to it based on some other MIR construct, in this case the intrinsic validity assertions, which are declared here:

rust/library/core/src/intrinsics.rs

Lines 1068 to 1092 in 028b6d1

	/// A guard for unsafe functions that cannot ever be executed if `T` is uninhabited:
	/// This will statically either panic, or do nothing.
	///
	/// This intrinsic does not have a stable counterpart.
	#[rustc_const_stable(feature = "const_assert_type", since = "1.59.0")]
	#[rustc_safe_intrinsic]
	#[rustc_nounwind]
	pub fn assert_inhabited<T>();
	/// A guard for unsafe functions that cannot ever be executed if `T` does not permit
	/// zero-initialization: This will statically either panic, or do nothing.
	///
	/// This intrinsic does not have a stable counterpart.
	#[rustc_const_stable(feature = "const_assert_type2", since = "1.75.0")]
	#[rustc_safe_intrinsic]
	#[rustc_nounwind]
	pub fn assert_zero_valid<T>();
	/// A guard for `std::mem::uninitialized`. This will statically either panic, or do nothing.
	///
	/// This intrinsic does not have a stable counterpart.
	#[rustc_const_stable(feature = "const_assert_type2", since = "1.75.0")]
	#[rustc_safe_intrinsic]
	#[rustc_nounwind]
	pub fn assert_mem_uninitialized_valid<T>();

and the codegen for them is here:

rust/compiler/rustc_codegen_ssa/src/mir/block.rs

Lines 682 to 685 in 028b6d1

	// Emit a panic or a no-op for `assert_*` intrinsics.
	// These are intrinsics that compile to panics so that we can get a message
	// which mentions the offending type, even from a const context.
	let panic_intrinsic = intrinsic.and_then(|s| ValidityRequirement::from_intrinsic(s));

. I basically combined the way codegen detects these intrinsics with how MirUsedCollector adds mono items.

With that fixed, I got an undefined reference to lang_start. That one was rather confusing because it's already special-cased in MirUsedCollector here:

rust/compiler/rustc_monomorphize/src/collector.rs

Lines 1246 to 1256 in 028b6d1

	/// As a special case, when/if we encounter the
	/// `main()` function, we also have to generate a
	/// monomorphized copy of the start lang item based on
	/// the return type of `main`. This is not needed when
	/// the user writes their own `start` manually.
	fn push_extra_entry_roots(&mut self) {
	let Some((main_def_id, EntryFnType::Main { .. })) = self.entry_fn else {
	return;
	};
	let start_def_id = self.tcx.require_lang_item(LangItem::Start, None);

the problem is that though it gets collected, it gets dropped by CGU partitioning. We partition by InstantiationMode::GloballyShared items, then stick onto each CGU all the items that are reachable from the contents of the CGU. But if lang_start gets InstantiationMode::LocalCopy then it's not a root item and it's of course not called from anywhere. So CGU partitioning just forgets about it.

Then with that fixed, the compiler bootstraps with -Zcross-crate-inline-threshold=always. Nice! That should indicate we haven't forgotten cases that can produce linker errors if #[inline] gets added.

Phew that's a lot of text. Does this help?

[nnethercote]

A comment on this block, possibly mentioning assert_inhabited, assert_zero_valid, and assert_mem_uniniitalized_valid, would help a lot.

[nnethercote]

Thanks, that explanation definitely helps. I think it'll be good to merge once the comments I've requested have been added. They don't need to be as long as the explanation above, but some information in comments will be very helpful. The chances of a casual reader following git blame back to this PR and then seeing that long explanation aren't that high.

[saethlin]

Took a shot at adding such comments, don't hesitate to ask for more detail if you think it is merited.

[nnethercote]

Suggested change

	// So even if its mode is LocalCopy need to treat it like a root.
	// So even if its mode is LocalCopy we need to treat it like a root.

[nnethercote]

Much improved! Thanks. One tiny nit.

@bors delegate=saethlin

[bors]

✌️ @saethlin, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

[saethlin]

@bors r=nnethercote

[bors]

📌 Commit e559172 has been approved by nnethercote

It is now in the queue for this repository.

[bors]

⌛ Testing commit e559172 with merge f444ef2...

[rust-log-analyzer]

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

error: test run failed!
status: exit status: 101
command: RUST_TEST_THREADS="8" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "run" "0" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/structs-enums/struct-order-of-eval-3/a"
--- stdout -------------------------------
uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/structs-enums/struct-order-of-eval-3/a", waiting for result
--- stderr -------------------------------
thread 'main' panicked at src/tools/remote-test-client/src/main.rs:310:9:
client.read_exact(&mut header) failed with Connection reset by peer (os error 104)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[bors]

💔 Test failed - checks-actions

[saethlin]

I don't remember if the thing to do here is re-add the approval or retry...

@bors r=nnethercote

[bors]

💡 This pull request was already approved, no need to approve it again.

• This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
• There's another pull request that is currently being tested, blocking this pull request: Rollup of 4 pull requests #118949

[bors]

📌 Commit e559172 has been approved by nnethercote

It is now in the queue for this repository.

[bors]

⌛ Testing commit e559172 with merge dbe6e3b...

[rust-log-analyzer]

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] rustc_driver_impl test:false 54.410
[RUSTC-TIMING] rustc_symbol_mangling test:true 1.637
[RUSTC-TIMING] rustc_pattern_analysis test:true 1.987
[RUSTC-TIMING] rustc_passes test:false 83.883
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
##[group]Clock drift check
Session terminated, killing shell... ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

[workingjubilee]

still seems like a network issue?

@bors retry rollup=iffy

[bors]

⌛ Testing commit e559172 with merge 1559dd2...

[bors]

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 1559dd2 to master...

[rust-timer]

Finished benchmarking commit (1559dd2): 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

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.5%	[0.5%, 0.5%]	2
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-3.2%	[-4.9%, -1.5%]	2
All ❌✅ (primary)	0.1%	[-0.6%, 0.5%]	3

Cycles

Results

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-1.4%, -0.6%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-1.4%, 0.5%]	4

Binary size

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

Bootstrap: 670.068s -> 671.548s (0.22%)
Artifact size: 312.45 MiB -> 312.42 MiB (-0.01%)