coverage: Emit the filenames section before encoding per-function mappings #117042

Zalathar · 2023-10-22T07:30:23Z

When embedding coverage information in LLVM IR (and ultimately in the resulting binary), there are two main things that each CGU needs to emit:

A single __llvm_covmap record containing a coverage header, which mostly consists of a list of filenames used by the CGU's coverage mappings.
Several __llvm_covfun records, one for each instrumented function, each of which contains the hash of the list of filenames in the header.

There is a kind of loose cyclic dependency between the two: we need the hash of the file table before we can emit the covfun records, but we need to traverse all of the instrumented functions in order to build the file table.

The existing code works by processing the individual functions first. It lazily adds filenames to the file table, and stores the mostly-complete function records in a temporary list. After this it hashes the file table, emits the header (containing the file table), and then uses the hash to emit all of the function records.

This PR reverses that order: first we traverse all of the functions (without trying to prepare their function records) to build a complete file table, and then emit it immediately. At this point we have the file table hash, so we can then proceed to build and emit all of the function records, without needing to store them in an intermediate list.

Along the way, this PR makes some necessary changes that are also worthwhile in their own right:

We split FunctionCoverage into distinct collector/finished phases, which neatly avoids some borrow-checker hassles when extracting a function's final expression/mapping data.
We avoid having to re-sort a function's mappings when preparing the list of filenames that it uses.

rustbot · 2023-10-22T07:30:30Z

r? @davidtwco

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

rustbot · 2023-10-22T07:30:33Z

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.

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

Zalathar · 2023-10-22T08:24:57Z

@rustbot label +A-code-coverage

…hases This gives us a clearly-defined place to run code after the instance's MIR has been traversed by codegen, but before we emit its `__llvm_covfun` record.

The combined `get_expressions_and_counter_regions` method was an artifact of having to prepare the expressions and mappings at the same time, to avoid ownership/lifetime problems with temporary data used by both. Now that we have an explicit transition from `FunctionCoverageCollector` to the final `FunctionCoverage`, we can prepare any shared data during that step and store it in the final struct.

The main change here is that `VirtualFileMapping` now uses an internal hashmap to de-duplicate incoming global file IDs. That removes the need for `encode_mappings_for_function` to re-sort its mappings by filename in order to de-duplicate them. (We still de-duplicate runs of identical filenames to save work, but this is not load-bearing for correctness, so a sort is not necessary.)

Zalathar · 2023-10-22T09:48:07Z

Addressed review feedback (diff).

cjgillot · 2023-10-22T10:56:00Z

@bors r+

bors · 2023-10-22T10:56:02Z

📌 Commit c46abe4 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-10-22T11:05:03Z

⌛ Testing commit c46abe4 with merge 0611ae2...

coverage: Emit the filenames section before encoding per-function mappings When embedding coverage information in LLVM IR (and ultimately in the resulting binary), there are two main things that each CGU needs to emit: - A single `__llvm_covmap` record containing a coverage header, which mostly consists of a list of filenames used by the CGU's coverage mappings. - Several `__llvm_covfun` records, one for each instrumented function, each of which contains the hash of the list of filenames in the header. There is a kind of loose cyclic dependency between the two: we need the hash of the file table before we can emit the covfun records, but we need to traverse all of the instrumented functions in order to build the file table. The existing code works by processing the individual functions first. It lazily adds filenames to the file table, and stores the mostly-complete function records in a temporary list. After this it hashes the file table, emits the header (containing the file table), and then uses the hash to emit all of the function records. This PR reverses that order: first we traverse all of the functions (without trying to prepare their function records) to build a *complete* file table, and then emit it immediately. At this point we have the file table hash, so we can then proceed to build and emit all of the function records, without needing to store them in an intermediate list. --- Along the way, this PR makes some necessary changes that are also worthwhile in their own right: - We split `FunctionCoverage` into distinct collector/finished phases, which neatly avoids some borrow-checker hassles when extracting a function's final expression/mapping data. - We avoid having to re-sort a function's mappings when preparing the list of filenames that it uses.

bors · 2023-10-22T11:56:10Z

💔 Test failed - checks-actions

…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 · 2023-10-22T12:21:40Z

Fixed outdated test (diff).

@rustbot ready

cjgillot · 2023-10-22T12:27:55Z

@bors r+

bors · 2023-10-22T12:27:57Z

