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.

Sign up for GitHub

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

Jump to bottom

(DRAFT) Combined draft branch for coverage work #115428

Closed
wants to merge 13 commits into from
Closed

(DRAFT) Combined draft branch for coverage work #115428

wants to merge 13 commits into from

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Sep 1, 2023 •
edited
Loading

While waiting for #114843, juggling my patch queue is getting to be quite unwieldy, so I'm going to try putting all of my pending PRs (including some not filed yet) and some of my in-progress work in one linear branch.

r? @ghost

@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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 1, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 1, 2023

@rustbot label +A-code-coverage +S-experimental

@rustbot rustbot added A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Sep 1, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 1, 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 1, 2023
@Zalathar Zalathar force-pushed the coverage branch 2 times, most recently from d86423d to 40bd399 Compare September 4, 2023 02:02
@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 4, 2023 •
edited
Loading

Quick summary of some of the changes I have queued up:

  • Add the new coverage-map test suite
  • Get rid of expression renumbering in coverage codegen
  • Simplify the coverageinfo query to just one pass
  • Change StatementKind::Coverage to store a vector of code regions, instead of just 0-1
  • Rename Operand to CovTerm so that it can represent things that aren't operands
    • LLVM itself doesn't have a good coherent name for this concept (despite using it a lot), so I had to get a bit creative to come up with a name that I was reasonably happy with
    • I chose “term” because it's similar to “expression”, while not conflicting with the related-but-distinct concept of a coverage expression
    • I added the Cov prefix to avoid potential confusion with rustc_middle::ty::Term, especially for reviewers
      • This was one of the pieces of feedback I got for Operand, which can be confused with the otherwise unrelated mir::Operand
  • Extract a GlobalFileTable helper in mapgen.rs
  • Minor improvements to write_coverage_mapping, and rename it to encode_mappings_for_function
  • Change FunctionCoverage to store all of its mappings as FxHashMap<Symbol, Vec<(CovTerm, CodeRegion)>>
    • Mappings are grouped by filename (since this needs to happen anyway), then stored as a list of kind/region pairs
    • Currently just reusing CovTerm as the mapping kind (since we only emit Code mappings), but eventually this will need to be replaced with a MappingKind enum that can represent expansion/gap/skip/branch mappings as well
    • This is an important step towards being able to introduce a CoverageKind::Mappings, which is itself an intermediate step towards storing coverage mappings in a per-function table in mir::Body

@bors
Copy link
Contributor

bors commented Sep 5, 2023

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

@Zalathar Zalathar force-pushed the coverage branch 3 times, most recently from 1b1d270 to efae00c Compare September 7, 2023 12:57
@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 7, 2023

@rustbot label -A-testsuite -T-bootstrap

@rustbot rustbot removed A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 7, 2023
@Zalathar Zalathar force-pushed the coverage branch 2 times, most recently from 56a3e06 to 27dbb3c Compare September 10, 2023 03:19
@bors
Copy link
Contributor

bors commented Sep 12, 2023

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

@Zalathar Zalathar force-pushed the coverage branch 7 times, most recently from f4a5170 to 741330a Compare September 13, 2023 11:59
@Zalathar Zalathar force-pushed the coverage branch 11 times, most recently from 51a3e7c to 1f016d8 Compare October 19, 2023 10:30
@bors
Copy link
Contributor

bors commented Oct 19, 2023

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

@Zalathar
coverage: Emit mappings for unused functions without generating stubs
7a4ec81
@Zalathar
coverage: Move unused-function helpers closer to where they are used
6b2e80c
@Zalathar
coverage: Change query codegened_and_inlined_items to a plain function
15ffe3d 
This query has a name that sounds general-purpose, but in fact it has
coverage-specific semantics, and (fortunately) is only used by coverage code.

Because it is only ever called once, it doesn't need to be a query, and we can
change it to a regular function instead.
@Zalathar
coverage: Force -Copt-level=2 in the unreachable-code test
11aeb69 
This test is mainly for detecting problems triggered by MIR optimizations, but
the run-coverage tests don't enable optimization by default.

(The coverage-map tests do enable optimization by default, but I'm updating the
test anyway so that the two `unreachable.rs` files don't drift out of sync.)
@Zalathar
coverage: Repair instrumented functions that have lost all their coun…
9886ac6 
…ters

If a function has been instrumented for coverage, but MIR optimizations
subsequently remove all of its counter-increment statements, then we won't emit
LLVM counter-increment intrinsics. LLVM will think the function is not
instrumented, and it will disappear from coverage mappings and coverage
reports.

This new MIR pass detects when that has happened, and re-inserts a dummy
counter-increment statement so that LLVM knows to treat the function as
instrumented.
@Zalathar
Allow MIR pass UnreachableProp when coverage is enabled
ea929a5
@Zalathar
coverage: Split FunctionCoverage into distinct collector/finished p…
81b4ab7 
…hases
@Zalathar
coverage: Fetch expressions and mappings separately
c4f5e60
@Zalathar
coverage: Build the global file table ahead of time
b8fc5a0
@Zalathar
coverage: Encapsulate local-to-global file mappings
63ecc25
@Zalathar
coverage: Encode function mappings without re-sorting them
340b7d1
@Zalathar
coverage: Emit the filenames section before encoding per-function map…
639d00c 
…pings

Most coverage metadata is encoded into two sections in the final executable.
The `__llvm_covmap` section mostly just contains a list of filenames, while the
`__llvm_covfun` section contains encoded coverage maps for each instrumented
function.

The catch is that each per-function record also needs to contain a hash of the
filenames list that it refers to. Historically this was handled by assembling
most of the per-function data into a temporary list, then assembling the
filenames buffer, then using the filenames hash to emit the per-function data,
and then finally emitting the filenames table itself.

However, now that we build the filenames table up-front (via a separate
traversal of the per-function data), we can hash and emit that part first, and
then emit each of the per-function records immediately after building. This
removes the awkwardness of having to temporarily store nearly-complete
per-function records.
@Zalathar
coverage: Consolidate creation of __llvm_covmap records
db35232 
The code for creating these records was spread across multiple functions and
multiple files, so this change moves as much of it as possible into one place.
@bors
Copy link
Contributor

bors commented Oct 21, 2023

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

@Zalathar Zalathar closed this Oct 29, 2023
@Zalathar Zalathar deleted the coverage branch October 29, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Zalathar @bors @rust-log-analyzer @rustbot