Auto merge of #123317 - RalfJung:test-in-miri, r=m-ou-se,saethlin,onu…

…r-ozkan

Support running library tests in Miri

This adds a new bootstrap subcommand `./x.py miri` which can test libraries in Miri. This is in preparation for eventually doing that as part of bors CI, but this PR only adds the infrastructure, and doesn't enable it yet.

`@rust-lang/bootstrap` should this be `x.py test --miri library/core` or `x.py miri library/core`? The flag has the advantage that we don't have to copy all the arguments from `Subcommand::Test`. It has the disadvantage that most test steps just ignore `--miri` and still run tests the regular way. For clippy you went the route of making it a separate subcommand. ~~I went with a flag now as that seemed easier, but I can change this.~~ I made it a new subcommand. Note however that the regular cargo invocation would be `cargo miri test ...`, so `x.py` is still going to be different in that the `test` is omitted. That said, we could also make it `./x.py miri-test` to make that difference smaller -- that's in fact more consistent with the internal name of the command when bootstrap invokes cargo.

`@rust-lang/libs` ~~unfortunately this PR does some unholy things to the `lib.rs` files of our library crates.~~
`@m-ou-se` found a way that entirely avoids library-level hacks, except for some new small `lib.miri.rs` files that hopefully you will never have to touch. There's a new hack in cargo-miri but there it is in good company...

README.md

@@ -507,6 +507,8 @@ binaries, and as such worth documenting:
   crate currently being compiled.
 * `MIRI_ORIG_RUSTDOC` is set and read by different phases of `cargo-miri` to remember the
   value of `RUSTDOC` from before it was overwritten.
+* `MIRI_REPLACE_LIBRS_IF_NOT_TEST` when set to any value enables a hack that helps bootstrap
+  run the standard library tests in Miri.
 * `MIRI_VERBOSE` when set to any value tells the various `cargo-miri` phases to
   perform verbose logging.
 * `MIRI_HOST_SYSROOT` is set by bootstrap to tell `cargo-miri` which sysroot to use for *host*

cargo-miri/src/phases.rs

@@ -3,7 +3,7 @@
 use std::env;
 use std::fs::{self, File};
 use std::io::BufReader;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 
 use rustc_version::VersionMeta;
@@ -412,9 +412,25 @@ pub fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
     // Arguments are treated very differently depending on whether this crate is
     // for interpretation by Miri, or for use by a build script / proc macro.
     if target_crate {
-        // Forward arguments, but remove "link" from "--emit" to make this a check-only build.
+        // Forward arguments, but patched.
         let emit_flag = "--emit";
+        // This hack helps bootstrap run standard library tests in Miri. The issue is as follows:
+        // when running `cargo miri test` on libcore, cargo builds a local copy of core and makes it
+        // a dependency of the integration test crate. This copy duplicates all the lang items, so
+        // the build fails. (Regular testing avoids this because the sysroot is a literal copy of
+        // what `cargo build` produces, but since Miri builds its own sysroot this does not work for
+        // us.) So we need to make it so that the locally built libcore contains all the items from
+        // `core`, but does not re-define them -- we want to replace the entire crate but a
+        // re-export of the sysroot crate. We do this by swapping out the source file: if
+        // `MIRI_REPLACE_LIBRS_IF_NOT_TEST` is set and we are building a `lib.rs` file, and a
+        // `lib.miri.rs` file exists in the same folder, we build that instead. But crucially we
+        // only do that for the library, not the unit test crate (which would be runnable) or
+        // rustdoc (which would have a different `phase`).
+        let replace_librs = env::var_os("MIRI_REPLACE_LIBRS_IF_NOT_TEST").is_some()
+            && !runnable_crate
+            && phase == RustcPhase::Build;
         while let Some(arg) = args.next() {
+            // Patch `--emit`: remove "link" from "--emit" to make this a check-only build.
             if let Some(val) = arg.strip_prefix(emit_flag) {
                 // Patch this argument. First, extract its value.
                 let val =
@@ -429,13 +445,36 @@ pub fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
                     }
                 }
                 cmd.arg(format!("{emit_flag}={}", val.join(",")));
-            } else if arg == "--extern" {
-                // Patch `--extern` filenames, since Cargo sometimes passes stub `.rlib` files:
-                // https://github.com/rust-lang/miri/issues/1705
+                continue;
+            }
+            // Patch `--extern` filenames, since Cargo sometimes passes stub `.rlib` files:
+            // https://github.com/rust-lang/miri/issues/1705
+            if arg == "--extern" {
                 forward_patched_extern_arg(&mut args, &mut cmd);
-            } else {
-                cmd.arg(arg);
+                continue;
             }
