Skip to content
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.

Sign up for GitHub

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

Don't call env::set_var in rustc_driver::install_ice_hook #125063

Merged
merged 1 commit into from
May 13, 2024
Merged

Don't call env::set_var in rustc_driver::install_ice_hook #125063

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions compiler/rustc_driver_impl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#![allow(internal_features)]
#![feature(decl_macro)]
#![feature(let_chains)]
#![feature(panic_backtrace_config)]
#![feature(panic_update_hook)]
#![feature(result_flattening)]

Expand Down Expand Up @@ -1317,8 +1318,8 @@ pub fn install_ice_hook(
// by the user. Compiler developers and other rustc users can
// opt in to less-verbose backtraces by manually setting "RUST_BACKTRACE"
// (e.g. `RUST_BACKTRACE=1`)
if std::env::var_os("RUST_BACKTRACE").is_none() {
std::env::set_var("RUST_BACKTRACE", "full");
if env::var_os("RUST_BACKTRACE").is_none() {
panic::set_backtrace_style(panic::BacktraceStyle::Full);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only sets the style, but without the env. variable, this will think that backtraces are disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, TIL.

Do you know if that is relevant in this context?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I thought that it is, otherwise I wouldn't comment, but now that I look at it, maybe it isn't. The default panic hook does not seem to use Backtrace::capture and reads the style directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, @Kobzol!

Yes, this is relevant here. We want to enable backtraces (unless there is an explicit setting for it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the compiler calls Backtrace::capture somewhere explicitly, or if it just depends on the default panic hook. If the latter is true, then maybe we really don't need to set the env. variable.

That being said, wrapping this line with unsafe seems fine to me, it's not like the compiler uses #[forbid_unsafe_code)] anyway, and this line has been clearly working fine in the past.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to make the default hook here always print a backtrace. We can't change the default hook to use Backtrace::force_capture. We would have to copy it from libstd into this function instead and make sure to keep it in sync.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to make the default hook here always print a backtrace.

This behavior is still preserved, unless I'm mistaken. As @Kobzol pointed out, the default hook does not depend on the RUST_BACKTRACE env var. My comment about force_capture was wrt to @Kobzol's concern that other (possibly future) code somewhere in the compiler might rely on this code setting the RUST_BACKTRACE env var if no explicit setting is provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are uses of Backtrace::capture in compiler/rustc_errors/src/lib.rs and compiler/rustc_log/src/lib.rs. Should those be changed somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on whether these call sites want to depend on the RUST_BACKTRACE env var or not.

For rustc_log, it looks like that should indeed use force_capture.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #125355 for the rustc_log case.

I'm less sure about the compiler/rustc_errors/src/lib.rs cases but I think these should use force_capture too. @tbu-, do you want to open a PR, so we can engage with the diagnostics team?

Thanks for reporting this, @RalfJung!

}

let using_internal_features = Arc::new(std::sync::atomic::AtomicBool::default());
Expand Down
Loading