-
Notifications
You must be signed in to change notification settings - Fork 13k
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
generate MIR for the nightlies' standard library #38350
Conversation
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
r? @eddyb |
Does this increase build times? If so, we may want to think about which one we should optimize for. I personally would prefer decreasing build times over decreasing size. |
Why would the end user ever care about having MIR for the libstd? |
Sorry for the basic questions, but does this change how programs compile when they use std items? For example: right now, a development build using |
It should not. In #38217 we moved the logic that decided whether to inline code from metadata creation to trans. If anything, that PR has a bug, not this one.
Because they'll be able to interpret arbitrary Rust code through miri.
I haven't tested, but to the best of my knowledge metadata encoding takes much less time than compilation |
I find this argument to not be persuasive, I’d understand if the implementation only unconditionally included MIR in libstd, libcollections and libcore only, not every single crate. The distribution is already pretty big and 10% increase for the whole distribution is not trivial. I also have my doubts about advertising this particular use-case. Remember that the flag in question is unstable. While it doesn’t prevent anybody from relying on miri, it doesn’t make any behaviour stable either, What this PR does is including MIR into stable libraries unconditionally as well, enabling people to use stable libraries on miri and therefore allowing them to complain once/if we decide we do not actually want to put all the MIR into all the crates. |
How about only generating it for nightlies? I can totally see miri not being a relevant use case for this change. I'm fine with closing this and finding another solution. But I at least wanted to try ;) |
This pr does not unconditionally enable mir for all crates. Downstream everyone still needs to enable the flag. It also doesn't allow miri to suddenly work on stable. But yes, miri can now interpret any crate, stable or not and undoing this change will break that ability. The obvious solution is to use xargo, which will custom build libcore. Not sure what is necessary to make it compile libstd though. I'll try the xargo route |
I would not object against including full MIR for |
// for the stdlib | ||
// Don't do this for stage0 yet, since always_encode_mir isn't part of it yet | ||
if stage != "0" { | ||
cmd.arg("-Zalways_encode_mir"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it dashes, not underscores? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, this was the correct version (especially since it worked)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's... weird? It sounds like a bug, let me check something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no we accept both underscores and dashes, but only dashes are canonical (I tried with -C debug-assertions
and -C debug_assertions
, both appear to work).
I changed it so the MIR is only encoded on nightly and dev |
LGTM. CC @rust-lang/compiler @rust-lang/tools |
This has become obsolete due to https://users.rust-lang.org/t/xargo-v0-3-0-released-build-your-own-std/8571 . |
I got a little overexcited. |
Hmm. I also feel the motivation for this is somewhat weak. Although we don't particularly optimize for it, distribution size does matter for some users. It'd be nice if one could "opt in" to it. Would we ever want this for |
If the motivation is to make miri work, there may be other ways. For example xargo could produce a miri std pretty easily. I don't know that this has a path to stabilization, but distributing special binaries in nightly strongly implies it does. I would not want to do this unless there's clear agreement that this is a thing we want to do on stable. And even then I might suggest we do it another way: by putting mir on all channels but making it unusable without nightly. That seems more consistent with how nightly is currently used - feature exists, can't be used. |
Ok, I'll investigate xargo more. |
Run the miri test suite on the aux builder and travis Reopen of #38350 see #43340 (comment) for earlier discussion Rationale for running miri's test suite in rustc's CI is that miri currently contains many features that we want in const eval in the future, and these features would break if the test suite is not run. fixes #44077 r? @nikomatsakis cc @eddyb
sizes of files in /lib after this PR:
sizes of files in /lib before this PR:
so there's about a 10% increase in library size