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.

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

coverage: Carve out hole spans in a separate early pass #125921

Merged
merged 6 commits into from
Jun 5, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jun 3, 2024

When extracting spans from MIR for use in coverage instrumentation, we sometimes need to identify hole spans (currently just closures), and carve up the other spans so that they don't overlap with holes.

This PR simplifies the main coverage-span-refiner by extracting the hole-carving process into a separate early pass. That pass produces a series of independent buckets, and we run the span-refiner on each bucket separately.

There is almost no difference in the resulting mappings, other than in some edge cases involving macros.

@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2024

r? @lcnr

rustbot has assigned @lcnr.
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

@rustbot 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 Jun 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@Zalathar Zalathar force-pushed the buckets branch 2 times, most recently from 5a0e1ce to d90f2a0 Compare June 3, 2024 10:57
@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 3, 2024

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jun 3, 2024
This is the counterpart of `Span::trim_start`.
This will allow the span extractor to produce multiple separate buckets,
instead of just one flat list of spans.
This is less elegant than returning an iterator, but more flexible.
…kets

This performs the same task as the hole-carving code in the main span refiner,
but in a separate earlier pass.
Now that hole spans are handled by a separate earlier pass, this code never
sees hole spans, and therefore doesn't need to deal with them.
@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 4, 2024

Tweaked how fragments are juggled (diff).

Comment on lines +119 to +128
fn compare_spans(a: Span, b: Span) -> std::cmp::Ordering {
// First sort by span start.
Ord::cmp(&a.lo(), &b.lo())
// If span starts are the same, sort by span end in reverse order.
// This ensures that if spans A and B are adjacent in the list,
// and they overlap but are not equal, then either:
// - Span A extends further left, or
// - Both have the same start and span A extends further right
.then_with(|| Ord::cmp(&a.hi(), &b.hi()).reverse())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a good ordering for spans in general (I'm reducing span ordering to just care about lo and hi in #123165)

@oli-obk
Copy link
Contributor

oli-obk commented Jun 4, 2024

r? @oli-obk

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2024

📌 Commit c57a1d1 has been approved by oli-obk

It is now in the queue for this repository.

@rustbot rustbot assigned oli-obk and unassigned lcnr Jun 4, 2024
@bors 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 Jun 4, 2024
fmease added a commit to fmease/rust that referenced this pull request Jun 4, 2024
coverage: Carve out hole spans in a separate early pass

When extracting spans from MIR for use in coverage instrumentation, we sometimes need to identify *hole spans* (currently just closures), and carve up the other spans so that they don't overlap with holes.

This PR simplifies the main coverage-span-refiner by extracting the hole-carving process into a separate early pass. That pass produces a series of independent buckets, and we run the span-refiner on each bucket separately.

There is almost no difference in the resulting mappings, other than in some edge cases involving macros.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#125273 (bootstrap: implement new feature `bootstrap-self-test`)
 - rust-lang#125800 (Fix `mut` static task queue in SGX target)
 - rust-lang#125903 (rustc_span: Inline some hot functions)
 - rust-lang#125920 (Allow static mut definitions with #[linkage])
 - rust-lang#125921 (coverage: Carve out hole spans in a separate early pass)
 - rust-lang#125995 (Use inline const blocks to create arrays of `MaybeUninit`.)
 - rust-lang#125996 (Closures are recursively reachable)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#124746 (`rustc --explain E0582` additional example)
 - rust-lang#125407 (Detect when user is trying to create a lending `Iterator` and give a custom explanation)
 - rust-lang#125505 (Add intra-doc-links to rustc_middle crate-level docs.)
 - rust-lang#125792 (Don't drop `Unsize` candidate in intercrate mode)
 - rust-lang#125819 (Various `HirTyLowerer` cleanups)
 - rust-lang#125861 (rustc_codegen_ssa: fix `get_rpath_relative_to_output` panic when lib only contains file name)
 - rust-lang#125911 (delete bootstrap build before switching to bumped rustc)
 - rust-lang#125921 (coverage: Carve out hole spans in a separate early pass)
 - rust-lang#125940 (std::unix::fs::get_path: using fcntl codepath for netbsd instead.)
 - rust-lang#126022 (set `has_unconstrained_ty_var` when generalizing aliases in bivariant contexts)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 79bb336 into rust-lang:master Jun 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup merge of rust-lang#125921 - Zalathar:buckets, r=oli-obk

coverage: Carve out hole spans in a separate early pass

When extracting spans from MIR for use in coverage instrumentation, we sometimes need to identify *hole spans* (currently just closures), and carve up the other spans so that they don't overlap with holes.

This PR simplifies the main coverage-span-refiner by extracting the hole-carving process into a separate early pass. That pass produces a series of independent buckets, and we run the span-refiner on each bucket separately.

There is almost no difference in the resulting mappings, other than in some edge cases involving macros.
@Zalathar Zalathar deleted the buckets branch June 6, 2024 00:00
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 12, 2024
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
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants