Skip to content

when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation #124116

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

RalfJung
Copy link
Member

@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2024

r? @Nilstrieb

rustbot has assigned @Nilstrieb.
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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the miri-rust-backtrace branch from db97444 to 3e63398 Compare April 18, 2024 10:05
@Noratrieb
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 18, 2024

📌 Commit 3e63398 has been approved by Nilstrieb

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 Apr 18, 2024
@workingjubilee
Copy link
Member

Huh.

@RalfJung Would you rather we always print a backtrace when we panic while running in Miri? That was one of the solutions discussed in rust-lang/miri#2855

Note this is different from "setting RUST_BACKTRACE=1", it won't affect user library behavior.

@workingjubilee
Copy link
Member

Well, approximately-equal-to one of the solutions.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 18, 2024
…lstrieb

when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation

Fixes rust-lang/miri#2855
@RalfJung
Copy link
Member Author

@RalfJung Would you rather we always print a backtrace when we panic while running in Miri? That was one of the solutions discussed in rust-lang/miri#2855

I don't think that would be a good default for Miri, just like it is not a good default for regular execution.

@workingjubilee
Copy link
Member

🤔 It would seem to me you usually would want a backtrace? But okay!

@saethlin
Copy link
Member

just like it is not a good default for regular execution.

Funny you should say that. Every codebase at my employer has this at the top of main:

    if std::env::var("RUST_BACKTRACE").is_err() {
        std::env::set_var("RUST_BACKTRACE", "1");
    }    

And I know from a discussion on Mastodon a few days ago that we are not the only ones doing this.

@workingjubilee
Copy link
Member

workingjubilee commented Apr 18, 2024

Yes. I think we would want to default to it on, except we may wish to get better at printing shorter backtraces first, so that RUST_BACKTRACE=1 strikes the right balance of output and informative (or maybe RUST_BACKTRACE=0 just becomes a very short backtrace by default and =1 is longer).

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 18, 2024
…lstrieb

when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation

Fixes rust-lang/miri#2855
@RalfJung
Copy link
Member Author

Okay, let me rephrase: I don't think Miri's default should be different from that of regular Rust.

@workingjubilee
Copy link
Member

Ah, I see! I only figured that since Miri strongly functions as an error-reporting tool, that you might consider deviating on this point acceptable. But I do understand preferring to avoid differences in execution, considering it mostly is good at error-reporting by being good at simulation.

@RalfJung
Copy link
Member Author

Almost every time we made Miri divert from regular execution, we eventually decided that was a bad idea. Maybe this is one of the few exceptions, but it doesn't seem obvious to me.

@saethlin
Copy link
Member

Yeah; on balance I think the extra note is the best approach.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Apr 18, 2024
…lstrieb

when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation

Fixes rust-lang/miri#2855
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 18, 2024
…kingjubilee

Rollup of 6 pull requests

Successful merges:

 - rust-lang#117919 (Introduce perma-unstable `wasm-c-abi` flag)
 - rust-lang#123571 (Correctly change type when adding adjustments on top of `NeverToAny`)
 - rust-lang#123752 (Properly handle emojis as literal prefix in macros)
 - rust-lang#123980 ( Add an opt-in to store incoming edges in `VecGraph` + misc)
 - rust-lang#124110 (Fix negating `f16` and `f128` constants)
 - rust-lang#124116 (when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Apr 18, 2024
…lstrieb

when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation

Fixes rust-lang/miri#2855
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117919 (Introduce perma-unstable `wasm-c-abi` flag)
 - rust-lang#123406 (Force exhaustion in iter::ArrayChunks::into_remainder)
 - rust-lang#123752 (Properly handle emojis as literal prefix in macros)
 - rust-lang#123935 (Don't inline integer literals when they overflow - new attempt)
 - rust-lang#123980 ( Add an opt-in to store incoming edges in `VecGraph` + misc)
 - rust-lang#124019 (Use raw-dylib for Windows synchronization functions)
 - rust-lang#124110 (Fix negating `f16` and `f128` constants)
 - rust-lang#124112 (Fix ICE when there is a non-Unicode entry in the incremental crate directory)
 - rust-lang#124116 (when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2024
…kingjubilee

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123406 (Force exhaustion in iter::ArrayChunks::into_remainder)
 - rust-lang#123752 (Properly handle emojis as literal prefix in macros)
 - rust-lang#123935 (Don't inline integer literals when they overflow - new attempt)
 - rust-lang#123980 ( Add an opt-in to store incoming edges in `VecGraph` + misc)
 - rust-lang#124019 (Use raw-dylib for Windows synchronization functions)
 - rust-lang#124110 (Fix negating `f16` and `f128` constants)
 - rust-lang#124116 (when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c8d58fa into rust-lang:master Apr 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2024
Rollup merge of rust-lang#124116 - RalfJung:miri-rust-backtrace, r=Nilstrieb

when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation

Fixes rust-lang/miri#2855
@rustbot rustbot added this to the 1.79.0 milestone Apr 19, 2024
@RalfJung RalfJung deleted the miri-rust-backtrace branch April 20, 2024 06:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

By default, RUST_BACKTRACE=1 silently has no effect on Miri, even though it is suggested in panic messages
7 participants