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

(cargo-)miri doesn't pass through --color=always? #2037

Closed
saethlin opened this issue Mar 24, 2022 · 12 comments · Fixed by #2243
Closed

(cargo-)miri doesn't pass through --color=always? #2037

saethlin opened this issue Mar 24, 2022 · 12 comments · Fixed by #2243
Labels
A-cargo Area: affects the cargo wrapper (cargo miri) C-bug Category: This is a bug.

Comments

@saethlin
Copy link
Member

I'm building a website to browse my automated cargo miri test outputs as they appear. But I want to display logs with all the helpful coloring that appears in my terminal and I can't figure out how to do it. cargo miri test --color=always -- --color=always produces color in the compiling messages and the very last line that says error: test failed, but not the actual diagnostics.

Is this a bug with cargo-miri? I'm not above using dirty Linux hacks to trick my experiment into thinking that a file is a TTY to get the escape codes, but I'd rather fix the problem if there is one.


If anyone wants to check it out, the current prototype is at https://miri.saethlin.dev/ub. It's WIP, I may change anything about it, etc.

@RalfJung
Copy link
Member

cargo-miri just forwards all flags to cargo. But it looks like the part where the colors are missing is the output of Miri itself? That is basically rustc with a different 'codegen backend' -- "the miri driver". I don't know how colors are controlled there, but I see rustc has a --color flag. But cargo won't know it is running something rustc-like so it won't do anything with colors, and the arguments after -- are passed to the interpreted program, not the Miri driver.

You could try passing things with MIRIFLAGS, that will be handed to the Miri driver.

@saethlin
Copy link
Member Author

Perfect. That works.

I've definitely noticed before that the rest of MIRIFLAGS is passed along to the driver. But I don't think I see that documented in the README. Should it be?

@saethlin
Copy link
Member Author

Actually no, this doesn't work, because doctests. They all crash with this error:

---- src/lib.rs - Slab<T>::clear (line 537) stdout ----
Test executable failed (exit code 1).

stderr:
error: Option 'color' given more than once

@oli-obk
Copy link
Contributor

oli-obk commented Mar 24, 2022

Hmm... technically we should fix rustc to accept such flags multiple times...

But I think going via the flags env var should not be necessary, so we should look into fixing that.

Is cargo miri run working correctly by any chance?I think this may be specific to cargo miri test?

@RalfJung
Copy link
Member

RalfJung commented Mar 24, 2022

But I think going via the flags env var should not be necessary, so we should look into fixing that.

Well, we are already passing all the arguments cargo passes to rustc, when we do the actual interpretation. We collect that info here:

let env = CrateRunEnv::collect(args, inside_rustdoc);

and then pass it to the driver here:

let mut args = info.args.into_iter();

So not sure why --color gets lost. It looks like it is preserved for rustdoc somehow, seeing as that complains about getting the flag twice?
You could play around with cargo miri test -v (and no MIRIFLAGS) a bit to see which flags are passed where.

@RalfJung
Copy link
Member

I've definitely noticed before that the rest of MIRIFLAGS is passed along to the driver. But I don't think I see that documented in the README. Should it be?

"The rest"? Simply all of it is passed to the driver.
But I am always open to improving the docs.

@saethlin
Copy link
Member Author

Perhaps I'm reading too much into this error, but this error makes me think that cargo-miri is stripping out the flags that it recognizes and passing the rest along to rustc:

╰ ➤ MIRIFLAGS=-Zmiri-oops cargo +miri miri test
   Compiling scratch v0.1.0 (/tmp/scratch)
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running unittests src/main.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/scratch-9d7717efc37bb64c)
error: unknown debugging option: `miri-oops`

error: test failed, to rerun pass '--bin scratch'

@RalfJung
Copy link
Member

No, that's not what happens. cargo miri test flags1 -- flags2 literally invokes cargo test flags -- flags2 (with only minor tweaks, e.g. adding --target if it was not already present). However it also sets some env vars so that when cargo invokes rustc or runs a binary, it actually calls back into cargo-miri which will then invoke the Miri driver as suitable.

Miri's -Z flags are handled by the Miri driver, and that one indeed passes all the ones it does not recognize on to the general code that is shared with rustc.

So I'd start debugging this by doing cargo miri test -v --color=always (no MIRIFLAGS) and seeing where the color flag is propagated and where it is lost.

