Add #[cfg(panic = '...')]

[davidhewitt]

This PR adds conditional compilation according to the panic strategy.

I've come across a need for a flag like this a couple of times while writing tests: #74301 , #73670 (comment)

I'm not sure if I need to add a feature gate for this flag?

[rust-highfive]

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

[ecstatic-morse]

I'd like to see a test verifying that -C panic=abort actually causes the cfg(panic = "abort") code to run and vice versa. Also, this needs sign off from @rust-lang/lang, but an RFC/unstable period is probably overkill.

[Mark-Simulacrum]

Please use sym::panic and modify PanicStrategy to also provide a pre-interned symbol (code is here:

rust/src/librustc_target/spec/mod.rs

Lines 171 to 176 in 584e83d

	pub fn desc(&self) -> &str {
	match *self {
	PanicStrategy::Unwind => "unwind",
	PanicStrategy::Abort => "abort",
	}
	}

). Both sym::abort and sym::unwind are already available.

Edit: You probably want a desc_symbol method or so.

[nikomatsakis]

Nominating for discussion in a @rust-lang/lang meeting regarding the amount of process required, but I tend to agree this is one of those "no RFC required" sort of changes.

[davidhewitt]

Thanks for all the reviews - I plan to find time to update this PR over the weekend 👍

[joshtriplett]

This looks great! I know that some crates depend on the panic strategy, and will appreciate being able to use this with compile_error!.

[davidhewitt]

I'd like to see a test verifying that -C panic=abort actually causes the cfg(panic = "abort") code to run

I had a go at writing a ui test with // compile-flags: -C panic=abort, but I get an error during test execution:

error: the linked panic runtime `panic_unwind` is not compiled with this crate's panic strategy `abort`

error: aborting due to previous error

This speedbump is why I ended up with the ui test I did. Maybe there's a way to work around it?

In lieu of adding such a test I've also sprinkled this #[cfg( )] attribute on a couple of the other UI tests where appropriate. If that's not desirable I can revert some or all of those additions.

[joshtriplett]

We discussed this in the @rust-lang/lang meeting today. We're in favor of this, and we don't think this needs a full RFC. We'd like to see a feature-gate added, so that this isn't insta-stable, but otherwise this looks great.

@rfcbot merge
@rfcbot concern needs-feature-gate

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

Concerns:

• breakage-if-we-add-more-unwind-strategies resolved by Add #[cfg(panic = '...')] #74754 (comment)
• needs-feature-gate resolved by Add #[cfg(panic = '...')] #74754 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Mark-Simulacrum]

One question I have -- should we be documenting this as an extensible set, i.e., panic = "somethingelse" would perhaps eventually be added? I note that the tests here, for example, explicitly use not(panic = "abort") in a few places.

I can't think of other panic implementation strategies that would be reasonable for code to gate on, but I'm not very familiar with the possibilities here.

One option, if we're concerned about this, is to designate aborting as "special" (since it is the immediate termination option) and only add that (or just leave unwind unstable indefinitely).

[joshtriplett]

One question I have -- should we be documenting this as an extensible set, i.e., panic = "somethingelse" would perhaps eventually be added? I note that the tests here, for example, explicitly use not(panic = "abort") in a few places.

That's an excellent point. I do think this is an extensible set. We should discourage the assumption that there are only two panic strategies.

@davidhewitt Could you please use cfg(panic = "unwind") instead of cfg(not(panic = "abort")) for tests where you want to make sure panics are unwound?

[Mark-Simulacrum]

I personally suspect that no matter what we do, adding this basically stabilizes that there's only two panic strategies -- there is all but guaranteed to be downstream breakage in the wild no matter how we go about stabilizing.

This also increases the chances of crates saying "I only work with panic = abort" or so, whereas today the lack of this cfg means that it is pretty hard to make that call as a crate author (unless in some very specialized domain, where unwinding is never permitted, but those seem like major exceptions).

Looking at the use cases given so far (though I recall @joshtriplett mentioning others, maybe you can elaborate?):

#74301 notes should_panic tests behaving poorly in -Cpanic=abort mode. This is "expected" -- and also seems solvable without this feature. The compiler can annotate such tests specially in -Cpanic=abort mode and libtest can use the already existing process-per-test mode for them. Perhaps there's room for a #[in_a_process] flag as well that users can opt-in to.

#73670 (comment) discusses a limitation of our compiletest infrastructure, rather than a language limitation. It does seem true that some tests / programs require panic=abort to be sound (or panic=unwind to work), but this cfg seems like the wrong way to go about that. Like mentioned around the oneof RFC, maybe it merits a dedicated compile_error_if!(panic = abort, "....") but not general cfg support.

[nikomatsakis]

@rfcbot concern breakage-if-we-add-more-unwind-strategies

I'm registering this concern to represent @Mark-Simulacrum's excellent point that this likely means that we would "de facto" break code when/if we add new unwind strategies in the future.

[nikomatsakis]

I guess a reasonable question then is whether we can address known use cases another way and/or how much we care about that.

Another question:

