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

RalfJung · RalfJung · commit f7c938aaf0f4 · 2024-08-10T12:59:18.000+02:00
diff --git a/src/tools/miri/.cargo/config.toml b/src/tools/miri/.cargo/config.toml
@@ -0,0 +1,9 @@
+[unstable]
+profile-rustflags = true
+
+# Add back the containing directory of the packages we have to refer to using --manifest-path.
+# Per-package profiles avoid adding this to build dependencies.
+[profile.dev.package."cargo-miri"]
+rustflags = ["--remap-path-prefix", "=cargo-miri"]
+[profile.dev.package."miri-script"]
+rustflags = ["--remap-path-prefix", "=miri-script"]
diff --git a/src/tools/miri/CONTRIBUTING.md b/src/tools/miri/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.
diff --git a/src/tools/miri/cargo-miri/miri b/src/tools/miri/cargo-miri/miri
diff --git a/src/tools/miri/cargo-miri/src/util.rs b/src/tools/miri/cargo-miri/src/util.rs
@@ -93,12 +93,9 @@ pub fn find_miri() -> PathBuf {
     if let Some(path) = env::var_os("MIRI") {
         return path.into();
     }
+    // Assume it is in the same directory as ourselves.
     let mut path = std::env::current_exe().expect("current executable path invalid");
-    if cfg!(windows) {
-        path.set_file_name("miri.exe");
-    } else {
-        path.set_file_name("miri");
-    }
+    path.set_file_name(format!("miri{}", env::consts::EXE_SUFFIX));
     path
 }
 
diff --git a/src/tools/miri/miri b/src/tools/miri/miri
@@ -1,8 +1,15 @@
 #!/usr/bin/env bash
 set -e
+# 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
+# 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
+# We need a nightly toolchain, for the `profile-rustflags` cargo feature.
+cargo +nightly 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 'nightly' 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="$(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 || \
-  ( echo "Failed to build miri-script. Is the 'stable' toolchain installed?"; exit 1 )
 "$MIRI_SCRIPT_TARGET_DIR"/debug/miri-script "$@"
diff --git a/src/tools/miri/miri-script/miri b/src/tools/miri/miri-script/miri
diff --git a/src/tools/miri/miri-script/src/commands.rs b/src/tools/miri/miri-script/src/commands.rs
@@ -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,7 +36,6 @@ 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.
         self.build(path!(self.miri_dir / "Cargo.toml"), &[], quiet)?;
@@ -56,10 +56,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 +461,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 +506,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 +515,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 +543,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 +577,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)
     }
 }
diff --git a/src/tools/miri/miri-script/src/util.rs b/src/tools/miri/miri-script/src/util.rs
@@ -7,7 +7,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 +28,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 +55,14 @@ 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);
+
+        // Hard-code the target dir, since we rely on all binaries ending up in the same spot.
+        sh.set_var("CARGO_TARGET_DIR", path!(miri_dir / "target"));
 
         // We configure dev builds to not be unusably slow.
         let devel_opt_level =
@@ -91,17 +91,34 @@ 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());
+        cmd!(
+            self.sh,
+            "cargo +{toolchain} {cmd} {cargo_extra_flags...} --manifest-path {manifest_path}"
+        )
+    }
+
     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 +129,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 +166,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 +176,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 +208,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 +218,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 +230,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);
                         })?;
diff --git a/src/tools/miri/tests/ui.rs b/src/tools/miri/tests/ui.rs
@@ -13,7 +13,7 @@ use ui_test::{
 };
 
 fn miri_path() -> PathBuf {
-    PathBuf::from(option_env!("MIRI").unwrap_or(env!("CARGO_BIN_EXE_miri")))
+    PathBuf::from(env::var("MIRI").unwrap_or_else(|_| env!("CARGO_BIN_EXE_miri").into()))
 }
 
 fn get_host() -> String {
@@ -29,7 +29,7 @@ pub fn flagsplit(flags: &str) -> Vec<String> {
 
 // Build the shared object file for testing native function calls.
 fn build_native_lib() -> PathBuf {
-    let cc = option_env!("CC").unwrap_or("cc");
+    let cc = env::var("CC").unwrap_or_else(|_| "cc".into());
     // Target directory that we can write to.
     let so_target_dir =
         Path::new(&env::var_os("CARGO_TARGET_DIR").unwrap()).join("miri-native-lib");
@@ -84,9 +84,11 @@ fn miri_config(target: &str, path: &str, mode: Mode, with_dependencies: bool) ->
     if with_dependencies {
         // Set the `cargo-miri` binary, which we expect to be in the same folder as the `miri` binary.
         // (It's a separate crate, so we don't get an env var from cargo.)
-        let mut prog = miri_path();
-        prog.set_file_name("cargo-miri");
-        config.dependency_builder.program = prog;
+        config.dependency_builder.program = {
+            let mut prog = miri_path();
+            prog.set_file_name(format!("cargo-miri{}", env::consts::EXE_SUFFIX));
+            prog
+        };
         let builder_args = ["miri", "run"]; // There is no `cargo miri build` so we just use `cargo miri run`.
         config.dependency_builder.args = builder_args.into_iter().map(Into::into).collect();
         config.dependencies_crate_manifest_path =

Original file line number	Diff line number	Diff line change
`@@ -93,12 +93,9 @@ pub fn find_miri() -> PathBuf {`
`93`	`93`	`if let Some(path) = env::var_os("MIRI") {`
`94`	`94`	`return path.into();`
`95`	`95`	`}`
	`96`	`+ // Assume it is in the same directory as ourselves.`
`96`	`97`	`let mut path = std::env::current_exe().expect("current executable path invalid");`
`97`		`- if cfg!(windows) {`
`98`		`- path.set_file_name("miri.exe");`
`99`		`- } else {`
`100`		`- path.set_file_name("miri");`
`101`		`- }`
	`98`	`+ path.set_file_name(format!("miri{}", env::consts::EXE_SUFFIX));`
`102`	`99`	`path`
`103`	`100`	`}`
`104`	`101`