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");
badumbatish marked this conversation as resolved.
Show resolved Hide resolved
cmd.args(get_miriflags_cargo_mini());
Copy link
Member

Choose a reason for hiding this comment

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

So you are taking the miri flags and passing them to cargo...? How does that make sense?

// 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
badumbatish marked this conversation as resolved.
Show resolved Hide resolved
flags.split(' ').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect()
}

pub fn get_miriflags_cargo_mini() -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a doc comment explaining what this does. (Seems like you have a bunch of that inside the function, but that's now how doc comments work in Rust.)

Why "mini"?

// 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.
Copy link
Member

Choose a reason for hiding this comment

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

Please add a link to where this behavior is documented for cargo.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Ralf, this is from PR #2451 https://github.com/rust-lang/miri/pull/2451/files#diff-922365a8fd004632c70c2426b2851c89ed2e13229b7e7693550b87f00b100badR552 for the function get_miriflags().

I'll remove this block of comment from this function.

The comment can be made clearer by specifying flags from cargo config to flags from .cargo/config.toml (configuration file instead of --config option) since we run this in get_miriflags_runner

    let mut cmd = cargo();
    cmd.args(["-Zunstable-options", "config", "get", "miri.flags", "--format=json-value"]);

The behavior is from https://doc.rust-lang.org/cargo/reference/config.html#command-line-overrides

Cargo also accepts arbitrary configuration overrides through the --config command-line option. The argument should be in TOML syntax of KEY=VALUE:
cargo --config net.git-fetch-with-cli=true fetch
The --config option may be specified multiple times, in which case the values are merged in left-to-right order, using the same merging logic that is used when multiple configuration files apply.
Configuration values specified this way take precedence over environment variables, which take precedence over configuration files.

We check the arguments from --config first, then environment variable, then check the configuration files via cargo config get.

I couldn't find the evidence for This matches cargo behavior for RUSTFLAGS. Should I remove that line?

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find the evidence for This matches cargo behavior for RUSTFLAGS.

There is https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags. So please add a link to that -- and make sure that MIRIFLAGS behaves the same, except that for now we don't have to support setting CARGO_ENCODED_MIRIFLAGS or target.<triple>.miriflags.

//
// 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)
badumbatish marked this conversation as resolved.
Show resolved Hide resolved
//
// 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())
Copy link
Member

@RalfJung RalfJung Sep 28, 2024

Choose a reason for hiding this comment

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

This doesn't make sense, "ENCODED" env vars shouldn't use flagsplit. They use a different format that doesn't rely on whitespace. See the cargo docs.

Copy link
Author

Choose a reason for hiding this comment

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

From https://doc.rust-lang.org/cargo/reference/environment-variables.html

CARGO_ENCODED_RUSTFLAGS — A list of custom flags separated by 0x1f (ASCII Unit Separator) to pass to all compiler invocations that Cargo performs.

I'll change this soon. Keeping this unresolved. Thank you

} else if cargo_extra_flags().iter().any(|s| s.contains(&"-Zmiri".to_string())) {
Copy link
Member

Choose a reason for hiding this comment

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

What are you trying to do here? cargo_extra_flags will never contain any -Zmiri. Please read the doc comment of the functions you call.

// (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.
badumbatish marked this conversation as resolved.
Show resolved Hide resolved
// 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