Skip to content

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

Merged
bors merged 3 commits intorust-lang:masterfrom
Zalathar:test-coverage-map
Sep 5, 2023
Merged

coverage: Explicitly test the coverage maps produced by codegen/LLVM#114843
bors merged 3 commits intorust-lang:masterfrom
Zalathar:test-coverage-map

Conversation

@Zalathar
Copy link
Member

@Zalathar Zalathar commented Aug 15, 2023 •
edited
Loading

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
Member Author

Zalathar commented Aug 15, 2023

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.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Zalathar

This comment was marked as resolved.

Sign in to view
Zalathar
Zalathar commented Aug 15, 2023
View reviewed changes
src/tools/coverage-dump/Cargo.toml Outdated
Comment on lines 8 to 14
Copy link
Member Author

@Zalathar Zalathar Aug 15, 2023

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.

Swatinem
Swatinem reviewed Aug 15, 2023
View reviewed changes
@Zalathar Zalathar force-pushed the test-coverage-map branch 2 times, most recently from 6c19230 to 5a5511c Compare August 16, 2023 01:59
Zalathar
Zalathar commented Aug 16, 2023
View reviewed changes
Cargo.lock Outdated
Comment on lines 2024 to 2061
Copy link
Member Author

@Zalathar Zalathar Aug 16, 2023

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.)

Swatinem
Swatinem approved these changes Aug 16, 2023
View reviewed changes
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 Zalathar mentioned this pull request Aug 17, 2023
Draft
@Zalathar
Copy link
Member Author

Zalathar commented Aug 17, 2023

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 Zalathar force-pushed the test-coverage-map branch 3 times, most recently from db419f6 to 307d599 Compare August 20, 2023 02:02
@Zalathar Zalathar mentioned this pull request Aug 20, 2023
Merged
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2023
@ehuss
Copy link
Contributor

ehuss commented Sep 4, 2023

Looks like a legitimate error for this PR this time.

@Zalathar
Copy link
Member Author

Zalathar commented Sep 4, 2023

This looks like a genuine test failure, so presumably the CI problem is fixed now.

@Zalathar
Copy link
Member Author

Zalathar commented Sep 5, 2023

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2023
@Zalathar
Copy link
Member Author

Zalathar commented Sep 5, 2023 •
edited
Loading

OK, I think what's happening here is that the snapshots assume -O, and then fail under a configuration with --disable-optimize-tests.

So for now I should probably enable an optimization level explicitly in runtest.rs, instead of inappropriately relying on the optimize-tests setting.

@Zalathar
Add test suite coverage-map to test coverage mappings emitted by LLVM
004db47 
We compile each test file to LLVM IR assembly, and then pass that IR to a
dedicated program that can decode LLVM coverage maps and print them in a more
human-readable format. We can then check that output against known-good
snapshots.

This test suite has some advantages over the existing `run-coverage` tests:

- We can test coverage instrumentation without needing to run target binaries.

- We can observe subtle improvements/regressions in the underlying coverage
mappings that don't make a visible difference to coverage reports.
@Zalathar
Copy most of tests/run-coverage into tests/coverage-map/status-quo
3141177 
The output of these tests is too complicated to comfortably verify by hand, but
we can still use them to observe changes to the underlying mappings produced by
codegen/LLVM.

If these tests fail due to non-coverage changes (e.g. in HIR-to-MIR lowering or
MIR optimizations), it should usually be OK to just `--bless` them, as long as
the `run-coverage` test suite still works.
@Zalathar
Copy link
Member Author

Zalathar commented Sep 5, 2023

Changes since last review:

  • coverage-map tests are now unaffected by the --optimize-tests flag, and instead explicitly specify -Copt-level=2 by default
  • Made the test suite readme easier to find, and made it more explicit about the fact that non-coverage developers should feel free to just --bless in most cases

@Zalathar
Copy link
Member Author

Zalathar commented Sep 5, 2023

I've confirmed that with these changes, the new test suite now passes on x86_64-gnu-nopt.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2023
@Zalathar
Copy link
Member Author

Zalathar commented Sep 5, 2023

(Oh, and presumably this doesn't need to be p=101 any more; that's just left over from this PR being used to check whether CI was still broken.)

@oli-obk
Copy link
Contributor

oli-obk commented Sep 5, 2023

@bors r+ p=0

@bors
Copy link
Collaborator

bors commented Sep 5, 2023

📌 Commit 3141177 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 5, 2023

⌛ Testing commit 3141177 with merge 8d14f2b0f8bb5a9ebb73ab19c480ec321215fc7a...

@bors
Copy link
Collaborator

bors commented Sep 5, 2023

💔 Test failed - checks-actions

@rust-log-analyzer
Copy link
Collaborator

rust-log-analyzer commented Sep 5, 2023

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:f43a0e5ff2bd294095638e18286ca9a3d1956744)
##[error]Can't use 'tar -xzf' extract archive file: /gha/_work/_actions/_temp_cb0ec03c-e231-49e7-83cf-f96d92af72a5/03bad07f-21fc-4edc-add3-8052522c39b5.tar.gz. return code: 2.

@Zalathar Zalathar mentioned this pull request Sep 5, 2023
Merged
@Zalathar
Copy link
Member Author

Zalathar commented Sep 5, 2023

Looks like CI flaked out again on that run.

@Zalathar
Copy link
Member 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
Copy link
Collaborator

bors commented Sep 5, 2023

⌛ Testing commit 3141177 with merge ab45885...

@bors
Copy link
Collaborator

bors commented Sep 5, 2023

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

@rust-timer
Copy link
Collaborator

rust-timer commented Sep 5, 2023

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 mentioned this pull request May 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obk oli-obk oli-obk left review comments

+1 more reviewer

@Swatinem Swatinem Swatinem approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@oli-obk oli-obk

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

Milestone

1.74.0

Development

Successfully merging this pull request may close these issues.

9 participants

@Zalathar @rustbot @rust-log-analyzer @bors @oli-obk @Swatinem @ehuss @rust-timer @compiler-errors