coverage: Rearrange the code for embedding per-function coverage metadata #134163

Zalathar · 2024-12-11T07:33:43Z

This is a series of refactorings to the code that prepares and embeds per-function coverage metadata records (“covfun records”) in the __llvm_covfun linker section of the final binary. The llvm-cov tool reads this metadata from the binary when preparing a coverage report.

Beyond general cleanup, a big motivation behind these changes is to pave the way for re-landing an updated version of #133418.

There should be no change in compiler output, as demonstrated by the absence of (meaningful) changes to coverage tests.

The first patch is just moving code around, so I suggest looking at the other patches to see the actual changes.

try-job: x86_64-gnu
try-job: x86_64-msvc
try-job: aarch64-apple

rustbot · 2024-12-11T07:33:51Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Zalathar · 2024-12-11T08:07:43Z

@bors try

coverage: Rearrange the code for embedding per-function coverage metadata This is a series of refactorings to the code that prepares and embeds per-function coverage metadata records (“covfun records”) in the `__llvm_covfun` linker section of the final binary. The `llvm-cov` tool reads this metadata from the binary when preparing a coverage report. Beyond general cleanup, a big motivation behind these changes is to pave the way for re-landing an updated version of rust-lang#133418. --- There should be no change in compiler output, as demonstrated by the absence of (meaningful) changes to coverage tests. --- try-job: x86_64-gnu try-job: x86_64-msvc try-job: aarch64-apple

bors · 2024-12-11T08:08:54Z

⌛ Trying commit ec7eb7e with merge 4bb99ca...

SparrowLii

Thanks!
I think this PR doesn't change the functionality. Just making some comments for the code.

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

bors · 2024-12-11T10:13:46Z

☀️ Try build successful - checks-actions
Build commit: 4bb99ca (4bb99ca1934dc99af504ea1693260dc0e4452999)

Zalathar · 2024-12-11T10:20:01Z

Added some more comments in response to feedback (diff).

tests/codegen/instrument-coverage/testprog.rs

…globals

This defers the call to `llvm_cov::write_function_mappings_to_buffer` until just before its enclosing global variable is created.

Zalathar · 2024-12-11T10:37:41Z

I had another look at that test, and changing it to use CHECK-DAG was easier than I expected, so I just fixed it.

jieyouxu · 2024-12-11T10:47:00Z

No further concerns from me, feel free to r=Sparrow and me if @SparrowLii has no further concerns.

SparrowLii · 2024-12-11T10:52:32Z

LGTM

Zalathar · 2024-12-11T11:42:21Z

Thanks for the reviews.

@bors r=SparrowLii,jieyouxu

bors · 2024-12-11T11:42:24Z

📌 Commit 3f3a9bf has been approved by SparrowLii,jieyouxu

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#132975 (De-duplicate and improve definition of core::ffi::c_char) - rust-lang#133598 (Change `GetManyMutError` to match T-libs-api decision) - rust-lang#134148 (add comments in check_expr_field) - rust-lang#134163 (coverage: Rearrange the code for embedding per-function coverage metadata) - rust-lang#134165 (wasm(32|64): update alignment string) - rust-lang#134170 (Subtree update of `rust-analyzer`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134163 - Zalathar:covfun, r=SparrowLii,jieyouxu coverage: Rearrange the code for embedding per-function coverage metadata This is a series of refactorings to the code that prepares and embeds per-function coverage metadata records (“covfun records”) in the `__llvm_covfun` linker section of the final binary. The `llvm-cov` tool reads this metadata from the binary when preparing a coverage report. Beyond general cleanup, a big motivation behind these changes is to pave the way for re-landing an updated version of rust-lang#133418. --- There should be no change in compiler output, as demonstrated by the absence of (meaningful) changes to coverage tests. The first patch is just moving code around, so I suggest looking at the other patches to see the actual changes. --- try-job: x86_64-gnu try-job: x86_64-msvc try-job: aarch64-apple

jieyouxu · 2024-12-12T11:49:02Z

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

+    // If there are no covfun records for this CGU, don't generate a covmap record.
+    // Emitting a covmap record without any covfun records causes `llvm-cov` to
+    // fail when generating coverage reports, and if there are no covfun records
+    // then the covmap record isn't useful anyway.
+    // This should prevent a repeat of <https://github.com/rust-lang/rust/issues/133606>.
+    if covfun_records.is_empty() {
+        return;
+    }


Sorry, it only occurred to me post-merge as I was looking at #134208. Did we ever have a regression test for this case (I imagine it can be hard to come with up with one)?

It should be possible, but I would have to do some non-trivial detective work, because currently we don't have any examples smaller than “the coverage tests for my entire workspace, in CI”.

…-errors coverage: Tidy up creation of covmap and covfun records This is a small follow-up to rust-lang#134163 that mostly just inlines and renames some variables, and adds a few comments. It also slightly defers the creation of the LLVM value that holds the filename table, to just before the value is needed.

coverage: Tidy up creation of covmap and covfun records This is a small follow-up to rust-lang#134163 that mostly just inlines and renames some variables, and adds a few comments. It also slightly defers the creation of the LLVM value that holds the filename table, to just before the value is needed. --- try-job: x86_64-mingw-2

…-errors coverage: Tidy up creation of covmap and covfun records This is a small follow-up to rust-lang#134163 that mostly just inlines and renames some variables, and adds a few comments. It also slightly defers the creation of the LLVM value that holds the filename table, to just before the value is needed. --- try-job: x86_64-mingw-2

coverage: Tidy up creation of covmap and covfun records This is a small follow-up to rust-lang#134163 that mostly just inlines and renames some variables, and adds a few comments. It also slightly defers the creation of the LLVM value that holds the filename table, to just before the value is needed. --- try-job: x86_64-mingw-2 try-job: dist-x86_64-linux

…-errors coverage: Tidy up creation of covmap and covfun records This is a small follow-up to rust-lang#134163 that mostly just inlines and renames some variables, and adds a few comments. It also slightly defers the creation of the LLVM value that holds the filename table, to just before the value is needed. --- try-job: x86_64-mingw-2 try-job: dist-x86_64-linux

Rollup merge of rust-lang#134208 - Zalathar:covmap-covfun, r=compiler-errors coverage: Tidy up creation of covmap and covfun records This is a small follow-up to rust-lang#134163 that mostly just inlines and renames some variables, and adds a few comments. It also slightly defers the creation of the LLVM value that holds the filename table, to just before the value is needed. --- try-job: x86_64-mingw-2 try-job: dist-x86_64-linux

coverage: Store coverage source regions as `Span` until codegen (take 2) This is an attempt to re-land rust-lang#133418: > Historically, coverage spans were converted into line/column coordinates during the MIR instrumentation pass. > This PR moves that conversion step into codegen, so that coverage spans spend most of their time stored as Span instead. > In addition to being conceptually nicer, this also reduces the size of coverage mappings in MIR, because Span is smaller than 4x u32. That PR was reverted by rust-lang#133608, because in some circumstances not covered by our test suite we were emitting coverage metadata that was causing `llvm-cov` to exit with an error (rust-lang#133606). --- The implementation here is *mostly* the same, but adapted for subsequent changes in the relevant code (e.g. rust-lang#134163). I believe that the changes in rust-lang#134163 should be sufficient to prevent the problem that required the original PR to be reverted. But I haven't been able to reproduce the original breakage in a regression test, and the `llvm-cov` error message is extremely unhelpful, so I can't completely rule out the possibility of this breaking again. r? jieyouxu (reviewer of the original PR)

