coverage: Hoist some complex code out of the main span refinement loop #119208

Zalathar · 2023-12-22T06:20:11Z

The span refinement loop in spans.rs takes the spans that have been extracted from MIR, and modifies them to produce more helpful output in coverage reports.

It is also one of the most complicated pieces of code in the coverage instrumentor. It has an abundance of moving pieces that make it difficult to understand, and most attempts to modify it end up accidentally changing its behaviour in unacceptable ways.

This PR nevertheless tries to make a dent in it by hoisting two pieces of special-case logic out of the main loop, and into separate preprocessing passes. Coverage tests show that the resulting mappings are almost identical, with all known differences being unimportant.

This should hopefully unlock further simplifications to the refinement loop, since it now has fewer edge cases to worry about.

rustbot · 2023-12-22T06:20:19Z

r? @WaffleLapkin

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

rustbot · 2023-12-22T06:20:22Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-12-22T08:11:52Z

@rustbot label +A-code-coverage

Zalathar · 2023-12-25T01:37:55Z

I've been playing around with various alternative implementations, in an attempt to match the old mappings exactly.

But because the old code is sensitive to lots of seemingly-irrelevant details (which is what I'm trying to get away from), I haven't come up with anything that matches perfectly.

I suspect that the way the existing code handles macro-expansions is not quite right. But the changes in this PR should be a stepping-stone towards being able to do something about that.

Zalathar · 2023-12-28T05:24:26Z

I've completed my planned overhaul (diff).

The main change is that after #119336 I found a way to simplify the handling of “visible macros”, which then makes a few of the later parts of this PR a bit simpler.

I still need to make sure that the changes to closure.cov-map are acceptable; after that I'll mark this as ready.

Zalathar · 2023-12-28T05:28:19Z

Looks good to me.

@rustbot ready

WaffleLapkin

This looks fine to me, but I'm lacking context on coverage stuff.

@Swatinem could you take a closer look?

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

Swatinem

lgtm, though I agree with the PR description that these parts of the code are very intricate and hard to understand ;-)

The changes to the test output also look like clear improvement.

Zalathar · 2023-12-28T10:06:53Z

Added better documentation for unexpand_into_body_span_with_prev (diff).

bors · 2024-01-03T21:32:08Z

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

Zalathar · 2024-01-03T22:59:50Z

Rebased to resolve trivial conflicts with #119444.

@rustbot ready

…ans`

This draws a clear distinction between the fields/methods that are needed by initial span extraction and preprocessing, and those that are needed by the main "refinement" loop.

The struct itself is already non-public, so having public fields doesn't achieve anything.

cjgillot · 2024-01-06T02:53:57Z

@bors r=WaffleLapkin,Swatinem

bors · 2024-01-06T02:54:00Z

📌 Commit 514e026 has been approved by WaffleLapkin,Swatinem

It is now in the queue for this repository.

…mpiler-errors Rollup of 9 pull requests Successful merges: - rust-lang#119208 (coverage: Hoist some complex code out of the main span refinement loop) - rust-lang#119216 (Use diagnostic namespace in stdlib) - rust-lang#119414 (bootstrap: Move -Clto= setting from Rustc::run to rustc_cargo) - rust-lang#119420 (Handle ForeignItem as TAIT scope.) - rust-lang#119468 (rustdoc-search: tighter encoding for f index) - rust-lang#119628 (remove duplicate test) - rust-lang#119638 (fix cyle error when suggesting to use associated function instead of constructor) - rust-lang#119640 (library: Fix warnings in rtstartup) - rust-lang#119642 (library: Fix a symlink test failing on Windows) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#119208 - Zalathar:hoist, r=WaffleLapkin,Swatinem coverage: Hoist some complex code out of the main span refinement loop The span refinement loop in `spans.rs` takes the spans that have been extracted from MIR, and modifies them to produce more helpful output in coverage reports. It is also one of the most complicated pieces of code in the coverage instrumentor. It has an abundance of moving pieces that make it difficult to understand, and most attempts to modify it end up accidentally changing its behaviour in unacceptable ways. This PR nevertheless tries to make a dent in it by hoisting two pieces of special-case logic out of the main loop, and into separate preprocessing passes. Coverage tests show that the resulting mappings are *almost* identical, with all known differences being unimportant. This should hopefully unlock further simplifications to the refinement loop, since it now has fewer edge cases to worry about.

coverage: Replace the old span refiner with a single function As more and more of the span refiner's functionality has been pulled out into separate early passes, it has finally reached the point where we can remove the rest of the old `SpansRefiner` code, and replace it with a single modestly-sized function. ~~There should be no change to the resulting coverage mappings, as demonstrated by the lack of changes to test output.~~ There is *almost* no change to the resulting coverage mappings. There are some minor changes to `loop` that on inspection appear to be neutral in terms of accuracy, with the old behaviour being a slightly-horrifying implementation detail of the old code, so I think they're acceptable. Previous work in this direction includes: - rust-lang#125921 - rust-lang#121019 - rust-lang#119208

Rollup merge of rust-lang#126294 - Zalathar:spans-refiner, r=oli-obk coverage: Replace the old span refiner with a single function As more and more of the span refiner's functionality has been pulled out into separate early passes, it has finally reached the point where we can remove the rest of the old `SpansRefiner` code, and replace it with a single modestly-sized function. ~~There should be no change to the resulting coverage mappings, as demonstrated by the lack of changes to test output.~~ There is *almost* no change to the resulting coverage mappings. There are some minor changes to `loop` that on inspection appear to be neutral in terms of accuracy, with the old behaviour being a slightly-horrifying implementation detail of the old code, so I think they're acceptable. Previous work in this direction includes: - rust-lang#125921 - rust-lang#121019 - rust-lang#119208

rustbot assigned WaffleLapkin Dec 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 Dec 22, 2023

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

Zalathar force-pushed the hoist branch from f70be0e to 062c4d0 Compare December 23, 2023 02:30

Swatinem approved these changes Dec 23, 2023

View reviewed changes

Zalathar force-pushed the hoist branch 4 times, most recently from 1941cb0 to 614a611 Compare December 25, 2023 01:29

Zalathar force-pushed the hoist branch 3 times, most recently from 8ad6d8b to 5a97970 Compare December 28, 2023 03:50

This comment was marked as outdated.

Sign in to view

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 Dec 28, 2023

Zalathar force-pushed the hoist branch from 5a97970 to 46f6aac Compare December 28, 2023 05:20

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 Dec 28, 2023

WaffleLapkin approved these changes Dec 28, 2023

View reviewed changes

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs Show resolved Hide resolved

Swatinem approved these changes Dec 28, 2023

View reviewed changes

Zalathar force-pushed the hoist branch from 46f6aac to 6b72330 Compare December 28, 2023 10:06

This comment has been minimized.

Sign in to view

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 Jan 2, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 3, 2024

Zalathar force-pushed the hoist branch from aa84b52 to a892f1c Compare January 3, 2024 22:58

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 Jan 3, 2024

Zalathar added 6 commits January 5, 2024 12:53

coverage: Avoid early returns from `mir_to_initial_sorted_coverage_sp…

506b9f9

…ans`

coverage: Overhaul how "visible macros" are determined

df0df52

coverage: Hoist the removal of unwanted macro expansion spans

cd3a976

coverage: Hoist the splitting of visible macro invocations

d4d2f14

coverage: Split out SpanFromMir from CoverageSpan

cd50843

This draws a clear distinction between the fields/methods that are needed by initial span extraction and preprocessing, and those that are needed by the main "refinement" loop.

coverage: Make the remaining fields of CoverageSpan non-public

514e026

The struct itself is already non-public, so having public fields doesn't achieve anything.

Zalathar force-pushed the hoist branch from a892f1c to 514e026 Compare January 5, 2024 02:41

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 Jan 6, 2024

This was referenced Jan 6, 2024

Rollup of 9 pull requests #119648

Merged

Rollup of 9 pull requests #119649

Closed

bors merged commit bacddd3 into rust-lang:master Jan 6, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 6, 2024

Zalathar deleted the hoist branch January 6, 2024 08:26

Zalathar mentioned this pull request Jun 12, 2024

coverage: Replace the old span refiner with a single function #126294

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Hoist some complex code out of the main span refinement loop #119208

coverage: Hoist some complex code out of the main span refinement loop #119208

Zalathar commented Dec 22, 2023 •

edited

Loading

rustbot commented Dec 22, 2023

rustbot commented Dec 22, 2023

Zalathar commented Dec 22, 2023

Zalathar commented Dec 25, 2023

This comment was marked as outdated.

Zalathar commented Dec 28, 2023

Zalathar commented Dec 28, 2023

WaffleLapkin left a comment

Swatinem left a comment

Zalathar commented Dec 28, 2023

This comment has been minimized.

bors commented Jan 3, 2024

Zalathar commented Jan 3, 2024 •

edited

Loading

cjgillot commented Jan 6, 2024

bors commented Jan 6, 2024

coverage: Hoist some complex code out of the main span refinement loop #119208

coverage: Hoist some complex code out of the main span refinement loop #119208

Conversation

Zalathar commented Dec 22, 2023 • edited Loading

rustbot commented Dec 22, 2023

rustbot commented Dec 22, 2023

Zalathar commented Dec 22, 2023

Zalathar commented Dec 25, 2023

This comment was marked as outdated.

Zalathar commented Dec 28, 2023

Zalathar commented Dec 28, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

Swatinem left a comment

Choose a reason for hiding this comment

Zalathar commented Dec 28, 2023

This comment has been minimized.

bors commented Jan 3, 2024

Zalathar commented Jan 3, 2024 • edited Loading

cjgillot commented Jan 6, 2024

bors commented Jan 6, 2024

Zalathar commented Dec 22, 2023 •

edited

Loading

Zalathar commented Jan 3, 2024 •

edited

Loading