Implement immediate-abort by hooking #[rustc_panic_entrypoint] and lint for functions that look like a panic entrypoint

[saethlin]

View all comments

There are a handful of functions, mostly in core, which have a mess of attributes on them which try to ensure that they compile down to just intrinsics::abort() when cfg(panic = "immediate-abort"), and otherwise are outlined. These functions now all get #[rustc_panic_entrypoint] which makes the functions #[cold], #[inline(never)], and #[optimize(size)]. But when immediate-abort is enabled, these functions are #[inline].

And since these functions are now all known to the compiler, when immediate-abort is enabled, both their bodies and calls to them are directly replaced intrinsics::abort(). Replacing both body and calls means that we rely much less on optimization to make immediate-abort do its job, but if for some godforsaken reason you are indirectly calling a panic entrypoint, that will still immediately abort.

There is a fair bit of inconsistency in what attributes are on panic entrypoints, only some have #[optimize(size)] and about half have #[inline(never)]. The single attribute makes sure they are all treated the same. (I am making an informed guess that the inconsistency was accidental, and mostly not impactful)

This PR also adds an allow-by-default lint, missing_panic_entrypoint which looks for diverging functions that do not have one of #[rustc_panic_entrypoint] or #[inline]. Panic wrappers sometimes have #[inline] because they are the const fn for a const_eval_select, so adding #[inline] ensures that monomorphization doesn't eagerly codegen a useless copy of them.

The net effect is that most but not all of the cfgs for immediate-abort are gone, and the hack in cg_ssa that replaced any upstream diverging function call with abort can now check specifically for panic entrypoints.

[bors]

☔ The latest upstream changes (presumably #150386) made this pull request unmergeable. Please resolve the merge conflicts.

[bjorn3]

Should #[rustc_panic_entrypoint] imply #[cold] #[inline(never)] #[track_caller]? Is that even easy to implement?

Should be possible to check #[rustc_panic_entrypoint] when computing the CodegenFnAttrs.

[saethlin]

Should be possible to check #[rustc_panic_entrypoint] when computing the CodegenFnAttrs.

Yep. But it turns out #[track_caller] is fiddly because codegen backends have assumed that for example panic_nounwind doesn't have the extra location argument and threading through what location to pass them is fiddly. Maybe something to fiddle with in a second PR. It's not clear to me that the entrypoints that don't have track_caller are actually good. But that might have perf implications on normal builds, which I'd rather minimize.

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0de2b26 (0de2b267a76559681f8e610c347a4a016cdb10b5, parent: 0e8999942552691afc20495af6227eca8ab0af05)

[rust-timer]

Finished benchmarking commit (0de2b26): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 0.9%)

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

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

Cycles

Results (secondary 2.3%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary -0.0%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	4

Bootstrap: 480.336s -> 480.598s (0.05%)
Artifact size: 390.83 MiB -> 390.83 MiB (0.00%)

[Noratrieb]

This should be an internal lint

[saethlin]

Should it? Internal lints are enabled on the compiler crates, and I only want this on the standard library.

[saethlin]

I don't understand how this could be an internal lint. Can you explain? I don't want this enabled on the compiler, and that seems to be the definition of internal lints.

[saethlin]

I'm lost. How did trying to add an internal lint (which uses completely different macros from adding a builtin lint??) remove another lint?

	warning: unknown lint: `rustc::default_hash_types`
	 --> compiler/rustc_fluent_macro/src/lib.rs:2:10
	  |
	2 | #![allow(rustc::default_hash_types)]
	  |          ^^^^^^^^^^^^^^^^^^^^^^^^^
	

[rust-bors]

☔ The latest upstream changes made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #150628) made this pull request unmergeable. Please resolve the merge conflicts.

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: e6cf4f6 (e6cf4f64da15878e71294222e78d1c90d8793b07, parent: 4c37f6d78c9c83e05caa0d1e43ce7d0c3f27538e)

[rust-timer]

Finished benchmarking commit (e6cf4f6): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary -1.3%, secondary 0.4%)

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

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	1.8%	[0.6%, 2.4%]	5
Improvements ✅ (primary)	-3.0%	[-3.0%, -3.0%]	1
Improvements ✅ (secondary)	-6.4%	[-6.4%, -6.4%]	1
All ❌✅ (primary)	-1.3%	[-3.0%, 0.5%]	2

Cycles

Results (primary -8.4%, secondary -1.0%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.5%, 2.8%]	3
Improvements ✅ (primary)	-8.4%	[-13.0%, -2.5%]	9
Improvements ✅ (secondary)	-3.7%	[-5.3%, -2.3%]	4
All ❌✅ (primary)	-8.4%	[-13.0%, -2.5%]	9

Binary size

Results (primary 0.0%, secondary 0.1%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	4
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	4

Bootstrap: 480.702s -> 486.725s (1.25%)
Artifact size: 397.88 MiB -> 398.03 MiB (0.04%)

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: d8b0d0e (d8b0d0ef7a5aa4b1fbee386c94f7ffef22251168, parent: 873b4beb0cc726493b94c8ef21f68795c04fbbc1)

[rust-timer]

Finished benchmarking commit (d8b0d0e): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 5.5%)

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

	mean	range	count
Regressions ❌ (primary)	5.5%	[5.5%, 5.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.5%	[5.5%, 5.5%]	1

Cycles

Results (primary -7.9%, secondary -5.8%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-7.9%	[-10.5%, -2.4%]	8
Improvements ✅ (secondary)	-5.8%	[-5.8%, -5.8%]	1
All ❌✅ (primary)	-7.9%	[-10.5%, -2.4%]	8

Binary size

Results (primary 0.0%, secondary -0.1%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	3

Bootstrap: 481.904s -> 483.897s (0.41%)
Artifact size: 397.95 MiB -> 398.01 MiB (0.02%)

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

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

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

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @scottmcm, libs
• @scottmcm, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, joboet, scottmcm

[RalfJung]

This is a MIR transform that actually changes the behavior of the program. That effectively makes the transform part of the spec. Do we have some proper way of calling that out, some good way of documenting this?

Also, please add a test to Miri to ensure it behaves correctly there.

[RalfJung]

Doesn't this have massive amounts of false positives? A function could easily be diverging without being a panic entrypoint -- it could deliberately loop, or call exit/abort.