Redo cargo-miri logic

[RalfJung]

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: cargo miri run/test $FLAGS (almost) literally invokes cargo run/test $FLAGS but with some environment variables set so that we can control what happens:

• RUSTC_WRAPPER is set so that we get invoked instead of rustc. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
• CARGO_TARGET_$TARGET_RUNNER is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need cargo-metadata any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure --emit does not contain link, and then more patching was required of the --extern flags for the binary because those referenced the .rlib files but now only .rmeta exists, and that is still not fully working because cargo seems to expect those .rmeta files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ @ehuss your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match cargo. So -Zmiri flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:

• Fix Windows.
• Figure out how we can do check-only builds without the rebuild problem above. I am also worried about cases where build.rs script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well. (postponed that until we have a concrete example)
• Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving cargo miri test does not run doc-tests #584 is left for a future PR).
• For some time, detect the old way of passing -Zmiri flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
• Add a test for stdin handling (Miri stdout is buffered and stdin in always empty #1505). This should work now but we should be sure.
• Add a test for cargo env vars (Cargo env vars aren't set #1515).
• Check if Tests with harness = false aren't run #1516 is resolved.
• Check if cargo-miri does not support workspaces #1001 and env::current_dir() is not updated for non-root packages in a workspace #1514 are resolved.
• Check if "cargo miri" has slightly different non-UTF-8 output than "cargo run" #1312 is resolved (not sure if it is wort adding a test).
• Check if "cargo miri test" ignores "required-features" #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc @alecmocatta who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? @oli-obk

[RalfJung]

CI says

fatal error: File "target/i686-pc-windows-msvc/debug/cargo-miri-test.exe" not found or cargo-miri invoked incorrectly; please only invoke this binary through cargo miri

Looks like Windows needs a special case.

[RalfJung]

@oli-obk so regarding that check-build situation, I wonder if there isn't a better way than to mess with the flags... the best way of course would be to actually convince cargo to do a "check-build version of cargo test", but that might not be possible (Cc @ehuss). But as another option, is there a way to adjust this code to tell rustc to not do any codegen? Like, swap in a no-op-codegen-backend or so?

[bjorn3]

But as another option, is there a way to adjust this code to tell rustc to not do any codegen? Like, swap in a no-op-codegen-backend or so?

rustc_driver currently doesn't have an interface to switch codegen backends apart from passing -Zcodegen-backend=/path/to/codegen_backend.so. I was actually planing to open a PR for this.

[RalfJung]

rustc_driver currently doesn't have an interface to switch codegen backends apart from passing -Zcodegen-backend=/path/to/codegen_backend.so. I was actually planing to open a PR for this.

If we could pass -Zcodegen-backend=none and make the driver recognize that value to plug in some dummy backend, that would also work for us here I think.

I assume this means rustc would still produce both a .rlib and .rmeta file, so there is some unnecessary disk I/O, but I also assume that MIR-only .rlib are not big enough to make this a problem.

[bjorn3]

If we could pass -Zcodegen-backend=none and make the driver recognize that value to plug in some dummy backend, that would also work for us here I think.

I added -Zcodegen-backend=metadata-only when I initially worked on making rustc compile without LLVM, but I removed it later because it was unused.

[RalfJung]

Ah, that sounds like exactly what we'd need here. :) Could you point me to the relevant PRs? Will that code still work today?

(If you have some time and want to just re-add it yourself that would be much appreciated. :D I can also do it though, just might take a few days until I get around to it.)

[bjorn3]

It was removed in rust-lang/rust#58847.

[RalfJung]

Uff, this will be annoying... to implement the supported_target_features query, I'll basically have to copy these arrays. And that is after copying the code from the hotplug_codegen_backend test.

Also I am getting errors like this:

error[E0463]: can't find crate for `std`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
error: could not compile `std`.

Not sure where that is coming from. But this is much less clean than I expected...

[RalfJung]

Oh, I forgot that we are also using Miri for "host" crates that need actual binaries. Those will need a proper codegen backend. That probably explains the "can't find crate for" issue.

The problem with the target features remains, though.

[bjorn3]

Uff, this will be annoying... to implement the supported_target_features query, I'll basically have to copy these arrays. And that is after copying the code from the hotplug_codegen_backend test.

I think at least the stable target features should be moved out of rustc_codegen_llvm, as they will need to be listed by every codegen backend for corearch to compile, even when none of it's functions are used.

[RalfJung]

I think at least the stable target features should be moved out of rustc_codegen_llvm, as they will need to be listed by every codegen backend for corearch to compile, even when none of it's functions are used.

Yeah, compiling that part of libstd is also where the errors show up for Miri.

[RalfJung]

I did some more work on the hacky approach of messing with --emit, and now got it to work reasonably well -- cargo doesn't incorrectly rebuild things any more, and it seems to work for Windows targets, too. So IMO this is an okay solution for now, though longer-term I'd like to move to something more robust, like a NOP codegen backend.

[oli-obk]

this seems reasonable to me, though it does require quite some hacks (which has never been a blocker for me 😆 )

[alecmocatta]

Getting rid of cargo metadata and using CARGO_TARGET_$TARGET_RUNNER looks great to me. Eyeballing the changes I believe this fixes the three issues I raised. I'll confirm and share some tests by end of week.

[alecmocatta]

I put some tests here.

I observed two issues:

$ MIRIFLAGS='-Zmiri-disable-isolation' cargo miri test
   Compiling root v0.1.0 (/Users/alecmocatta/Documents/deploy/miri-tests)
    Finished test [unoptimized + debuginfo] target(s) in 0.10s
     Running target/x86_64-apple-darwin/debug/deps/root-2b5386c0b2f28133

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/x86_64-apple-darwin/debug/deps/harness_false-420a48ffadc82089
fatal error: `cargo-miri` called with unexpected first argument `/Users/alecmocatta/Documents/deploy/miri-tests/target/x86_64-apple-darwin/debug/deps/harness_false-420a48ffadc82089`; please only invoke this binary through `cargo miri`
error: test failed, to rerun pass '--test harness_false'

and

$ MIRIFLAGS='-Zmiri-disable-isolation' cargo miri test -p sub
   Compiling sub v0.1.0 (/Users/alecmocatta/Documents/deploy/miri-tests/sub)
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running target/x86_64-apple-darwin/debug/deps/sub-04947ff4d5c37221
error: couldn't read sub/src/main.rs: No such file or directory (os error 2)

error: aborting due to previous error

error: test failed, to rerun pass '-p sub --bin sub'

but #1512 and #1514 are fixed.

[RalfJung]

@alecmocatta thanks for testing! I also had just noticed the harness = false issue, that should be fixed now. I'll check the workspace example.

[RalfJung]

Ah, the workspace thing is annoying... the working directory is different between build-time and run-time. Miri now runs in the working directory the test runs, but then the relative path sub/src/main.rs makes no sense any more.

[RalfJung]

@alecmocatta all right, this should work now with the latest version.

[RalfJung]

(fixed conflict)

[oli-obk]

@bors r+

nope I'm fine continuing with the current approach

[bors]

📌 Commit ae859c3 has been approved by oli-obk

[bors]

⌛ Testing commit ae859c3 with merge ce29fbf...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing ce29fbf to master...