From e530829797ed2dd0cbab0309979ad5846c69b497 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 1 Jan 2020 04:12:27 -0500 Subject: [PATCH 1/4] Use 'cargo check' to build the sysroot and target crate Fixes #1057 Since we are no longer using "cargo rustc", we now use the rustc arguments passed by Cargo to determine whether we are building a build dependency, normal dependency, or "target" (final binary or test) crate. --- Cargo.toml | 1 + src/bin/cargo-miri.rs | 88 +++++++++++++++++++++++++++++++------------ 2 files changed, 65 insertions(+), 24 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index da9481cce5..236905b895 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,7 @@ rustc-workspace-hack = "1.0.0" # between "cargo build" and "cargo intall". num-traits = "*" serde = { version = "*", features = ["derive"] } +serde_json = "1.0.44" [build-dependencies] vergen = "3" diff --git a/src/bin/cargo-miri.rs b/src/bin/cargo-miri.rs index e990bc0027..55503c635d 100644 --- a/src/bin/cargo-miri.rs +++ b/src/bin/cargo-miri.rs @@ -6,7 +6,7 @@ use std::ops::Not; use std::path::{Path, PathBuf}; use std::process::Command; -const XARGO_MIN_VERSION: (u32, u32, u32) = (0, 3, 17); +const XARGO_MIN_VERSION: (u32, u32, u32) = (0, 3, 19); const CARGO_MIRI_HELP: &str = r#"Interprets bin crates and tests in Miri @@ -84,6 +84,34 @@ fn get_arg_flag_value(name: &str) -> Option { } } + +/// Determines if we are being invoked (as rustc) to build a runnable +/// executable. We run "cargo check", so this should only happen when +/// we are trying to compile a build script or build script dependency, +/// which actually needs to be executed on the host platform. +/// +/// Currently, we detect this by checking for "--emit=link", +/// which indicates that Cargo instruced rustc to output +/// a native object. +fn is_build_dep() -> bool { + std::env::args().any(|arg| arg.starts_with("--emit=") && arg.contains("link")) +} + +/// Returns whether or not Cargo invoked the wrapper (this binary) to compile +/// the final, target crate (either a test for 'cargo test', or a binary for 'cargo run') +/// Cargo does not give us this information directly, so we need to check +/// various command-line flags. +fn is_target_crate(is_build_script: bool) -> bool { + let is_bin = get_arg_flag_value("--crate-type").as_deref() == Some("bin"); + let is_test = std::env::args().find(|arg| arg == "--test").is_some(); + + // The final runnable (under Miri) crate will either be a binary crate + // or a test crate. We make sure to exclude build scripts here, since + // they are also build with "--crate-type bin" + (is_bin || is_test) && !is_build_script +} + + fn list_targets() -> impl Iterator { // We need to get the manifest, and then the metadata, to enumerate targets. let manifest_path = @@ -197,7 +225,7 @@ fn xargo() -> Command { // Bootstrap tells us where to find xargo Command::new(val) } else { - Command::new("xargo") + Command::new("xargo-check") } } @@ -467,7 +495,7 @@ fn in_cargo_miri() { // change to add additional arguments. `FLAGS` is set to identify // this target. The user gets to control what gets actually passed to Miri. let mut cmd = cargo(); - cmd.arg("rustc"); + cmd.arg("check"); match (subcommand, kind.as_str()) { (MiriCommand::Run, "bin") => { // FIXME: we just run all the binaries here. @@ -494,10 +522,15 @@ fn in_cargo_miri() { } cmd.arg(arg); } - // Add `--` (to end the `cargo` flags), and then the user flags. We add markers around the - // user flags to be able to identify them later. "cargo rustc" adds more stuff after this, - // so we have to mark both the beginning and the end. - cmd.arg("--").arg("cargo-miri-marker-begin").args(args).arg("cargo-miri-marker-end"); + + // Serialize our actual args into a special environemt variable. + // This will be read by `inside_cargo_rustc` when we go to invoke + // our actual target crate (the binary or the test we are running). + // Since we're using "cargo check", we have no other way of passing + // these arguments. + let args_vec: Vec = args.collect(); + cmd.env("MIRI_MAGIC_ARGS", serde_json::to_string(&args_vec).expect("failed to serialize args")); + let path = std::env::current_exe().expect("current executable path invalid"); cmd.env("RUSTC_WRAPPER", path); if verbose { @@ -517,25 +550,32 @@ fn inside_cargo_rustc() { let sysroot = std::env::var("MIRI_SYSROOT").expect("The wrapper should have set MIRI_SYSROOT"); let rustc_args = std::env::args().skip(2); // skip `cargo rustc` - let mut args: Vec = - rustc_args.chain(Some("--sysroot".to_owned())).chain(Some(sysroot)).collect(); - args.splice(0..0, miri::miri_default_args().iter().map(ToString::to_string)); - // See if we can find the `cargo-miri` markers. Those only get added to the binary we want to - // run. They also serve to mark the user-defined arguments, which we have to move all the way - // to the end (they get added somewhere in the middle). + let in_build_script = is_build_dep(); + + // Build scripts need to be compiled to actual runnable executables, + // and therefore completely bypass Miri. We make sure to only specify + // our custom Xargo sysroot for non-build-script crate - that is, + // crates which are ultimately going to get interpreted by Miri. + let mut args = if in_build_script { + rustc_args.collect() + } else { + let mut args: Vec = rustc_args + .chain(Some("--sysroot".to_owned())) + .chain(Some(sysroot)) + .collect(); + args.splice(0..0, miri::miri_default_args().iter().map(ToString::to_string)); + args + }; + let needs_miri = - if let Some(begin) = args.iter().position(|arg| arg == "cargo-miri-marker-begin") { - let end = args - .iter() - .position(|arg| arg == "cargo-miri-marker-end") - .expect("cannot find end marker"); - // These mark the user arguments. We remove the first and last as they are the markers. - let mut user_args = args.drain(begin..=end); - assert_eq!(user_args.next().unwrap(), "cargo-miri-marker-begin"); - assert_eq!(user_args.next_back().unwrap(), "cargo-miri-marker-end"); - // Collect the rest and add it back at the end. - let mut user_args = user_args.collect::>(); + if is_target_crate(in_build_script) { + // This is the 'target crate '- the binary or test crate that + // we want to interpret under Miri. We deserialize the user-provided arguments + // from the special environment variable "MIRI_MAGIC_ARGS", and feed them + // to the 'miri' binary. + let magic = std::env::var("MIRI_MAGIC_ARGS").expect("missing MIRI_MAGIC_ARGS"); + let mut user_args: Vec = serde_json::from_str(&magic).expect("failed to deserialize args"); args.append(&mut user_args); // Run this in Miri. true From 443163f93071587f72014018eeb45a2225fdff79 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 24 Feb 2020 14:17:19 +0100 Subject: [PATCH 2/4] refactor cargo-miri a bit --- src/bin/cargo-miri.rs | 170 +++++++++++++++++++++--------------------- 1 file changed, 86 insertions(+), 84 deletions(-) diff --git a/src/bin/cargo-miri.rs b/src/bin/cargo-miri.rs index 55503c635d..17063b6ff6 100644 --- a/src/bin/cargo-miri.rs +++ b/src/bin/cargo-miri.rs @@ -23,7 +23,7 @@ Common options: --features Features to compile for the package -V, --version Print version info and exit -Other [options] are the same as `cargo rustc`. Everything after the first "--" is +Other [options] are the same as `cargo check`. Everything after the first "--" is passed verbatim to Miri, which will pass everything after the second "--" verbatim to the interpreted program. "#; @@ -84,33 +84,30 @@ fn get_arg_flag_value(name: &str) -> Option { } } - -/// Determines if we are being invoked (as rustc) to build a runnable -/// executable. We run "cargo check", so this should only happen when -/// we are trying to compile a build script or build script dependency, -/// which actually needs to be executed on the host platform. -/// -/// Currently, we detect this by checking for "--emit=link", -/// which indicates that Cargo instruced rustc to output -/// a native object. -fn is_build_dep() -> bool { - std::env::args().any(|arg| arg.starts_with("--emit=") && arg.contains("link")) +/// Returns the path to the `miri` binary +fn find_miri() -> PathBuf { + let mut path = std::env::current_exe().expect("current executable path invalid"); + path.set_file_name("miri"); + path } -/// Returns whether or not Cargo invoked the wrapper (this binary) to compile -/// the final, target crate (either a test for 'cargo test', or a binary for 'cargo run') -/// Cargo does not give us this information directly, so we need to check -/// various command-line flags. -fn is_target_crate(is_build_script: bool) -> bool { - let is_bin = get_arg_flag_value("--crate-type").as_deref() == Some("bin"); - let is_test = std::env::args().find(|arg| arg == "--test").is_some(); - - // The final runnable (under Miri) crate will either be a binary crate - // or a test crate. We make sure to exclude build scripts here, since - // they are also build with "--crate-type bin" - (is_bin || is_test) && !is_build_script +fn cargo() -> Command { + if let Ok(val) = std::env::var("CARGO") { + // Bootstrap tells us where to find cargo + Command::new(val) + } else { + Command::new("cargo") + } } +fn xargo() -> Command { + if let Ok(val) = std::env::var("XARGO") { + // Bootstrap tells us where to find xargo + Command::new(val) + } else { + Command::new("xargo-check") + } +} fn list_targets() -> impl Iterator { // We need to get the manifest, and then the metadata, to enumerate targets. @@ -155,13 +152,6 @@ fn list_targets() -> impl Iterator { package.targets.into_iter() } -/// Returns the path to the `miri` binary -fn find_miri() -> PathBuf { - let mut path = std::env::current_exe().expect("current executable path invalid"); - path.set_file_name("miri"); - path -} - /// Make sure that the `miri` and `rustc` binary are from the same sysroot. /// This can be violated e.g. when miri is locally built and installed with a different /// toolchain than what is used when `cargo miri` is run. @@ -211,24 +201,6 @@ fn test_sysroot_consistency() { } } -fn cargo() -> Command { - if let Ok(val) = std::env::var("CARGO") { - // Bootstrap tells us where to find cargo - Command::new(val) - } else { - Command::new("cargo") - } -} - -fn xargo() -> Command { - if let Ok(val) = std::env::var("XARGO") { - // Bootstrap tells us where to find xargo - Command::new(val) - } else { - Command::new("xargo-check") - } -} - fn xargo_version() -> Option<(u32, u32, u32)> { let out = xargo().arg("--version").output().ok()?; if !out.status.success() { @@ -385,6 +357,7 @@ features = ["panic_unwind"] ) .unwrap(); // The boring bits: a dummy project for xargo. + // FIXME: With xargo-check, can we avoid doing this? File::create(dir.join("Cargo.toml")) .unwrap() .write_all( @@ -447,12 +420,12 @@ fn main() { } if let Some("miri") = std::env::args().nth(1).as_ref().map(AsRef::as_ref) { - // This arm is for when `cargo miri` is called. We call `cargo rustc` for each applicable target, + // This arm is for when `cargo miri` is called. We call `cargo check` for each applicable target, // but with the `RUSTC` env var set to the `cargo-miri` binary so that we come back in the other branch, // and dispatch the invocations to `rustc` and `miri`, respectively. in_cargo_miri(); } else if let Some("rustc") = std::env::args().nth(1).as_ref().map(AsRef::as_ref) { - // This arm is executed when `cargo-miri` runs `cargo rustc` with the `RUSTC_WRAPPER` env var set to itself: + // This arm is executed when `cargo-miri` runs `cargo check` with the `RUSTC_WRAPPER` env var set to itself: // dependencies get dispatched to `rustc`, the final test/binary to `miri`. inside_cargo_rustc(); } else { @@ -491,7 +464,7 @@ fn in_cargo_miri() { .kind .get(0) .expect("badly formatted cargo metadata: target::kind is an empty array"); - // Now we run `cargo rustc $FLAGS $ARGS`, giving the user the + // Now we run `cargo check $FLAGS $ARGS`, giving the user the // change to add additional arguments. `FLAGS` is set to identify // this target. The user gets to control what gets actually passed to Miri. let mut cmd = cargo(); @@ -515,7 +488,7 @@ fn in_cargo_miri() { // The remaining targets we do not even want to build. _ => continue, } - // Add user-defined args until first `--`. + // Forward user-defined `cargo` args until first `--`. while let Some(arg) = args.next() { if arg == "--" { break; @@ -523,17 +496,21 @@ fn in_cargo_miri() { cmd.arg(arg); } - // Serialize our actual args into a special environemt variable. + // Serialize the remaining args into a special environemt variable. // This will be read by `inside_cargo_rustc` when we go to invoke // our actual target crate (the binary or the test we are running). // Since we're using "cargo check", we have no other way of passing // these arguments. let args_vec: Vec = args.collect(); - cmd.env("MIRI_MAGIC_ARGS", serde_json::to_string(&args_vec).expect("failed to serialize args")); + cmd.env("MIRI_ARGS", serde_json::to_string(&args_vec).expect("failed to serialize 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 + // the two codepaths. let path = std::env::current_exe().expect("current executable path invalid"); cmd.env("RUSTC_WRAPPER", path); if verbose { + cmd.env("MIRI_VERBOSE", ""); // this makes `inside_cargo_rustc` verbose. eprintln!("+ {:?}", cmd); } @@ -547,45 +524,71 @@ fn in_cargo_miri() { } fn inside_cargo_rustc() { - let sysroot = std::env::var("MIRI_SYSROOT").expect("The wrapper should have set MIRI_SYSROOT"); - - let rustc_args = std::env::args().skip(2); // skip `cargo rustc` + /// Determines if we are being invoked (as rustc) to build a runnable + /// executable. We run "cargo check", so this should only happen when + /// we are trying to compile a build script or build script dependency, + /// which actually needs to be executed on the host platform. + /// + /// Currently, we detect this by checking for "--emit=link", + /// which indicates that Cargo instruced rustc to output + /// a native object. + fn is_target_crate() -> bool { + // `--emit` is sometimes missing, e.g. cargo calls rustc for "--print". + // That is definitely not a target crate. + // If `--emit` is present, then host crates are built ("--emit=link,...), + // while the rest is only checked. + get_arg_flag_value("--emit").map_or(false, |emit| !emit.contains("link")) + } - let in_build_script = is_build_dep(); + /// Returns whether or not Cargo invoked the wrapper (this binary) to compile + /// the final, target crate (either a test for 'cargo test', or a binary for 'cargo run') + /// Cargo does not give us this information directly, so we need to check + /// various command-line flags. + fn is_runnable_crate() -> bool { + let is_bin = get_arg_flag_value("--crate-type").as_deref() == Some("bin"); + let is_test = has_arg_flag("--test"); + + // The final runnable (under Miri) crate will either be a binary crate + // or a test crate. We make sure to exclude build scripts here, since + // they are also build with "--crate-type bin" + is_bin || is_test + } - // Build scripts need to be compiled to actual runnable executables, - // and therefore completely bypass Miri. We make sure to only specify - // our custom Xargo sysroot for non-build-script crate - that is, - // crates which are ultimately going to get interpreted by Miri. - let mut args = if in_build_script { - rustc_args.collect() - } else { - let mut args: Vec = rustc_args - .chain(Some("--sysroot".to_owned())) - .chain(Some(sysroot)) - .collect(); + let verbose = std::env::var("MIRI_VERBOSE").is_ok(); + let target_crate = is_target_crate(); + + // Figure out which arguments we need to pass. + let mut args: Vec = std::env::args().skip(2).collect(); // skip `cargo-miri rustc` + // We make sure to only specify our custom Xargo sysroot and + // other args for target crates - that is, crates which are ultimately + // going to get interpreted by Miri. + if target_crate { + let sysroot = std::env::var("MIRI_SYSROOT").expect("The wrapper should have set MIRI_SYSROOT"); + args.push("--sysroot".to_owned()); + args.push(sysroot); args.splice(0..0, miri::miri_default_args().iter().map(ToString::to_string)); - args - }; + } - let needs_miri = - if is_target_crate(in_build_script) { - // This is the 'target crate '- the binary or test crate that + // Figure out the binary we need to call. If this is a runnable target crate, we want to call + // Miri to start interpretation; otherwise we want to call rustc to build the crate as usual. + let mut command = + if target_crate && is_runnable_crate() { + // This is the 'target crate' - the binary or test crate that // we want to interpret under Miri. We deserialize the user-provided arguments - // from the special environment variable "MIRI_MAGIC_ARGS", and feed them + // from the special environment variable "MIRI_ARGS", and feed them // to the 'miri' binary. - let magic = std::env::var("MIRI_MAGIC_ARGS").expect("missing MIRI_MAGIC_ARGS"); - let mut user_args: Vec = serde_json::from_str(&magic).expect("failed to deserialize args"); + let magic = std::env::var("MIRI_ARGS").expect("missing MIRI_ARGS"); + let mut user_args: Vec = serde_json::from_str(&magic).expect("failed to deserialize MIRI_ARGS"); args.append(&mut user_args); // Run this in Miri. - true + Command::new(find_miri()) } else { - false + Command::new("rustc") }; - let mut command = if needs_miri { Command::new(find_miri()) } else { Command::new("rustc") }; + // Run it. command.args(&args); - if has_arg_flag("-v") { + if verbose { eprintln!("+ {:?}", command); } @@ -594,7 +597,6 @@ fn inside_cargo_rustc() { if !exit.success() { std::process::exit(exit.code().unwrap_or(42)); }, - Err(ref e) if needs_miri => panic!("error during miri run: {:?}", e), - Err(ref e) => panic!("error during rustc call: {:?}", e), + Err(ref e) => panic!("error running {:?}:\n{:?}", command, e), } } From 47f2b127350e7d95fac67191467ecba3aed1befb Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 24 Feb 2020 14:42:45 +0100 Subject: [PATCH 3/4] fix Cargo.toml --- Cargo.toml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 236905b895..896c726b9a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,8 @@ required-features = ["rustc_tests"] cargo_metadata = { version = "0.9.0", optional = true } directories = { version = "2.0", optional = true } rustc_version = { version = "0.2.3", optional = true } +serde_json = { version = "1.0.44", optional = true } + getrandom = { version = "0.1.8", features = ["std"] } byteorder = "1.3" env_logger = "0.7.1" @@ -50,14 +52,13 @@ rustc-workspace-hack = "1.0.0" # between "cargo build" and "cargo intall". num-traits = "*" serde = { version = "*", features = ["derive"] } -serde_json = "1.0.44" [build-dependencies] vergen = "3" [features] default = ["cargo_miri"] -cargo_miri = ["cargo_metadata", "directories", "rustc_version"] +cargo_miri = ["cargo_metadata", "directories", "rustc_version", "serde_json"] rustc_tests = [] [dev-dependencies] From faf7bf538d3bf61321ca0128ad039d5d13fc2a40 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 24 Feb 2020 14:45:35 +0100 Subject: [PATCH 4/4] update lockfile --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index 7f5719b82a..951e91dad5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -334,6 +334,7 @@ dependencies = [ "rustc-workspace-hack 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "rustc_version 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.104 (registry+https://github.com/rust-lang/crates.io-index)", + "serde_json 1.0.48 (registry+https://github.com/rust-lang/crates.io-index)", "shell-escape 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", "vergen 3.0.4 (registry+https://github.com/rust-lang/crates.io-index)", ]