Auto merge of #1842 - hyd-dev:target-dir, r=RalfJung

Use `miri` inside the target directory used by rustc as Miri's target directory

Resolves #1311.

This PR makes Miri use `miri` inside the rustc target directory as its target directory, by letting `cargo-miri` get the rustc target directory by calling `cargo metadata`, append `miri` to it, and pass it with `--target-dir` to Cargo.

Getting the rustc target directory accurately requires calling `cargo metadata` as far as I know, because the `target-dir` can be set in config files in various places that are hard for `cargo-miri` to find.

I also considered https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#custom-named-profiles, but it looks like that requires adding `cargo-features = ["named-profiles"]` to **`Cargo.toml`**, which would be tricky for `cargo-miri`:
```
$ cargo +nightly-2021-06-23 test --config 'profile.miri.inherits="release"' --profile=miri -Z named-profiles -Z unstable-options
error: config profile `miri` is not valid (defined in `--config cli option`)

Caused by:
  feature `named-profiles` is required

  consider adding `cargo-features = ["named-profiles"]` to the manifest
```

Author: bors

.github/workflows/ci.yml

@@ -74,9 +74,11 @@ jobs:
           else
             RUSTC_HASH=$(< rust-version)
           fi
+          # We need a nightly cargo for parts of the cargo miri test suite.
           rustup-toolchain-install-master \
             -f \
             -n master "$RUSTC_HASH" \
+            -c cargo \
             -c rust-src \
             -c rustc-dev \
             -c llvm-tools \

cargo-miri/bin.rs

@@ -6,7 +6,7 @@ use std::io::{self, BufRead, BufReader, BufWriter, Read, Write};
 use std::iter::TakeWhile;
 use std::ops::Not;
 use std::path::{Path, PathBuf};
-use std::process::Command;
+use std::process::{self, Command};
 
 use serde::{Deserialize, Serialize};
 
@@ -112,40 +112,60 @@ fn has_arg_flag(name: &str) -> bool {
     args.any(|val| val == name)
 }
 
-/// Yields all values of command line flag `name`.
-struct ArgFlagValueIter<'a> {
-    args: TakeWhile<env::Args, fn(&String) -> bool>,
+/// Yields all values of command line flag `name` as `Ok(arg)`, and all other arguments except
+/// the flag as `Err(arg)`. (The flag `name` itself is not yielded at all, only its values are.)
+struct ArgSplitFlagValue<'a, I> {
+    args: TakeWhile<I, fn(&String) -> bool>,
     name: &'a str,
 }
 
-impl<'a> ArgFlagValueIter<'a> {
-    fn new(name: &'a str) -> Self {
+impl<'a, I: Iterator<Item = String>> ArgSplitFlagValue<'a, I> {
+    fn new(args: I, name: &'a str) -> Self {
         Self {
             // Stop searching at `--`.
-            args: env::args().take_while(|val| val != "--"),
+            args: args.take_while(|val| val != "--"),
             name,
         }
     }
 }
 
-impl Iterator for ArgFlagValueIter<'_> {
-    type Item = String;
+impl<I: Iterator<Item = String>> Iterator for ArgSplitFlagValue<'_, I> {
+    type Item = Result<String, String>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        loop {
-            let arg = self.args.next()?;
-            if !arg.starts_with(self.name) {
-                continue;
-            }
+        let arg = self.args.next()?;
+        if arg.starts_with(self.name) {
             // Strip leading `name`.
             let suffix = &arg[self.name.len()..];
             if suffix.is_empty() {
                 // This argument is exactly `name`; the next one is the value.
-                return self.args.next();
+                return self.args.next().map(Ok);
             } else if suffix.starts_with('=') {
                 // This argument is `name=value`; get the value.
                 // Strip leading `=`.
-                return Some(suffix[1..].to_owned());
+                return Some(Ok(suffix[1..].to_owned()));
+            }
+        }
+        Some(Err(arg))
+    }
+}
+
+/// Yields all values of command line flag `name`.
+struct ArgFlagValueIter<'a>(ArgSplitFlagValue<'a, env::Args>);
+
+impl<'a> ArgFlagValueIter<'a> {
+    fn new(name: &'a str) -> Self {
+        Self(ArgSplitFlagValue::new(env::args(), name))
+    }
+}
+
+impl Iterator for ArgFlagValueIter<'_> {
+    type Item = String;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        loop {
+            if let Ok(value) = self.0.next()? {
+                return Some(value);
             }
         }
     }
@@ -213,7 +233,7 @@ fn exec(mut cmd: Command) {
 /// If it fails, fail this process with the same exit code.
 /// Otherwise, continue.
 fn exec_with_pipe(mut cmd: Command, input: &[u8]) {
-    cmd.stdin(std::process::Stdio::piped());
+    cmd.stdin(process::Stdio::piped());
     let mut child = cmd.spawn().expect("failed to spawn process");
     {
         let stdin = child.stdin.as_mut().expect("failed to open stdin");
@@ -452,6 +472,43 @@ path = "lib.rs"
     }
 }
 
+/// Detect the target directory by calling `cargo metadata`.
+fn detect_target_dir() -> PathBuf {
+    #[derive(Deserialize)]
+    struct Metadata {
+        target_directory: PathBuf,
+    }
+    let mut cmd = cargo();
+    // `-Zunstable-options` is required by `--config`.
+    cmd.args(["metadata", "--no-deps", "--format-version=1", "-Zunstable-options"]);
+    // The `build.target-dir` config can be passed by `--config` flags, so forward them to
+    // `cargo metadata`.
+    let config_flag = "--config";
+    for arg in ArgSplitFlagValue::new(
+        env::args().skip(3), // skip the program name, "miri" and "run" / "test"
+        config_flag,
+    ) {
+        if let Ok(config) = arg {
+            cmd.arg(config_flag).arg(config);
+        }
+    }
+    let mut child = cmd
+        .stdin(process::Stdio::null())
+        .stdout(process::Stdio::piped())
+        .spawn()
+        .expect("failed ro run `cargo metadata`");
+    // Check this `Result` after `status.success()` is checked, so we don't print the error
+    // to stderr if `cargo metadata` is also printing to stderr.
+    let metadata: Result<Metadata, _> = serde_json::from_reader(child.stdout.take().unwrap());
+    let status = child.wait().expect("failed to wait for `cargo metadata` to exit");
+    if !status.success() {
+        std::process::exit(status.code().unwrap_or(-1));
+    }
+    metadata
+        .unwrap_or_else(|e| show_error(format!("invalid `cargo metadata` output: {}", e)))
+        .target_directory
+}
+
 fn phase_cargo_miri(mut args: env::Args) {
     // Check for version and help flags even when invoked as `cargo-miri`.
     if has_arg_flag("--help") || has_arg_flag("-h") {
@@ -510,8 +567,32 @@ fn phase_cargo_miri(mut args: env::Args) {
         &host
     };
 
-    // Forward all further arguments to cargo.
-    cmd.args(args);
+    let mut target_dir = None;
+
+    // Forward all arguments before `--` other than `--target-dir` and its value to Cargo.
+    for arg in ArgSplitFlagValue::new(&mut args, "--target-dir") {
+        match arg {
+            Ok(value) => {
+                if target_dir.is_some() {
+                    show_error(format!("`--target-dir` is provided more than once"));
+                }
+                target_dir = Some(value.into());
+            }
+            Err(arg) => {
+                cmd.arg(arg);
+            }
+        }
+    }
+
+    // Detect the target directory if it's not specified via `--target-dir`.
+    let target_dir = target_dir.get_or_insert_with(detect_target_dir);
+
+    // Set `--target-dir` to `miri` inside the original target directory.
+    target_dir.push("miri");
+    cmd.arg("--target-dir").arg(target_dir);
+
+    // Forward all further arguments after `--` to cargo.
+    cmd.arg("--").args(args);
 
     // Set `RUSTC_WRAPPER` to ourselves.  Cargo will prepend that binary to its usual invocation,
     // i.e., the first argument is `rustc` -- which is what we use in `main` to distinguish

rustup-toolchain

@@ -39,7 +39,7 @@ fi
 
 # Install and setup new toolchain.
 rustup toolchain uninstall miri
-rustup-toolchain-install-master -n miri -c rust-src -c rustc-dev -c llvm-tools -- "$NEW_COMMIT"
+rustup-toolchain-install-master -n miri -c cargo -c rust-src -c rustc-dev -c llvm-tools -- "$NEW_COMMIT"
 rustup override set miri
 
 # Cleanup.

test-cargo-miri/.gitignore

@@ -1 +1,4 @@
 *.real
+custom-run
+custom-test
+config-cli

test-cargo-miri/run-test.py

@@ -101,6 +101,11 @@ def test_cargo_miri_run():
         "run.subcrate.stdout.ref", "run.subcrate.stderr.ref",
         env={'MIRIFLAGS': "-Zmiri-disable-isolation"},
     )
+    test("`cargo miri run` (custom target dir)",
+        # Attempt to confuse the argument parser.
+        cargo_miri("run") + ["--target-dir=custom-run", "--", "--target-dir=target/custom-run"],
+        "run.args.stdout.ref", "run.custom-target-dir.stderr.ref",
+    )
 
 def test_cargo_miri_test():
     # rustdoc is not run on foreign targets
@@ -144,8 +149,18 @@ def test_cargo_miri_test():
         cargo_miri("test") + ["-p", "subcrate", "--doc"],
         "test.stdout-empty.ref", "test.stderr-proc-macro-doctest.ref",
     )
+    test("`cargo miri test` (custom target dir)",
+        cargo_miri("test") + ["--target-dir=custom-test"],
+        default_ref, "test.stderr-empty.ref",
+    )
+    del os.environ["CARGO_TARGET_DIR"] # this overrides `build.target-dir` passed by `--config`, so unset it
+    test("`cargo miri test` (config-cli)",
+        cargo_miri("test") + ["--config=build.target-dir=\"config-cli\"", "-Zunstable-options"],
+        default_ref, "test.stderr-empty.ref",
+    )
 
 os.chdir(os.path.dirname(os.path.realpath(__file__)))
+os.environ["CARGO_TARGET_DIR"] = "target" # this affects the location of the target directory that we need to check
 os.environ["RUST_TEST_NOCAPTURE"] = "0" # this affects test output, so make sure it is not set
 os.environ["RUST_TEST_THREADS"] = "1" # avoid non-deterministic output due to concurrent test runs
 
@@ -158,6 +173,12 @@ def test_cargo_miri_test():
     subprocess.run(cargo_miri("setup"), check=True)
 test_cargo_miri_run()
 test_cargo_miri_test()
+# Ensure we did not create anything outside the expected target dir.
+for target_dir in ["target", "custom-run", "custom-test", "config-cli"]:
+    if os.listdir(target_dir) != ["miri"]:
+        fail(f"`{target_dir}` contains unexpected files")
+    # Ensure something exists inside that target dir.
+    os.access(os.path.join(target_dir, "miri", "debug", "deps"), os.F_OK)
 
 print("\nTEST SUCCESSFUL!")
 sys.exit(0)

test-cargo-miri/run.custom-target-dir.stderr.ref

@@ -0,0 +1,2 @@
+main
+--target-dir=target/custom-run