Skip to content

Pass -Cpanic=abort for the panic_abort crate #140254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Apr 24, 2025

The panic_abort crate must be compiled with panic=abort, but cargo doesn't allow setting the panic strategy for a single crate the usual way using panic="abort", but luckily per-package rustflags do allow this. Bootstrap previously handled this in its rustc wrapper, but for example the build systems of cg_clif and cg_gcc don't use the rustc wrapper, so they would either need to add one, patch the standard library or be unable to build a sysroot suitable for both panic=abort and panic=unwind (as is currently the case).

Required for rust-lang/rustc_codegen_cranelift#1567

@rustbot
Copy link
Collaborator

rustbot commented Apr 24, 2025

r? @petrochenkov

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 24, 2025
@petrochenkov
Copy link
Contributor

It's less of a practical concern, but shouldn't panic_unwind also implicitly use panic=unwind?

I guess the proper solution would be to move fn panic_strategy from sess to tcx so it can use tcx.is_panic_runtime(), and extend the #![panic_runtime] attribute to #![panic_runtime = "abort|unwind"] to avoid relying on the crate name.
What do you think?

@petrochenkov
Copy link
Contributor

Ah, crate loader still searches for panic_(unwind,abort) by crate name, but we can at least sanity check that the crate found that way is indeed #![panic_runtime = "unwind" or "abort"] respectively.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2025
@bjorn3
Copy link
Member Author

bjorn3 commented Apr 25, 2025

It's less of a practical concern, but shouldn't panic_unwind also implicitly use panic=unwind?

If panic=abort was used while compiling the standard library, panic_unwind won't be used anyway as any panic=abort dependency will force panic=abort to be used and thus panic_unwind doesn't get linked in.

I guess the proper solution would be to move fn panic_strategy from sess to tcx so it can use tcx.is_panic_runtime(), and extend the #![panic_runtime] attribute to #![panic_runtime = "abort|unwind"] to avoid relying on the crate name.
What do you think?

The LLVM backend needs to know if the current crate uses panic=unwind or panic=abort before any source code gets parsed:

&& sess.panic_strategy() == PanicStrategy::Unwind

Ah, crate loader still searches for panic_(unwind,abort) by crate name, but we can at least sanity check that the crate found that way is indeed #![panic_runtime = "unwind" or "abort"] respectively.

There already is a sanity check that the found crate is a panic runtime:

if !data.is_panic_runtime() {
self.dcx().emit_err(errors::CrateNotPanicRuntime { crate_name: name });
}

@bjorn3 bjorn3 force-pushed the rustc_panic_abort_abort branch from 88b79e8 to 7e58a46 Compare June 18, 2025 08:04
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 20, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2025
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 20, 2025

📌 Commit 7e58a46 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2025
@klensy
Copy link
Contributor

klensy commented Jun 20, 2025

Should be possible with https://doc.rust-lang.org/cargo/reference/unstable.html#profile-rustflags-option ?

