Working branch-level code coverage #77080

richkadel · 2020-09-22T22:41:47Z

Add a generalized implementation for computing branch-level coverage spans.

This iteration resolves some of the challenges I had identified a few weeks ago.

I've tried to implement a solution that is general enough to work for a lot of different graphs/patterns. It's encouraging to see the results on fairly large and complex crates seem to meet my expectations. This may be a "functionally complete" implementation.

Except for bug fixes or edge cases I haven't run into yet, the next and essentially final step, I think, is to replace some Counters with CounterExpressions (where their counter values can be computed by adding or subtracting other counters/expressions).

Examples of branch-level coverage support enabled in this PR:

Examples of coverage analysis results (MIR spanview files) used to inject counters in the right BasicBlocks:

Here is some sample coverage output after compiling a few real-world crates with the new branch-level coverage features:

r? @tmandry
FYI: @wesleywiser

compiler/rustc_data_structures/src/graph/dominators/mod.rs

richkadel · 2020-09-23T17:10:32Z

🦈 we're gonna need a bigger tooltip 🦈

Using spanview is great for understanding how the coverage algorithm is determined and the content of the BasicCoverageBlocks, but in some cases they can get a bit long. Native tooltips have limits (perhaps browser dependent, a maximum number of lines or characters or something), so unless there's a workaround for those limits, I may have to convert to using some JavaScript and use a DOM-based tooltip.

I liked having an implementation that avoided JavaScript, but considering the DOM features are fairly advanced already, some JavaScript is probably not a major ask.

The downside is, given I usually view these inside VSCode panels, the tooltips would often extend beyond the frame of the HTML preview, and native tooltips have no problem with overlapping the frame (displaying above the VSCode editor window).

JavaScript tooltips are going to get cutoff (require scrolling or growing the pane).

Tradeoffs.

richkadel · 2020-09-24T05:52:04Z

@tmandry @wesleywiser -

Most of the changed files are auto-generated test results (using --bless).

You may be interested in seeing the actual coverage results, and a good place to look is at the .txt files produced by llvm-cov show:

https://github.com/rust-lang/rust/pull/77080/files#diff-6502ae3b67c4afbb171d2c9525127e69

These results look right to me.

You can also browse, hover, and click on the rendered spanview files by opening the github location with the additional prefix (in front of the entire URL) https://htmlpreview.github.io/?.

For example:

https://htmlpreview.github.io/?https://github.com/rust-lang/rust/blob/93fb99282c5aa2805451b0400c667b91bec4cb78/src/test/run-make-fulldeps/instrument-coverage-mir-cov-html-base/expected_mir_dump.coverage_of_simple_loop/coverage_of_simple_loop.main.-------.InstrumentCoverage.0.html

