Add option for generating coverage reports #3954

Mandragorian · 2024-10-09T10:00:31Z

Add a --coverage option in the test subcommand of the miri script. This option, when set, will generate a coverage report after running the tests.

cargo-binutils is needed as a dependency to generate the reports.

RalfJung · 2024-10-09T10:05:47Z

Thanks! :-)

Would be good to also have a smoke test in CI to at least ensure this works. That should likely go somewhere in ci/ci.sh.

oli-obk · 2024-10-09T13:19:26Z

If you additionally pipe the output to $GITHUB_STEP_SUMMARY we can see it in CI of every run

Little effort for a minimal report shown on every run

saethlin · 2024-10-09T15:01:45Z

miri-script/src/commands.rs

+        let home = std::env::var("HOME")?;
+        let ignored = format!("{home}/*|miri/target/*|target/debug/*|rust/deps/*");


What exactly is this supposed to ignore? Or, what undesirable thing happens if we don't ignore anything?

If I don't add these ignore regular expressions, the report also show files like:

miri/target/debug/build/chrono-tz-65fd732542149546/out/timezones.rs

root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/aes-0.8.4/src/autodetect.rs

rust/deps/tracing-0.1.37/src/macros.rs

(the root path is because I run it in a container, so it is the home directory)

Additionally the coverage percentages shown in the report are smaller than the real ones, since non-miri files are included.

Ignoring these files is the simplest way I could think to fix this.

Interesting. I suppose the report isn't supposed to look like this?

