coverage: Separate initial span extraction from span processing #116409

Zalathar · 2023-10-04T03:07:39Z

One of the main subtasks of coverage instrumentation is looking through MIR to determine a list of source code spans that require coverage counters.

That task is in turn subdivided into a few main steps:

Getting the initial spans from MIR statements/terminators
Processing the list of spans to merge or truncate nearby spans as necessary
Grouping the processed spans by their corresponding coverage graph node

This PR enforces a firmer separation between the first two steps (span extraction and span processing), which ends up slightly simplifying both steps, since they don't need to deal with state that is only meaningful for the other step.

@rustbot label +A-code-coverage

rustbot · 2023-10-04T03:07:47Z

r? @WaffleLapkin

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

rustbot · 2023-10-04T03:07:50Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-10-04T03:10:12Z

The first patch only moves code and updates imports.
The second patch then simplifies the moved code by disconnecting it from CoverageSpansGenerator.
- This converts a method function to an ordinary function, so there's a lot of indentation churn.
The third patch cleans up some unnecessary data structures that were left over from coverage: Remove debug code from the instrumentor #115962.

WaffleLapkin · 2023-10-05T17:41:51Z

r? compiler

oli-obk · 2023-10-07T13:42:05Z

r? @oli-obk

By performal initial span extraction in a separate free function, we can remove some accidental complexity from the main generator code.

This enum was mainly needed to track the precise origin of a span in MIR, for debug printing purposes. Since the old debug code was removed in rust-lang#115962, we can replace it with just the span itself.

oli-obk · 2023-10-10T09:41:25Z

@bors r+ rollup

bors · 2023-10-10T09:41:28Z

📌 Commit 6c44425 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-10-10T09:55:18Z

⌛ Testing commit 6c44425 with merge 5c37696...

bors · 2023-10-10T11:41:19Z

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

rust-timer · 2023-10-10T13:57:17Z

Finished benchmarking commit (5c37696): 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)	-4.1%	[-4.1%, -4.1%]	1
Improvements ✅ (secondary)	-2.3%	[-2.7%, -2.0%]	3
All ❌✅ (primary)	-4.1%	[-4.1%, -4.1%]	1

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: 627.172s -> 627.375s (0.03%)
Artifact size: 270.83 MiB -> 270.86 MiB (0.01%)

coverage: Fix inconsistent handling of function signature spans While doing some more cleanup of `spans`, I noticed a strange inconsistency in how function signatures are handled. Normally the function signature span is treated as though it were executable as part of the start of the function, but in some cases the signature span disappears entirely from coverage, for no obvious reason. This is caused by the fact that spans created by `CoverageSpan::for_fn_sig` don't add the span to their `merged_spans` field (unlike normal statement/terminator spans). In cases where the span-processing code looks at those merged spans, it thinks the signature span is no longer visible and deletes it. Adding the signature span to `merged_spans` resolves the inconsistency. (Prior to rust-lang#116409 this wouldn't have been possible, because there was no case in the old `CoverageStatement` enum representing a signature. Now that `merged_spans` is just a list of spans, that's no longer an obstacle.)

…k,cjgillot coverage: Fix inconsistent handling of function signature spans While doing some more cleanup of `spans`, I noticed a strange inconsistency in how function signatures are handled. Normally the function signature span is treated as though it were executable as part of the start of the function, but in some cases the signature span disappears entirely from coverage, for no obvious reason. This is caused by the fact that spans created by `CoverageSpan::for_fn_sig` don't add the span to their `merged_spans` field (unlike normal statement/terminator spans). In cases where the span-processing code looks at those merged spans, it thinks the signature span is no longer visible and deletes it. Adding the signature span to `merged_spans` resolves the inconsistency. (Prior to rust-lang#116409 this wouldn't have been possible, because there was no case in the old `CoverageStatement` enum representing a signature. Now that `merged_spans` is just a list of spans, that's no longer an obstacle.)

Rollup merge of rust-lang#116974 - Zalathar:signature-spans, r=oli-obk,cjgillot coverage: Fix inconsistent handling of function signature spans While doing some more cleanup of `spans`, I noticed a strange inconsistency in how function signatures are handled. Normally the function signature span is treated as though it were executable as part of the start of the function, but in some cases the signature span disappears entirely from coverage, for no obvious reason. This is caused by the fact that spans created by `CoverageSpan::for_fn_sig` don't add the span to their `merged_spans` field (unlike normal statement/terminator spans). In cases where the span-processing code looks at those merged spans, it thinks the signature span is no longer visible and deletes it. Adding the signature span to `merged_spans` resolves the inconsistency. (Prior to rust-lang#116409 this wouldn't have been possible, because there was no case in the old `CoverageStatement` enum representing a signature. Now that `merged_spans` is just a list of spans, that's no longer an obstacle.)

rustbot assigned WaffleLapkin Oct 4, 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 4, 2023

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

Swatinem approved these changes Oct 4, 2023

View reviewed changes

rustbot assigned davidtwco and unassigned WaffleLapkin Oct 5, 2023

Zalathar force-pushed the span-extraction branch from 3dc45f5 to 80c1809 Compare October 6, 2023 01:05

rustbot assigned oli-obk and unassigned davidtwco Oct 7, 2023

Zalathar force-pushed the span-extraction branch from 80c1809 to 89d502a Compare October 9, 2023 02:49

Zalathar added 3 commits October 10, 2023 13:39

coverage: Move initial MIR span extraction into a submodule

972ab88

coverage: Disconnect span extraction from CoverageSpansGenerator

4b471df

By performal initial span extraction in a separate free function, we can remove some accidental complexity from the main generator code.

coverage: Remove enum CoverageStatement

6c44425

This enum was mainly needed to track the precise origin of a span in MIR, for debug printing purposes. Since the old debug code was removed in rust-lang#115962, we can replace it with just the span itself.

Zalathar force-pushed the span-extraction branch from 89d502a to 6c44425 Compare October 10, 2023 02:42

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 10, 2023

bors merged commit 5c37696 into rust-lang:master Oct 10, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 10, 2023

bors mentioned this pull request Oct 10, 2023

(DRAFT) Combined draft branch for coverage work #115428

Closed

Zalathar deleted the span-extraction branch October 10, 2023 12:31

Dajamante mentioned this pull request Oct 12, 2023

Pull upstream master 2023 10 12 ferrocene/ferrocene#48

Merged

Zalathar mentioned this pull request Oct 20, 2023

coverage: Fix inconsistent handling of function signature spans #116974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Separate initial span extraction from span processing #116409

coverage: Separate initial span extraction from span processing #116409

Zalathar commented Oct 4, 2023

rustbot commented Oct 4, 2023

rustbot commented Oct 4, 2023

Zalathar commented Oct 4, 2023

WaffleLapkin commented Oct 5, 2023

oli-obk commented Oct 7, 2023

oli-obk commented Oct 10, 2023

bors commented Oct 10, 2023

bors commented Oct 10, 2023

bors commented Oct 10, 2023

rust-timer commented Oct 10, 2023

coverage: Separate initial span extraction from span processing #116409

coverage: Separate initial span extraction from span processing #116409

Conversation

Zalathar commented Oct 4, 2023

rustbot commented Oct 4, 2023

rustbot commented Oct 4, 2023

Zalathar commented Oct 4, 2023

WaffleLapkin commented Oct 5, 2023

oli-obk commented Oct 7, 2023

oli-obk commented Oct 10, 2023

bors commented Oct 10, 2023

bors commented Oct 10, 2023

bors commented Oct 10, 2023

rust-timer commented Oct 10, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size