From 4fccde54acc986a7446ae487cb028fec59fadcb4 Mon Sep 17 00:00:00 2001 From: Tristan Dannenberg Date: Tue, 12 Jan 2021 22:16:48 +0100 Subject: [PATCH 1/8] make cargo-miri run doc-tests --- README.md | 4 + cargo-miri/bin.rs | 211 ++++++++++++++++-- test-cargo-miri/run-test.py | 19 +- test-cargo-miri/test.cross-target.stdout.ref | 10 + test-cargo-miri/test.default.stdout.ref | 6 + .../test.filter.cross-target.stdout.ref | 11 + test-cargo-miri/test.filter.stdout.ref | 5 + test-cargo-miri/test.stderr-rustdoc.ref | 1 - 8 files changed, 239 insertions(+), 28 deletions(-) create mode 100644 test-cargo-miri/test.cross-target.stdout.ref create mode 100644 test-cargo-miri/test.filter.cross-target.stdout.ref delete mode 100644 test-cargo-miri/test.stderr-rustdoc.ref diff --git a/README.md b/README.md index 8a0c5180e0..8a80a57e2d 100644 --- a/README.md +++ b/README.md @@ -290,6 +290,10 @@ different Miri binaries, and as such worth documenting: that the compiled `rlib`s are compatible with Miri. When set while running `cargo-miri`, it indicates that we are part of a sysroot build (for which some crates need special treatment). +* `MIRI_CALLED_FROM_RUSTDOC` when set to any value tells `cargo-miri` that it is + running as a child process of `rustdoc`, which invokes it twice for each doc-test + and requires special treatment, most notably a check-only build before interpretation. + This is set by `cargo-miri` itself when running as a `rustdoc`-wrapper. * `MIRI_CWD` when set to any value tells the Miri driver to change to the given directory after loading all the source files, but before commencing interpretation. This is useful if the interpreted program wants a different diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index af1dbad32a..1f749b077a 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -1,8 +1,8 @@ use std::env; use std::ffi::OsString; use std::fs::{self, File}; -use std::io::{self, BufRead, BufReader, BufWriter, Write}; use std::iter::TakeWhile; +use std::io::{self, BufRead, BufReader, BufWriter, Read, Write}; use std::ops::Not; use std::path::{Path, PathBuf}; use std::process::Command; @@ -46,6 +46,8 @@ struct CrateRunEnv { env: Vec<(OsString, OsString)>, /// The current working directory. current_dir: OsString, + /// The contents passed via standard input. + stdin: Vec, } /// The information Miri needs to run a crate. Stored as JSON when the crate is "compiled". @@ -63,7 +65,13 @@ impl CrateRunInfo { let args = args.collect(); let env = env::vars_os().collect(); let current_dir = env::current_dir().unwrap().into_os_string(); - Self::RunWith(CrateRunEnv { args, env, current_dir }) + + let mut stdin = Vec::new(); + if env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() { + std::io::stdin().lock().read_to_end(&mut stdin).expect("cannot read stdin"); + } + + Self::RunWith(CrateRunEnv { args, env, current_dir, stdin }) } fn store(&self, filename: &Path) { @@ -190,6 +198,22 @@ fn exec(mut cmd: Command) { } } +/// Execute the command and pipe `input` into its stdin. +/// If it fails, fail this process with the same exit code. +/// Otherwise, continue. +fn exec_with_pipe(mut cmd: Command, input: &[u8]) { + cmd.stdin(std::process::Stdio::piped()); + let mut child = cmd.spawn().expect("failed to spawn process"); + { + let stdin = child.stdin.as_mut().expect("failed to open stdin"); + stdin.write_all(input).expect("failed to write out test source"); + } + let exit_status = child.wait().expect("failed to run command"); + if exit_status.success().not() { + std::process::exit(exit_status.code().unwrap_or(-1)) + } +} + fn xargo_version() -> Option<(u32, u32, u32)> { let out = xargo_check().arg("--version").output().ok()?; if !out.status.success() { @@ -591,24 +615,29 @@ fn phase_cargo_rustc(mut args: env::Args) { } fn out_filename(prefix: &str, suffix: &str) -> PathBuf { - let mut path = PathBuf::from(get_arg_flag_value("--out-dir").unwrap()); - path.push(format!( - "{}{}{}{}", - prefix, - get_arg_flag_value("--crate-name").unwrap(), - // This is technically a `-C` flag but the prefix seems unique enough... - // (and cargo passes this before the filename so it should be unique) - get_arg_flag_value("extra-filename").unwrap_or(String::new()), - suffix, - )); - path + if let Some(out_dir) = get_arg_flag_value("--out-dir") { + let mut path = PathBuf::from(out_dir); + path.push(format!( + "{}{}{}{}", + prefix, + get_arg_flag_value("--crate-name").unwrap(), + // This is technically a `-C` flag but the prefix seems unique enough... + // (and cargo passes this before the filename so it should be unique) + get_arg_flag_value("extra-filename").unwrap_or(String::new()), + suffix, + )); + path + } else { + let out_file = get_arg_flag_value("-o").unwrap(); + PathBuf::from(out_file) + } } let verbose = std::env::var_os("MIRI_VERBOSE").is_some(); let target_crate = is_target_crate(); let print = get_arg_flag_value("--print").is_some(); // whether this is cargo passing `--print` to get some infos - let store_json = |info: CrateRunInfo| { + let store_json = |info: &CrateRunInfo| { // Create a stub .d file to stop Cargo from "rebuilding" the crate: // https://github.com/rust-lang/miri/issues/1724#issuecomment-787115693 // As we store a JSON file instead of building the crate here, an empty file is fine. @@ -636,7 +665,47 @@ fn phase_cargo_rustc(mut args: env::Args) { // like we want them. // Instead of compiling, we write JSON into the output file with all the relevant command-line flags // and environment variables; this is used when cargo calls us again in the CARGO_TARGET_RUNNER phase. - store_json(CrateRunInfo::collect(args)); + let info = CrateRunInfo::collect(args); + store_json(&info); + + // Rustdoc expects us to exit with an error code if the test is marked as `compile_fail`, + // just creating the JSON file is not enough: we need to detect syntax errors, + // so we need to run Miri with `MIRI_BE_RUSTC` for a check-only build. + if std::env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() { + let mut cmd = miri(); + let env = if let CrateRunInfo::RunWith(env) = info { + env + } else { + return; + }; + + // use our own sysroot + if !has_arg_flag("--sysroot") { + let sysroot = env::var_os("MIRI_SYSROOT") + .expect("the wrapper should have set MIRI_SYSROOT"); + cmd.arg("--sysroot").arg(sysroot); + } + + // ensure --emit argument for a check-only build is present + if let Some(i) = env.args.iter().position(|arg| arg.starts_with("--emit=")) { + // We need to make sure we're not producing a binary that overwrites the JSON file. + // rustdoc should only ever pass an --emit=metadata argument for tests marked as `no_run`: + assert_eq!(env.args[i], "--emit=metadata"); + } else { + cmd.arg("--emit=dep-info,metadata"); + } + + cmd.args(env.args); + cmd.env("MIRI_BE_RUSTC", "1"); + + if verbose { + eprintln!("[cargo-miri rustc] captured input:\n{}", std::str::from_utf8(&env.stdin).unwrap()); + eprintln!("[cargo-miri rustc] {:?}", cmd); + } + + exec_with_pipe(cmd, &env.stdin); + } + return; } @@ -644,7 +713,7 @@ fn phase_cargo_rustc(mut args: env::Args) { // This is a "runnable" `proc-macro` crate (unit tests). We do not support // interpreting that under Miri now, so we write a JSON file to (display a // helpful message and) skip it in the runner phase. - store_json(CrateRunInfo::SkipProcMacroTest); + store_json(&CrateRunInfo::SkipProcMacroTest); return; } @@ -715,6 +784,18 @@ fn phase_cargo_rustc(mut args: env::Args) { } } +fn forward_patched_extern_arg(args: &mut impl Iterator, cmd: &mut Command) { + cmd.arg("--extern"); // always forward flag, but adjust filename: + let path = args.next().expect("`--extern` should be followed by a filename"); + if let Some(lib) = path.strip_suffix(".rlib") { + // If this is an rlib, make it an rmeta. + cmd.arg(format!("{}.rmeta", lib)); + } else { + // Some other extern file (e.g. a `.so`). Forward unchanged. + cmd.arg(path); + } +} + fn phase_cargo_runner(binary: &Path, binary_args: env::Args) { let verbose = std::env::var_os("MIRI_VERBOSE").is_some(); @@ -801,6 +882,73 @@ fn phase_cargo_runner(binary: &Path, binary_args: env::Args) { if verbose { eprintln!("[cargo-miri runner] {:?}", cmd); } + + if std::env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() { + exec_with_pipe(cmd, &info.stdin) + } else { + exec(cmd) + } +} + +fn phase_cargo_rustdoc(fst_arg: &str, mut args: env::Args) { + let verbose = std::env::var_os("MIRI_VERBOSE").is_some(); + + // phase_cargo_miri sets the RUSTDOC env var to ourselves, so we can't use that here; + // just default to a straight-forward invocation for now: + let mut cmd = Command::new(OsString::from("rustdoc")); + + // Because of the way the main function is structured, we have to take the first argument spearately + // from the rest; to simplify the following argument patching loop, we'll just skip that one. + // This is fine for now, because cargo will never pass --extern arguments in the first position, + // but we should defensively assert that this will work. + let extern_flag = "--extern"; + assert!(fst_arg != extern_flag); + cmd.arg(fst_arg); + + let runtool_flag = "--runtool"; + let mut crossmode = fst_arg == runtool_flag; + while let Some(arg) = args.next() { + if arg == extern_flag { + // Patch --extern arguments to use *.rmeta files, since phase_cargo_rustc only creates stub *.rlib files. + forward_patched_extern_arg(&mut args, &mut cmd); + } else if arg == runtool_flag { + // An existing --runtool flag indicates cargo is running in cross-target mode, which we don't support. + // Note that this is only passed when cargo is run with the unstable -Zdoctest-xcompile flag; + // otherwise, we won't be called as rustdoc at all. + crossmode = true; + break; + } else { + cmd.arg(arg); + } + } + + if crossmode { + eprintln!("Cross-interpreting doc-tests is not currently supported by Miri."); + return; + } + + // For each doc-test, rustdoc starts two child processes: first the test is compiled, + // then the produced executable is invoked. We want to reroute both of these to cargo-miri, + // such that the first time we'll enter phase_cargo_rustc, and phase_cargo_runner second. + // + // rustdoc invokes the test-builder by forwarding most of its own arguments, which makes + // it difficult to determine when phase_cargo_rustc should run instead of phase_cargo_rustdoc. + // Furthermore, the test code is passed via stdin, rather than a temporary file, so we need + // to let phase_cargo_rustc know to expect that. We'll use this environment variable as a flag: + cmd.env("MIRI_CALLED_FROM_RUSTDOC", "1"); + + // The `--test-builder` and `--runtool` arguments are unstable rustdoc features, + // which are disabled by default. We first need to enable them explicitly: + cmd.arg("-Z").arg("unstable-options"); + + let cargo_miri_path = std::env::current_exe().expect("current executable path invalid"); + cmd.arg("--test-builder").arg(&cargo_miri_path); // invoked by forwarding most arguments + cmd.arg("--runtool").arg(&cargo_miri_path); // invoked with just a single path argument + + if verbose { + eprintln!("[cargo-miri rustdoc] {:?}", cmd); + } + exec(cmd) } @@ -817,6 +965,30 @@ fn main() { return; } + // The way rustdoc invokes rustc is indistuingishable from the way cargo invokes rustdoc + // by the arguments alone, and we can't take from the args iterator in this case. + // phase_cargo_rustdoc sets this environment variable to let us disambiguate here + let invoked_by_rustdoc = env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some(); + if invoked_by_rustdoc { + // ...however, we then also see this variable when rustdoc invokes us as the testrunner! + // The runner is invoked as `$runtool ($runtool-arg)* output_file`; + // since we don't specify any runtool-args, and rustdoc supplies multiple arguments to + // the test-builder unconditionally, we can just check the number of remaining arguments: + if args.len() == 1 { + let arg = args.next().unwrap(); + let binary = Path::new(&arg); + if binary.exists() { + phase_cargo_runner(binary, args); + } else { + show_error(format!("`cargo-miri` called with non-existing path argument `{}` in rustdoc mode; please invoke this binary through `cargo miri`", arg)); + } + } else { + phase_cargo_rustc(args); + } + + return; + } + // Dispatch to `cargo-miri` phase. There are three phases: // - When we are called via `cargo miri`, we run as the frontend and invoke the underlying // cargo. We set RUSTC_WRAPPER and CARGO_TARGET_RUNNER to ourselves. @@ -829,16 +1001,15 @@ fn main() { Some("miri") => phase_cargo_miri(args), Some("rustc") => phase_cargo_rustc(args), Some(arg) => { - // We have to distinguish the "runner" and "rustfmt" cases. + // We have to distinguish the "runner" and "rustdoc" cases. // As runner, the first argument is the binary (a file that should exist, with an absolute path); - // as rustfmt, the first argument is a flag (`--something`). + // as rustdoc, the first argument is a flag (`--something`). let binary = Path::new(arg); if binary.exists() { assert!(!arg.starts_with("--")); // not a flag phase_cargo_runner(binary, args); } else if arg.starts_with("--") { - // We are rustdoc. - eprintln!("Running doctests is not currently supported by Miri.") + phase_cargo_rustdoc(arg, args); } else { show_error(format!("`cargo-miri` called with unexpected first argument `{}`; please only invoke this binary through `cargo miri`", arg)); } diff --git a/test-cargo-miri/run-test.py b/test-cargo-miri/run-test.py index 81f589705d..c8b85316bb 100755 --- a/test-cargo-miri/run-test.py +++ b/test-cargo-miri/run-test.py @@ -5,7 +5,7 @@ and the working directory to contain the cargo-miri-test project. ''' -import sys, subprocess, os +import sys, subprocess, os, re CGREEN = '\33[32m' CBOLD = '\33[1m' @@ -23,6 +23,10 @@ def cargo_miri(cmd, quiet = True): args += ["--target", os.environ['MIRI_TEST_TARGET']] return args +def normalize_stdout(str): + str = str.replace("src\\", "src/") # normalize paths across platforms + return re.sub("finished in \d+\.\d\ds", "finished in $TIME", str) + def test(name, cmd, stdout_ref, stderr_ref, stdin=b'', env={}): print("Testing {}...".format(name)) ## Call `cargo miri`, capture all output @@ -38,7 +42,7 @@ def test(name, cmd, stdout_ref, stderr_ref, stdin=b'', env={}): (stdout, stderr) = p.communicate(input=stdin) stdout = stdout.decode("UTF-8") stderr = stderr.decode("UTF-8") - if p.returncode == 0 and stdout == open(stdout_ref).read() and stderr == open(stderr_ref).read(): + if p.returncode == 0 and normalize_stdout(stdout) == open(stdout_ref).read() and stderr == open(stderr_ref).read(): # All good! return # Show output @@ -101,26 +105,27 @@ def test_cargo_miri_run(): def test_cargo_miri_test(): # rustdoc is not run on foreign targets is_foreign = 'MIRI_TEST_TARGET' in os.environ - rustdoc_ref = "test.stderr-empty.ref" if is_foreign else "test.stderr-rustdoc.ref" + default_ref = "test.cross-target.stdout.ref" if is_foreign else "test.default.stdout.ref" + filter_ref = "test.filter.cross-target.stdout.ref" if is_foreign else "test.filter.stdout.ref" test("`cargo miri test`", cargo_miri("test"), - "test.default.stdout.ref", rustdoc_ref, + default_ref, "test.stderr-empty.ref", env={'MIRIFLAGS': "-Zmiri-seed=feed"}, ) test("`cargo miri test` (no isolation)", cargo_miri("test"), - "test.default.stdout.ref", rustdoc_ref, + default_ref, "test.stderr-empty.ref", env={'MIRIFLAGS': "-Zmiri-disable-isolation"}, ) test("`cargo miri test` (raw-ptr tracking)", cargo_miri("test"), - "test.default.stdout.ref", rustdoc_ref, + default_ref, "test.stderr-empty.ref", env={'MIRIFLAGS': "-Zmiri-track-raw-pointers"}, ) test("`cargo miri test` (with filter)", cargo_miri("test") + ["--", "--format=pretty", "le1"], - "test.filter.stdout.ref", rustdoc_ref, + filter_ref, "test.stderr-empty.ref", ) test("`cargo miri test` (test target)", cargo_miri("test") + ["--test", "test", "--", "--format=pretty"], diff --git a/test-cargo-miri/test.cross-target.stdout.ref b/test-cargo-miri/test.cross-target.stdout.ref new file mode 100644 index 0000000000..7079798e42 --- /dev/null +++ b/test-cargo-miri/test.cross-target.stdout.ref @@ -0,0 +1,10 @@ + +running 1 test +. +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out + + +running 7 tests +..i.... +test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out + diff --git a/test-cargo-miri/test.default.stdout.ref b/test-cargo-miri/test.default.stdout.ref index 7079798e42..7ed0b2dbae 100644 --- a/test-cargo-miri/test.default.stdout.ref +++ b/test-cargo-miri/test.default.stdout.ref @@ -8,3 +8,9 @@ running 7 tests ..i.... test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out + +running 1 test +test src/lib.rs - make_true (line 2) ... ok + +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME + diff --git a/test-cargo-miri/test.filter.cross-target.stdout.ref b/test-cargo-miri/test.filter.cross-target.stdout.ref new file mode 100644 index 0000000000..37efb8c3ee --- /dev/null +++ b/test-cargo-miri/test.filter.cross-target.stdout.ref @@ -0,0 +1,11 @@ + +running 0 tests + +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out + + +running 1 test +test simple1 ... ok + +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 6 filtered out + diff --git a/test-cargo-miri/test.filter.stdout.ref b/test-cargo-miri/test.filter.stdout.ref index 37efb8c3ee..18174cadd2 100644 --- a/test-cargo-miri/test.filter.stdout.ref +++ b/test-cargo-miri/test.filter.stdout.ref @@ -9,3 +9,8 @@ test simple1 ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 6 filtered out + +running 0 tests + +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in $TIME + diff --git a/test-cargo-miri/test.stderr-rustdoc.ref b/test-cargo-miri/test.stderr-rustdoc.ref deleted file mode 100644 index a310169e30..0000000000 --- a/test-cargo-miri/test.stderr-rustdoc.ref +++ /dev/null @@ -1 +0,0 @@ -Running doctests is not currently supported by Miri. From dd393f21c7f6a6b997d241e82a8dc89ee42cd6e8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Apr 2021 11:46:20 +0200 Subject: [PATCH 2/8] make attempt to cross-interpret a hard error --- cargo-miri/bin.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index 1f749b077a..f4f18929ba 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -923,8 +923,7 @@ fn phase_cargo_rustdoc(fst_arg: &str, mut args: env::Args) { } if crossmode { - eprintln!("Cross-interpreting doc-tests is not currently supported by Miri."); - return; + show_error(format!("cross-interpreting doc-tests is not currently supported by Miri.")); } // For each doc-test, rustdoc starts two child processes: first the test is compiled, From 9083e00b2c27145551a24dbd06d2ca40986b0592 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Apr 2021 11:55:53 +0200 Subject: [PATCH 3/8] resolve semantic conflicts --- cargo-miri/bin.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index f4f18929ba..bb6d4b8789 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -572,7 +572,7 @@ fn phase_cargo_miri(mut args: env::Args) { // us in order to skip them. cmd.env(&host_runner_env_name, &cargo_miri_path); - // Set rustdoc to us as well, so we can make it do nothing (see issue #584). + // Set rustdoc to us as well, so we can run doctests. cmd.env("RUSTDOC", &cargo_miri_path); // Run cargo. @@ -784,18 +784,6 @@ fn phase_cargo_rustc(mut args: env::Args) { } } -fn forward_patched_extern_arg(args: &mut impl Iterator, cmd: &mut Command) { - cmd.arg("--extern"); // always forward flag, but adjust filename: - let path = args.next().expect("`--extern` should be followed by a filename"); - if let Some(lib) = path.strip_suffix(".rlib") { - // If this is an rlib, make it an rmeta. - cmd.arg(format!("{}.rmeta", lib)); - } else { - // Some other extern file (e.g. a `.so`). Forward unchanged. - cmd.arg(path); - } -} - fn phase_cargo_runner(binary: &Path, binary_args: env::Args) { let verbose = std::env::var_os("MIRI_VERBOSE").is_some(); From 65597951b7e51473dc27cdbc5699f8d165197d01 Mon Sep 17 00:00:00 2001 From: hyd-dev Date: Sun, 14 Feb 2021 18:57:03 +0800 Subject: [PATCH 4/8] Fix sysroot for rustdoc --- cargo-miri/bin.rs | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index bb6d4b8789..350efbf182 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -685,7 +685,7 @@ fn phase_cargo_rustc(mut args: env::Args) { .expect("the wrapper should have set MIRI_SYSROOT"); cmd.arg("--sysroot").arg(sysroot); } - + // ensure --emit argument for a check-only build is present if let Some(i) = env.args.iter().position(|arg| arg.starts_with("--emit=")) { // We need to make sure we're not producing a binary that overwrites the JSON file. @@ -702,7 +702,7 @@ fn phase_cargo_rustc(mut args: env::Args) { eprintln!("[cargo-miri rustc] captured input:\n{}", std::str::from_utf8(&env.stdin).unwrap()); eprintln!("[cargo-miri rustc] {:?}", cmd); } - + exec_with_pipe(cmd, &env.stdin); } @@ -841,11 +841,13 @@ fn phase_cargo_runner(binary: &Path, binary_args: env::Args) { cmd.arg(arg); } } - // Set sysroot. - let sysroot = - env::var_os("MIRI_SYSROOT").expect("the wrapper should have set MIRI_SYSROOT"); - cmd.arg("--sysroot"); - cmd.arg(sysroot); + if env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_none() { + // Set sysroot. + let sysroot = + env::var_os("MIRI_SYSROOT").expect("the wrapper should have set MIRI_SYSROOT"); + cmd.arg("--sysroot"); + cmd.arg(sysroot); + } // Respect `MIRIFLAGS`. if let Ok(a) = env::var("MIRIFLAGS") { // This code is taken from `RUSTFLAGS` handling in cargo. @@ -892,7 +894,7 @@ fn phase_cargo_rustdoc(fst_arg: &str, mut args: env::Args) { let extern_flag = "--extern"; assert!(fst_arg != extern_flag); cmd.arg(fst_arg); - + let runtool_flag = "--runtool"; let mut crossmode = fst_arg == runtool_flag; while let Some(arg) = args.next() { @@ -917,21 +919,27 @@ fn phase_cargo_rustdoc(fst_arg: &str, mut args: env::Args) { // For each doc-test, rustdoc starts two child processes: first the test is compiled, // then the produced executable is invoked. We want to reroute both of these to cargo-miri, // such that the first time we'll enter phase_cargo_rustc, and phase_cargo_runner second. - // + // // rustdoc invokes the test-builder by forwarding most of its own arguments, which makes // it difficult to determine when phase_cargo_rustc should run instead of phase_cargo_rustdoc. // Furthermore, the test code is passed via stdin, rather than a temporary file, so we need // to let phase_cargo_rustc know to expect that. We'll use this environment variable as a flag: cmd.env("MIRI_CALLED_FROM_RUSTDOC", "1"); - + // The `--test-builder` and `--runtool` arguments are unstable rustdoc features, // which are disabled by default. We first need to enable them explicitly: cmd.arg("-Z").arg("unstable-options"); - + + // Use our custom sysroot. + let sysroot = + env::var_os("MIRI_SYSROOT").expect("the wrapper should have set MIRI_SYSROOT"); + cmd.arg("--sysroot"); + cmd.arg(sysroot); + let cargo_miri_path = std::env::current_exe().expect("current executable path invalid"); cmd.arg("--test-builder").arg(&cargo_miri_path); // invoked by forwarding most arguments cmd.arg("--runtool").arg(&cargo_miri_path); // invoked with just a single path argument - + if verbose { eprintln!("[cargo-miri rustdoc] {:?}", cmd); } From 29bc8a57b09f3259ae83b844c6c29e01f1ba00c6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Apr 2021 12:16:31 +0200 Subject: [PATCH 5/8] add test for compile_fail; de-duplicate sysroot forwarding --- cargo-miri/bin.rs | 34 ++++++++++--------------- test-cargo-miri/src/lib.rs | 3 +++ test-cargo-miri/test.default.stdout.ref | 5 ++-- 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index 350efbf182..1127cef593 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -163,6 +163,13 @@ fn forward_patched_extern_arg(args: &mut impl Iterator, 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); +} + /// Returns the path to the `miri` binary fn find_miri() -> PathBuf { if let Some(path) = env::var_os("MIRI") { @@ -679,13 +686,6 @@ fn phase_cargo_rustc(mut args: env::Args) { return; }; - // use our own sysroot - if !has_arg_flag("--sysroot") { - let sysroot = env::var_os("MIRI_SYSROOT") - .expect("the wrapper should have set MIRI_SYSROOT"); - cmd.arg("--sysroot").arg(sysroot); - } - // ensure --emit argument for a check-only build is present if let Some(i) = env.args.iter().position(|arg| arg.starts_with("--emit=")) { // We need to make sure we're not producing a binary that overwrites the JSON file. @@ -750,10 +750,7 @@ fn phase_cargo_rustc(mut args: env::Args) { } // Use our custom sysroot. - let sysroot = - env::var_os("MIRI_SYSROOT").expect("the wrapper should have set MIRI_SYSROOT"); - cmd.arg("--sysroot"); - cmd.arg(sysroot); + forward_miri_sysroot(&mut cmd); } else { // For host crates or when we are printing, just forward everything. cmd.args(args); @@ -842,11 +839,8 @@ fn phase_cargo_runner(binary: &Path, binary_args: env::Args) { } } if env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_none() { - // Set sysroot. - let sysroot = - env::var_os("MIRI_SYSROOT").expect("the wrapper should have set MIRI_SYSROOT"); - cmd.arg("--sysroot"); - cmd.arg(sysroot); + // Set sysroot (if we are inside rustdoc, we already did that in `phase_cargo_rustdoc`). + forward_miri_sysroot(&mut cmd); } // Respect `MIRIFLAGS`. if let Ok(a) = env::var("MIRIFLAGS") { @@ -930,12 +924,10 @@ fn phase_cargo_rustdoc(fst_arg: &str, mut args: env::Args) { // which are disabled by default. We first need to enable them explicitly: cmd.arg("-Z").arg("unstable-options"); - // Use our custom sysroot. - let sysroot = - env::var_os("MIRI_SYSROOT").expect("the wrapper should have set MIRI_SYSROOT"); - cmd.arg("--sysroot"); - cmd.arg(sysroot); + // rustdoc needs to know the right sysroot. + forward_miri_sysroot(&mut cmd); + // Make rustdoc call us back. let cargo_miri_path = std::env::current_exe().expect("current executable path invalid"); cmd.arg("--test-builder").arg(&cargo_miri_path); // invoked by forwarding most arguments cmd.arg("--runtool").arg(&cargo_miri_path); // invoked with just a single path argument diff --git a/test-cargo-miri/src/lib.rs b/test-cargo-miri/src/lib.rs index 33bbd8c966..45973522f7 100644 --- a/test-cargo-miri/src/lib.rs +++ b/test-cargo-miri/src/lib.rs @@ -2,6 +2,9 @@ /// ```rust /// assert!(cargo_miri_test::make_true()); /// ``` +/// ```rust,compile_fail +/// assert!(cargo_miri_test::make_true() == 5); +/// ``` pub fn make_true() -> bool { issue_1567::use_the_dependency(); issue_1705::use_the_dependency(); diff --git a/test-cargo-miri/test.default.stdout.ref b/test-cargo-miri/test.default.stdout.ref index 7ed0b2dbae..893350ae6e 100644 --- a/test-cargo-miri/test.default.stdout.ref +++ b/test-cargo-miri/test.default.stdout.ref @@ -9,8 +9,9 @@ running 7 tests test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out -running 1 test +running 2 tests +test src/lib.rs - make_true (line 5) ... ok test src/lib.rs - make_true (line 2) ... ok -test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME +test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME From e66a89c8afe8da0c784943502343c8620c156366 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Apr 2021 12:26:27 +0200 Subject: [PATCH 6/8] avoid some dead code and test no_run tests --- cargo-miri/bin.rs | 50 ++++++++++++------------- test-cargo-miri/src/lib.rs | 3 ++ test-cargo-miri/test.default.stdout.ref | 5 ++- test-cargo-miri/test.filter.stdout.ref | 2 +- 4 files changed, 31 insertions(+), 29 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index 1127cef593..5d2759deea 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -38,7 +38,7 @@ enum MiriCommand { } /// The information to run a crate with the given environment. -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Clone)] struct CrateRunEnv { /// The command-line arguments. args: Vec, @@ -50,16 +50,7 @@ struct CrateRunEnv { stdin: Vec, } -/// The information Miri needs to run a crate. Stored as JSON when the crate is "compiled". -#[derive(Serialize, Deserialize)] -enum CrateRunInfo { - /// Run it with the given environment. - RunWith(CrateRunEnv), - /// Skip it as Miri does not support interpreting such kind of crates. - SkipProcMacroTest, -} - -impl CrateRunInfo { +impl CrateRunEnv { /// Gather all the information we need. fn collect(args: env::Args) -> Self { let args = args.collect(); @@ -71,9 +62,20 @@ impl CrateRunInfo { std::io::stdin().lock().read_to_end(&mut stdin).expect("cannot read stdin"); } - Self::RunWith(CrateRunEnv { args, env, current_dir, stdin }) + CrateRunEnv { args, env, current_dir, stdin } } +} +/// The information Miri needs to run a crate. Stored as JSON when the crate is "compiled". +#[derive(Serialize, Deserialize)] +enum CrateRunInfo { + /// Run it with the given environment. + RunWith(CrateRunEnv), + /// Skip it as Miri does not support interpreting such kind of crates. + SkipProcMacroTest, +} + +impl CrateRunInfo { fn store(&self, filename: &Path) { let file = File::create(filename) .unwrap_or_else(|_| show_error(format!("cannot create `{}`", filename.display()))); @@ -644,7 +646,7 @@ fn phase_cargo_rustc(mut args: env::Args) { let target_crate = is_target_crate(); let print = get_arg_flag_value("--print").is_some(); // whether this is cargo passing `--print` to get some infos - let store_json = |info: &CrateRunInfo| { + let store_json = |info: CrateRunInfo| { // Create a stub .d file to stop Cargo from "rebuilding" the crate: // https://github.com/rust-lang/miri/issues/1724#issuecomment-787115693 // As we store a JSON file instead of building the crate here, an empty file is fine. @@ -672,30 +674,24 @@ fn phase_cargo_rustc(mut args: env::Args) { // like we want them. // Instead of compiling, we write JSON into the output file with all the relevant command-line flags // and environment variables; this is used when cargo calls us again in the CARGO_TARGET_RUNNER phase. - let info = CrateRunInfo::collect(args); - store_json(&info); + let env = CrateRunEnv::collect(args); // Rustdoc expects us to exit with an error code if the test is marked as `compile_fail`, // just creating the JSON file is not enough: we need to detect syntax errors, // so we need to run Miri with `MIRI_BE_RUSTC` for a check-only build. if std::env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() { let mut cmd = miri(); - let env = if let CrateRunInfo::RunWith(env) = info { - env - } else { - return; - }; - // ensure --emit argument for a check-only build is present + // Ensure --emit argument for a check-only build is present. if let Some(i) = env.args.iter().position(|arg| arg.starts_with("--emit=")) { - // We need to make sure we're not producing a binary that overwrites the JSON file. - // rustdoc should only ever pass an --emit=metadata argument for tests marked as `no_run`: + // For `no_run` tests, rustdoc passes a `--emit` flag; make sure it has the right shape. assert_eq!(env.args[i], "--emit=metadata"); } else { - cmd.arg("--emit=dep-info,metadata"); + // For all other kinds of tests, we can just add our flag. + cmd.arg("--emit=metadata"); } - cmd.args(env.args); + cmd.args(&env.args); cmd.env("MIRI_BE_RUSTC", "1"); if verbose { @@ -706,6 +702,8 @@ fn phase_cargo_rustc(mut args: env::Args) { exec_with_pipe(cmd, &env.stdin); } + store_json(CrateRunInfo::RunWith(env)); + return; } @@ -713,7 +711,7 @@ fn phase_cargo_rustc(mut args: env::Args) { // This is a "runnable" `proc-macro` crate (unit tests). We do not support // interpreting that under Miri now, so we write a JSON file to (display a // helpful message and) skip it in the runner phase. - store_json(&CrateRunInfo::SkipProcMacroTest); + store_json(CrateRunInfo::SkipProcMacroTest); return; } diff --git a/test-cargo-miri/src/lib.rs b/test-cargo-miri/src/lib.rs index 45973522f7..0c268a18f6 100644 --- a/test-cargo-miri/src/lib.rs +++ b/test-cargo-miri/src/lib.rs @@ -2,6 +2,9 @@ /// ```rust /// assert!(cargo_miri_test::make_true()); /// ``` +/// ```rust,no_run +/// assert!(cargo_miri_test::make_true()); +/// ``` /// ```rust,compile_fail /// assert!(cargo_miri_test::make_true() == 5); /// ``` diff --git a/test-cargo-miri/test.default.stdout.ref b/test-cargo-miri/test.default.stdout.ref index 893350ae6e..72ae7e94a1 100644 --- a/test-cargo-miri/test.default.stdout.ref +++ b/test-cargo-miri/test.default.stdout.ref @@ -9,9 +9,10 @@ running 7 tests test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out -running 2 tests +running 3 tests test src/lib.rs - make_true (line 5) ... ok +test src/lib.rs - make_true (line 8) ... ok test src/lib.rs - make_true (line 2) ... ok -test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME +test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME diff --git a/test-cargo-miri/test.filter.stdout.ref b/test-cargo-miri/test.filter.stdout.ref index 18174cadd2..11e47e8ff8 100644 --- a/test-cargo-miri/test.filter.stdout.ref +++ b/test-cargo-miri/test.filter.stdout.ref @@ -12,5 +12,5 @@ test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 6 filtered out running 0 tests -test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in $TIME +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 3 filtered out; finished in $TIME From f9bd6b0756c12e9f33223bd0a8460cd1129160a6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Apr 2021 12:46:36 +0200 Subject: [PATCH 7/8] nits; test running no doctests --- cargo-miri/bin.rs | 9 +++++---- test-cargo-miri/run-test.py | 6 +++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index 5d2759deea..b9c46ff41c 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -683,6 +683,7 @@ fn phase_cargo_rustc(mut args: env::Args) { let mut cmd = miri(); // Ensure --emit argument for a check-only build is present. + // We cannot use the usual helpers since we need to check specifically in `env.args`. if let Some(i) = env.args.iter().position(|arg| arg.starts_with("--emit=")) { // For `no_run` tests, rustdoc passes a `--emit` flag; make sure it has the right shape. assert_eq!(env.args[i], "--emit=metadata"); @@ -877,7 +878,7 @@ fn phase_cargo_rustdoc(fst_arg: &str, mut args: env::Args) { // phase_cargo_miri sets the RUSTDOC env var to ourselves, so we can't use that here; // just default to a straight-forward invocation for now: - let mut cmd = Command::new(OsString::from("rustdoc")); + let mut cmd = Command::new("rustdoc"); // Because of the way the main function is structured, we have to take the first argument spearately // from the rest; to simplify the following argument patching loop, we'll just skip that one. @@ -888,6 +889,7 @@ fn phase_cargo_rustdoc(fst_arg: &str, mut args: env::Args) { cmd.arg(fst_arg); let runtool_flag = "--runtool"; + // `crossmode` records if *any* argument matches `runtool_flag`; here we check the first one. let mut crossmode = fst_arg == runtool_flag; while let Some(arg) = args.next() { if arg == extern_flag { @@ -950,9 +952,8 @@ fn main() { return; } - // The way rustdoc invokes rustc is indistuingishable from the way cargo invokes rustdoc - // by the arguments alone, and we can't take from the args iterator in this case. - // phase_cargo_rustdoc sets this environment variable to let us disambiguate here + // The way rustdoc invokes rustc is indistuingishable from the way cargo invokes rustdoc by the + // arguments alone. `phase_cargo_rustdoc` sets this environment variable to let us disambiguate. let invoked_by_rustdoc = env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some(); if invoked_by_rustdoc { // ...however, we then also see this variable when rustdoc invokes us as the testrunner! diff --git a/test-cargo-miri/run-test.py b/test-cargo-miri/run-test.py index c8b85316bb..84c4a129ad 100755 --- a/test-cargo-miri/run-test.py +++ b/test-cargo-miri/run-test.py @@ -113,9 +113,9 @@ def test_cargo_miri_test(): default_ref, "test.stderr-empty.ref", env={'MIRIFLAGS': "-Zmiri-seed=feed"}, ) - test("`cargo miri test` (no isolation)", - cargo_miri("test"), - default_ref, "test.stderr-empty.ref", + test("`cargo miri test` (no isolation, no doctests)", + cargo_miri("test") + ["--bins", "--tests"], # no `--lib`, we disabled that in `Cargo.toml` + "test.cross-target.stdout.ref", "test.stderr-empty.ref", env={'MIRIFLAGS': "-Zmiri-disable-isolation"}, ) test("`cargo miri test` (raw-ptr tracking)", From 2f6dff6da81e4bf351f82c648b79fbfddd175b3e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Apr 2021 13:18:59 +0200 Subject: [PATCH 8/8] nits and fix non-deterministic test output --- cargo-miri/bin.rs | 11 ++++++----- test-cargo-miri/run-test.py | 1 + test-cargo-miri/test.default.stdout.ref | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index b9c46ff41c..b70d95c604 100644 --- a/cargo-miri/bin.rs +++ b/cargo-miri/bin.rs @@ -38,7 +38,7 @@ enum MiriCommand { } /// The information to run a crate with the given environment. -#[derive(Serialize, Deserialize, Clone)] +#[derive(Serialize, Deserialize)] struct CrateRunEnv { /// The command-line arguments. args: Vec, @@ -52,13 +52,13 @@ struct CrateRunEnv { impl CrateRunEnv { /// Gather all the information we need. - fn collect(args: env::Args) -> Self { + fn collect(args: env::Args, capture_stdin: bool) -> Self { let args = args.collect(); let env = env::vars_os().collect(); let current_dir = env::current_dir().unwrap().into_os_string(); let mut stdin = Vec::new(); - if env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() { + if capture_stdin { std::io::stdin().lock().read_to_end(&mut stdin).expect("cannot read stdin"); } @@ -669,17 +669,18 @@ fn phase_cargo_rustc(mut args: env::Args) { let runnable_crate = !print && is_runnable_crate(); if runnable_crate && target_crate { + let inside_rustdoc = env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some(); // This is the binary or test crate that we want to interpret under Miri. // But we cannot run it here, as cargo invoked us as a compiler -- our stdin and stdout are not // like we want them. // Instead of compiling, we write JSON into the output file with all the relevant command-line flags // and environment variables; this is used when cargo calls us again in the CARGO_TARGET_RUNNER phase. - let env = CrateRunEnv::collect(args); + let env = CrateRunEnv::collect(args, inside_rustdoc); // Rustdoc expects us to exit with an error code if the test is marked as `compile_fail`, // just creating the JSON file is not enough: we need to detect syntax errors, // so we need to run Miri with `MIRI_BE_RUSTC` for a check-only build. - if std::env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() { + if inside_rustdoc { let mut cmd = miri(); // Ensure --emit argument for a check-only build is present. diff --git a/test-cargo-miri/run-test.py b/test-cargo-miri/run-test.py index 84c4a129ad..9185c2507b 100755 --- a/test-cargo-miri/run-test.py +++ b/test-cargo-miri/run-test.py @@ -143,6 +143,7 @@ def test_cargo_miri_test(): os.chdir(os.path.dirname(os.path.realpath(__file__))) os.environ["RUST_TEST_NOCAPTURE"] = "0" # this affects test output, so make sure it is not set +os.environ["RUST_TEST_THREADS"] = "1" # avoid non-deterministic output due to concurrent test runs target_str = " for target {}".format(os.environ['MIRI_TEST_TARGET']) if 'MIRI_TEST_TARGET' in os.environ else "" print(CGREEN + CBOLD + "## Running `cargo miri` tests{}".format(target_str) + CEND) diff --git a/test-cargo-miri/test.default.stdout.ref b/test-cargo-miri/test.default.stdout.ref index 72ae7e94a1..6e35c374e1 100644 --- a/test-cargo-miri/test.default.stdout.ref +++ b/test-cargo-miri/test.default.stdout.ref @@ -10,9 +10,9 @@ test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out running 3 tests +test src/lib.rs - make_true (line 2) ... ok test src/lib.rs - make_true (line 5) ... ok test src/lib.rs - make_true (line 8) ... ok -test src/lib.rs - make_true (line 2) ... ok test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME