Allow `panic = "abort"` for benchmarks when `harness = false`.

[Imberflur]

Problem

The panic setting is ignored for benchmarks since (by default) they link libtest which is compiled with panic = "unwind". This would otherwise lead to confusing errors at link time when "abort" is used (https://github.com/rust-lang/cargo/issues/3166, https://github.com/rust-lang/cargo/pull/3175).

However, it is possible to disable the default harness for a benchmark (which means libtest is no longer linked):

[[bench]]
name = "my_bench"
harness = false

(this is common when using external benchmarking frameworks such as criterion)

The ability to disable unwinding when benchmarking is useful since the inclusion of unwinding machinery can affect performance.

Proposed Solution

Allow panic = "abort" when harness = false.

Cargo currently always prints a warning when the bench profile contains panic = "abort". With this proposal that warning would only be printed when building/running a benchmark where the setting is ignored. (Note: currently running a benchmark with a different profile, e.g. cargo bench --profile release that has panic = "abort" will ignore the panic setting but not print any warning)

Notes

No response

[weihanglo]

Hmm… really interesting! That is a limitation of libtest but it seems unnecessary to hinder custom test harnesses from adjusting the panic strategy. For myself, I'd like to see Cargo relax it a bit.

Haven't tracked its progress for a while, but there is -Z panic-abort-tests you might want to try out at this moment.

[weihanglo]

Sorry for pinging you folks. My little brain only remembers two custom test harnesses nextest-rs/nextest and bheisler/criterion.rs. @bheisler and @sunshowers, I'd love to know your opinions on relaxing panic strategy for custom harnesses. How would it help your awesome project if Cargo provides this flexibility?

[bheisler]

Well, I don't think I have too much to say except the obvious. Panicking in a benchmark would be very unusual, and an uncaught panic would be a bug the benchmark author should fix.

Production code sometimes changes the panic handler for performance or other reasons, and it would make sense for the panic handler of the benchmarks to match that of the production code (to make benchmark measurements more similar to, and thus more predictive of, the performance of that code in production).

I had never thought to change the panic handler for a benchmark suite, but now that someone has pointed out that we can't, it does seem like a very strange limitation.

I can't think of a way that Criterion-rs would directly make use of this feature - panics are mostly orthogonal to benchmarking. I can imagine Criterion-rs users - benchmark authors - wanting to change the panic handler and being surprised that they can't.

[sunshowers]

Nextest isn't a custom test harness (it's a custom test runner) so I don't think there's any upside or downside of supporting this for nextest.

[weihanglo]

Thank you for all the rapid and valuable feedback!. Seems it won't break things from your side. Pretty good!

The only breaking change I can think of is that tests with custom harness set to panic = abort suddenly work (which is exactly what we want to solve). Depending on the old behaviour sounds rare and is a mistake. The fix from the user side should also be an easy change on panic strategy. I'll bring this to the next meeting or do an FCP.

(and sorry for mistaking nextest for a test harness 🫣)

[c410-f3r]

What is the status of this feature?

[weihanglo]

There is an open PR proposing to relax the limitation a bit: https://github.com/rust-lang/cargo/pull/11272

[c410-f3r]

There is an open PR proposing to relax the limitation a bit: #11272

Thanks!

[weihanglo]

While I opened #11272, I forgot the exact use cases of this feature.

@c410-f3r and @Imberflur, could you folks share more about your scenarios and also some aspects/data about the performance story with the current limitation?

[c410-f3r]

I don't have data but panic = abort usually reduces binary size and improves runtime performance. That is my use case :)

[Imberflur]

My case is that we use panic = "abort" for our release binaries. And that can potentially affect runtime performance. So being able to toggle this on for benchmarks would make them more accurate gauges for the performance of our final builds.

[Imberflur]

Code paths can also be conditionally included based on the panic setting, e.g. with #[cfg(panic = "unwind")]. Which could impact runtime performance in addition to the difference introduced by the setting itself.

[weihanglo]

Code paths can also be conditionally included based on the panic setting, e.g. with #[cfg(panic = "unwind")]. Which could impact runtime performance in addition to the difference introduced by the setting itself.

This is an interesting point I never thought of! Thank you people for your feedback :)