Don't call env::set_var in rustc_driver::install_ice_hook
#125063
Conversation
Modifying an environment variable would make the function unsafe to call.
|
rustbot has assigned @michaelwoerister. Use |
rustbot
added
S-waiting-on-review
| 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); |
There was a problem hiding this comment.
This only sets the style, but without the env. variable, this will think that backtraces are disabled.
There was a problem hiding this comment.
Interesting, TIL.
Do you know if that is relevant in this context?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch, @Kobzol!
Yes, this is relevant here. We want to enable backtraces (unless there is an explicit setting for it).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…lwoerister Don't call `env::set_var` in `rustc_driver::install_ice_hook` Modifying an environment variable would make the function unsafe to call.
…lwoerister Don't call `env::set_var` in `rustc_driver::install_ice_hook` Modifying an environment variable would make the function unsafe to call.
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#119515 (style-guide: Format single associated type `where` clauses on the same line) - rust-lang#119959 ([meta] Clarify prioritization alert) - rust-lang#123817 (Stabilize `seek_seek_relative`) - rust-lang#124532 (elaborate obligations in coherence) - rust-lang#125063 (Don't call `env::set_var` in `rustc_driver::install_ice_hook`) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#119515 (style-guide: Format single associated type `where` clauses on the same line) - rust-lang#119959 ([meta] Clarify prioritization alert) - rust-lang#123817 (Stabilize `seek_seek_relative`) - rust-lang#125063 (Don't call `env::set_var` in `rustc_driver::install_ice_hook`) - rust-lang#125071 (Migrate rustdoc target spec json path) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#125063 - tbu-:pr_set_ice_hook_env, r=michaelwoerister Don't call `env::set_var` in `rustc_driver::install_ice_hook` Modifying an environment variable would make the function unsafe to call.
…pture, r=nnethercote Use Backtrace::force_capture instead of Backtrace::capture in rustc_log After rust-lang#125063, the compiler and custom drivers won't automatically set the RUST_BACKTRACE environment variable anymore, so we have to call `Backtrace::force_capture` instead of `Backtrace::capture` to unconditionally capture a backtrace. rustc_log handles enabling backtraces via env vars itself, so we don't want RUST_BACKTRACE to make a difference.
…pture, r=nnethercote Use Backtrace::force_capture instead of Backtrace::capture in rustc_log After rust-lang#125063, the compiler and custom drivers won't automatically set the RUST_BACKTRACE environment variable anymore, so we have to call `Backtrace::force_capture` instead of `Backtrace::capture` to unconditionally capture a backtrace. rustc_log handles enabling backtraces via env vars itself, so we don't want RUST_BACKTRACE to make a difference.
Rollup merge of rust-lang#125355 - michaelwoerister:rust_log_force_capture, r=nnethercote Use Backtrace::force_capture instead of Backtrace::capture in rustc_log After rust-lang#125063, the compiler and custom drivers won't automatically set the RUST_BACKTRACE environment variable anymore, so we have to call `Backtrace::force_capture` instead of `Backtrace::capture` to unconditionally capture a backtrace. rustc_log handles enabling backtraces via env vars itself, so we don't want RUST_BACKTRACE to make a difference.
…nnethercote Use Backtrace::force_capture instead of Backtrace::capture in rustc_log After rust-lang/rust#125063, the compiler and custom drivers won't automatically set the RUST_BACKTRACE environment variable anymore, so we have to call `Backtrace::force_capture` instead of `Backtrace::capture` to unconditionally capture a backtrace. rustc_log handles enabling backtraces via env vars itself, so we don't want RUST_BACKTRACE to make a difference.
Modifying an environment variable would make the function unsafe to call.