Merge miri into librustc_mir

[oli-obk]

r? @eddyb

cc @solson @RalfJung @dwrensha @rust-lang/compiler

My plan is:

1. move miri here (this PR)
2. get the tests and everything working again in the miri repository
3. then actually do something with miri in the compiler
   • test miri as part of the rustc test suite
     • possibly by creating a new attribute that will run miri while compiling run-pass tests
   • compute constants in parallel with HIR-const-eval
   • report mismatches between HIR-const-eval and miri
   • scrap HIR-const-eval 🎉
   • compute constants in parallel with trans-const-eval
   • report mismatches between trans-const-eval and miri
   • scrap trans-const-eval

wrt the new git initial commit: rust-lang/miri#105 (comment)

[RalfJung]

I am worried about moving miri here before having a plan for how to continue running its test suite automatically whenever it gets changed.

[oli-obk]

Maybe we should revisit #38350 (including MIR for the entire standard library, which introduces a 10% increase in lib size)

The new motivation is that miri needs to keep getting tested, because the features we will want to have in the future (after RFCs) are working already, but without tests they will very likely break.

Assuming that const fns become more common with more features becoming legal in const eval, this 10% increase might come anyway (or nearly at least).

[RalfJung]

The new motivation is that miri needs to keep getting tested, because the features we will want to have in the future (after RFCs) are working already, but without tests they will very likely break.

Also, miri has other uses, like my "Types as Contracts" work and your priroda.

[oli-obk]

priroda will still work with the xargo method. As will cargo miri.

[RalfJung]

Yes, but it relies on the non-CTFE parts of miri continuing to work -- which you argued in the text I quoted will break if we stop testing it.

[eddyb]

I haven't looked yet, but are the non-CTFE parts here, not abstracted away? What I had in mind (which we could do later) is have the interpreter parametrized by a "machine" trait which could choose how abstract to be and would handle all the C function emulation and whatnot.

[oli-obk]

I haven't looked yet, but are the non-CTFE parts here, not abstracted away?

no

What I had in mind (which we could do later) is have the interpreter parametrized by a "machine" trait which could choose how abstract to be and would handle all the C function emulation and whatnot.

While this is trivial for c functions and friends, weaving this through miri for every relevant location will get very messy. There's all kinds of locations where the decision between CTFE and interpretation mode is done. I've already marked most with an if self.const_env() check. Some of these checks are in the middle of binary operations. Others just abort if certain conditions for CTFE aren't met.

[eddyb]

It's fine to have some primitive restrictions for the more internal details, but the "machine" could have different flags for different categories. The one thing I'm not sure is readily doable this way is integer pointers but the "machine" could have a way to attempt memory accesses on integer addresses (which would always fail for CTFE).

[oli-obk]

It's fine to have some primitive restrictions for the more internal details, but the "machine" could have different flags for different categories. The one thing I'm not sure is readily doable this way is integer pointers but the "machine" could have a way to attempt memory accesses on integer addresses (which would always fail for CTFE).

Ok, I can get behind that. But that still leaves the issue of code that is in miri, will most likely be part of CTFE in the future, but isn't right now. Should these also get moved back to the miri repository? Where do we draw the line, and how do we test those paths properly?

[eddyb]

@oli-obk I think they should be separated, if nothing else to avoid the risk of accidentally turning them on and exposing people to e.g. dynamic allocation at compile-time (before we're sure how it will actually work). cc @alexcrichton about cargotest(?), for running external miri tests on bors.

[alexcrichton]

Er sorry, I have very little context and this is a massive change. At first glance there's licensing to worry about, source structure with a new /tests folder, and 13k new lines of code. And you're also wondering how to integrate this into rustbuild?

Is there perhaps a comprehensive summary of what's happening here so I might be able to help?

[oli-obk]

So the high level goal is to get miri into the compiler in order to scrap the two const evaluators that are currently in the compiler and just use miri. Miri is much more extensible than the existing const evaluators, so new RFCs can then be opened in order to activate or implement miri features in const eval.

The goal of this PR is to discuss which parts of miri go into the compiler now.

The 13k lines of code will be reduced significantly before merging, since rustc doesn't need everything.

The issue with testing is that currently miri is way overpowered compared to const eval. So if we just test miri by testing whether constant evaluation works the same with miri as with the builtin const eval, we'll be ignoring an enormous part of miri code. This ignored part will probably break very quickly due to the lack of testing and the high rate of change that MIR and other internals code has.

Much of the "untested" code would be required some day in the future by some RFC, so we don't want it to break. Thus the idea was to run miri's test suite along with the compiler's test suite, but keep the non-CTFE parts outside the rustc repository.

Miri's test suite custom builds a libstd with -Zalways-encode-mir via xargo and then runs miri against a list of tests with the custom libstd.

[solson]

there's licensing to worry about

Miri has been intentionally licensed the same as rustc from the beginning.

[alexcrichton]

@oli-obk thanks for the explanation! I think it's totally fine to have miri tests be "special" and only works sometimes or perhaps not others. It sounds like what we may want is a subdirectory for all miri tests (probably under the miri directory), new rules in src/bootstrap/step.rs and definitions in src/bootstrap/check.rs to actually execute these tests, and additional support to bootstrap with -Zalways-encode-mir. I'd recommend updating the x86_64-gnu-aux builder with these changes and avoid updating any other builders.

@solson yeah that's great! To me "licensing worry" is equivalent to "something about licensing changed" and this PR, for example, currently adds new license files, which to me just raises red flags. (if we can agree those should be removed that sounds fine)

[oli-obk]

It sounds like what we may want is a subdirectory for all miri tests

There's also the option of adding an attribute #![miri_test] which causes a run-pass test to be run via miri before while it is compiled. There's quite some overlap between run-pass tests and miri's tests (since we copied many run-pass tests over to miri).

The same would go for compile-fail or ui tests.

I think it's totally fine to have miri tests be "special" and only works sometimes or perhaps not others

So failing miri tests wouldn't block PRs from passing?

[eddyb]

@alexcrichton What I was interested in, was whether non-CTFE miri tests could live in the out-of-tree miri and get tested like several crates are tested (e.g. regex and cargo, AFAIK) by bors.
I would very much want to avoid having the libc emulation functionality in-tree, as it's not something rustc should know to do, and there are risks of a more complex const fn system accidentally exposing that functionality to unstable users, or, worse, stable ones.

[alexcrichton]

@oli-obk oh if that's the case then we could have a new // miri-test or something like that directive for either a whitelist or a blacklist, depending on what the majority is. Essentially we'd just add a new compiletest mode here perhaps.

@eddyb certainly that can always be done! It's just a matter of changing this array

[solson]

this PR, for example, currently adds new license files, which to me just raises red flags

@alexcrichton Ah, perfectly understandable. :)

Yeah, I assume we can just ditch the new license files - the new code falls under the same top-level license declarations in this repo.

[RalfJung]

@eddyb that sounds like a nice plan. It's still not straight-forward though, with miri's test suite currently depending on rustup to fetch rust-src and then xargo to compile it. That wouldn't work with cargotest and things are not in rustup yet, right?
Could we instead use the sources of whatever we just built, and point xargo there?

[oli-obk]

We don't need xargo at all I think. As long as we start generating fullmir libstd on the x86_64-gnu-aux builder and test it there, everything should be good.

Basically I'd add a config.toml flag for testing non CTFE miri, and only set that flag for the one builder (and travis?).

[oli-obk]

Closing this to keep the queue clean. The changes are in progress inside the miri repository. I'll open a new PR when they are done.