From f7c938aaf0f497bb23e7338156435c3bf261e052 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 9 Aug 2024 23:54:02 +0200 Subject: [PATCH] miri-script: use --remap-path-prefix to print errors relative to the right root --- src/tools/miri/.cargo/config.toml | 9 +++ src/tools/miri/CONTRIBUTING.md | 1 + src/tools/miri/cargo-miri/miri | 4 -- src/tools/miri/cargo-miri/src/util.rs | 7 +- src/tools/miri/miri | 13 +++- src/tools/miri/miri-script/miri | 4 -- src/tools/miri/miri-script/src/commands.rs | 42 ++++++------ src/tools/miri/miri-script/src/util.rs | 75 +++++++++++++--------- src/tools/miri/tests/ui.rs | 12 ++-- 9 files changed, 92 insertions(+), 75 deletions(-) create mode 100644 src/tools/miri/.cargo/config.toml delete mode 100755 src/tools/miri/cargo-miri/miri delete mode 100755 src/tools/miri/miri-script/miri diff --git a/src/tools/miri/.cargo/config.toml b/src/tools/miri/.cargo/config.toml new file mode 100644 index 000000000000..42e7c2c48189 --- /dev/null +++ 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 index 9067cbc60326..8aca8d459ddc 100644 --- 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 deleted file mode 100755 index cf3ad06788ab..000000000000 --- a/src/tools/miri/cargo-miri/miri +++ /dev/null @@ -1,4 +0,0 @@ -#!/bin/sh -# RA invokes `./miri cargo ...` for each workspace, so we need to forward that to the main `miri` -# script. See . -exec "$(dirname "$0")"/../miri "$@" diff --git a/src/tools/miri/cargo-miri/src/util.rs b/src/tools/miri/cargo-miri/src/util.rs index 5f2794e2244f..56f38de8de62 100644 --- 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 index 07383bb59ebc..5d07ad9e249d 100755 --- 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 deleted file mode 100755 index cf3ad06788ab..000000000000 --- a/src/tools/miri/miri-script/miri +++ /dev/null @@ -1,4 +0,0 @@ -#!/bin/sh -# RA invokes `./miri cargo ...` for each workspace, so we need to forward that to the main `miri` -# script. See . -exec "$(dirname "$0")"/../miri "$@" diff --git a/src/tools/miri/miri-script/src/commands.rs b/src/tools/miri/miri-script/src/commands.rs index fc205040baf5..705ddaa9eade 100644 --- 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, target: Option) -> 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| -> Result<()> { + let run_miri = |e: &MiriEnv, seed_flag: Option| -> 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 index e1b77be192e3..568dc1ec0b38 100644 --- 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 { const MIRI_SCRIPT_ROOT_DIR: &str = env!("CARGO_MANIFEST_DIR"); @@ -28,13 +28,14 @@ pub fn flagsplit(flags: &str) -> Vec { } /// 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, + cargo_extra_flags: Vec, /// 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,10 +91,26 @@ 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, 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, @@ -102,6 +118,7 @@ impl MiriEnv { ) -> 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, 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, 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, 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>, - 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, - 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 index 95f8b0541029..9cbcf6e42a79 100644 --- 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 { // 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 =