One could imagine exposing the "panic strategy" as something that can be dynamically tested (perhaps with a #[non_exhaustive] enum), which might allow us to (e.g.) have compile-test skip tests or do something intelligent for when running with panic=abort (e.g., spawn a new process for tests, or at least for #[should_panic] tests).

[davidhewitt]

I personally suspect that no matter what we do, adding this basically stabilizes that there's only two panic strategies -- there is all but guaranteed to be downstream breakage in the wild no matter how we go about stabilizing.

I take this point though am not convinced it matches precedent. We already have extensible cfg sets such as target_os or target_arch. Perhaps the only difference here is that users are more likely to assume there are only two panic strategies.

If the concern is that stabilizing this flag allows crates to build "bad habits" regarding panic compatibility, I'm willing to withdraw this PR and work towards other solutions. So far the only motivations that I've seen personally are when trying to write tests. As such we could solve this differently with patches to the test frameworks (as noted e.g. in #74301).

Though others may have legitimate non-test uses. As well as Josh above, @tmandry spoke of Fuschia adding this cfg in via cmdline. Perhaps they are also willing to elaborate?

[nikomatsakis]

I do feel like this has the feel of "people can misuse the feature", which is sometimes a valid concern, but often just an unavoidable fact of life. I'm not sure what a better solution is really.

[ecstatic-morse]

@davidhewitt BTW, have you tried a -C panic=abort test in the compile-fail directory?

[tmandry]

Custom test frameworks are a major use of the feature. As it stands, libtest running in the process-per-test mode that @Mark-Simulacrum mentioned relies on the compiler activating a different part of its interface when panic=abort. (This would be the case even if we stabilized it and made it the default for panic=abort, which has been discussed.) Other test frameworks cannot rely on this level of integration with the compiler.

Another use I've had on Fuchsia is making a scoped task abstraction that always killed subprocesses when a guard is dropped or your process panics or exits. This was also for tests, incidentally. When panic = "abort" we kill all spawned subprocesses at once in our panic hook, whereas when panic = "unwind" we kill individual subprocesses when their guard's destructor is run.

One option, if we're concerned about this, is to designate aborting as "special" (since it is the immediate termination option) and only add that (or just leave unwind unstable indefinitely).

The most important information you'd want to know is

1. Will my destructors run?
2. Will my panic hooks run?
3. Is any other code in this thread going to run?

You may also want to know "Is any other code in other threads going to run?", but the answer is almost always "in general yes" and this makes writing teardown hooks tricky.

I don't know what other panic strategies are on the table exactly, but if making abort the only special one allows us to answer these in some way, then it ought to work. Though (2) brings to mind that we might also want to add panic_immediate_abort one day.

One could imagine exposing the "panic strategy" as something that can be dynamically tested (perhaps with a #[non_exhaustive] enum)

I do feel that this belongs in the realm of compile-time knowledge, especially given how pervasive the panic strategy is and the potential impact on code speed and size. One could argue that optimizations can fix that, but then you'd have compile time concerns.

It is tempting to think of a new conditional compilation mechanism that allows for things like #[non_exhaustive]. One could imagine replacing cfg attributes with some kind of const match thing, but I'm getting a little ahead of myself :)

[joshtriplett]

We discussed this in the @rust-lang/lang meeting today. (Note: @nikomatsakis, who raised the concern, was not present, but @Mark-Simulacrum, who the concern was raised on behalf of, was.)

We agreed that while it ispossible for code that uses cfg(panic = '...') to not account for future panic strategies (e.g. a crate that uses catch_unwind having a compile_error! on cfg(panic="abort") instead of cfg(not(panic="unwind"))), the same thing is possible for many other kinds of cfg. For instance, it's also possible to assume that cfg(not(unix)) means Windows, or that #[cfg(not(target_pointer_width = "64"))] means 32-bit. A crate should try to account for future possibilities, but cannot necessarily anticipate every possible new alternative configuration. We did not feel that was a reason to avoid providing this.

@nikomatsakis, @Mark-Simulacrum, thoughts?

[nikomatsakis]

@rfcbot resolve breakage-if-we-add-more-unwind-strategies

I'm persuaded by the above logic. =)

[davidhewitt]

CI all green. @ecstatic-morse this is now ready for another round of review (and hopefully to merge)! Thanks again for the reviews and advice.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[ecstatic-morse]

Sorry for the delay @davidhewitt. I wasn't able to do any Rust stuff this week. r=me after a rebase (bors r=ecstatic-morse).

@bors delegate+ rollup

[bors]

✌️ @davidhewitt can now approve this pull request

[davidhewitt]

@bors r=ecstatic-morse

[bors]

📌 Commit 3e6acd8 has been approved by ecstatic-morse

[jonas-schievink]

@bors r- failed in #78328 (comment)

[davidhewitt]

Sorry about that; I've pushed a commit which I've verified fixes the build on the wasm target which failed in the merge pipeline. @ecstatic-morse can I squash and r+ again?

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[davidhewitt]

I've rebased and squashed; I'm going to assume it's okay for me to r+ again.

@bors r=ecstatic-morse

[bors]

📌 Commit 8d43b3c has been approved by ecstatic-morse