$ cargo-cov -- report --instr-profile=merged.profdata --object target/debug/miri -ignore-filename-regex=/home/ben/*|miri/target/*|target/debug/*|rust/deps/* Filename Regions Missed Regions Cover Functions Missed Functions Executed Lines Missed Lines Cover Branches Missed Branches Cover ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- TOTAL 0 0 - 0 0 - 0 0 - 0 0 -

I do all development work from directories under $HOME which I think is pretty typical. So I think this regex is excluding all the profile data.

Is it possible for us to only include certain directories, instead of excluding them? We just want to include src/ I think, which seems like a much more robust filter, if it's possible to implement.

yeah that makes sense. Working in the container, I did my dev work under /miri, which was not under $HOME (/root). So I didn't think of that. Unfortunately I can't find an option to include files/directories, instead of excluding them.

We could use the llvm_profparser crate, used in tarpaulin, as i suggested in #240. It seems to provide a mapping which can produce reports only for the paths that we are interested in.

I went with using cargo-binutils for this PR, since it seemed like a more common crate, than tarpauling/llvm_profparser. Maybe adding llvm_profparser as a dependency for coverage reports is acceptable.

The quick solution would be to exclude $HOME/.{cargo, rustup} as suggested in another comment.

Might be worth a feature request for cargo-binutils?

I'd rather avoid that dependency, at least for now.

I think replacing -ignore-filename-regex={ignored} with --sources src/ selects the files we want. At least the coverage report it emits looks reasonable to me?

I don't think there's any need to use cargo-binutils, in this case cargo-cov is a trivial wrapper around llvm-cov.

Yes, --sources src/ actually works. I tried using it like --sources=src/ and it didn't work for me. Thanks for finding this option!

I use cargo-binutils because I saw this in the instrument coverage documentation:

note that the coverage mapping data generated by the Rust compiler requires LLVM version 12 or higher, and processing the raw data may require exactly the LLVM version used by the compiler. (llvm-cov --version typically shows the tool's LLVM version number, and rustc --verbose --version shows the version of LLVM used by the Rust

You can install compatible versions of these tools via the rustup component llvm-tools-preview. This component is the recommended path, though the specific tools available and their interface is not currently subject to Rust's usual stability guarantees. In this case, you may also find cargo-binutils useful as a wrapper around these tools.

I am not really familiar with llvm ecosystem so I don't know how easy it is to get compatible versions of the llvm tools. I used cargo-binutils to be more portable.

As the cargo-binutils docs say, all that package does is exec the llvm tools from the sysroot, relying on the llvm-tools-preview component being installed. We're already doing toolchain management and finding stuff in the sysroot as part of miri-script. Can you reuse that code? Or make it reusable?

During the toolchain subcommand we install the llvm-tools component (not the -preview version). I copy-pasted and modified some code from cargo-binutils and it seems to work. (do we need to do anything license-wise? I don't think so but I am not sure. Both miri and cargo-binutils have an MIT license).

After doing some more search it seems that llvm-tools should never have existed and the "proper" name for it should have been llvm-tools-preview. rust-lang/rust#119164. However after inadvertently using the name llvm-tools and people started depending on it it was decided not to change the name again and break people. Indeed when I add the llvm-tools-preview component I see:

$ rustup component add llvm-tools-preview info: downloading component 'llvm-tools' info: installing component 'llvm-tools'

Mandragorian · 2024-10-09T22:54:30Z

Regarding smoke tests/showing the output in the CI:

I am only worried about the run time of the tests growing. I could maybe add a filter in the ./miri test invocation so that it only runs one test, so the smoke test doesn't take too long. However if we do this then we won't get a meaningful report in the CI.

I am not sure how github actions work. Would creating a new action so that it can run in parallel with the other tests work?

This also makes me think that the way this is implemented, will only produce coverage reports for a single target. It wont aggregate coverage information across different targets, so we won't have a complete picture of the coverage in a single report. A possible solution to this might be to support running multiple targets with a single ./miri test invocation, as we did with cargo-miri. This way the profraw files from all targets would me merged together. But then this might give wrong results when we see how many times a line was executed during the tests?

@RalfJung @oli-obk

RalfJung · 2024-10-10T07:51:06Z

Regarding the multi-target situation, let's move this discussion to the issue as it is not needed for an MVP. What is the runtime overhead of this? We can add a new dedicated coverage target if needed, though it will duplicate all the work of downloading the right rustc and building Miri. Or does it need its own separate build anyway?

RalfJung · 2024-10-10T07:51:44Z

Yeah excluding HOME is definitely not going to work. Does excluding HOME/.cargo work?

RalfJung · 2024-10-10T07:53:34Z

Also, why does it exclude "target/debug" on top of the others? Hard-coding "debug" is not great. Please have a comment explaining for every exclusion why it is needed. (Sorry I can't reply in the thread right now, only via email.)

RalfJung · 2024-10-10T08:02:07Z

miri-script/src/commands.rs

@@ -468,6 +468,9 @@ impl Command {
        if bless {
            e.sh.set_var("RUSTC_BLESS", "Gesundheit");
        }
+        if coverage {
+            e.sh.set_var("RUSTFLAGS", "-C instrument-coverage");


This will overwrite the previous RUSTFLAGS that is carefully computed in MiriEnv::new. Instead, please append the flag.

Also, Miri will be built twice this way: once without the flag in e.build_miri_sysroot, and then again with the flag in e.test.

So the flag should probably be set before build_miri_sysroot.

i am now appending the flag, and have moved the code before building the sysroot.

Mandragorian · 2024-10-12T15:07:12Z

I am now not excluding directories, and I use the --sources option in llvm-cov to only include the src/ dir.

For the smoke test, I invoke ./miri test --coverage do_not_match_a_test at the very end of ci.sh. This way no tests are actually (re-)run, but a coverage report is still produced, and should fail the test if something fails.

What I am not sure about is how I find the miri binary to give to llvm-cov. What I currently have is:

--object {miri_dir}/target/debug/miri{suffix}

with suffix and miri dir being:

let miri_dir = e.miri_dir.as_os_str();
let suffix = if cfg!(windows) { ".exe" } else { "" };

I tried reading through the code but I couldn't find a similar pattern to see if there is a better way to get the miri binary's path. I couldn't find a way to not hardcode debug here.

I also added *.profraw and *.profdata in .gitignore.

RalfJung · 2024-10-12T15:23:09Z

For the smoke test, I invoke ./miri test --coverage do_not_match_a_test at the very end of ci.sh. This way no tests are actually (re-)run, but a coverage report is still produced, and should fail the test if something fails.

This will still do a full rebuild, right, because it needs an instrumented Miri? And then it will run zero tests, so it doesn't actually check that we can invoke Miri with instrumentation and gather the results.

Note that ./miri test runs Miri many times, so what I would expect to happen is that the profile gets accumulated from all invocations -- and ./miri test --coverage do_not_match_a_test would show a basically empty profile since nothing runs.

I think I prefer the option where we add a new job that does the full ./miri test --coverage.

What I am not sure about is how I find the miri binary

There's a let miri_bin in fn run, is that what you need?

RalfJung · 2024-10-12T15:25:05Z

miri-script/src/commands.rs

+        pathbuf.push("rustlib");
+        pathbuf.push(rustc_version::version_meta()?.host);
+        pathbuf.push("bin");
+        Ok(pathbuf)


This seems to duplicate logic in MiriEnv::new. Instead, please just store the libdir that is computed there in a field. path!(libdir / ".." / "bin") should be the directory you are looking for.

ah I missed that! thanks!

RalfJung · 2024-10-12T15:26:49Z

miri-script/src/commands.rs

        Ok(())
    }

+    fn show_coverage_report(e: &MiriEnv) -> Result<()> {
+        let profraw_files: Vec<_> = Self::profraw_files(".")?;


This seems a bit messy... it will just go over all files that happened to be called .profraw, no matter whether they are from this run or not? Isn't there a cleaner way, like tell the instrumented binary to put the output into a particular folder?

We should not be creating files just randomly in the Miri source tree, and we certainly should not delete files randomly from the Miri source tree. Temporary files should go in a tempdir, and then it also shouldn't be needed to change .gitignore.

I am using the tempfile crate now. I saw that it is used in dev-dependencies for some crates so maybe it is ok depending on it for the script? Otherwise I could use std::env::temp_dir and manually create a subdir there.

Yeah tempfile is good. It pulls in a few more dependencies, I hope those don't have significant build time.

RalfJung · 2024-10-13T19:11:17Z

miri-script/src/coverage.rs

+        let rustflags = format!("{rustflags} -C instrument-coverage");
+        e.sh.set_var("RUSTFLAGS", rustflags);
+
+        let file_template = self.path.path().join("default_%m_%p.profraw");


Please explain the magic formatting symbols here. Also, why "default"?

done. as for "default" i just copy pasted the default value for the env var.

Maybe we should just make it miri_ then to make it a bit more self-explaining 🤷

RalfJung · 2024-10-13T19:12:01Z

miri-script/src/coverage.rs

+        let rustflags = e.sh.var("RUSTFLAGS")?;
+        let rustflags = format!("{rustflags} -C instrument-coverage");


Suggested change

let rustflags = e.sh.var("RUSTFLAGS")?;

let rustflags = format!("{rustflags} -C instrument-coverage");

let mut rustflags = e.sh.var("RUSTFLAGS")?;

rustflags.push_str(" -C instrument-coverage");

RalfJung · 2024-10-13T19:12:30Z

miri-script/src/coverage.rs

+        let mut cov_bin = e.libdir.clone();
+        cov_bin.push("..");
+        cov_bin.push("bin");
+        cov_bin.push("llvm-cov");


Can this be written nicer with the path! macro?

yes, thanks!

Mandragorian · 2024-10-13T21:41:17Z

@rustbot ready

RalfJung · 2024-10-21T21:08:37Z

It's in my queue, but $DAYJOB is keeping me quite busy these days.

bors · 2024-10-22T11:58:17Z

☔ The latest upstream changes (presumably #3981) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2024-10-25T10:40:22Z

miri-script/src/commands.rs

+                    bless,
+                    flags,
+                    target,
+                    coverage.then_some(crate::coverage::CoverageReport::new()?),


Please move this logic into the test function; the arguments should corresponds to the enum variant.

miri-script/src/coverage.rs

RalfJung · 2024-10-25T10:47:06Z

@rustbot author

bors · 2024-10-25T15:17:39Z

☔ The latest upstream changes (presumably #3989) made this pull request unmergeable. Please resolve the merge conflicts.

Mandragorian · 2024-10-26T16:04:33Z

@rustbot ready

RalfJung · 2024-10-27T20:36:37Z

Please don't squash commits while review is in progress. Thanks to github being kind of bad at dealing with force pushes, that makes it totally impossible to tell what changed since my last review.

RalfJung

Just some nits left :)

miri-script/src/coverage.rs

.github/workflows/ci.yml

RalfJung · 2024-10-27T20:52:26Z

@rustbot author

Mandragorian · 2024-10-27T21:15:31Z

Ah, sorry about the squashing! Didn't know that the diff would get messed up :/

@rustbot ready

RalfJung · 2024-10-27T21:18:31Z

No worries. :)

All looking good, so now is the time to squash. :) (with git rebase --keep-base)

Add a `--coverage` option in the `test` subcommand of the miri script. This option, when set, will generate a coverage report after running the tests. `cargo-binutils` is needed as a dependency to generate the reports.

Mandragorian · 2024-10-27T23:05:39Z

Hoping this worked as expected (out of curiosity, do you use the "changes from last review" in the "files changed" tab? just asking so I can understand the workflow better)

RalfJung · 2024-10-28T05:37:55Z

Looks good, thanks!

(out of curiosity, do you use the "changes from last review" in the "files changed" tab? just asking so I can understand the workflow better)

No, I have not found that to work reliably. After a force-push, it always says "We went looking everywhere, but couldn’t find those commits". I am looking at the commits you push and the "force-push" diffs.

Mandragorian mentioned this pull request Oct 9, 2024

Coverage #240

Open

Mandragorian force-pushed the coverage-report branch from 48780bf to 5b1666a Compare October 9, 2024 11:49

saethlin reviewed Oct 9, 2024

View reviewed changes

RalfJung reviewed Oct 10, 2024

View reviewed changes

Mandragorian force-pushed the coverage-report branch 3 times, most recently from 8f36706 to b6fcc50 Compare October 12, 2024 15:04

RalfJung reviewed Oct 12, 2024

View reviewed changes

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Oct 12, 2024

Mandragorian force-pushed the coverage-report branch 3 times, most recently from 682a86b to 5b32bef Compare October 13, 2024 19:09

RalfJung reviewed Oct 13, 2024

View reviewed changes

Mandragorian force-pushed the coverage-report branch 3 times, most recently from 4aa0f4d to 309459f Compare October 13, 2024 21:14

Mandragorian marked this pull request as ready for review October 13, 2024 21:14

rustbot removed the S-waiting-on-author Status: Waiting for the PR author to address review comments label Oct 13, 2024

Mandragorian force-pushed the coverage-report branch from 0247e83 to 609d897 Compare October 23, 2024 16:20

RalfJung reviewed Oct 25, 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 Oct 25, 2024

Mandragorian force-pushed the coverage-report branch 3 times, most recently from 4df23a8 to 6688670 Compare October 26, 2024 15:53

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

RalfJung reviewed Oct 27, 2024

View reviewed changes

miri-script/src/coverage.rs Outdated Show resolved Hide resolved

.github/workflows/ci.yml Outdated Show resolved Hide resolved

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 Oct 27, 2024

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

Add option for generating coverage reports

45ea681

Add a `--coverage` option in the `test` subcommand of the miri script. This option, when set, will generate a coverage report after running the tests. `cargo-binutils` is needed as a dependency to generate the reports.

Mandragorian force-pushed the coverage-report branch from f2acadc to 45ea681 Compare October 27, 2024 23:03

RalfJung added this pull request to the merge queue Oct 28, 2024

Merged via the queue into rust-lang:master with commit 3e8619a Oct 28, 2024
7 checks passed

		let home = std::env::var("HOME")?;
		let ignored = format!("{home}/\|miri/target/\|target/debug/\|rust/deps/");

		let rustflags = e.sh.var("RUSTFLAGS")?;
		let rustflags = format!("{rustflags} -C instrument-coverage");

Add option for generating coverage reports #3954

Add option for generating coverage reports #3954

Conversation

Mandragorian commented Oct 9, 2024

RalfJung commented Oct 9, 2024 • edited Loading

oli-obk commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mandragorian Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mandragorian Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mandragorian commented Oct 9, 2024

RalfJung commented Oct 10, 2024 via email

RalfJung commented Oct 10, 2024 via email

RalfJung commented Oct 10, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mandragorian commented Oct 12, 2024 • edited Loading

RalfJung commented Oct 12, 2024

RalfJung Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mandragorian commented Oct 13, 2024

RalfJung commented Oct 21, 2024

bors commented Oct 22, 2024

Choose a reason for hiding this comment

RalfJung commented Oct 25, 2024

bors commented Oct 25, 2024

Mandragorian commented Oct 26, 2024

RalfJung commented Oct 27, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Oct 27, 2024

Mandragorian commented Oct 27, 2024

RalfJung commented Oct 27, 2024

Mandragorian commented Oct 27, 2024

RalfJung commented Oct 28, 2024

RalfJung commented Oct 9, 2024 •

edited

Loading

Mandragorian Oct 10, 2024 •

edited

Loading

saethlin Oct 10, 2024 •

edited

Loading

Mandragorian Oct 10, 2024 •

edited

Loading

Mandragorian commented Oct 12, 2024 •

edited

Loading

RalfJung Oct 12, 2024 •

edited

Loading