(If there's an easier way to view HTML files from github, let me know.)

richkadel · 2020-09-24T17:54:22Z

New commit corrects for a couple of problems I just noticed in the spanview test output:

Corrected a fairly recent assumption in runtest.rs that all MIR dump
files end in .mir. (It was appending .mir to the graphviz .dot and
spanview .html file names when generating blessed output files. That
also left outdated files in the baseline alongside the files with the
incorrect names, which I've now removed.)

Updated spanview HTML title elements to match their content, replacing a
hardcoded and incorrect name that was left in accidentally when
originally submitted.

richkadel · 2020-09-25T04:38:17Z

Ready for review (no longer "draft/WIP").

Last commit:

added more test examples

also improved Makefiles with support for non-zero exit status and to
force validation of tests unless a specific test overrides it with a
specific comment.

@tmandry @wesleywiser - I added a few more examples. The results look really good. Take a look at the coverage results in

src/test/run-make-fulldeps/instrument-coverage-cov-reports-link-dead-code/typical_*

And the coverage spanview files for reference.

Hopefully this gives you confidence as well.

I know we can add more tests, and I'll work on those.

Other than that, I believe the next (perhaps last) thing needed is to replace some counters with expressions, where they can be computed.

Thanks!
Rich

compiler/rustc_data_structures/src/graph/dominators/mod.rs

compiler/rustc_middle/src/mir/coverage/mod.rs

compiler/rustc_mir/src/transform/instrument_coverage.rs

richkadel · 2020-09-25T21:16:09Z

I compiled json5format and its dependencies with branch-level coverage. There were some unhandled edge cases, which I fixed, and I was able to successfully compile, run, and show the coverage. I'm going to drop some example output into the PR comment.

richkadel · 2020-09-25T21:27:30Z

Note to those following:

Rust compiler MCP rust-lang/compiler-team#278
Relevant issue: #34701 - Implement support for LLVMs code coverage instrumentation

This PR implements branch-level coverage. Support for LLVM code coverage is nearly complete.

See some examples in the main PR comment.

The one known remaining feature I'll be implementing next is to replace some counters with counter expressions, where possible.

richkadel · 2020-09-26T03:34:41Z

@tmandry - I made updates based on your feedback, or in some cases, I replied to your comments here in the PR. Please take a look and see if this answers your questions.

Let me know if these answers resolve your feedback/questions.

Thanks!

richkadel · 2020-09-26T17:05:15Z

[UPDATE: This is fixed. See the comment later in this thread.]

I wanted to highlight one problem that I did notice when compiling the json5format crate with coverage. I thought it was wrong, but actually it turns at to be "right" (from a MIR-coverage perspective). It's just problematic when trying to interpret the coverage analysis with llvm-cov show:

If you look at the closure alone, the coverage seems to be wrong. It's showing 1 for line coverage, but all of the coverage spans show ^0 (and are highlighted in red.

This closure was not executed.

But the method unwrap_or_else(...) was called, and that function has a span that starts at self.get_matches_from_safe_borrow(iter).unwrap_or_else( and ends at the ) after the closure body!

So the line coverage of 1 is counting the execution of that outer function call, not the execution of the closure.

The coverage algorithm processes one MIR (function/closure) at a time, so even though the coverage algorithm deconflicts spans within a MIR, it doesn't have the context (I guess) to know to deconflict spans across different MIRs. (Or maybe it does, ...still thinking about that.)

Another possible solution, if not at the MIR coverage generation level, would be to resolve this at the module level during coverage map generation.

I think this is OK for this PR, and it is still technically correct, but I'll think about how to fix it and try to do something better in a follow-up PR, if the reviewers are OK with that.

richkadel · 2020-09-26T17:07:26Z

(It is odd that some lines have 0 for coverage. That has to do with how llvm-cov is deciding to count lines, based on the overlapping spans it has.)

richkadel · 2020-09-27T17:59:25Z

Update on the closure issue. I believe I have a fairly sensible solution. I'm testing it now and will update this PR.

richkadel · 2020-09-28T00:13:59Z

closure span handling is fixed! (See changes in 6ec5b4a)

I added two new test cases, one for closures (to debug and resolve that problem), and one for inner items (including inner functions). The inner items worked fine without the new changes, but I hadn't tested that and didn't know until I did.

richkadel · 2020-09-28T00:33:51Z

@tmandry @wesleywiser -

I had an idea for improving test coverage (for a future PR). Let me know your thoughts.

The tests in https://github.com/richkadel/rust/tree/llvm-coverage-counters-2/src/test/run-make-fulldeps/ for instrument-coverage-mir-cov-html-* and instrument-coverage-cov-reports-* are good for validating the coverage spans are where we expect them, and the resulting coverage numbers are what we expect, and those tests share the same single set of Rust test program sources, at:

https://github.com/richkadel/rust/tree/llvm-coverage-counters-2/src/test/run-make-fulldeps/instrument-coverage/

By contrast, the mir-opt test is still quite primitive. It doesn't even have Counter Expressions.

I think it's a good idea to validate the MIR results from the InstrumentCoverage pass for the same kinds of Rust code situations tested by the run-make-fulldeps tests, but I don't want to copy and maintain two sets of test programs for the same test code.

It should be easy for me to change the run-make-fulldeps tests to use test sources from src/test/mir-opt/instrument-coverage/ (moving the existing tests from src/test/run-make-fulldeps/instrument-coverage/) so they can be used for both test suites.

The Makefiles already pull the sources from a different source directory anyway.

Thoughts?

richkadel · 2020-09-28T17:40:07Z

Sorry, but one more new test. I was wondering if the latest changes would still work for structs/functions with parameterized types, and it appears they do still work:

https://github.com/richkadel/rust/blob/llvm-coverage-counters-2/src/test/run-make-fulldeps/instrument-coverage-cov-reports-base/expected_show_coverage.coverage_of_generics.txt

richkadel · 2020-09-28T17:50:51Z

Note: Using this PR's rustc build, I compiled the json5format crate (including all of its dev dependencies) with and without -Z instrument-coverage.

Overall cargo build time (after clean) took around 74% more time to compile, and the target directory increased in size by 92%.

This is hardly scientific, and there may be outliers in this particular test. (The proptest compile stage seemed to take a very long portion of the compile time, for instance, but it's not clear if that's due to unusual complexities in proptest, or how it's used, or where it falls in the sequence of build stages.)

And the size increase is worth exploring at a more granular level as well.

But at least this gives us some idea of the order of magnitude of impact.

richkadel · 2020-09-28T18:05:19Z

There should be some reduction in binary size after I replace some counters with expressions, but that will also add some time to the compiling. I think the bigger impact of expressions will be binary execution time (which I haven't performance-tested here).

I'd love to find ways to reduce the compile-time impact. I don't know of any low-hanging fruit to impact that, off the top of my head. Maybe reviewers will see something. Performance profiling of the instrumentation MIR pass and coverage map generation, will be good to do eventually. I'm also not sure how much LLVM's codegen implementation of this is adding. (I'm curious how these numbers stack up against Clang.)

richkadel · 2020-09-28T20:25:33Z

Update: I believe I've addressed all feedback (to date) from @tmandry. @wesleywiser has a review in progress, so we want to wait for that before merging.

tmandry

The added comments help a lot, thanks!

compiler/rustc_mir/src/transform/instrument_coverage.rs

tmandry · 2020-09-28T23:55:57Z

compiler/rustc_mir/src/transform/instrument_coverage.rs

@@ -787,7 +793,11 @@ fn filtered_statement_span(statement: &'a Statement<'tcx>, body_span: Span) -> O
        // These statements have spans that are often outside the scope of the executed source code


General comment on this code: I think the ideal we'd like to work toward is not having special handling for each StatementKind, i.e. this feels like a workaround for issues in the SourceInfo that could maybe be fixed elsewhere.

@tmandry @wesleywiser - I don't think this is practical at the current time. There are very few special cases now. (There used to be more, but I was able to remove most of them.) The special cases that are left are (for the most part) necessary right now.

I think the only way to avoid this in the long term would require removing the spans from StorageLive and StorageDead (which I doubt is feasible). But that's just moving the special case handling outside of the InstrumentCoverage pass.

From a coverage perspective, these statements are problematic because their spans refer to the variable declarations, which are quite often in a completely different area in the source from where they actually need to come alive or die.

(Control Flow vs. Data Flow)

As for the other "special cases", I could remove Coverage and Nop. They are just included for completeness, but should not crop up here anyway.

The ForGuardBinding special case is definitely something I would like to remove, but there's already a FIXME for that. That's there because there is a bug somewhere else in the MIR code (I guess) producing an invalid Span.

(I'm actually amazed that these are the only special cases. It's way better than I expected, and I'm impressed that the MIR generally stays pretty true to the original source code.)

compiler/rustc_mir/src/transform/instrument_coverage.rs

tmandry · 2020-09-29T01:37:50Z

compiler/rustc_mir/src/transform/instrument_coverage.rs

+                let right_cutoff = curr.span.hi();
+                let has_pre_closure_span = prev.span.lo() < right_cutoff;
+                let has_post_closure_span = prev.span.hi() > right_cutoff;
+                if has_pre_closure_span {


I noticed that right around here my eyes start glossing over a bit. If you have any ideas for how to simplify this nested logic tree, I think it would help. Maybe extracting helper functions so it's all high level code.

I've been refactoring this today. Update coming shortly. Thanks.

Done.

@tmandry - I refactored the coverage_span() logic into a new struct/impl CoverageSpanRefinery. Let me know if this works for you.

tmandry · 2020-09-29T01:42:26Z

For the performance concerns, we could add either a representative benchmark or just a benchmarking mode with -Zinject-coverage to rustc-perf, so we can track improvements over time.

I'm not sure how keen people are on adding a new mode, cc @Mark-Simulacrum

richkadel · 2020-10-05T16:40:46Z

@bors r=tmandry

bors · 2020-10-05T16:40:48Z

📌 Commit 6f62766 has been approved by tmandry

richkadel · 2020-10-05T16:41:00Z

(shoot... forgot to git add the fixed format)

bors · 2020-10-05T16:41:14Z

⌛ Testing commit 6f62766 with merge c04e76bea9a378b482f845ef87e11f2419be7480...

bors · 2020-10-05T17:15:53Z

💔 Test failed - checks-actions

richkadel · 2020-10-05T17:17:38Z

Spurious network error...


warning: spurious network error (2 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/parking_lot_core/0.8.0/download`, got 502
warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/parking_lot_core/0.8.0/download`, got 502
error: failed to download from `https://crates.io/api/v1/crates/parking_lot_core/0.8.0/download`

Caused by:
  failed to get 200 response from `https://crates.io/api/v1/crates/parking_lot_core/0.8.0/download`, got 502
command did not execute successfully: "\\\\?\\D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "build" "--target" "x86_64-pc-windows-msvc" "-Zbinary-dep-depinfo" "-j" "8" "--release" "--locked" "--color" "always" "--features" " llvm max_level_info" "--manifest-path" "D:\\a\\rust\\rust\\compiler/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
expected success, got: exit code: 101
failed to run: D:\a\rust\rust\build\bootstrap\debug\bootstrap --stage 2 test src/tools/cargotest src/tools/cargo
Build completed unsuccessfully in 0:05:23
== clock drift check ==
  local time: Mon Oct  5 16:52:43 CUT 2020
  network time: Mon, 05 Oct 2020 16:52:43 GMT
== end clock drift check ==

richkadel · 2020-10-05T17:18:12Z

@bors r=tmandry

bors · 2020-10-05T17:18:14Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: Better sso structures #77171

bors · 2020-10-05T17:18:15Z

📌 Commit 6f62766 has been approved by tmandry

tmandry · 2020-10-05T17:19:05Z

@bors retry

bors · 2020-10-05T19:34:50Z

⌛ Testing commit 6f62766 with merge a1dfd24...

bors · 2020-10-05T21:49:53Z

☀️ Test successful - checks-actions, checks-azure
Approved by: tmandry
Pushing a1dfd24 to master...

Don't create a separate "basename" when naming and opening a MIR dump file These functions were split up by rust-lang#77080, in order to support passing the dump file's “basename” (filename without extension) to the implementation of `-Zdump-mir-spanview`, so that it could be used as a page title. That flag has since been removed (rust-lang#119566), so now there's no particular reason for this code to handle the basename separately from the filename or full path. This PR therefore restores things to (roughly) how they were before rust-lang#77080.

Rollup merge of rust-lang#120038 - Zalathar:dump-path, r=WaffleLapkin Don't create a separate "basename" when naming and opening a MIR dump file These functions were split up by rust-lang#77080, in order to support passing the dump file's “basename” (filename without extension) to the implementation of `-Zdump-mir-spanview`, so that it could be used as a page title. That flag has since been removed (rust-lang#119566), so now there's no particular reason for this code to handle the basename separately from the filename or full path. This PR therefore restores things to (roughly) how they were before rust-lang#77080.

rust-highfive assigned tmandry Sep 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2020

richkadel marked this pull request as draft September 22, 2020 22:42

richkadel commented Sep 22, 2020

View reviewed changes

compiler/rustc_data_structures/src/graph/dominators/mod.rs Show resolved Hide resolved

richkadel marked this pull request as ready for review September 25, 2020 04:33

richkadel changed the title ~~WIP - Updates to experimental coverage counter injection~~ Updates to experimental coverage counter injection Sep 25, 2020

tmandry reviewed Sep 25, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-counters-2 branch from 7c1a040 to 60a480d Compare September 25, 2020 21:14

richkadel changed the title ~~Updates to experimental coverage counter injection~~ Working branch-level code coverage Sep 25, 2020

richkadel force-pushed the llvm-coverage-counters-2 branch 2 times, most recently from 5d87d4f to 9f153d1 Compare September 26, 2020 05:34

tmandry reviewed Sep 29, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-counters-2 branch from a8233f9 to 6f62766 Compare October 5, 2020 16:40

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 5, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 5, 2020

bors merged commit a1dfd24 into rust-lang:master Oct 5, 2020

rustbot added this to the 1.49.0 milestone Oct 5, 2020

jonas-schievink mentioned this pull request Oct 6, 2020

Rollup of 11 pull requests #77589

Closed

richkadel mentioned this pull request Oct 6, 2020

AArch64 Linux CI failure #77615

Closed

klensy mentioned this pull request Apr 12, 2021

Wrong line numbers produces in rustc_span::SourceFile::lookup_file_pos* #84127

Open

Skepfyr mentioned this pull request Jun 14, 2021

Compile error: static lifetime not satisfied but it is #86172

Open

Zalathar mentioned this pull request Jan 17, 2024

Don't create a separate "basename" when naming and opening a MIR dump file #120038

Merged

		@@ -787,7 +793,11 @@ fn filtered_statement_span(statement: &'a Statement<'tcx>, body_span: Span) -> O
		// These statements have spans that are often outside the scope of the executed source code

Working branch-level code coverage #77080

Working branch-level code coverage #77080

Conversation

richkadel commented Sep 22, 2020 • edited Loading

richkadel commented Sep 23, 2020 • edited Loading

richkadel commented Sep 24, 2020 • edited Loading

richkadel commented Sep 24, 2020

richkadel commented Sep 25, 2020

richkadel commented Sep 25, 2020

richkadel commented Sep 25, 2020

richkadel commented Sep 26, 2020

richkadel commented Sep 26, 2020 • edited Loading

richkadel commented Sep 26, 2020

richkadel commented Sep 27, 2020

richkadel commented Sep 28, 2020

richkadel commented Sep 28, 2020

richkadel commented Sep 28, 2020

richkadel commented Sep 28, 2020

richkadel commented Sep 28, 2020 • edited Loading

richkadel commented Sep 28, 2020

tmandry left a comment

Choose a reason for hiding this comment

tmandry Sep 28, 2020

Choose a reason for hiding this comment

richkadel Sep 29, 2020

Choose a reason for hiding this comment

richkadel Sep 29, 2020

Choose a reason for hiding this comment

tmandry Sep 29, 2020

Choose a reason for hiding this comment

richkadel Sep 30, 2020

Choose a reason for hiding this comment

richkadel Sep 30, 2020

Choose a reason for hiding this comment

tmandry commented Sep 29, 2020

richkadel commented Oct 5, 2020

bors commented Oct 5, 2020

richkadel commented Oct 5, 2020

bors commented Oct 5, 2020

bors commented Oct 5, 2020

richkadel commented Oct 5, 2020

richkadel commented Oct 5, 2020

bors commented Oct 5, 2020

bors commented Oct 5, 2020

tmandry commented Oct 5, 2020

bors commented Oct 5, 2020

bors commented Oct 5, 2020

richkadel commented Sep 22, 2020 •

edited

Loading

richkadel commented Sep 23, 2020 •

edited

Loading

richkadel commented Sep 24, 2020 •

edited

Loading

richkadel commented Sep 26, 2020 •

edited

Loading

richkadel commented Sep 28, 2020 •

edited

Loading