[Miri] Use a session variable instead of checking for an env var always by wesleywiser · Pull Request #69888 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
In CTFE heavy code, checking the env var everytime is inefficient. We
can do a lot better by using a Session variable instead.
r? @RalfJung
Part of #69297
| pub enum CtfeBacktrace { |
|---|
| /// Do nothing special, return the error as usual without a backtrace. |
| Disabled, |
| /// Capture a backtrace at the point the error is created and return it in the error. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Capture a backtrace at the point the error is created and return it in the error. |
|---|
| /// Capture a backtrace at the point the error is created and return it in the error |
| /// (to be printed later if/when the error ever actually gets shown to the user). |
LGTM aside from the nit, but this is also touching global compiler infrastructure that I know little about... @oli-obk does this look reasonable?
Yea this is OK, but at this point you could also just use a lazy static inside that function without affecting any code outside of it
But then how is Miri going to change its value at run-time?
You can lazily initialize from the env var. Since the initialization only ever happens once, just like in the current PR, you can't change it while it runs, but that was never a use case
you can't change it while it runs, but that was never a use case
Yes it was, Miri does exactly that.
That's OK, that happens before any error is ever thrown so the lazy in init hasn't happened yet
The intend is for that code to run after MIR got generated and const-eval happened. It gets called in after_analysis. That is why we delay the logger initialization in the first place, to log only Miri and not CTFE, to the extend possible.
Otherwise there'd be no reason for this entire init_early_loggers/init_late_loggers business, we'd just initialize everything early.
Right. I guess let's do this for now and reconsider if/when someone dislikes it
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with the doc nit fixed.
In CTFE heavy code, checking the env var everytime is inefficient. We
can do a lot better by using a Session variable instead.
📌 Commit 5357f83 has been approved by RalfJung
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened
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
bors added a commit that referenced this pull request
Rollup of 8 pull requests
Successful merges:
- #66472 (--show-coverage json)
- #69603 (tidy: replace
make checkwith./x.py testin documentation) - #69760 (Improve expression & attribute parsing)
- #69828 (fix memory leak when vec::IntoIter panics during drop)
- #69850 (panic_bounds_check: use caller_location, like PanicFnLangItem)
- #69876 (Add long error explanation for E0739)
- #69888 ([Miri] Use a session variable instead of checking for an env var always)
- #69893 (librustc_codegen_llvm: Use slices instead of 0-terminated strings)
Failed merges:
r? @ghost
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.