Skip to content

Commit 92ad0b1

Browse files
matthiaskrgrmatthiaskrgr
authored
Rollup merge of #126512 - RalfJung:miri-sync, r=RalfJung
Miri subtree update r? `@ghost`
2 parents 83cbcea + 3f2c50c commit 92ad0b1

22 files changed

+1522
-152
lines changed

‎src/tools/miri/README.md

+15-17
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,21 @@ platform. For example `cargo miri test --target s390x-unknown-linux-gnu`
151151
will run your test suite on a big-endian target, which is useful for testing
152152
endian-sensitive code.
153153

154+
### Testing multiple different executions
155+
156+
Certain parts of the execution are picked randomly by Miri, such as the exact base address
157+
allocations are stored at and the interleaving of concurrently executing threads. Sometimes, it can
158+
be useful to explore multiple different execution, e.g. to make sure that your code does not depend
159+
on incidental "super-alignment" of new allocations and to test different thread interleavings.
160+
This can be done with the `--many-seeds` flag:
161+
162+
```
163+
cargo miri test --many-seeds # tries the seeds in 0..64
164+
cargo miri test --many-seeds=0..16
165+
```
166+
167+
The default of 64 different seeds is quite slow, so you probably want to specify a smaller range.
168+
154169
### Running Miri on CI
155170

156171
When running Miri on CI, use the following snippet to install a nightly toolchain with the Miri
@@ -183,23 +198,6 @@ Here is an example job for GitHub Actions:
183198
The explicit `cargo miri setup` helps to keep the output of the actual test step
184199
clean.
185200

186-
### Testing for alignment issues
187-
188-
Miri can sometimes miss misaligned accesses since allocations can "happen to be"
189-
aligned just right. You can use `-Zmiri-symbolic-alignment-check` to definitely
190-
catch all such issues, but that flag will also cause false positives when code
191-
does manual pointer arithmetic to account for alignment. Another alternative is
192-
to call Miri with various values for `-Zmiri-seed`; that will alter the
193-
randomness that is used to determine allocation base addresses. The following
194-
snippet calls Miri in a loop with different values for the seed:
195-
196-
```
197-
for SEED in $(seq 0 255); do
198-
echo "Trying seed: $SEED"
199-
MIRIFLAGS=-Zmiri-seed=$SEED cargo miri test || { echo "Failing seed: $SEED"; break; };
200-
done
201-
```
202-
203201
### Supported targets
204202

205203
Miri does not support all targets supported by Rust. The good news, however, is

‎src/tools/miri/cargo-miri/src/phases.rs

+123-79
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
//! Implements the various phases of `cargo miri run/test`.
22
3-
use std::env;
43
use std::fs::{self, File};
5-
use std::io::BufReader;
4+
use std::io::{BufReader, Write};
65
use std::path::{Path, PathBuf};
76
use std::process::Command;
7+
use std::{env, thread};
88

99
use rustc_version::VersionMeta;
1010

@@ -34,6 +34,8 @@ Examples:
3434
3535
";
3636

37+
const DEFAULT_MANY_SEEDS: &str = "0..64";
38+
3739
fn show_help() {
3840
println!("{CARGO_MIRI_HELP}");
3941
}
@@ -119,7 +121,7 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
119121
// <https://github.com/rust-lang/miri/pull/1540#issuecomment-693553191> describes an alternative
120122
// approach that uses `cargo check`, making that part easier but target and binary handling
121123
// harder.
122-
let cargo_miri_path = std::env::current_exe()
124+
let cargo_miri_path = env::current_exe()
123125
.expect("current executable path invalid")
124126
.into_os_string()
125127
.into_string()
@@ -163,14 +165,22 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
163165
let target_dir = get_target_dir(&metadata);
164166
cmd.arg("--target-dir").arg(target_dir);
165167

168+
// Store many-seeds argument.
169+
let mut many_seeds = None;
166170
// *After* we set all the flags that need setting, forward everything else. Make sure to skip
167-
// `--target-dir` (which would otherwise be set twice).
171+
// `--target-dir` (which would otherwise be set twice) and `--many-seeds` (which is our flag, not cargo's).
168172
for arg in
169173
ArgSplitFlagValue::from_string_iter(&mut args, "--target-dir").filter_map(Result::err)
170174
{
171-
cmd.arg(arg);
175+
if arg == "--many-seeds" {
176+
many_seeds = Some(DEFAULT_MANY_SEEDS.to_owned());
177+
} else if let Some(val) = arg.strip_prefix("--many-seeds=") {
178+
many_seeds = Some(val.to_owned());
179+
} else {
180+
cmd.arg(arg);
181+
}
172182
}
173-
// Forward all further arguments (not consumed by `ArgSplitFlagValue`) to cargo.
183+
// Forward all further arguments after `--` (not consumed by `ArgSplitFlagValue`) to cargo.
174184
cmd.args(args);
175185

176186
// Set `RUSTC_WRAPPER` to ourselves. Cargo will prepend that binary to its usual invocation,
@@ -222,6 +232,9 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
222232
// Forward some crucial information to our own re-invocations.
223233
cmd.env("MIRI_SYSROOT", miri_sysroot);
224234
cmd.env("MIRI_LOCAL_CRATES", local_crates(&metadata));
235+
if let Some(many_seeds) = many_seeds {
236+
cmd.env("MIRI_MANY_SEEDS", many_seeds);
237+
}
225238
if verbose > 0 {
226239
cmd.env("MIRI_VERBOSE", verbose.to_string()); // This makes the other phases verbose.
227240
}
@@ -309,7 +322,7 @@ pub fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
309322
}
310323
}
311324

312-
let verbose = std::env::var("MIRI_VERBOSE")
325+
let verbose = env::var("MIRI_VERBOSE")
313326
.map_or(0, |verbose| verbose.parse().expect("verbosity flag must be an integer"));
314327
let target_crate = is_target_crate();
315328

@@ -489,7 +502,7 @@ pub fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
489502
// This is a host crate.
490503
// When we're running `cargo-miri` from `x.py` we need to pass the sysroot explicitly
491504
// due to bootstrap complications.
492-
if let Some(sysroot) = std::env::var_os("MIRI_HOST_SYSROOT") {
505+
if let Some(sysroot) = env::var_os("MIRI_HOST_SYSROOT") {
493506
cmd.arg("--sysroot").arg(sysroot);
494507
}
495508

@@ -532,7 +545,7 @@ pub enum RunnerPhase {
532545
}
533546

534547
pub fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: RunnerPhase) {
535-
let verbose = std::env::var("MIRI_VERBOSE")
548+
let verbose = env::var("MIRI_VERBOSE")
536549
.map_or(0, |verbose| verbose.parse().expect("verbosity flag must be an integer"));
537550

538551
let binary = binary_args.next().unwrap();
@@ -541,6 +554,7 @@ pub fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: Runner
541554
"file {:?} not found or `cargo-miri` invoked incorrectly; please only invoke this binary through `cargo miri`", binary
542555
));
543556
let file = BufReader::new(file);
557+
let binary_args = binary_args.collect::<Vec<_>>();
544558

545559
let info = serde_json::from_reader(file).unwrap_or_else(|_| {
546560
show_error!("file {:?} contains outdated or invalid JSON; try `cargo clean`", binary)
@@ -555,84 +569,114 @@ pub fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: Runner
555569
}
556570
};
557571

558-
let mut cmd = miri();
559-
560-
// Set missing env vars. We prefer build-time env vars over run-time ones; see
561-
// <https://github.com/rust-lang/miri/issues/1661> for the kind of issue that fixes.
562-
for (name, val) in info.env {
563-
// `CARGO_MAKEFLAGS` contains information about how to reach the jobserver, but by the time
564-
// the program is being run, that jobserver no longer exists (cargo only runs the jobserver
565-
// for the build portion of `cargo run`/`cargo test`). Hence we shouldn't forward this.
566-
// Also see <https://github.com/rust-lang/rust/pull/113730>.
567-
if name == "CARGO_MAKEFLAGS" {
568-
continue;
569-
}
570-
if let Some(old_val) = env::var_os(&name) {
571-
if old_val == val {
572-
// This one did not actually change, no need to re-set it.
573-
// (This keeps the `debug_cmd` below more manageable.)
572+
let many_seeds = env::var("MIRI_MANY_SEEDS");
573+
run_many_seeds(many_seeds.ok(), |seed| {
574+
let mut cmd = miri();
575+
576+
// Set missing env vars. We prefer build-time env vars over run-time ones; see
577+
// <https://github.com/rust-lang/miri/issues/1661> for the kind of issue that fixes.
578+
for (name, val) in &info.env {
579+
// `CARGO_MAKEFLAGS` contains information about how to reach the jobserver, but by the time
580+
// the program is being run, that jobserver no longer exists (cargo only runs the jobserver
581+
// for the build portion of `cargo run`/`cargo test`). Hence we shouldn't forward this.
582+
// Also see <https://github.com/rust-lang/rust/pull/113730>.
583+
if name == "CARGO_MAKEFLAGS" {
574584
continue;
575-
} else if verbose > 0 {
576-
eprintln!(
577-
"[cargo-miri runner] Overwriting run-time env var {name:?}={old_val:?} with build-time value {val:?}"
578-
);
579585
}
586+
if let Some(old_val) = env::var_os(name) {
587+
if *old_val == *val {
588+
// This one did not actually change, no need to re-set it.
589+
// (This keeps the `debug_cmd` below more manageable.)
590+
continue;
591+
} else if verbose > 0 {
592+
eprintln!(
593+
"[cargo-miri runner] Overwriting run-time env var {name:?}={old_val:?} with build-time value {val:?}"
594+
);
595+
}
596+
}
597+
cmd.env(name, val);
580598
}
581-
cmd.env(name, val);
582-
}
583599

584-
if phase != RunnerPhase::Rustdoc {
585-
// Set the sysroot. Not necessary in rustdoc, where we already set the sysroot in
586-
// `phase_rustdoc`. rustdoc will forward that flag when invoking rustc (i.e., us), so the
587-
// flag is present in `info.args`.
588-
cmd.arg("--sysroot").arg(env::var_os("MIRI_SYSROOT").unwrap());
589-
}
590-
// Forward rustc arguments.
591-
// We need to patch "--extern" filenames because we forced a check-only
592-
// build without cargo knowing about that: replace `.rlib` suffix by
593-
// `.rmeta`.
594-
// We also need to remove `--error-format` as cargo specifies that to be JSON,
595-
// but when we run here, cargo does not interpret the JSON any more. `--json`
596-
// then also needs to be dropped.
597-
let mut args = info.args.into_iter();
598-
while let Some(arg) = args.next() {
599-
if arg == "--extern" {
600-
forward_patched_extern_arg(&mut args, &mut cmd);
601-
} else if let Some(suffix) = arg.strip_prefix("--error-format") {
602-
assert!(suffix.starts_with('='));
603-
// Drop this argument.
604-
} else if let Some(suffix) = arg.strip_prefix("--json") {
605-
assert!(suffix.starts_with('='));
606-
// Drop this argument.
607-
} else {
608-
cmd.arg(arg);
600+
if phase != RunnerPhase::Rustdoc {
601+
// Set the sysroot. Not necessary in rustdoc, where we already set the sysroot in
602+
// `phase_rustdoc`. rustdoc will forward that flag when invoking rustc (i.e., us), so the
603+
// flag is present in `info.args`.
604+
cmd.arg("--sysroot").arg(env::var_os("MIRI_SYSROOT").unwrap());
605+
}
606+
// Forward rustc arguments.
607+
// We need to patch "--extern" filenames because we forced a check-only
608+
// build without cargo knowing about that: replace `.rlib` suffix by
609+
// `.rmeta`.
610+
// We also need to remove `--error-format` as cargo specifies that to be JSON,
611+
// but when we run here, cargo does not interpret the JSON any more. `--json`
612+
// then also needs to be dropped.
613+
let mut args = info.args.iter();
614+
while let Some(arg) = args.next() {
615+
if arg == "--extern" {
616+
forward_patched_extern_arg(&mut (&mut args).cloned(), &mut cmd);
617+
} else if let Some(suffix) = arg.strip_prefix("--error-format") {
618+
assert!(suffix.starts_with('='));
619+
// Drop this argument.
620+
} else if let Some(suffix) = arg.strip_prefix("--json") {
621+
assert!(suffix.starts_with('='));
622+
// Drop this argument.
623+
} else {
624+
cmd.arg(arg);
625+
}
626+
}
627+
// Respect `MIRIFLAGS`.
628+
if let Ok(a) = env::var("MIRIFLAGS") {
629+
let args = flagsplit(&a);
630+
cmd.args(args);
631+
}
632+
// Set the current seed.
633+
if let Some(seed) = seed {
634+
eprintln!("Trying seed: {seed}");
635+
cmd.arg(format!("-Zmiri-seed={seed}"));
609636
}
610-
}
611-
// Respect `MIRIFLAGS`.
612-
if let Ok(a) = env::var("MIRIFLAGS") {
613-
let args = flagsplit(&a);
614-
cmd.args(args);
615-
}
616-
617-
// Then pass binary arguments.
618-
cmd.arg("--");
619-
cmd.args(binary_args);
620-
621-
// Make sure we use the build-time working directory for interpreting Miri/rustc arguments.
622-
// But then we need to switch to the run-time one, which we instruct Miri to do by setting `MIRI_CWD`.
623-
cmd.current_dir(info.current_dir);
624-
cmd.env("MIRI_CWD", env::current_dir().unwrap());
625637

626-
// Run it.
627-
debug_cmd("[cargo-miri runner]", verbose, &cmd);
628-
match phase {
629-
RunnerPhase::Rustdoc => exec_with_pipe(cmd, &info.stdin, format!("{binary}.stdin")),
630-
RunnerPhase::Cargo => exec(cmd),
631-
}
638+
// Then pass binary arguments.
639+
cmd.arg("--");
640+
cmd.args(&binary_args);
641+
642+
// Make sure we use the build-time working directory for interpreting Miri/rustc arguments.
643+
// But then we need to switch to the run-time one, which we instruct Miri to do by setting `MIRI_CWD`.
644+
cmd.current_dir(&info.current_dir);
645+
cmd.env("MIRI_CWD", env::current_dir().unwrap());
646+
647+
// Run it.
648+
debug_cmd("[cargo-miri runner]", verbose, &cmd);
649+
650+
match phase {
651+
RunnerPhase::Rustdoc => {
652+
cmd.stdin(std::process::Stdio::piped());
653+
let mut child = cmd.spawn().expect("failed to spawn process");
654+
let child_stdin = child.stdin.take().unwrap();
655+
// Write stdin in a background thread, as it may block.
656+
let exit_status = thread::scope(|s| {
657+
s.spawn(|| {
658+
let mut child_stdin = child_stdin;
659+
// Ignore failure, it is most likely due to the process having terminated.
660+
let _ = child_stdin.write_all(&info.stdin);
661+
});
662+
child.wait().expect("failed to run command")
663+
});
664+
if !exit_status.success() {
665+
std::process::exit(exit_status.code().unwrap_or(-1));
666+
}
667+
}
668+
RunnerPhase::Cargo => {
669+
let exit_status = cmd.status().expect("failed to run command");
670+
if !exit_status.success() {
671+
std::process::exit(exit_status.code().unwrap_or(-1));
672+
}
673+
}
674+
}
675+
});
632676
}
633677

634678
pub fn phase_rustdoc(mut args: impl Iterator<Item = String>) {
635-
let verbose = std::env::var("MIRI_VERBOSE")
679+
let verbose = env::var("MIRI_VERBOSE")
636680
.map_or(0, |verbose| verbose.parse().expect("verbosity flag must be an integer"));
637681

638682
// phase_cargo_miri sets the RUSTDOC env var to ourselves, and puts a backup
@@ -681,7 +725,7 @@ pub fn phase_rustdoc(mut args: impl Iterator<Item = String>) {
681725
cmd.arg("--cfg").arg("miri");
682726

683727
// Make rustdoc call us back.
684-
let cargo_miri_path = std::env::current_exe().expect("current executable path invalid");
728+
let cargo_miri_path = env::current_exe().expect("current executable path invalid");
685729
cmd.arg("--test-builder").arg(&cargo_miri_path); // invoked by forwarding most arguments
686730
cmd.arg("--runtool").arg(&cargo_miri_path); // invoked with just a single path argument
687731

‎src/tools/miri/cargo-miri/src/util.rs

+31-5
Original file line numberDiff line numberDiff line change
@@ -171,11 +171,16 @@ where
171171
drop(path); // We don't need the path, we can pipe the bytes directly
172172
cmd.stdin(std::process::Stdio::piped());
173173
let mut child = cmd.spawn().expect("failed to spawn process");
174-
{
175-
let stdin = child.stdin.as_mut().expect("failed to open stdin");
176-
stdin.write_all(input).expect("failed to write out test source");
177-
}
178-
let exit_status = child.wait().expect("failed to run command");
174+
let child_stdin = child.stdin.take().unwrap();
175+
// Write stdin in a background thread, as it may block.
176+
let exit_status = std::thread::scope(|s| {
177+
s.spawn(|| {
178+
let mut child_stdin = child_stdin;
179+
// Ignore failure, it is most likely due to the process having terminated.
180+
let _ = child_stdin.write_all(input);
181+
});
182+
child.wait().expect("failed to run command")
183+
});
179184
std::process::exit(exit_status.code().unwrap_or(-1))
180185
}
181186
}
@@ -317,3 +322,24 @@ pub fn clean_target_dir(meta: &Metadata) {
317322

318323
remove_dir_all_idem(&target_dir).unwrap_or_else(|err| show_error!("{}", err))
319324
}
325+
326+
/// Run `f` according to the many-seeds argument. In single-seed mode, `f` will only
327+
/// be called once, with `None`.
328+
pub fn run_many_seeds(many_seeds: Option<String>, f: impl Fn(Option<u32>)) {
329+
let Some(many_seeds) = many_seeds else {
330+
return f(None);
331+
};
332+
let (from, to) = many_seeds
333+
.split_once("..")
334+
.unwrap_or_else(|| show_error!("invalid format for `--many-seeds`: expected `from..to`"));
335+
let from: u32 = if from.is_empty() {
336+
0
337+
} else {
338+
from.parse().unwrap_or_else(|_| show_error!("invalid `from` in `--many-seeds=from..to"))
339+
};
340+
let to: u32 =
341+
to.parse().unwrap_or_else(|_| show_error!("invalid `to` in `--many-seeds=from..to"));
342+
for seed in from..to {
343+
f(Some(seed));
344+
}
345+
}

0 commit comments

Comments
 (0)