📌 Commit 6af9fef has been approved by cjgillot

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#105666 (rustdoc: align stability badge to baseline instead of bottom) - rust-lang#117042 (coverage: Emit the filenames section before encoding per-function mappings) - rust-lang#117044 (Miri subtree update) - rust-lang#117049 (add a `csky-unknown-linux-gnuabiv2hf` target ) - rust-lang#117051 (fix broken link: update incremental compilation url) - rust-lang#117069 (x.ps1: remove the check for Python from Windows Store) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117042 - Zalathar:file-table, r=cjgillot coverage: Emit the filenames section before encoding per-function mappings When embedding coverage information in LLVM IR (and ultimately in the resulting binary), there are two main things that each CGU needs to emit: - A single `__llvm_covmap` record containing a coverage header, which mostly consists of a list of filenames used by the CGU's coverage mappings. - Several `__llvm_covfun` records, one for each instrumented function, each of which contains the hash of the list of filenames in the header. There is a kind of loose cyclic dependency between the two: we need the hash of the file table before we can emit the covfun records, but we need to traverse all of the instrumented functions in order to build the file table. The existing code works by processing the individual functions first. It lazily adds filenames to the file table, and stores the mostly-complete function records in a temporary list. After this it hashes the file table, emits the header (containing the file table), and then uses the hash to emit all of the function records. This PR reverses that order: first we traverse all of the functions (without trying to prepare their function records) to build a *complete* file table, and then emit it immediately. At this point we have the file table hash, so we can then proceed to build and emit all of the function records, without needing to store them in an intermediate list. --- Along the way, this PR makes some necessary changes that are also worthwhile in their own right: - We split `FunctionCoverage` into distinct collector/finished phases, which neatly avoids some borrow-checker hassles when extracting a function's final expression/mapping data. - We avoid having to re-sort a function's mappings when preparing the list of filenames that it uses.

rustbot assigned davidtwco Oct 22, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 22, 2023

cjgillot reviewed Oct 22, 2023

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs Outdated Show resolved Hide resolved

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Oct 22, 2023

Zalathar added 5 commits October 22, 2023 20:11

coverage: Split FunctionCoverage into distinct collector/finished p…

371883a

…hases This gives us a clearly-defined place to run code after the instance's MIR has been traversed by codegen, but before we emit its `__llvm_covfun` record.

coverage: Build the global file table ahead of time

e985ae5

coverage: Encapsulate local-to-global file mappings

88159ca

Zalathar force-pushed the file-table branch from ba3859b to c46abe4 Compare October 22, 2023 09:46

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 22, 2023

This comment has been minimized.

Sign in to view

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 22, 2023

Zalathar force-pushed the file-table branch from c46abe4 to 6af9fef Compare October 22, 2023 12:19

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 22, 2023

matthiaskrgr mentioned this pull request Oct 23, 2023

Rollup of 6 pull requests #117071

Merged

bors merged commit dde77f7 into rust-lang:master Oct 23, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 23, 2023

Zalathar deleted the file-table branch October 23, 2023 09:40

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Emit the filenames section before encoding per-function mappings #117042

coverage: Emit the filenames section before encoding per-function mappings #117042

Zalathar commented Oct 22, 2023

rustbot commented Oct 22, 2023

rustbot commented Oct 22, 2023

Zalathar commented Oct 22, 2023

Zalathar commented Oct 22, 2023

cjgillot commented Oct 22, 2023

bors commented Oct 22, 2023

bors commented Oct 22, 2023

This comment has been minimized.

bors commented Oct 22, 2023

Zalathar commented Oct 22, 2023

cjgillot commented Oct 22, 2023

bors commented Oct 22, 2023

coverage: Emit the filenames section before encoding per-function mappings #117042

coverage: Emit the filenames section before encoding per-function mappings #117042

Conversation

Zalathar commented Oct 22, 2023

rustbot commented Oct 22, 2023

rustbot commented Oct 22, 2023

Zalathar commented Oct 22, 2023

Zalathar commented Oct 22, 2023

cjgillot commented Oct 22, 2023

bors commented Oct 22, 2023

bors commented Oct 22, 2023

This comment has been minimized.

bors commented Oct 22, 2023

Zalathar commented Oct 22, 2023

cjgillot commented Oct 22, 2023

bors commented Oct 22, 2023