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

Tools, tests, and experimenting with MIR-derived coverage counters #76004

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

richkadel
Copy link
Contributor

@richkadel richkadel commented Aug 27, 2020

Leverages the new mir_dump output file in HTML+CSS (from #76074) to visualize coverage code regions
and the MIR features that they came from (including overlapping spans).

See example below.

The run-make-fulldeps/instrument-coverage test has been refactored to maximize test coverage and reduce code duplication. The new tests support testing with and without -Clink-dead-code, so Rust coverage can be tested on MSVC (which, currently, only works with link-dead-code disabled).

New tests validate coverage region generation and coverage reports with multiple counters per function. Starting with a simple if-else branch tests, coverage tests for each additional syntax type can be added by simply dropping in a new Rust sample program.

Includes a basic, MIR-block-based implementation of coverage injection,
available via -Zexperimental-coverage. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing -Zinstrument-coverage option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage
works as intended, the -Zexperimental-coverage option should be
removed.

This PR replaces the bulk of PR #75828, with the remaining parts of
that PR distributed among other separate and indentpent PRs.

This PR depends on two of those other PRs: #76002, #76003 and #76074

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: #34701 - Implement support for LLVMs code coverage
instrumentation

Screen-Recording-2020-08-21-at-2

r? @tmandry
FYI: @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2020
@richkadel
Copy link
Contributor Author

richkadel commented Aug 27, 2020

@tmandry @wesleywiser - Note there are 3 commits in this PR but two of them are cherry-picked from PRs: #76002 and #76003.

So check out the last commit for the changes specific to this PR.

Note that most of the files are in the refactored tests and test data. The actual source changes here are much smaller than the number of "files changed" might indicate.

@richkadel
Copy link
Contributor Author

I had to update my comments on this PR regarding the PRs this depends on. Sorry.

Actually, they are #76002 and #76003.

@tmandry
Copy link
Member

tmandry commented Aug 28, 2020

Is there a reason not to split the tooling out from the changes to coverage instrumentation?

The tests for the HTML have a lot of duplicated content. Can we test that output only once? We don't have to cover every case for that since it's not an end user thing (I think we have a single empty-main test for the graphviz output for instance; it mostly tests that the wiring for the feature is working).

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 28, 2020
…2, r=wesleywiser

Adds --bless support to test/run-make-fulldeps

The ability to "bless" output for some of these tests is critical to
making it practical to adapt tests to unrelated changes.

This is needed for new coverage tests, as shown in PR rust-lang#76004 .

r? @tmandry
FYI: @wesleywiser
@richkadel
Copy link
Contributor Author

Is there a reason not to split the tooling out from the changes to coverage instrumentation?

I'm looking into this. If there is something reasonable that I can split out, I'll do it. If I don't see a good way to split it up without consequences, I'll explain my thinking.

@richkadel
Copy link
Contributor Author

The tests for the HTML have a lot of duplicated content.

As mentioned in sidebar conversation, I realized that there were several testprog related expected and typical test results left over from when testprog was included in the common test sources in test/run-make-fulldeps/instrument_covarege/.

With --bless, the Makefile was rebuilding the expected/typical results for known test sources (there is only one right now: coverage_of_if_else.rs but the Makefile wasn't smart enough to delete old results from test sources no longer present.

I improved the Makefiles to wipe out expected results (on --bless) based on a wildcard instead of doing it for each known test, and this removed those "extra" results that are no longer used.

I went ahead and pushed the changes to this PR, so it no longer includes files that it shouldn't.

Copy link
Contributor Author

@richkadel richkadel left a comment

Choose a reason for hiding this comment

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

@tmandry said:

Is there a reason not to split the tooling out from the changes to coverage instrumentation?

Check out what I did with the new commit and see if this helps mitigate your concern.

It's possible to split these into separate commits, but the reason I didn't do it originally (which still holds true), is the refactored tests depend on changes to both InstrumentCoverage and the new file format.

src/librustc_session/options.rs Outdated Show resolved Hide resolved
@richkadel
Copy link
Contributor Author

It's possible to split these into separate commits

I decided to do that after all. See PR #76074, which has only the new spanview document generation capability (and API to be used by this PR).

PR #76074 also adds mir-opt tests for the feature that mirror the simple test for graphviz.

I'll update this PR to depend on PR #76074.

@richkadel richkadel force-pushed the llvm-coverage-map-gen-6b.5 branch 3 times, most recently from 50908e0 to 53945a3 Compare August 29, 2020 20:41
@bors
Copy link
Contributor

bors commented Aug 30, 2020

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

richkadel added a commit to richkadel/rust that referenced this pull request Sep 1, 2020
Similar to `-Z dump-mir-graphviz`, this adds the option to write
HTML+CSS files that allow users to analyze the spans associated with MIR
elements (by individual statement, just terminator, or overall basic
block).

This PR was split out from PR rust-lang#76004, and exposes an API for spanview
HTML+CSS files that is also used to analyze code regions chosen for
coverage instrumentation (in a follow-on PR).

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation
tmandry added a commit to tmandry/rust that referenced this pull request Sep 2, 2020
…5.1, r=wesleywiser

Add new `-Z dump-mir-spanview` option

Similar to `-Z dump-mir-graphviz`, this adds the option to write
HTML+CSS files that allow users to analyze the spans associated with MIR
elements (by individual statement, just terminator, or overall basic
block).

This PR was split out from PR rust-lang#76004, and exposes an API for spanview
HTML+CSS files that is also used to analyze code regions chosen for
coverage instrumentation (in a follow-on PR).

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation

r? @tmandry
FYI @wesleywiser
@richkadel
Copy link
Contributor Author

@tmandry @wesleywiser - rebased after the last dependency just landed.

This PR should be ready for bors.

Thanks!

Adds a new mir_dump output file in HTML/CSS to visualize code regions
and the MIR features that they came from (including overlapping spans).
See example below:

Includes a basic, MIR-block-based implementation of coverage injection,
available via `-Zexperimental-coverage`. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing `-Zinstrument-coverage` option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage
works as intended, the `-Zexperimental-coverage` option should be
removed.

This PR replaces the bulk of PR rust-lang#75828, with the remaining parts of
that PR distributed among other separate and indentpent PRs.

This PR depends on three of those other PRs: rust-lang#76000, rust-lang#76002, and

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation

![Screen-Recording-2020-08-21-at-2](https://user-images.githubusercontent.com/3827298/90972923-ff417880-e4d1-11ea-92bb-8713c6198f6d.gif)
@richkadel
Copy link
Contributor Author

@tmandry - I addressed your feedback. This should be good to go I believe.

@richkadel
Copy link
Contributor Author

@tmandry @wesleywiser - I have tested things locally, including with MSVC on Windows.

But since this is the first time running this new refactored test framework, including the special cases for MSVC, I recommend "rollup never".

@tmandry
Copy link
Member

tmandry commented Sep 3, 2020

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 3, 2020

📌 Commit 51d692c has been approved by tmandry

@bors 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 Sep 3, 2020
@bors
Copy link
Contributor

bors commented Sep 4, 2020

⌛ Testing commit 51d692c with merge 4ffb5c5...

@bors
Copy link
Contributor

bors commented Sep 4, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 4, 2020
@bors bors merged commit 4ffb5c5 into rust-lang:master Sep 4, 2020
@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants