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

Add a way to extract miri flags from --config, env and toml #3875

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
20 changes: 18 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ their name.

You can pass [flags][miri-flags] to Miri via `MIRIFLAGS`. For example,
`MIRIFLAGS="-Zmiri-disable-stacked-borrows" cargo miri run` runs the program
without checking the aliasing of references.
without checking the aliasing of references. Also, you can set the `miri.flags`
setting in [cargo configuration](https://doc.rust-lang.org/cargo/reference/config.html).

When compiling code via `cargo miri`, the `cfg(miri)` config flag is set for code
that will be interpreted under Miri. You can use this to ignore test cases that fail
Expand Down Expand Up @@ -275,7 +276,22 @@ Try running `cargo miri clean`.
[miri-flags]: #miri--z-flags-and-environment-variables

Miri adds its own set of `-Z` flags, which are usually set via the `MIRIFLAGS`
environment variable. We first document the most relevant and most commonly used flags:
environment variable or using the `miri.flags` setting in
[cargo configuration](https://doc.rust-lang.org/cargo/reference/config.html),
they are mutually exclusive with priority of `MIRIFLAGS` environment variable:

```bash
MIRIFLAGS="-Zmiri-disable-stacked-borrows" cargo miri run
```

```toml
# .cargo/config.toml

[miri]
flags = ["-Zmiri-disable-isolation", "-Zmiri-report-progress"]
```

We first document the most relevant and most commonly used flags:

* `-Zmiri-address-reuse-rate=<rate>` changes the probability that a freed *non-stack* allocation
will be added to the pool for address reuse, and the probability that a new *non-stack* allocation
Expand Down
10 changes: 5 additions & 5 deletions cargo-miri/src/phases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
let target_dir = get_target_dir(&metadata);
cmd.arg("--target-dir").arg(target_dir);

// eprintln!("Getting miri flags in phase_cargo_miri");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commented eprintln might need to be removed.

But I am not the maintainer and this PR might be on a draft state, so this is just a reminder. :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha yes I'll remove it on the next iteration :) ty for the review

cmd.args(get_miriflags_cargo_mini());
// Store many-seeds argument.
let mut many_seeds = None;
// *After* we set all the flags that need setting, forward everything else. Make sure to skip
Expand Down Expand Up @@ -640,11 +642,9 @@ pub fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: Runner
cmd.arg(arg);
}
}
// Respect `MIRIFLAGS`.
if let Ok(a) = env::var("MIRIFLAGS") {
let args = flagsplit(&a);
cmd.args(args);
}
// Respect miriflags.
// eprintln!("Get miri flags in phase_runner");
cmd.args(get_miriflags_runner());
// Set the current seed.
if let Some(seed) = seed {
eprintln!("Trying seed: {seed}");
Expand Down
76 changes: 71 additions & 5 deletions cargo-miri/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,77 @@ pub fn escape_for_toml(s: &str) -> String {
format!("\"{s}\"")
}

pub fn flagsplit(flags: &str) -> Vec<String> {
// This code is taken from `RUSTFLAGS` handling in cargo.
// Taken from miri-script util.rs
flags.split(' ').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect()
}

pub fn get_miriflags_cargo_mini() -> Vec<String> {
// Respect `MIRIFLAGS` and `miri.flags` setting in cargo config.
// If MIRIFLAGS is present, flags from cargo config are ignored.
// This matches cargo behavior for RUSTFLAGS.
//
// Strategy: (1) check pseudo var CARGO_ENCODED_MIRIFLAGS first (this is only set after we check for --config
// in the cargo_dash_dash in the if else)
//
// if CARGO_ENCODED_MIRIFLAGS doesn't exist, we check in --config (2)
// if --config doesn't exist, we check offical env var MIRIFLAGS (3)
//
// if MIRIFLAGS is non-existent, we then check for toml (4)
if let Ok(cargo_encoded_miri_flags) = env::var("CARGO_ENCODED_MIRIFLAGS") {
// (1)
// eprintln!("Choice 1");
flagsplit(cargo_encoded_miri_flags.as_str())
} else if cargo_extra_flags().iter().any(|s| s.contains(&"-Zmiri".to_string())) {
// (2)
// eprintln!("Choice 2");
let cargo_dash_dash_config = cargo_extra_flags();
let miri_flags_vec = cargo_dash_dash_config
.into_iter()
.filter(|arg| arg.contains(&"-Zmiri".to_string()))
.collect::<Vec<String>>();
let miri_flags_string = miri_flags_vec.join(" ");
env::set_var("CARGO_ENCODED_MIRIFLAGS", miri_flags_string);
miri_flags_vec
} else {
Vec::default()
}
}
pub fn get_miriflags_runner() -> Vec<String> {
// TODO: I quite not understand what Carl Jung means by Oh and please add a link to https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags.
// I guess we don't support the target.rustflags part yet? (That's okay but should be mentioned in a comment.)
//
// Fetch miri flags from cargo config.
let mut cmd = cargo();
cmd.args(["-Zunstable-options", "config", "get", "miri.flags", "--format=json-value"]);
let output = cmd.output().expect("failed to run `cargo config`");
let config_miriflags =
std::str::from_utf8(&output.stdout).expect("failed to get `cargo config` output");

// Respect `MIRIFLAGS` and `miri.flags` setting in cargo config.
// If MIRIFLAGS is present, flags from cargo config are ignored.
// This matches cargo behavior for RUSTFLAGS.
//
// Strategy: (1) check pseudo var CARGO_ENCODED_MIRIFLAGS first (this is only set after we check for --config
// in the cargo_dash_dash in the if else)
//
// if CARGO_ENCODED_MIRIFLAGS doesn't exist, we check in --config (2)
// if --config doesn't exist, we check offical env var MIRIFLAGS (3)
//
// if MIRIFLAGS is non-existent, we then check for toml (4)
if let Ok(a) = env::var("MIRIFLAGS") {
// (3)
// This code is taken from `RUSTFLAGS` handling in cargo.
// eprintln!("Choice 3");
// eprintln!("{}", a);
a.split(' ').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect()
} else {
// (4)
// eprintln!("Choice 4");
serde_json::from_str::<Vec<String>>(config_miriflags).unwrap_or_default()
}
}
/// Returns the path to the `miri` binary
pub fn find_miri() -> PathBuf {
if let Some(path) = env::var_os("MIRI") {
Expand Down Expand Up @@ -118,11 +189,6 @@ pub fn cargo() -> Command {
Command::new(env::var_os("CARGO").unwrap_or_else(|| OsString::from("cargo")))
}

pub fn flagsplit(flags: &str) -> Vec<String> {
// This code is taken from `RUSTFLAGS` handling in cargo.
flags.split(' ').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect()
}

/// Execute the `Command`, where possible by replacing the current process with a new process
/// described by the `Command`. Then exit this process with the exit code of the new process.
pub fn exec(mut cmd: Command) -> ! {
Expand Down
44 changes: 44 additions & 0 deletions test-cargo-miri/run-test.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ def test_no_rebuild(name, cmd, env=None):
fail("Something was being rebuilt when it should not be (or we got no output)")

def test_cargo_miri_run():
# Try to remove cargo config to avoid unexpected settings
try:
os.remove('.cargo/config.toml')
except OSError:
pass

test("`cargo miri run` (no isolation)",
cargo_miri("run"),
"run.default.stdout.ref", "run.default.stderr.ref",
Expand All @@ -115,6 +121,44 @@ def test_cargo_miri_run():
'MIRITESTVAR': "wrongval", # make sure the build.rs value takes precedence
},
)

# Create a config with isolation disabled
try:
os.mkdir('.cargo')
except OSError:
pass

with open(".cargo/config.toml", "w") as f:
f.write('[miri]\nflags = ["-Zmiri-disable-isolation"]')

# Testing miri.flags are taken in account
test("`cargo miri run` (no isolation, set from cargo config)",
cargo_miri("run"),
"run.default.stdout.ref", "run.default.stderr.ref",
stdin=b'12\n21\n',
env={
'MIRITESTVAR': "wrongval", # make sure the build.rs value takes precedence
},
)

# Create an invalid config
with open(".cargo/config.toml", "w") as f:
f.write('[miri]\nflags = ["-Zmiri-there-is-no-such-flag"]')

# Check that MIRIFLAGS envar has higher precedence tha cargo config
test("`cargo miri run` (no isolation, ignoring cargo config)",
cargo_miri("run"),
"run.default.stdout.ref", "run.default.stderr.ref",
stdin=b'12\n21\n',
env={
'MIRIFLAGS': "-Zmiri-disable-isolation",
'MIRITESTVAR': "wrongval", # make sure the build.rs value takes precedence
},
)

# Cleaning up config after all config-related tests
os.remove('.cargo/config.toml')

# Special test: run it again *without* `-q` to make sure nothing is being rebuilt (Miri issue #1722)
test_no_rebuild("`cargo miri run` (no rebuild)",
cargo_miri("run", quiet=False) + ["--", ""],
Expand Down