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

coverage: Explicitly test the coverage maps produced by codegen/LLVM #114843

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Aug 15, 2023

Our existing coverage tests verify the output of end-to-end coverage reports, but we don't have any way to test the specific mapping information (code regions and their associated counters) that are emitted by rustc_codegen_llvm and LLVM. That makes it harder to to be confident in changes that would modify those mappings (whether deliberately or accidentally).

This PR addresses that by adding a new coverage-map test suite that does the following:

  • Compiles test files to LLVM IR assembly (.ll)
  • Feeds those IR files to a custom tool (src/tools/coverage-dump) that extracts and decodes coverage mappings, and prints them in a more human-readable format
  • Checks the output of that tool against known-good snapshots

I recommend excluding the last commit while reviewing the main changes, because that last commit is just ~40 test files copied over from tests/run-coverage, plus their blessed coverage-map snapshots and a readme file. Those snapshots aren't really intended to be checked by hand; they're mostly there to increase the chances that an unintended change to coverage maps will be observable (even if it requires relatively specific circumstances to manifest).

@rustbot
Copy link
Collaborator

rustbot commented Aug 15, 2023

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 15, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Aug 15, 2023
@Zalathar
Copy link
Contributor Author

I plan to use these tests to help ensure that the changes in #114399 don't regress/bloat the emitted coverage mappings.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Zalathar

This comment was marked as resolved.

Comment on lines 8 to 14
[dependencies]
anyhow = "1.0.71"
md5 = { package = "md-5" , version = "0.10.5" }
miniz_oxide = "0.7.1"
regex = "1.8.4"
rustc-demangle = "0.1.23"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these version numbers are just copied from whatever happened to be in Cargo.lock at the time, to avoid bumping the existing versions.

src/tools/coverage-dump/src/parser.rs Outdated Show resolved Hide resolved
tests/coverage-map/if.cov-map Outdated Show resolved Hide resolved
@Zalathar Zalathar force-pushed the test-coverage-map branch 2 times, most recently from 6c19230 to 5a5511c Compare August 16, 2023 01:59
Comment on lines +2024 to +2061
[[package]]
name = "leb128"
version = "0.2.5"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "884e2677b40cc8c339eaefcb701c32ef1fd2493d71118dc0ca4b6a736c93bd67"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, leb128 is maintained and published by the same org as gimli, under the same Apache2/MIT license.

(gimli previously used leb128 as a dependency, but nowadays it uses its own slightly-modified copy in a submodule.)

Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

🎉 its a lot more readable like this :-)

@Zalathar Zalathar force-pushed the test-coverage-map branch 3 times, most recently from 2d68e55 to 33f3722 Compare August 17, 2023 02:10
@Zalathar
Copy link
Contributor Author

I decided to remove #![deny(warnings)] from the new tests, because I realised that it would be really annoying for someone else to have their CI run fail just because of a new warning.

@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 5, 2023

Looks like CI flaked out again on that run.

@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 5, 2023

Also note that this and #114656 are racing in silent conflict; whichever one lands first will require the other to update #[no_coverage] in this PR’s tests.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 5, 2023

@bors retry

@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 5, 2023
@bors
Copy link
Contributor

bors commented Sep 5, 2023

⌛ Testing commit 3141177 with merge ab45885...

@bors
Copy link
Contributor

bors commented Sep 5, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing ab45885 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2023
@bors bors merged commit ab45885 into rust-lang:master Sep 5, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 5, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ab45885): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.0% [-2.5%, -1.0%] 6
Improvements ✅
(secondary)
-1.7% [-1.7%, -1.7%] 1
All ❌✅ (primary) -2.0% [-2.5%, -1.0%] 6

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 628.171s -> 626.58s (-0.25%)
Artifact size: 316.34 MiB -> 316.27 MiB (-0.02%)

@Zalathar Zalathar deleted the test-coverage-map branch September 6, 2023 00:37
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2023
coverage: Don't bother renumbering expressions on the Rust side

The LLVM API that we use to encode coverage mappings already has its own code for removing unused coverage expressions and renumbering the rest.

This lets us get rid of our own complex renumbering code, making it easier to change our coverage code in other ways.

---

Now that we have tests for coverage mappings (rust-lang#114843), I've been able to verify that this PR doesn't make the coverage mappings worse, thanks to an explicit simplification step.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 29, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 29, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 29, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 30, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 1, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 1, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 2, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 3, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 5, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 5, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 7, 2023
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
fmease added a commit to fmease/rust that referenced this pull request May 30, 2024
compiletest: Unify `cmd2procres` with `run_command_to_procres`

Historical context: I originally added `run_command_to_procres` in rust-lang#112300 and rust-lang#114843, because I didn't like the overly-specific failure message in `cmd2procres`, but at the time I didn't feel confident enough to change the existing code, so I just added my own similar code.

Now I'm going back to remove this redundancy by eliminating `cmd2procress`, and adjusting all callers to use a slightly-tweaked `run_command_to_procres` instead.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 30, 2024
compiletest: Unify `cmd2procres` with `run_command_to_procres`

Historical context: I originally added `run_command_to_procres` in rust-lang#112300 and rust-lang#114843, because I didn't like the overly-specific failure message in `cmd2procres`, but at the time I didn't feel confident enough to change the existing code, so I just added my own similar code.

Now I'm going back to remove this redundancy by eliminating `cmd2procress`, and adjusting all callers to use a slightly-tweaked `run_command_to_procres` instead.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2024
Rollup merge of rust-lang#125753 - Zalathar:procres, r=jieyouxu

compiletest: Unify `cmd2procres` with `run_command_to_procres`

Historical context: I originally added `run_command_to_procres` in rust-lang#112300 and rust-lang#114843, because I didn't like the overly-specific failure message in `cmd2procres`, but at the time I didn't feel confident enough to change the existing code, so I just added my own similar code.

Now I'm going back to remove this redundancy by eliminating `cmd2procress`, and adjusting all callers to use a slightly-tweaked `run_command_to_procres` instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-testsuite Area: The testsuite used to check the correctness of rustc 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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants