miri-script: use --remap-path-prefix to print errors relative to the right root

Author: RalfJung

CONTRIBUTING.md

@@ -309,6 +309,7 @@ anyone but Miri itself. They are used to communicate between different Miri
 binaries, and as such worth documenting:
 
 * `CARGO_EXTRA_FLAGS` is understood by `./miri` and passed to all host cargo invocations.
+  It is reserved for CI usage; setting the wrong flags this way can easily confuse the script.
 * `MIRI_BE_RUSTC` can be set to `host` or `target`. It tells the Miri driver to
   actually not interpret the code but compile it like rustc would. With `target`, Miri sets
   some compiler flags to prepare the code for interpretation; with `host`, this is not done.

cargo-miri/miri

@@ -1,8 +1,16 @@
 #!/usr/bin/env bash
 set -e
-# Instead of doing just `cargo run --manifest-path .. $@`, we invoke miri-script binary directly. Invoking `cargo run` goes through
-# rustup (that sets it's own environmental variables), which is undesirable.
+# We want to call the binary directly, so we need to know where it ends up.
 MIRI_SCRIPT_TARGET_DIR="$(dirname "$0")"/miri-script/target
-cargo +stable build $CARGO_EXTRA_FLAGS -q --target-dir "$MIRI_SCRIPT_TARGET_DIR" --manifest-path "$(dirname "$0")"/miri-script/Cargo.toml || \
+# If stdout is not a terminal and we are not on CI, assume that we are being invoked by RA, and use JSON output.
+if ! [ -t 1 ] && [ -z "$CI" ]; then
+  MESSAGE_FORMAT="--message-format=json"
+fi
+# Set --remap-path-prefix so miri-script build failures are printed relative to the Miri root.
+RUSTFLAGS="$RUSTFLAGS --remap-path-prefix =miri-script" \
+  cargo +stable build $CARGO_EXTRA_FLAGS --manifest-path "$(dirname "$0")"/miri-script/Cargo.toml \
+  -q --target-dir "$MIRI_SCRIPT_TARGET_DIR" $MESSAGE_FORMAT || \
   ( echo "Failed to build miri-script. Is the 'stable' toolchain installed?"; exit 1 )
+# Instead of doing just `cargo run --manifest-path .. $@`, we invoke miri-script binary directly. Invoking `cargo run` goes through
+# rustup (that sets it's own environmental variables), which is undesirable.
 "$MIRI_SCRIPT_TARGET_DIR"/debug/miri-script "$@"

miri