Rollup merge of rust-lang#134497 - Zalathar:spans, r=jieyouxu coverage: Store coverage source regions as `Span` until codegen (take 2) This is an attempt to re-land rust-lang#133418: > Historically, coverage spans were converted into line/column coordinates during the MIR instrumentation pass. > This PR moves that conversion step into codegen, so that coverage spans spend most of their time stored as Span instead. > In addition to being conceptually nicer, this also reduces the size of coverage mappings in MIR, because Span is smaller than 4x u32. That PR was reverted by rust-lang#133608, because in some circumstances not covered by our test suite we were emitting coverage metadata that was causing `llvm-cov` to exit with an error (rust-lang#133606). --- The implementation here is *mostly* the same, but adapted for subsequent changes in the relevant code (e.g. rust-lang#134163). I believe that the changes in rust-lang#134163 should be sufficient to prevent the problem that required the original PR to be reverted. But I haven't been able to reproduce the original breakage in a regression test, and the `llvm-cov` error message is extremely unhelpful, so I can't completely rule out the possibility of this breaking again. r? jieyouxu (reviewer of the original PR)

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#132975 (De-duplicate and improve definition of core::ffi::c_char) - rust-lang#133598 (Change `GetManyMutError` to match T-libs-api decision) - rust-lang#134148 (add comments in check_expr_field) - rust-lang#134163 (coverage: Rearrange the code for embedding per-function coverage metadata) - rust-lang#134165 (wasm(32|64): update alignment string) - rust-lang#134170 (Subtree update of `rust-analyzer`) r? `@ghost` `@rustbot` modify labels: rollup

Zalathar added 2 commits December 11, 2024 17:49

coverage: Extract function metadata handling to a covfun submodule

7c4ac71

coverage: Reify CovfunRecord as an intermediate step

6a8c016

Zalathar added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Dec 11, 2024

rustbot assigned SparrowLii Dec 11, 2024

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 Dec 11, 2024

This comment has been minimized.

# to view

Zalathar force-pushed the covfun branch 2 times, most recently from e0ad6ad to ec7eb7e Compare December 11, 2024 08:04

SparrowLii reviewed Dec 11, 2024

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs Show resolved Hide resolved

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

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

Zalathar force-pushed the covfun branch from ec7eb7e to bb9e33a Compare December 11, 2024 10:19

Zalathar force-pushed the covfun branch from bb9e33a to 3c3eba0 Compare December 11, 2024 10:21

Zalathar commented Dec 11, 2024

View reviewed changes

tests/codegen/instrument-coverage/testprog.rs Outdated Show resolved Hide resolved

Zalathar added 3 commits December 11, 2024 21:34

coverage: Adjust a codegen test to ignore the order of covmap/covfun …

9e6b7c1

…globals

coverage: Only generate a CGU's covmap record if it has covfun records

512f3fd

coverage: Store intermediate region tables in CovfunRecord

3f3a9bf

This defers the call to `llvm_cov::write_function_mappings_to_buffer` until just before its enclosing global variable is created.

Zalathar force-pushed the covfun branch from 3c3eba0 to 3f3a9bf Compare December 11, 2024 10:36

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 Dec 11, 2024

matthiaskrgr mentioned this pull request Dec 11, 2024

Rollup of 6 pull requests #134177

Merged

bors merged commit 13c13ee into rust-lang:master Dec 11, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 11, 2024

Zalathar deleted the covfun branch December 11, 2024 23:12

Zalathar mentioned this pull request Dec 12, 2024

coverage: Tidy up creation of covmap and covfun records #134208

Merged

jieyouxu reviewed Dec 12, 2024

View reviewed changes

Zalathar mentioned this pull request Dec 19, 2024

coverage: Store coverage source regions as Span until codegen (take 2) #134497

Merged

coverage: Rearrange the code for embedding per-function coverage metadata #134163

coverage: Rearrange the code for embedding per-function coverage metadata #134163

Uh oh!

Conversation

Zalathar commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 11, 2024

Uh oh!

This comment has been minimized.

Zalathar commented Dec 11, 2024

Uh oh!

bors commented Dec 11, 2024

Uh oh!

SparrowLii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Dec 11, 2024

Uh oh!

Zalathar commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Zalathar commented Dec 11, 2024

Uh oh!

jieyouxu commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparrowLii commented Dec 11, 2024

Uh oh!

Zalathar commented Dec 11, 2024

Uh oh!

bors commented Dec 11, 2024

Uh oh!

Uh oh!

jieyouxu Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zalathar commented Dec 11, 2024 •

edited

Loading

Zalathar commented Dec 11, 2024 •

edited

Loading

jieyouxu commented Dec 11, 2024 •

edited

Loading