Skip to content

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
Member

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
Member

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
bors added a commit that referenced this pull request Jun 22, 2025
Rollup of 9 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)
 - #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
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)
 - #142600 (Port `#[rustc_pub_transparent]` to the new attribute system)
 - #142617 (improve search graph docs, reset `encountered_overflow` between reruns)
 - #142747 (rustdoc_json: conversion cleanups)
 - #142776 (All HIR attributes are outer)
 - #142800 (integer docs: remove extraneous text)
 - #142841 (Enable fmt-write-bloat for Windows)
 - #142845 (Enable textrel-on-minimal-lib for Windows)
 - #142850 (remove asm_goto feature annotation, for it is now stabilized)
 - #142860 (Notify me on tidy changes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit aef8a76 into rust-lang:master Jun 22, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 22, 2025
@bjorn3 bjorn3 deleted the rustc_panic_abort_abort branch June 22, 2025 12:48
rust-timer added a commit that referenced this pull request Jun 22, 2025
Rollup merge of #140254 - bjorn3:rustc_panic_abort_abort, r=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
tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request Jul 3, 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