@@ -22,6 +22,7 @@ const JOSH_FILTER: &str =
 const JOSH_PORT: &str = "42042";
 
 impl MiriEnv {
+    /// Prepares the environment: builds miri and cargo-miri and a sysroot.
     /// Returns the location of the sysroot.
     ///
     /// If the target is None the sysroot will be built for the host machine.
@@ -35,11 +36,25 @@ impl MiriEnv {
             return Ok(miri_sysroot.into());
         }
         let manifest_path = path!(self.miri_dir / "cargo-miri" / "Cargo.toml");
-        let Self { toolchain, cargo_extra_flags, .. } = &self;
 
-        // Make sure everything is built. Also Miri itself.
+        // Make sure everything is built, and set the env vars for that.
+        // (`cargo_cmd` sets `CARGO_TARGET_DIR` to ensure they are actually there.)
         self.build(path!(self.miri_dir / "Cargo.toml"), &[], quiet)?;
+        self.sh.set_var(
+            "MIRI",
+            path!(self.miri_dir / "target" / "debug" / format!("miri{}", env::consts::EXE_SUFFIX)),
+        );
         self.build(&manifest_path, &[], quiet)?;
+        self.sh.set_var(
+            "CARGO_MIRI",
+            path!(
+                self.miri_dir
+                    / "cargo-miri"
+                    / "target"
+                    / "debug"
+                    / format!("cargo-miri{}", env::consts::EXE_SUFFIX)
+            ),
+        );
 
         let target_flag = if let Some(target) = &target {
             vec![OsStr::new("--target"), target.as_ref()]
@@ -56,10 +71,12 @@ impl MiriEnv {
             eprintln!();
         }
 
-        let mut cmd = cmd!(self.sh,
-            "cargo +{toolchain} --quiet run {cargo_extra_flags...} --manifest-path {manifest_path} --
-             miri setup --print-sysroot {target_flag...}"
-        );
+        let mut cmd = self
+            .cargo_cmd(&manifest_path, "run")
+            .arg("--quiet")
+            .arg("--")
+            .args(&["miri", "setup", "--print-sysroot"])
+            .args(target_flag);
         cmd.set_quiet(quiet);
         let output = cmd.read()?;
         self.sh.set_var("MIRI_SYSROOT", &output);
@@ -459,7 +476,7 @@ impl Command {
     fn test(bless: bool, mut flags: Vec<String>, target: Option<String>) -> Result<()> {
         let mut e = MiriEnv::new()?;
 
-        // Prepare a sysroot.
+        // Prepare a sysroot. (Also builds cargo-miri, which we need.)
         e.build_miri_sysroot(/* quiet */ false, target.as_deref())?;
 
         // Forward information to test harness.
@@ -504,7 +521,7 @@ impl Command {
         early_flags.push("--edition".into());
         early_flags.push(edition.as_deref().unwrap_or("2021").into());
 
-        // Prepare a sysroot, add it to the flags.
+        // Prepare a sysroot, add it to the flags. (Also builds cargo-miri, which we need.)
         let miri_sysroot = e.build_miri_sysroot(/* quiet */ !verbose, target.as_deref())?;
         early_flags.push("--sysroot".into());
         early_flags.push(miri_sysroot.into());
@@ -513,23 +530,19 @@ impl Command {
         let miri_manifest = path!(e.miri_dir / "Cargo.toml");
         let miri_flags = e.sh.var("MIRIFLAGS").unwrap_or_default();
         let miri_flags = flagsplit(&miri_flags);
-        let toolchain = &e.toolchain;
-        let extra_flags = &e.cargo_extra_flags;
         let quiet_flag = if verbose { None } else { Some("--quiet") };
         // This closure runs the command with the given `seed_flag` added between the MIRIFLAGS and
         // the `flags` given on the command-line.
-        let run_miri = |sh: &Shell, seed_flag: Option<String>| -> Result<()> {
+        let run_miri = |e: &MiriEnv, seed_flag: Option<String>| -> Result<()> {
             // The basic command that executes the Miri driver.
             let mut cmd = if dep {
-                cmd!(
-                    sh,
-                    "cargo +{toolchain} {quiet_flag...} test {extra_flags...} --manifest-path {miri_manifest} --test ui -- --miri-run-dep-mode"
-                )
+                e.cargo_cmd(&miri_manifest, "test")
+                    .args(&["--test", "ui"])
+                    .args(quiet_flag)
+                    .arg("--")
+                    .args(&["--miri-run-dep-mode"])
             } else {
-                cmd!(
-                    sh,
-                    "cargo +{toolchain} {quiet_flag...} run {extra_flags...} --manifest-path {miri_manifest} --"
-                )
+                e.cargo_cmd(&miri_manifest, "run").args(quiet_flag).arg("--")
             };
             cmd.set_quiet(!verbose);
             // Add Miri flags
@@ -545,14 +558,14 @@ impl Command {
         };
         // Run the closure once or many times.
         if let Some(seed_range) = many_seeds {
-            e.run_many_times(seed_range, |sh, seed| {
+            e.run_many_times(seed_range, |e, seed| {
                 eprintln!("Trying seed: {seed}");
-                run_miri(sh, Some(format!("-Zmiri-seed={seed}"))).inspect_err(|_| {
+                run_miri(e, Some(format!("-Zmiri-seed={seed}"))).inspect_err(|_| {
                     eprintln!("FAILING SEED: {seed}");
                 })
             })?;
         } else {
-            run_miri(&e.sh, None)?;
+            run_miri(&e, None)?;
         }
         Ok(())
     }
@@ -579,6 +592,6 @@ impl Command {
             .filter_ok(|item| item.file_type().is_file())
             .map_ok(|item| item.into_path());
 
-        e.format_files(files, &e.toolchain[..], &config_path, &flags)
+        e.format_files(files, &config_path, &flags)
     }
 }

miri-script/miri

@@ -1,4 +1,5 @@
 use std::ffi::{OsStr, OsString};
+use std::fmt::Write;
 use std::ops::Range;
 use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicBool, AtomicU32, Ordering};
@@ -7,7 +8,7 @@ use std::thread;
 use anyhow::{anyhow, Context, Result};
 use dunce::canonicalize;
 use path_macro::path;
-use xshell::{cmd, Shell};
+use xshell::{cmd, Cmd, Shell};
 
 pub fn miri_dir() -> std::io::Result<PathBuf> {
     const MIRI_SCRIPT_ROOT_DIR: &str = env!("CARGO_MANIFEST_DIR");
@@ -28,13 +29,14 @@ pub fn flagsplit(flags: &str) -> Vec<String> {
 }
 
 /// Some extra state we track for building Miri, such as the right RUSTFLAGS.
+#[derive(Clone)]
 pub struct MiriEnv {
     /// miri_dir is the root of the miri repository checkout we are working in.
     pub miri_dir: PathBuf,
     /// active_toolchain is passed as `+toolchain` argument to cargo/rustc invocations.
     pub toolchain: String,
     /// Extra flags to pass to cargo.
-    pub cargo_extra_flags: Vec<String>,
+    cargo_extra_flags: Vec<String>,
     /// The rustc sysroot
     pub sysroot: PathBuf,
     /// The shell we use.
@@ -54,15 +56,11 @@ impl MiriEnv {
 
         // Determine some toolchain properties
         if !libdir.exists() {
-            println!("Something went wrong determining the library dir.");
-            println!("I got {} but that does not exist.", libdir.display());
-            println!("Please report a bug at https://github.com/rust-lang/miri/issues.");
+            eprintln!("Something went wrong determining the library dir.");
+            eprintln!("I got {} but that does not exist.", libdir.display());
+            eprintln!("Please report a bug at https://github.com/rust-lang/miri/issues.");
             std::process::exit(2);
         }
-        // Share target dir between `miri` and `cargo-miri`.
-        let target_dir = std::env::var_os("CARGO_TARGET_DIR")
-            .unwrap_or_else(|| path!(miri_dir / "target").into());
-        sh.set_var("CARGO_TARGET_DIR", target_dir);
 
         // We configure dev builds to not be unusably slow.
         let devel_opt_level =
@@ -91,17 +89,48 @@ impl MiriEnv {
         // Get extra flags for cargo.
         let cargo_extra_flags = std::env::var("CARGO_EXTRA_FLAGS").unwrap_or_default();
         let cargo_extra_flags = flagsplit(&cargo_extra_flags);
+        if cargo_extra_flags.iter().any(|a| a == "--release" || a.starts_with("--profile")) {
+            // This makes binaries end up in different paths, let's not do that.
+            eprintln!(
+                "Passing `--release` or `--profile` in `CARGO_EXTRA_FLAGS` will totally confuse miri-script, please don't do that."
+            );
+            std::process::exit(1);
+        }
 
         Ok(MiriEnv { miri_dir, toolchain, sh, sysroot, cargo_extra_flags })
     }
 
+    pub fn cargo_cmd(&self, manifest_path: impl AsRef<OsStr>, cmd: &str) -> Cmd<'_> {
+        let MiriEnv { toolchain, cargo_extra_flags, .. } = self;
+        let manifest_path = Path::new(manifest_path.as_ref());
+        let cmd = cmd!(
+            self.sh,
+            "cargo +{toolchain} {cmd} {cargo_extra_flags...} --manifest-path {manifest_path}"
+        );
+        // Hard-code the target dir, since we rely on knowing where the binaries end up.
+        let manifest_dir = manifest_path.parent().unwrap();
+        let cmd = cmd.env("CARGO_TARGET_DIR", path!(manifest_dir / "target"));
+        // Apply path remapping to have errors printed relative to `miri_dir`.
+        let cmd = if let Ok(relative_to_miri) = manifest_dir.strip_prefix(&self.miri_dir) {
+            // Add `--remap-path-prefix` to RUSTFLAGS.
+            let mut rustflags = self.sh.var("RUSTFLAGS").unwrap();
+            write!(rustflags, " --remap-path-prefix ={}", relative_to_miri.display()).unwrap();
+            cmd.env("RUSTFLAGS", rustflags)
+        } else {
+            cmd
+        };
+        // All set up!
+        cmd
+    }
+
     pub fn install_to_sysroot(
         &self,
         path: impl AsRef<OsStr>,
         args: impl IntoIterator<Item = impl AsRef<OsStr>>,
     ) -> Result<()> {
         let MiriEnv { sysroot, toolchain, cargo_extra_flags, .. } = self;
         // Install binaries to the miri toolchain's `sysroot` so they do not interact with other toolchains.
+        // (Not using `cargo_cmd` as `install` is special and doesn't use `--manifest-path`.)
         cmd!(self.sh, "cargo +{toolchain} install {cargo_extra_flags...} --path {path} --force --root {sysroot} {args...}").run()?;
         Ok(())
     }
@@ -112,40 +141,34 @@ impl MiriEnv {
         args: &[String],
         quiet: bool,
     ) -> Result<()> {
-        let MiriEnv { toolchain, cargo_extra_flags, .. } = self;
         let quiet_flag = if quiet { Some("--quiet") } else { None };
         // We build the tests as well, (a) to avoid having rebuilds when building the tests later
         // and (b) to have more parallelism during the build of Miri and its tests.
-        let mut cmd = cmd!(
-            self.sh,
-            "cargo +{toolchain} build --bins --tests {cargo_extra_flags...} --manifest-path {manifest_path} {quiet_flag...} {args...}"
-        );
+        // This means `./miri run` without `--dep` will build Miri twice (for the sysroot with
+        // dev-dependencies, and then for running without dev-dependencies), but the way more common
+        // `./miri test` will avoid building Miri twice.
+        let mut cmd = self
+            .cargo_cmd(manifest_path, "build")
+            .args(&["--bins", "--tests"])
+            .args(quiet_flag)
+            .args(args);
         cmd.set_quiet(quiet);
         cmd.run()?;
         Ok(())
     }
 
     pub fn check(&self, manifest_path: impl AsRef<OsStr>, args: &[String]) -> Result<()> {
-        let MiriEnv { toolchain, cargo_extra_flags, .. } = self;
-        cmd!(self.sh, "cargo +{toolchain} check {cargo_extra_flags...} --manifest-path {manifest_path} --all-targets {args...}")
-            .run()?;
+        self.cargo_cmd(manifest_path, "check").arg("--all-targets").args(args).run()?;
         Ok(())
     }
 
     pub fn clippy(&self, manifest_path: impl AsRef<OsStr>, args: &[String]) -> Result<()> {
-        let MiriEnv { toolchain, cargo_extra_flags, .. } = self;
-        cmd!(self.sh, "cargo +{toolchain} clippy {cargo_extra_flags...} --manifest-path {manifest_path} --all-targets {args...}")
-            .run()?;
+        self.cargo_cmd(manifest_path, "clippy").arg("--all-targets").args(args).run()?;
         Ok(())
     }
 
     pub fn test(&self, manifest_path: impl AsRef<OsStr>, args: &[String]) -> Result<()> {
-        let MiriEnv { toolchain, cargo_extra_flags, .. } = self;
-        cmd!(
-            self.sh,
-            "cargo +{toolchain} test {cargo_extra_flags...} --manifest-path {manifest_path} {args...}"
-        )
-        .run()?;
+        self.cargo_cmd(manifest_path, "test").args(args).run()?;
         Ok(())
     }
 
@@ -155,7 +178,6 @@ impl MiriEnv {
     pub fn format_files(
         &self,
         files: impl Iterator<Item = Result<PathBuf, walkdir::Error>>,
-        toolchain: &str,
         config_path: &Path,
         flags: &[String],
     ) -> anyhow::Result<()> {
@@ -166,6 +188,7 @@ impl MiriEnv {
         // Format in batches as not all our files fit into Windows' command argument limit.
         for batch in &files.chunks(256) {
             // Build base command.
+            let toolchain = &self.toolchain;
             let mut cmd = cmd!(
                 self.sh,
                 "rustfmt +{toolchain} --edition=2021 --config-path {config_path} --unstable-features --skip-children {flags...}"
@@ -197,7 +220,7 @@ impl MiriEnv {
     pub fn run_many_times(
         &self,
         range: Range<u32>,
-        run: impl Fn(&Shell, u32) -> Result<()> + Sync,
+        run: impl Fn(&Self, u32) -> Result<()> + Sync,
     ) -> Result<()> {
         // `next` is atomic so threads can concurrently fetch their next value to run.
         let next = AtomicU32::new(range.start);
@@ -207,10 +230,10 @@ impl MiriEnv {
             let mut handles = Vec::new();
             // Spawn one worker per core.
             for _ in 0..thread::available_parallelism()?.get() {
-                // Create a copy of the shell for this thread.
-                let local_shell = self.sh.clone();
+                // Create a copy of the environment for this thread.
+                let local_miri = self.clone();
                 let handle = s.spawn(|| -> Result<()> {
-                    let local_shell = local_shell; // move the copy into this thread.
+                    let local_miri = local_miri; // move the copy into this thread.
                     // Each worker thread keeps asking for numbers until we're all done.
                     loop {
                         let cur = next.fetch_add(1, Ordering::Relaxed);
@@ -219,7 +242,7 @@ impl MiriEnv {
                             break;
                         }
                         // Run the command with this seed.
-                        run(&local_shell, cur).inspect_err(|_| {
+                        run(&local_miri, cur).inspect_err(|_| {
                             // If we failed, tell everyone about this.
                             failed.store(true, Ordering::Relaxed);
                         })?;