@saethlin
Copy link
Member Author

╰ ➤ cargo +miri miri test -v --color=always
[cargo-miri miri] RUSTC_WRAPPER="/home/ben/.cargo/bin/cargo-miri"
[cargo-miri miri] CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER="/home/ben/.cargo/bin/cargo-miri"
[cargo-miri miri] RUSTDOC="/home/ben/.cargo/bin/cargo-miri"
[cargo-miri miri] "/home/ben/.rustup/toolchains/miri/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-v" "--color=always" "--target-dir" "/tmp/scratch/target/miri" "--"
       Fresh scratch v0.1.0 (/tmp/scratch)
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running `/home/ben/.cargo/bin/cargo-miri /tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps/scratch-9d7717efc37bb64c`
[cargo-miri runner] Overwriting run-time env var "DESKTOP_STARTUP_ID"="i3/kitty/1269-6-archlinux_TIME20315109" with build-time value "i3/kitty/1269-3-archlinux_TIME5256330"
[cargo-miri runner] Overwriting run-time env var "KITTY_PID"="40406" with build-time value "10403"
[cargo-miri runner] Overwriting run-time env var "LD_LIBRARY_PATH"="/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps:/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug:/home/ben/.rustup/toolchains/miri/lib/rustlib/x86_64-unknown-linux-gnu/lib:/home/ben/.rustup/toolchains/miri/lib" with build-time value "/tmp/scratch/target/miri/debug/deps:/home/ben/.rustup/toolchains/miri/lib:/home/ben/.rustup/toolchains/miri/lib"
[cargo-miri runner] Overwriting run-time env var "WINDOWID"="44040203" with build-time value "67108875"
[cargo-miri runner] "/home/ben/.cargo/bin/miri" "--crate-name" "scratch" "--edition=2021" "src/main.rs" "--emit=dep-info,link" "-C" "embed-bitcode=no" "-C" "debuginfo=2" "--test" "-C" "metadata=9d7717efc37bb64c" "-C" "extra-filename=-9d7717efc37bb64c" "--out-dir" "/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps" "--target" "x86_64-unknown-linux-gnu" "-C" "incremental=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/incremental" "-L" "dependency=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps" "-L" "dependency=/tmp/scratch/target/miri/debug/deps" "--sysroot" "/home/ben/.cache/miri/HOST" "--"

This looks to me like the flag gets dropped by cargo, right?

@RalfJung
Copy link
Member

Maybe the cargo cache is playing with us -- did you try cargo clean?

@saethlin
Copy link
Member Author

Fresh project, ran cargo clean, even removed the Miri cache. I still don't see a --color on the cargo-miri rustc line or the miri line after.

╰ ➤ cargo +miri miri test -v --color=always
   Compiling compiler_builtins v0.1.70
    Checking core v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core)
   Compiling libc v0.2.116
   Compiling cc v1.0.69
   Compiling memchr v2.4.1
   Compiling std v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std)
   Compiling unwind v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/unwind)
    Checking rustc-std-workspace-core v1.99.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/rustc-std-workspace-core)
    Checking alloc v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc)
    Checking cfg-if v0.1.10
    Checking adler v0.2.3
    Checking rustc-demangle v0.1.21
    Checking rustc-std-workspace-alloc v1.99.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/rustc-std-workspace-alloc)
    Checking panic_unwind v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/panic_unwind)
    Checking panic_abort v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/panic_abort)
    Checking gimli v0.25.0
    Checking hashbrown v0.12.0
    Checking miniz_oxide v0.4.0
    Checking object v0.26.2
    Checking std_detect v0.1.5 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/stdarch/crates/std_detect)
    Checking addr2line v0.16.0
    Finished release [optimized] target(s) in 13.44s
    Checking rustc-std-workspace-std v1.99.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/rustc-std-workspace-std)
    Checking proc_macro v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/proc_macro)
    Checking unicode-width v0.1.8
    Checking getopts v0.2.21
    Checking test v0.0.0 (/home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/test)
    Finished release [optimized] target(s) in 1.04s