Something like (in case if it allowed to pass package name, didn't checked that):

[profile.release.package.panic_abort]
rustflags = [ "-C", "panic=abort" ]

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 20, 2025

Didn't know that was possible already. That is a much better solution.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Jun 20, 2025

Wouldn't that break -Zbuild-std? Actually, probably not, since this was done in the bootstrap wrapper before, which is also not used for -Zbuild-std.

@bjorn3 bjorn3 force-pushed the rustc_panic_abort_abort branch from 7e58a46 to 929e632 Compare June 20, 2025 11:32
@reneleonhardt
Copy link

reneleonhardt commented Jun 20, 2025

Off-topic: Just wondering, Rust 1.87 depends on LLVM 20, but the Jobs are named 19? 🤔

aarch64-gnu-llvm-19-1

# Try to keep the LLVM version here in sync with src/ci/scripts/install-clang.sh
LLVM=llvmorg-20.1.0-rc2

# Try to keep this in sync with src/ci/docker/scripts/build-clang.sh
LLVM_VERSION="20.1.3"

https://github.com/llvm/llvm-project/releases/tag/llvmorg-20.1.7

Why not use renovate comments instead to group those 2 scripts together?
Or even easier, have only a central constant which both scripts use? 😄

The panic_abort crate must be compiled with panic=abort, but cargo
doesn't allow setting the panic strategy for a single crate the usual
way using panic="abort", but luckily per-package rustflags do allow
this. Bootstrap previously handled this in its rustc wrapper, but for
example the build systems of cg_clif and cg_gcc don't use the rustc
wrapper, so they would either need to add one, patch the standard
library or be unable to build a sysroot suitable for both panic=abort
and panic=unwind (as is currently the case).
@bjorn3 bjorn3 force-pushed the rustc_panic_abort_abort branch from 929e632 to a0badba Compare June 20, 2025 12:14
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 20, 2025

Wouldn't that break -Zbuild-std? Actually, probably not, since this was done in the bootstrap wrapper before, which is also not used for -Zbuild-std.

I don't think it should break -Zbuild-std.

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 20, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Jun 20, 2025

Off-topic: Just wondering, Rust 1.87 depends on LLVM 20, but the Jobs are named 19? 🤔

aarch64-gnu-llvm-19-1

# Try to keep the LLVM version here in sync with src/ci/scripts/install-clang.sh
LLVM=llvmorg-20.1.0-rc2

# Try to keep this in sync with src/ci/docker/scripts/build-clang.sh
LLVM_VERSION="20.1.3"

https://github.com/llvm/llvm-project/releases/tag/llvmorg-20.1.7

Why not use renovate comments instead to group those 2 scripts together? Or even easier, have only a central constant which both scripts use? 😄

We test rustc with the current LLVM (20) and one version before (19). The scripts that you link in different environments (inside/outside) Docker, and on different OSes, and they are not always fully synchronized.

@reneleonhardt
Copy link

We test rustc with the current LLVM (20) and one version before (19). The scripts that you link in different environments (inside/outside) Docker, and on different OSes, and they are not always fully synchronized.

Thanks for explaining, I created a PR to merge those versions if you're interested: #142786

@petrochenkov
Copy link
Contributor

Cargo works in mysterious ways, why does it allow to specify rustflags per-profile, but doesn't allow to just specify rustflags regardless of profile?

In any case, from reading https://doc.rust-lang.org/cargo/reference/profiles.html it looks like all profiles (including the remaining built-in ones) ultimately inherit from either dev or release, so what this PR does should work.
@bors r+

@bors
Copy link
Collaborator

bors commented Jun 21, 2025

📌 Commit a0badba has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2025
@bjorn3 bjorn3 changed the title Make rustc implicitly use panic=abort for the panic_abort crate Pass -Cpanic=abort for the panic_abort crate Jun 21, 2025
jdonszelmann added a commit to jdonszelmann/rust that referenced this pull request Jun 21, 2025
…petrochenkov

Pass -Cpanic=abort for the panic_abort crate

The panic_abort crate must be compiled with panic=abort, but cargo doesn't allow setting the panic strategy for a single crate the usual way using `panic="abort"`, but luckily per-package rustflags do allow this. Bootstrap previously handled this in its rustc wrapper, but for example the build systems of cg_clif and cg_gcc don't use the rustc wrapper, so they would either need to add one, patch the standard library or be unable to build a sysroot suitable for both panic=abort and panic=unwind (as is currently the case).

Required for rust-lang/rustc_codegen_cranelift#1567
bors added a commit that referenced this pull request Jun 21, 2025
Rollup of 6 pull requests

Successful merges:

 - #140254 (Pass -Cpanic=abort for the panic_abort crate)
 - #142600 (Port `#[rustc_pub_transparent]` to the new attribute system)
 - #142617 (improve search graph docs, reset `encountered_overflow` between reruns)
 - #142641 (Generate symbols.o for proc-macros too)
 - #142776 (All HIR attributes are outer)
 - #142800 (integer docs: remove extraneous text)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 22, 2025
…petrochenkov

Pass -Cpanic=abort for the panic_abort crate

The panic_abort crate must be compiled with panic=abort, but cargo doesn't allow setting the panic strategy for a single crate the usual way using `panic="abort"`, but luckily per-package rustflags do allow this. Bootstrap previously handled this in its rustc wrapper, but for example the build systems of cg_clif and cg_gcc don't use the rustc wrapper, so they would either need to add one, patch the standard library or be unable to build a sysroot suitable for both panic=abort and panic=unwind (as is currently the case).

Required for rust-lang/rustc_codegen_cranelift#1567
bors added a commit that referenced this pull request Jun 22, 2025
Rollup of 10 pull requests

Successful merges:

 - #140254 (Pass -Cpanic=abort for the panic_abort crate)
 - #142594 (Add DesugaringKind::FormatLiteral)
 - #142600 (Port `#[rustc_pub_transparent]` to the new attribute system)
 - #142617 (improve search graph docs, reset `encountered_overflow` between reruns)
 - #142641 (Generate symbols.o for proc-macros too)
 - #142747 (rustdoc_json: conversion cleanups)
 - #142776 (All HIR attributes are outer)
 - #142800 (integer docs: remove extraneous text)
 - #142850 (remove asm_goto feature annotation, for it is now stabilized)
 - #142860 (Notify me on tidy changes)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 22, 2025
…petrochenkov

Pass -Cpanic=abort for the panic_abort crate

The panic_abort crate must be compiled with panic=abort, but cargo doesn't allow setting the panic strategy for a single crate the usual way using `panic="abort"`, but luckily per-package rustflags do allow this. Bootstrap previously handled this in its rustc wrapper, but for example the build systems of cg_clif and cg_gcc don't use the rustc wrapper, so they would either need to add one, patch the standard library or be unable to build a sysroot suitable for both panic=abort and panic=unwind (as is currently the case).

Required for rust-lang/rustc_codegen_cranelift#1567
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants