make miri a better RUSTC by default inside cargo-miri

this requires a change in sysroot handling: miri driver now requires
MIRI_SYSROOT to be set when it is in 'target' mode, rather than relying on
`--sysroot` always being present.

Author: RalfJung

README.md

@@ -409,10 +409,9 @@ Moreover, Miri recognizes some environment variables:
   checkout. Note that changing files in that directory does not automatically
   trigger a re-build of the standard library; you have to clear the Miri build
   cache manually (on Linux, `rm -rf ~/.cache/miri`).
-* `MIRI_SYSROOT` (recognized by `cargo miri` and the test suite) indicates the
+* `MIRI_SYSROOT` (recognized by `cargo miri` and the Miri driver) indicates the
   sysroot to use. Only set this if you do not want to use the automatically
-  created sysroot. (The `miri` driver sysroot is controlled via the `--sysroot`
-  flag instead.)
+  created sysroot.
 * `MIRI_TEST_TARGET` (recognized by the test suite and the `./miri` script) indicates which target
   architecture to test against.  `miri` and `cargo miri` accept the `--target` flag for the same
   purpose.

cargo-miri/bin.rs

@@ -205,12 +205,6 @@ fn forward_patched_extern_arg(args: &mut impl Iterator<Item = String>, cmd: &mut
     }
 }
 
-fn forward_miri_sysroot(cmd: &mut Command) {
-    let sysroot = env::var_os("MIRI_SYSROOT").expect("the wrapper should have set MIRI_SYSROOT");
-    cmd.arg("--sysroot");
-    cmd.arg(sysroot);
-}
-
 /// Escapes `s` in a way that is suitable for using it as a string literal in TOML syntax.
 fn escape_for_toml(s: &str) -> String {
     // We want to surround this string in quotes `"`. So we first escape all quotes,
@@ -237,8 +231,15 @@ fn miri() -> Command {
     Command::new(find_miri())
 }
 
+fn miri_for_host() -> Command {
+    let mut cmd = miri();
+    cmd.env("MIRI_BE_RUSTC", "host");
+    cmd
+}
+
 fn version_info() -> VersionMeta {
-    VersionMeta::for_command(miri()).expect("failed to determine underlying rustc version of Miri")
+    VersionMeta::for_command(miri_for_host())
+        .expect("failed to determine underlying rustc version of Miri")
 }
 
 fn cargo() -> Command {
@@ -336,7 +337,7 @@ fn ask_to_run(mut cmd: Command, ask: bool, text: &str) {
             a => show_error(format!("invalid answer `{}`", a)),
         };
     } else {
-        println!("Running `{:?}` to {}.", cmd, text);
+        eprintln!("Running `{:?}` to {}.", cmd, text);
     }
 
     if cmd.status().unwrap_or_else(|_| panic!("failed to execute {:?}", cmd)).success().not() {
@@ -364,7 +365,7 @@ fn write_to_file(filename: &Path, content: &str) {
 /// Performs the setup required to make `cargo miri` work: Getting a custom-built libstd. Then sets
 /// `MIRI_SYSROOT`. Skipped if `MIRI_SYSROOT` is already set, in which case we expect the user has
 /// done all this already.
-fn setup(subcommand: &MiriCommand) {
+fn setup(subcommand: &MiriCommand, host: &str, target: &str) {
     let only_setup = matches!(subcommand, MiriCommand::Setup);
     let ask_user = !only_setup;
     let print_sysroot = only_setup && has_arg_flag("--print-sysroot"); // whether we just print the sysroot path
@@ -398,8 +399,10 @@ fn setup(subcommand: &MiriCommand) {
         }
         None => {
             // Check for `rust-src` rustup component.
-            let output =
-                miri().args(&["--print", "sysroot"]).output().expect("failed to determine sysroot");
+            let output = miri_for_host()
+                .args(&["--print", "sysroot"])
+                .output()
+                .expect("failed to determine sysroot");
             if !output.status.success() {
                 show_error(format!(
                     "Failed to determine sysroot; Miri said:\n{}",
@@ -472,18 +475,21 @@ path = "lib.rs"
     );
     write_to_file(&dir.join("lib.rs"), "#![no_std]");
 
-    // Determine architectures.
-    // We always need to set a target so rustc bootstrap can tell apart host from target crates.
-    let host = version_info().host;
-    let target = get_arg_flag_value("--target");
-    let target = target.as_ref().unwrap_or(&host);
+    // Figure out where xargo will build its stuff.
+    // Unfortunately, it puts things into a different directory when the
+    // architecture matches the host.
+    let sysroot = if target == host { dir.join("HOST") } else { PathBuf::from(dir) };
+    // Make sure all target-level Miri invocations know their sysroot.
+    std::env::set_var("MIRI_SYSROOT", &sysroot);
+
     // Now invoke xargo.
     let mut command = xargo_check();
     command.arg("check").arg("-q");
-    command.arg("--target").arg(target);
     command.current_dir(&dir);
     command.env("XARGO_HOME", &dir);
     command.env("XARGO_RUST_SRC", &rust_src);
+    // We always need to set a target so rustc bootstrap can tell apart host from target crates.
+    command.arg("--target").arg(target);
     // Use Miri as rustc to build a libstd compatible with us (and use the right flags).
     // However, when we are running in bootstrap, we cannot just overwrite `RUSTC`,
     // because we still need bootstrap to distinguish between host and target crates.
@@ -523,6 +529,7 @@ path = "lib.rs"
         command.stdout(process::Stdio::null());
         command.stderr(process::Stdio::null());
     }
+
     // Finally run it!
     if command.status().expect("failed to run xargo").success().not() {
         if only_setup {
@@ -534,11 +541,6 @@ path = "lib.rs"
         }
     }
 
-    // That should be it! But we need to figure out where xargo built stuff.
-    // Unfortunately, it puts things into a different directory when the
-    // architecture matches the host.
-    let sysroot = if target == &host { dir.join("HOST") } else { PathBuf::from(dir) };
-    std::env::set_var("MIRI_SYSROOT", &sysroot); // pass the env var to the processes we spawn, which will turn it into "--sysroot" flags
     // Figure out what to print.
     if only_setup {
         eprintln!("A sysroot for Miri is now available in `{}`.", sysroot.display());
@@ -677,8 +679,13 @@ fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     };
     let verbose = num_arg_flag("-v");
 
+    // Determine the involved architectures.
+    let host = version_info().host;
+    let target = get_arg_flag_value("--target");
+    let target = target.as_ref().unwrap_or(&host);
+
     // We always setup.
-    setup(&subcommand);
+    setup(&subcommand, &host, target);
 
     // Invoke actual cargo for the job, but with different flags.
     // We re-use `cargo test` and `cargo run`, which makes target and binary handling very easy but
@@ -727,7 +734,7 @@ fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     if get_arg_flag_value("--target").is_none() {
         // No target given. Explicitly pick the host.
         cmd.arg("--target");
-        cmd.arg(version_info().host);
+        cmd.arg(&host);
     }
 
     // Set ourselves as runner for al binaries invoked by cargo.
@@ -754,16 +761,19 @@ fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
             "WARNING: Ignoring `RUSTC` environment variable; set `MIRI` if you want to control the binary used as the driver."
         );
     }
-    // We'd prefer to just clear this env var, but cargo does not always honor `RUSTC_WRAPPER`
-    // (https://github.com/rust-lang/cargo/issues/10885). There is no good way to single out these invocations;
-    // some build scripts use the RUSTC env var as well. So we set it directly to the `miri` driver and
-    // hope that all they do is ask for the version number -- things could quickly go downhill from here.
+    // Build scripts (and also cargo: https://github.com/rust-lang/cargo/issues/10885) will invoke `rustc` even when `RUSTC_WRAPPER` is set.
+    // To make sure everything is coherent, we want that to be the Miri driver, but acting as rustc, on the target level.
+    // (Target, rather than host, is needed for cross-interpretation situations.) This is not a
+    // perfect emulation of real rustc (it might be unable to produce binaries since the sysroot is
+    // check-only), but it's as close as we can get, and it's good enough for autocfg.
+    //
     // In `main`, we need the value of `RUSTC` to distinguish RUSTC_WRAPPER invocations from rustdoc
     // or TARGET_RUNNER invocations, so we canonicalize it here to make it exceedingly unlikely that
     // there would be a collision with other invocations of cargo-miri (as rustdoc or as runner).
     // We explicitly do this even if RUSTC_STAGE is set, since for these builds we do *not* want the
     // bootstrap `rustc` thing in our way! Instead, we have MIRI_HOST_SYSROOT to use for host builds.
     cmd.env("RUSTC", &fs::canonicalize(find_miri()).unwrap());
+    cmd.env("MIRI_BE_RUSTC", "target"); // we better remember to *unset* this in the other phases!
 
     // Set rustdoc to us as well, so we can run doctests.
     cmd.env("RUSTDOC", &cargo_miri_path);
@@ -832,6 +842,11 @@ fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
         }
     }
 
+    // phase_cargo_miri set `MIRI_BE_RUSTC` for when build scripts directly invoke the driver;
+    // however, if we get called back by cargo here, we'll carefully compute the right flags
+    // ourselves, so we first un-do what the earlier phase did.
+    env::remove_var("MIRI_BE_RUSTC");
+
     let verbose = std::env::var("MIRI_VERBOSE")
         .map_or(0, |verbose| verbose.parse().expect("verbosity flag must be an integer"));
     let target_crate = is_target_crate();
@@ -946,11 +961,6 @@ fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
             }
         }
 
-        // Use our custom sysroot (but not if that is what we are currently building).
-        if phase != RustcPhase::Setup {
-            forward_miri_sysroot(&mut cmd);
-        }
-
         // During setup, patch the panic runtime for `libpanic_abort` (mirroring what bootstrap usually does).
         if phase == RustcPhase::Setup
             && get_arg_flag_value("--crate-name").as_deref() == Some("panic_abort")
@@ -1010,6 +1020,11 @@ enum RunnerPhase {
 }
 
 fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: RunnerPhase) {
+    // phase_cargo_miri set `MIRI_BE_RUSTC` for when build scripts directly invoke the driver;
+    // however, if we get called back by cargo here, we'll carefully compute the right flags
+    // ourselves, so we first un-do what the earlier phase did.
+    env::remove_var("MIRI_BE_RUSTC");
+
     let verbose = std::env::var("MIRI_VERBOSE")
         .map_or(0, |verbose| verbose.parse().expect("verbosity flag must be an integer"));
 
@@ -1077,10 +1092,6 @@ fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: RunnerPhas
             cmd.arg(arg);
         }
     }
-    // Set sysroot (if we are inside rustdoc, we already did that in `phase_cargo_rustdoc`).
-    if phase != RunnerPhase::Rustdoc {
-        forward_miri_sysroot(&mut cmd);
-    }
     // Respect `MIRIFLAGS`.
     if let Ok(a) = env::var("MIRIFLAGS") {
         // This code is taken from `RUSTFLAGS` handling in cargo.
@@ -1151,7 +1162,7 @@ fn phase_rustdoc(mut args: impl Iterator<Item = String>) {
     cmd.arg("-Z").arg("unstable-options");
 
     // rustdoc needs to know the right sysroot.
-    forward_miri_sysroot(&mut cmd);
+    cmd.arg("--sysroot").arg(env::var_os("MIRI_SYSROOT").unwrap());
     // make sure the 'miri' flag is set for rustdoc
     cmd.arg("--cfg").arg("miri");
 

miri

@@ -131,7 +131,11 @@ export RUSTFLAGS="-C link-args=-Wl,-rpath,$LIBDIR $RUSTFLAGS"
 
 # Build a sysroot and set MIRI_SYSROOT to use it. Arguments are passed to `cargo miri setup`.
 build_sysroot() {
-    export MIRI_SYSROOT="$($CARGO run $CARGO_EXTRA_FLAGS --manifest-path "$MIRIDIR"/cargo-miri/Cargo.toml -q -- miri setup --print-sysroot "$@")"
+    if ! MIRI_SYSROOT="$($CARGO run $CARGO_EXTRA_FLAGS --manifest-path "$MIRIDIR"/cargo-miri/Cargo.toml -q -- miri setup --print-sysroot "$@")"; then
+        echo "'cargo miri setup' failed"
+        exit 1
+    fi
+    export MIRI_SYSROOT
 }
 
 # Prepare and set MIRI_SYSROOT. Respects `MIRI_TEST_TARGET` and takes into account
@@ -201,7 +205,7 @@ run)
     $CARGO build $CARGO_EXTRA_FLAGS --manifest-path "$MIRIDIR"/Cargo.toml
     find_sysroot
     # Then run the actual command.
-    exec $CARGO run $CARGO_EXTRA_FLAGS --manifest-path "$MIRIDIR"/Cargo.toml -- --sysroot "$MIRI_SYSROOT" $MIRIFLAGS "$@"
+    exec $CARGO run $CARGO_EXTRA_FLAGS --manifest-path "$MIRIDIR"/Cargo.toml -- $MIRIFLAGS "$@"
     ;;
 fmt)
     find "$MIRIDIR" -not \( -name target -prune \) -name '*.rs' \

src/bin/miri.rs

@@ -27,7 +27,7 @@ use rustc_middle::{
     },
     ty::{query::ExternProviders, TyCtxt},
 };
-use rustc_session::{search_paths::PathKind, CtfeBacktrace};
+use rustc_session::{config::CrateType, search_paths::PathKind, CtfeBacktrace};
 
 use miri::{BacktraceStyle, ProvenanceMode};
 
@@ -60,6 +60,10 @@ impl rustc_driver::Callbacks for MiriCompilerCalls {
 
         queries.global_ctxt().unwrap().peek_mut().enter(|tcx| {
             init_late_loggers(tcx);
+            if !tcx.sess.crate_types().contains(&CrateType::Executable) {
+                tcx.sess.fatal("miri only makes sense on bin crates");
+            }
+
             let (entry_def_id, entry_type) = if let Some(entry_def) = tcx.entry_fn(()) {
                 entry_def
             } else {
@@ -204,9 +208,9 @@ fn init_late_loggers(tcx: TyCtxt<'_>) {
     }
 }
 
-/// Returns the "default sysroot" that Miri will use if no `--sysroot` flag is set.
+/// Returns the "default sysroot" that Miri will use for host things if no `--sysroot` flag is set.
 /// Should be a compile-time constant.
-fn compile_time_sysroot() -> Option<String> {
+fn host_sysroot() -> Option<String> {
     if option_env!("RUSTC_STAGE").is_some() {
         // This is being built as part of rustc, and gets shipped with rustup.
         // We can rely on the sysroot computation in librustc_session.
@@ -227,7 +231,7 @@ fn compile_time_sysroot() -> Option<String> {
                 if toolchain_runtime != toolchain {
                     show_error(format!(
                         "This Miri got built with local toolchain `{toolchain}`, but now is being run under a different toolchain. \n\
-                    Make sure to run Miri in the toolchain it got built with, e.g. via `cargo +{toolchain} miri`."
+                        Make sure to run Miri in the toolchain it got built with, e.g. via `cargo +{toolchain} miri`."
                     ));
                 }
             }
@@ -246,25 +250,42 @@ fn compile_time_sysroot() -> Option<String> {
 /// Execute a compiler with the given CLI arguments and callbacks.
 fn run_compiler(
     mut args: Vec<String>,
+    target_crate: bool,
     callbacks: &mut (dyn rustc_driver::Callbacks + Send),
-    insert_default_args: bool,
 ) -> ! {
     // Make sure we use the right default sysroot. The default sysroot is wrong,
     // because `get_or_default_sysroot` in `librustc_session` bases that on `current_exe`.
     //
-    // Make sure we always call `compile_time_sysroot` as that also does some sanity-checks
-    // of the environment we were built in.
-    // FIXME: Ideally we'd turn a bad build env into a compile-time error via CTFE or so.
-    if let Some(sysroot) = compile_time_sysroot() {
-        let sysroot_flag = "--sysroot";
-        if !args.iter().any(|e| e == sysroot_flag) {
+    // Make sure we always call `host_sysroot` as that also does some sanity-checks
+    // of the environment we were built in and whether it matches what we are running in.
+    let host_default_sysroot = host_sysroot();
+    // Now see if we even need to set something.
+    let sysroot_flag = "--sysroot";
+    if !args.iter().any(|e| e == sysroot_flag) {
+        // No sysroot was set, let's see if we have a custom default we want to configure.
+        let default_sysroot = if target_crate {
+            // Using the built-in default here would be plain wrong, so we *require*
+            // the env var to make sure things make sense.
+            Some(env::var("MIRI_SYSROOT").unwrap_or_else(|_| {
+                show_error(format!(
+                    "Miri was invoked in 'target' mode without `MIRI_SYSROOT` or `--sysroot` being set"
+                ))
+            }))
+        } else {
+            host_default_sysroot
+        };
+        if let Some(sysroot) = default_sysroot {
             // We need to overwrite the default that librustc_session would compute.
             args.push(sysroot_flag.to_owned());
             args.push(sysroot);
         }
     }
 
-    if insert_default_args {
+    // Don't insert `MIRI_DEFAULT_ARGS`, in particular, `--cfg=miri`, if we are building
+    // a "host" crate. That may cause procedural macros (and probably build scripts) to
+    // depend on Miri-only symbols, such as `miri_resolve_frame`:
+    // https://github.com/rust-lang/miri/issues/1760
+    if target_crate {
         // Some options have different defaults in Miri than in plain rustc; apply those by making
         // them the first arguments after the binary name (but later arguments can overwrite them).
         args.splice(1..1, miri::MIRI_DEFAULT_ARGS.iter().map(ToString::to_string));
@@ -302,13 +323,8 @@ fn main() {
         // We cannot use `rustc_driver::main` as we need to adjust the CLI arguments.
         run_compiler(
             env::args().collect(),
+            target_crate,
             &mut MiriBeRustCompilerCalls { target_crate },
-            // Don't insert `MIRI_DEFAULT_ARGS`, in particular, `--cfg=miri`, if we are building
-            // a "host" crate. That may cause procedural macros (and probably build scripts) to
-            // depend on Miri-only symbols, such as `miri_resolve_frame`:
-            // https://github.com/rust-lang/miri/issues/1760
-            #[rustfmt::skip]
-            /* insert_default_args: */ target_crate,
         )
     }
 
@@ -502,9 +518,5 @@ fn main() {
 
     debug!("rustc arguments: {:?}", rustc_args);
     debug!("crate arguments: {:?}", miri_config.args);
-    run_compiler(
-        rustc_args,
-        &mut MiriCompilerCalls { miri_config },
-        /* insert_default_args: */ true,
-    )
+    run_compiler(rustc_args, /* target_crate: */ true, &mut MiriCompilerCalls { miri_config })
 }

test-cargo-miri/Cargo.lock

@@ -21,6 +21,9 @@ issue_rust_86261 = { path = "issue-rust-86261" }
 byteorder_2 = { package = "byteorder", version = "0.5" } # to test dev-dependencies behave as expected, with renaming
 serde_derive = "1.0" # not actually used, but exercises some unique code path (`--extern` .so file)
 
+[build-dependencies]
+autocfg = "1"
+
 [lib]
 test = false # test that this is respected (will show in the output)