[cargo-miri miri] RUSTC_WRAPPER="/home/ben/.cargo/bin/cargo-miri"
[cargo-miri miri] CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER="/home/ben/.cargo/bin/cargo-miri"
[cargo-miri miri] RUSTDOC="/home/ben/.cargo/bin/cargo-miri"
[cargo-miri miri] "/home/ben/.rustup/toolchains/miri/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-v" "--color=always" "--target-dir" "/tmp/scratch/target/miri" "--"
   Compiling scratch v0.1.0 (/tmp/scratch)
     Running `/home/ben/.cargo/bin/cargo-miri rustc --crate-name scratch --edition=2021 src/main.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 --test -C metadata=9d7717efc37bb64c -C extra-filename=-9d7717efc37bb64c --out-dir /tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps --target x86_64-unknown-linux-gnu -C incremental=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/incremental -L dependency=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps -L dependency=/tmp/scratch/target/miri/debug/deps`
[cargo-miri rustc] writing stub dep-info to `/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps/scratch-9d7717efc37bb64c.d`
[cargo-miri rustc] writing run info to `/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps/scratch-9d7717efc37bb64c`
    Finished test [unoptimized + debuginfo] target(s) in 0.08s
     Running `/home/ben/.cargo/bin/cargo-miri /tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps/scratch-9d7717efc37bb64c`
[cargo-miri runner] Overwriting run-time env var "LD_LIBRARY_PATH"="/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps:/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug:/home/ben/.rustup/toolchains/miri/lib/rustlib/x86_64-unknown-linux-gnu/lib:/home/ben/.rustup/toolchains/miri/lib" with build-time value "/tmp/scratch/target/miri/debug/deps:/home/ben/.rustup/toolchains/miri/lib:/home/ben/.rustup/toolchains/miri/lib"
[cargo-miri runner] "/home/ben/.cargo/bin/miri" "--crate-name" "scratch" "--edition=2021" "src/main.rs" "--emit=dep-info,link" "-C" "embed-bitcode=no" "-C" "debuginfo=2" "--test" "-C" "metadata=9d7717efc37bb64c" "-C" "extra-filename=-9d7717efc37bb64c" "--out-dir" "/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps" "--target" "x86_64-unknown-linux-gnu" "-C" "incremental=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/incremental" "-L" "dependency=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps" "-L" "dependency=/tmp/scratch/target/miri/debug/deps" "--sysroot" "/home/ben/.cache/miri/HOST" "--"

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

@RalfJung
Copy link
Member

This is the key line

     Running `/home/ben/.cargo/bin/cargo-miri rustc --crate-name scratch --edition=2021 src/main.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 --test -C metadata=9d7717efc37bb64c -C extra-filename=-9d7717efc37bb64c --out-dir /tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps --target x86_64-unknown-linux-gnu -C incremental=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/incremental -L dependency=/tmp/scratch/target/miri/x86_64-unknown-linux-gnu/debug/deps -L dependency=/tmp/scratch/target/miri/debug/deps`

Indeed no --color here. I guess that is because cargo is using --error-format=json, and my incling is that the diagnostic-rendered-ansi in --json tells rustc to put ANSI color codes into the JSON so that when cargo dumps that output to the terminal it has color.

So the culprit is this code:

miri/cargo-miri/bin.rs

Lines 927 to 950 in 65125df

// We need to patch "--extern" filenames because we forced a check-only
// build without cargo knowing about that: replace `.rlib` suffix by
// `.rmeta`.
// We also need to remove `--error-format` as cargo specifies that to be JSON,
// but when we run here, cargo does not interpret the JSON any more. `--json`
// then also nees to be dropped.
let mut args = info.args.into_iter();
let error_format_flag = "--error-format";
let json_flag = "--json";
while let Some(arg) = args.next() {
if arg == "--extern" {
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('='));
// Drop this argument.
} else if arg.starts_with(json_flag) {
let suffix = &arg[json_flag.len()..];
assert!(suffix.starts_with('='));
// Drop this argument.
} else {
cmd.arg(arg);
}
}

We can't have that JSON output (since we actually run this when cargo thinks it runs a binary, i.e., it wouldn't interpret that JSON), so we drop the --error-format and the --json. Looks like we should additionally, if --json contains diagnostic-rendered-ansi, add a --color=always to the flags as a replacement for the flags we dropped.

@RalfJung RalfJung added C-bug Category: This is a bug. A-cargo Area: affects the cargo wrapper (cargo miri) labels Jun 5, 2022
@bors bors closed this as completed in b2616ce Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo Area: affects the cargo wrapper (cargo miri) C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants