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.

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

[Miri] Use a session variable instead of checking for an env var always #69888

Merged

Conversation

@wesleywiser
Copy link
Member

wesleywiser commented Mar 10, 2020

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.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Mar 10, 2020

Member
Suggested change
/// 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).
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 10, 2020

LGTM aside from the nit, but this is also touching global compiler infrastructure that I know little about... @oli-obk does this look reasonable?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Mar 10, 2020

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

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 10, 2020

But then how is Miri going to change its value at run-time?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Mar 10, 2020

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

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 10, 2020

you can't change it while it runs, but that was never a use case

Yes it was, Miri does exactly that.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Mar 10, 2020

That's OK, that happens before any error is ever thrown so the lazy in init hasn't happened yet

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 10, 2020

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.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Mar 10, 2020

Right. I guess let's do this for now and reconsider if/when someone dislikes it

Copy link
Member

RalfJung left a comment

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.
@wesleywiser wesleywiser force-pushed the wesleywiser:miri_exception_env_var_to_session_var branch from ea85732 to 5357f83 Mar 10, 2020
@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Mar 11, 2020

@bors r=RalfJung

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 11, 2020

📌 Commit 5357f83 has been approved by RalfJung

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 11, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

bors added a commit that referenced this pull request Mar 11, 2020
Rollup of 8 pull requests

Successful merges:

 - #66472 (--show-coverage json)
 - #69603 (tidy: replace `make check` with `./x.py test` in 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
@bors bors merged commit b1471e0 into rust-lang:master Mar 11, 2020
4 checks passed
4 checks passed
pr Build #20200310.75 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.