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

badumbatish · 2024-09-10T07:03:24Z

The main driver is get_miri_flags in cargo-miri/src/util.rs, which follow this stratergy:

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

This is called once in phase_cargo_miri, then again in phase_runner.

For --config, I only retains all String that contains "miri", and then join them via " " for env. This is because flagsplit splits based on the space character. Please let me know if this is not the procedure.

In later stage this env is resolved into Vec via flagsplit().

I'm not sure how to write the test to describe this precedence of (1) over (2). Advice would be helpful :)

badumbatish · 2024-09-10T07:15:27Z

hmmm hahha ok maybe i should have retained "-Zmiri" instead of just miri. The latter makes file name and other things gets tangled in

badumbatish · 2024-09-10T09:08:35Z

after debugging i found out that in phase_cargo_miri if i try to extract MIRIFLAGS (a.k.a choice (3)) from env then some of the program will fail like this

FAILED TEST: tests/pass-dep/num_cpus.rs
command: "Dependencies"

full stderr:

   0: �[91mfailed to compile dependencies:
      command: env -u RUSTFLAGS "/home/jjasmine/Developer/github/miri/target/debug/cargo-miri" "miri" "run" "--target-dir" "/home/jjasmine/Developer/github/miri/target/miri_ui" "--manifest-path" "test_dependencies/Cargo.toml" "--target=x86_64-unknown-linux-gnu" "--locked" "--message-format=json"
      stderr:
      Getting miri flags in phase_cargo_miri
      Choice 3
       -Zmiri-provenance-gc=1
      error: unknown `-Z` flag specified: miri-provenance-gc

      For available unstable features, see https://doc.rust-lang.org/nightly/cargo/reference/unstable.html
      If you intended to use an unstable rustc feature, try setting `RUSTFLAGS="-Zmiri-provenance-gc"`


      stdout:�[0m

but if i seperate them into so that in phase_cargo_miri I only perform choice (1) and (2) and then in phase_runner the rest then the ui test passes fine. I think I'm missing sth important?

badumbatish · 2024-09-14T05:51:09Z

@rustbot ready

badumbatish · 2024-09-14T05:51:22Z

heheh now i know how to ask for review

RalfJung · 2024-09-14T07:11:12Z

Yeah I've seen the PR. :) Thanks for creating it!

It's quite busy in my life currently though, so I can't promise when I will get around to take a look at this.

cargo-miri/src/phases.rs

RalfJung

Thanks for the PR! However, I'm afraid I mostly don't understand what you are trying to do here...

We want to either compute the flags in phase_runner, or compute them once upfront in phase_cargo_miri. You currently have complicated logic in both places which definitely is not the right approach. Probably doing it once upfront is better, since computing the flags will be more expensive now.

So, in phase_cargo_miri we compute the flags (details TBD, should work exactly like RUSTFLAGS). We take the result and store it in CARGO_ENCODED_MIRIFLAGS, using the usual ENCODED format cargo uses (documented e.g. here). Then phase_runner simply has to read that env var and set the arguments.

cargo-miri/src/util.rs

RalfJung · 2024-09-28T13:33:21Z

cargo-miri/src/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> {


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"?

RalfJung · 2024-09-28T13:33:29Z

cargo-miri/src/util.rs

+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.


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

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?

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.

cargo-miri/src/util.rs

RalfJung · 2024-09-28T13:34:52Z

cargo-miri/src/util.rs

+    if let Ok(cargo_encoded_miri_flags) = env::var("CARGO_ENCODED_MIRIFLAGS") {
+        // (1)
+        // eprintln!("Choice 1");
+        flagsplit(cargo_encoded_miri_flags.as_str())


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.

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

RalfJung · 2024-09-28T13:36:17Z

cargo-miri/src/util.rs

+        // (1)
+        // eprintln!("Choice 1");
+        flagsplit(cargo_encoded_miri_flags.as_str())
+    } else if cargo_extra_flags().iter().any(|s| s.contains(&"-Zmiri".to_string())) {


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.

cargo-miri/src/util.rs

RalfJung · 2024-09-28T13:41:07Z

cargo-miri/src/phases.rs

@@ -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");
+    cmd.args(get_miriflags_cargo_mini());


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

RalfJung · 2024-09-28T14:00:19Z

@rustbot author

badumbatish · 2024-11-19T04:50:46Z

Apologies for the delay. I'm jampacked with classes, exams and projects right now. I'll get back to it soon. Really sorry for this

RalfJung · 2024-11-19T06:43:05Z

Thanks for the update, and no worries. :)

avrong and others added 10 commits September 10, 2024 02:07

Fetch miri flags from cargo config

821459d

Add tests for fetching flags from from cargo config

8273896

Add info on setting miriflags in cargo config

8959e9f

Move get_miri_flags to util.rs

d0f39d2

Update pseudo-flag and better comments

a8b44f3

Fix build warnings of unused

2734745

FIX: Fix clippy manual_unwrap_or_default

f69a651

FEAT: Add --config option for miri flags

2920eba

FIX: Add debug info, ready to debug

08668c7

Fix bug in cargo miri phase related to choice 3

3502a94

badumbatish force-pushed the 2347_new branch from 001faa5 to 3502a94 Compare September 10, 2024 09:08

DOCS: Comment out eprintln! after debug

914bda3

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Sep 14, 2024

tiif reviewed Sep 14, 2024

View reviewed changes

cargo-miri/src/phases.rs Show resolved Hide resolved

RalfJung reviewed Sep 28, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Sep 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

badumbatish commented Sep 10, 2024 •

edited

Loading

badumbatish commented Sep 10, 2024

badumbatish commented Sep 10, 2024

badumbatish commented Sep 14, 2024

badumbatish commented Sep 14, 2024

RalfJung commented Sep 14, 2024

RalfJung left a comment

RalfJung Sep 28, 2024

RalfJung Sep 28, 2024

badumbatish Oct 8, 2024

RalfJung Nov 4, 2024

RalfJung Sep 28, 2024 •

edited

Loading

badumbatish Oct 8, 2024

RalfJung Sep 28, 2024

RalfJung Sep 28, 2024

RalfJung commented Sep 28, 2024

badumbatish commented Nov 19, 2024

RalfJung commented Nov 19, 2024

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

Are you sure you want to change the base?

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

Conversation

badumbatish commented Sep 10, 2024 • edited Loading

badumbatish commented Sep 10, 2024

badumbatish commented Sep 10, 2024

badumbatish commented Sep 14, 2024

badumbatish commented Sep 14, 2024

RalfJung commented Sep 14, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung Sep 28, 2024

Choose a reason for hiding this comment

RalfJung Sep 28, 2024

Choose a reason for hiding this comment

badumbatish Oct 8, 2024

Choose a reason for hiding this comment

RalfJung Nov 4, 2024

Choose a reason for hiding this comment

RalfJung Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

badumbatish Oct 8, 2024

Choose a reason for hiding this comment

RalfJung Sep 28, 2024

Choose a reason for hiding this comment

RalfJung Sep 28, 2024

Choose a reason for hiding this comment

RalfJung commented Sep 28, 2024

badumbatish commented Nov 19, 2024

RalfJung commented Nov 19, 2024

badumbatish commented Sep 10, 2024 •

edited

Loading

RalfJung Sep 28, 2024 •

edited

Loading