Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make cargo-miri run documentation tests #1671

Closed
wants to merge 11 commits into from
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,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 validation.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
196 changes: 169 additions & 27 deletions cargo-miri/bin.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::env;
use std::ffi::OsString;
use std::fs::{self, File};
use std::io::{self, BufRead, BufReader, BufWriter, Write};
use std::io::{self, BufRead, BufReader, BufWriter, Read, Write};
use std::ops::Not;
use std::path::{Path, PathBuf};
use std::process::Command;
Expand Down Expand Up @@ -45,6 +45,8 @@ struct CrateRunInfo {
env: Vec<(OsString, OsString)>,
/// The current working directory.
current_dir: OsString,
/// The contents passed via standard input.
stdin: Vec<u8>,
}

impl CrateRunInfo {
Expand All @@ -53,7 +55,13 @@ impl CrateRunInfo {
let args = args.collect();
let env = env::vars_os().collect();
let current_dir = env::current_dir().unwrap().into_os_string();
CrateRunInfo { args, env, current_dir }

let mut stdin = Vec::new();
if env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() {
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
std::io::stdin().lock().read_to_end(&mut stdin).expect("cannot read stdin");
}

CrateRunInfo { args, env, current_dir, stdin }
}

fn store(&self, filename: &Path) {
Expand Down Expand Up @@ -149,6 +157,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() {
Expand Down Expand Up @@ -539,17 +563,22 @@ fn phase_cargo_rustc(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();
Expand Down Expand Up @@ -585,6 +614,39 @@ fn phase_cargo_rustc(args: env::Args) {
// (Need to do this here as cargo moves that "binary" to a different place before running it.)
info.store(&out_filename("", ".exe"));

// 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.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
if std::env::var_os("MIRI_CALLED_FROM_RUSTDOC").is_some() {
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
let mut cmd = miri();

// 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);
}
RalfJung marked this conversation as resolved.
Show resolved Hide resolved

// ensure --emit argument for a check-only build is present
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
if let Some(i) = info.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!(info.args[i], "--emit=metadata");
} else {
cmd.arg("--emit=dep-info,metadata");
}

cmd.args(info.args);
cmd.env("MIRI_BE_RUSTC", "1");

if verbose {
eprintln!("[cargo-miri rustc] captured input:\n{}", std::str::from_utf8(&info.stdin).unwrap());
eprintln!("[cargo-miri rustc] {:?}", cmd);
}

exec_with_pipe(cmd, &info.stdin);
}

return;
}

Expand Down Expand Up @@ -646,6 +708,18 @@ fn phase_cargo_rustc(args: env::Args) {
}
}

fn try_forward_patched_extern_arg(args: &mut impl Iterator<Item = String>, cmd: &mut Command) {
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
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();

Expand Down Expand Up @@ -683,16 +757,7 @@ fn phase_cargo_runner(binary: &Path, binary_args: env::Args) {
let json_flag = "--json";
while let Some(arg) = args.next() {
if arg == extern_flag {
cmd.arg(extern_flag); // always forward flag, but adjust filename
// `--extern` is always passed as a separate argument by cargo.
let next_arg = args.next().expect("`--extern` should be followed by a filename");
if let Some(next_lib) = next_arg.strip_suffix(".rlib") {
// If this is an rlib, make it an rmeta.
cmd.arg(format!("{}.rmeta", next_lib));
} else {
// Some other extern file (e.g., a `.so`). Forward unchanged.
cmd.arg(next_arg);
}
try_forward_patched_extern_arg(&mut args, &mut cmd);
} else if arg.starts_with(error_format_flag) {
let suffix = &arg[error_format_flag.len()..];
assert!(suffix.starts_with('='));
Expand Down Expand Up @@ -734,6 +799,60 @@ 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"));
RalfJung marked this conversation as resolved.
Show resolved Hide resolved

// 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 an --extern argument in the first position,
// but we should defensively assert that this will work.
let extern_flag = "--extern";
assert!(fst_arg != extern_flag);
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
cmd.arg(fst_arg);

// Patch --extern arguments to use *.rmeta files, since phase_cargo_rustc only creates stub *.rlib files.
while let Some(arg) = args.next() {
if arg == extern_flag {
try_forward_patched_extern_arg(&mut args, &mut cmd);
} else {
cmd.arg(arg);
}
}

// 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");
RalfJung marked this conversation as resolved.
Show resolved Hide resolved

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)
}

Expand All @@ -750,6 +869,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.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
// 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.
Expand All @@ -762,16 +905,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));
}
Expand Down
25 changes: 13 additions & 12 deletions test-cargo-miri/run-test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -21,6 +21,9 @@ def cargo_miri(cmd):
args += ["--target", os.environ['MIRI_TEST_TARGET']]
return args

def normalize_stdout(str):
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
Expand All @@ -36,7 +39,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
Expand Down Expand Up @@ -69,40 +72,38 @@ 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"
empty_ref = "test.stderr-empty.ref"

test("`cargo miri test`",
cargo_miri("test"),
"test.default.stdout.ref", rustdoc_ref,
"test.default.stdout.ref", empty_ref,
env={'MIRIFLAGS': "-Zmiri-seed=feed"},
)
test("`cargo miri test` (no isolation)",
cargo_miri("test"),
"test.default.stdout.ref", rustdoc_ref,
"test.default.stdout.ref", empty_ref,
env={'MIRIFLAGS': "-Zmiri-disable-isolation"},
)
test("`cargo miri test` (raw-ptr tracking)",
cargo_miri("test"),
"test.default.stdout.ref", rustdoc_ref,
"test.default.stdout.ref", 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,
"test.filter.stdout.ref", empty_ref,
)
test("`cargo miri test` (test target)",
cargo_miri("test") + ["--test", "test", "--", "--format=pretty"],
"test.test-target.stdout.ref", "test.stderr-empty.ref",
"test.test-target.stdout.ref", empty_ref,
)
test("`cargo miri test` (bin target)",
cargo_miri("test") + ["--bin", "cargo-miri-test", "--", "--format=pretty"],
"test.bin-target.stdout.ref", "test.stderr-empty.ref",
"test.bin-target.stdout.ref", empty_ref,
)
test("`cargo miri test` (subcrate, no isolation)",
cargo_miri("test") + ["-p", "subcrate"],
"test.subcrate.stdout.ref", "test.stderr-empty.ref",
"test.subcrate.stdout.ref", empty_ref,
env={'MIRIFLAGS': "-Zmiri-disable-isolation"},
)

Expand Down
6 changes: 6 additions & 0 deletions test-cargo-miri/test.default.stdout.ref
Original file line number Diff line number Diff line change
Expand Up @@ -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

5 changes: 5 additions & 0 deletions test-cargo-miri/test.filter.stdout.ref
Original file line number Diff line number Diff line change
Expand Up @@ -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

1 change: 0 additions & 1 deletion test-cargo-miri/test.stderr-rustdoc.ref

This file was deleted.