refactor cargo-miri a bit

Author: RalfJung

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<String> {
     }
 }
 
-
-/// 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<Item = cargo_metadata::Target> {
     // We need to get the manifest, and then the metadata, to enumerate targets.
@@ -155,13 +152,6 @@ fn list_targets() -> impl Iterator<Item = cargo_metadata::Target> {
     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,25 +488,29 @@ 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;
             }
             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<String> = 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<String> = 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<String> = 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<String> = 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<String> = 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),
     }
 }