+            // If the REPLACE_LIBRS hack is enabled and we are building a `lib.rs` file, and a
+            // `lib.miri.rs` file exists, then build that instead. We only consider relative paths
+            // as cargo uses those for files in the workspace; dependencies from crates.io get
+            // absolute paths.
+            if replace_librs {
+                let path = Path::new(&arg);
+                if path.is_relative()
+                    && path.file_name().is_some_and(|f| f == "lib.rs")
+                    && path.is_file()
+                {
+                    let miri_rs = Path::new(&arg).with_extension("miri.rs");
+                    if miri_rs.is_file() {
+                        if verbose > 0 {
+                            eprintln!("Performing REPLACE_LIBRS hack: {arg:?} -> {miri_rs:?}");
+                        }
+                        cmd.arg(miri_rs);
+                        continue;
+                    }
+                }
+            }
+            // Fallback: just propagate the argument.
+            cmd.arg(arg);
         }
 
         // During setup, patch the panic runtime for `libpanic_abort` (mirroring what bootstrap usually does).

src/helpers.rs

@@ -105,24 +105,41 @@ fn try_resolve_did(tcx: TyCtxt<'_>, path: &[&str], namespace: Option<Namespace>)
         (path, None)
     };
 
-    // First find the crate.
-    let krate =
-        tcx.crates(()).iter().find(|&&krate| tcx.crate_name(krate).as_str() == crate_name)?;
-    let mut cur_item = DefId { krate: *krate, index: CRATE_DEF_INDEX };
-    // Then go over the modules.
-    for &segment in modules {
-        cur_item = find_children(tcx, cur_item, segment)
-            .find(|item| tcx.def_kind(item) == DefKind::Mod)?;
-    }
-    // Finally, look up the desired item in this module, if any.
-    match item {
-        Some((item_name, namespace)) =>
-            Some(
-                find_children(tcx, cur_item, item_name)
-                    .find(|item| tcx.def_kind(item).ns() == Some(namespace))?,
-            ),
-        None => Some(cur_item),
+    // There may be more than one crate with this name. We try them all.
+    // (This is particularly relevant when running `std` tests as then there are two `std` crates:
+    // the one in the sysroot and the one locally built by `cargo test`.)
+    // FIXME: can we prefer the one from the sysroot?
+    'crates: for krate in
+        tcx.crates(()).iter().filter(|&&krate| tcx.crate_name(krate).as_str() == crate_name)
+    {
+        let mut cur_item = DefId { krate: *krate, index: CRATE_DEF_INDEX };
+        // Go over the modules.
+        for &segment in modules {
+            let Some(next_item) = find_children(tcx, cur_item, segment)
+                .find(|item| tcx.def_kind(item) == DefKind::Mod)
+            else {
+                continue 'crates;
+            };
+            cur_item = next_item;
+        }
+        // Finally, look up the desired item in this module, if any.
+        match item {
+            Some((item_name, namespace)) => {
+                let Some(item) = find_children(tcx, cur_item, item_name)
+                    .find(|item| tcx.def_kind(item).ns() == Some(namespace))
+                else {
+                    continue 'crates;
+                };
+                return Some(item);
+            }
+            None => {
+                // Just return the module.
+                return Some(cur_item);
+            }
+        }
     }
+    // Item not found in any of the crates with the right name.
+    None
 }
 
 /// Convert a softfloat type to its corresponding hostfloat type.
@@ -968,10 +985,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
     fn frame_in_std(&self) -> bool {
         let this = self.eval_context_ref();
-        let Some(start_fn) = this.tcx.lang_items().start_fn() else {
-            // no_std situations
-            return false;
-        };
         let frame = this.frame();
         // Make an attempt to get at the instance of the function this is inlined from.
         let instance: Option<_> = try {
@@ -982,13 +995,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         };
         // Fall back to the instance of the function itself.
         let instance = instance.unwrap_or(frame.instance);
-        // Now check if this is in the same crate as start_fn.
-        // As a special exception we also allow unit tests from
-        // <https://github.com/rust-lang/miri-test-libstd/tree/master/std_miri_test> to call these
-        // shims.
+        // Now check the crate it is in. We could try to be clever here and e.g. check if this is
+        // the same crate as `start_fn`, but that would not work for running std tests in Miri, so
+        // we'd need some more hacks anyway. So we just check the name of the crate. If someone
+        // calls their crate `std` then we'll just let them keep the pieces.
         let frame_crate = this.tcx.def_path(instance.def_id()).krate;
-        frame_crate == this.tcx.def_path(start_fn).krate
-            || this.tcx.crate_name(frame_crate).as_str() == "std_miri_test"
+        let crate_name = this.tcx.crate_name(frame_crate);
+        let crate_name = crate_name.as_str();
+        // On miri-test-libstd, the name of the crate is different.
+        crate_name == "std" || crate_name == "std_miri_test"
     }
 
     /// Handler that should be called when unsupported functionality is encountered.