[Amanieu]

This PR changes Drop to abort if an unwinding panic attempts to escape it, making the process abort instead. This has several benefits:

• The current behavior when unwinding out of Drop is very unintuitive and easy to miss: unwinding continues, but the remaining drops in scope are simply leaked.
• A lot of unsafe code doesn't expect drops to unwind, which can lead to unsoundness:
  • Panic safety in drop servo/rust-smallvec#14
  • Panics on drop will cause double drops. bluss/arrayvec#3
• There is a code size and compilation time cost to this: LLVM needs to generate extra landing pads out of all calls in a drop implementation. This can compound when functions are inlined since unwinding will then continue on to process drops in the callee, which can itself unwind, etc.
  • Initial measurements show a 3% size reduction and up to 10% compilation time reduction on some crates (syn).

One thing to note about -Z panic-in-drop=abort is that all crates must be built with this option for it to be sound since it makes the compiler assume that dropping Box<dyn Any> will never unwind.

cc rust-lang/lang-team#97

[rust-highfive]

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

[Amanieu]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 75b61d15a9d844666c07d09064bec26a595d83fd with merge 1f815a30705b7e96c149fc4fa88a98ca04e2deee...

[bors]

☀️ Try build successful - checks-actions
Build commit: 1f815a30705b7e96c149fc4fa88a98ca04e2deee (1f815a30705b7e96c149fc4fa88a98ca04e2deee)

[rust-timer]

Queued 1f815a30705b7e96c149fc4fa88a98ca04e2deee with parent c9db3e0, future comparison URL.

[rust-timer]

Finished benchmarking commit (1f815a30705b7e96c149fc4fa88a98ca04e2deee): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

• Large improvement in instruction counts (up to -9.9% on full builds of syn)
• Large regression in instruction counts (up to 2.0% on full builds of keccak)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

[jyn514]

One thing to note about -Z panic-in-drop=abort is that all crates must be built with this option for it to be sound since it makes the compiler assume that dropping Box will never unwind.

Could we make it a hard error if there's a mismatch in this between crates?

[Amanieu]

Could we make it a hard error if there's a mismatch in this between crates?

Where would be the best place to add this check? Do we have any precedent for validating crate compatibility?

[ghost]

I think there is a similar check here:

rust/compiler/rustc_metadata/src/dependency_format.rs

Lines 391 to 418 in c5cbf78

	// Next up, verify that all other crates are compatible with this panic
	// strategy. If the dep isn't linked, we ignore it, and if our strategy
	// is abort then it's compatible with everything. Otherwise all crates'
	// panic strategy must match our own.
	for (i, linkage) in list.iter().enumerate() {
	if let Linkage::NotLinked = *linkage {
	continue;
	}
	if desired_strategy == PanicStrategy::Abort {
	continue;
	}
	let cnum = CrateNum::new(i + 1);
	let found_strategy = tcx.panic_strategy(cnum);
	let is_compiler_builtins = tcx.is_compiler_builtins(cnum);
	if is_compiler_builtins || desired_strategy == found_strategy {
	continue;
	}
	sess.err(&format!(
	"the crate `{}` is compiled with the \
	panic strategy `{}` which is \
	incompatible with this crate's \
	strategy of `{}`",
	tcx.crate_name(cnum),
	found_strategy.desc(),
	desired_strategy.desc()
	));
	}

[Amanieu]

This shaves 5MB off librustc_driver.so, a 3% reduction (197MB -> 192MB).

[joshtriplett]

This looks really promising!

I think this would be a good first step. Before we can even consider changing the default behavior, we'd need to have an option for the new behavior, and experiment with it extensively.

[estebank]

Can somebody else in @rust-lang/compiler take this review on? I am not as confident in codegen as the rest of the compiler.

[eddyb]

r? @nagisa cc @nikic @wesleywiser

[eddyb]

LGTM so far, but I'd like to review test changes once the drop_in_place lang item isn't spuriously required, which seems to account for a lot of them.

[nagisa]

Another thing to add on a quick skim: in MIR when building cleanup blocks, we spend a lot of effort to build unwind ladders just in case a cleanup/drop panics. All of that can also be disabled when this flag is enabled, potentially giving you another significant perf boost in build times.

[Amanieu]

I've addressed all the review feedback.

I'm having a bit of trouble wrapping my head around the drop tree building code, so I haven't made any changes there yet.

[Amanieu]

Addressed feedback.

[klensy]

Add accepted values list and default (like in some other options), plus maybe unstable book entry?

[nagisa]

@bors r=nagisa,eddyb

I think this is reasonable starting point. I suspect there are more places we can reduce the amount of work we do, but there's no reason for all that to be covered in this specific PR.

[bors]

📌 Commit 5862a00 has been approved by nagisa,eddyb

[bors]

⌛ Testing commit 5862a00 with merge 51e514c...

[bors]

☀️ Test successful - checks-actions
Approved by: nagisa,eddyb
Pushing 51e514c to master...

[rust-timer]

Finished benchmarking commit (51e514c): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

[Wyvern]

Now, I use "-Zpanic-in-drop=abort" and build-std to build rust project hit lots of errors, like below:

error: the crate `std` is compiled with the panic-in-drop strategy `unwind` which is incompatible with this crate's strategy of `abort`

error: the crate `core` is compiled with the panic-in-drop strategy `unwind` which is incompatible with this crate's strategy of `abort`

error: the crate `rustc_std_workspace_core` is compiled with the panic-in-drop strategy `unwind` which is incompatible with this crate's strategy of `abort`

error: the crate `alloc` is compiled with the panic-in-drop strategy `unwind` which is incompatible with this crate's strategy of `abort`

error: the crate `libc` is compiled with the panic-in-drop strategy `unwind` which is incompatible with this crate's strategy of `abort`

Anybody knows how to resolve it?

[Amanieu]

Now, I use "-Zpanic-in-drop=abort" and build-std to build rust project hit lots of errors, like below:

It seems to work fine for me:

RUSTFLAGS=-Zpanic-in-drop=abort cargo build -Zbuild-std --target x86_64-unknown-linux-gnu