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: Split off mappings.rs from spans.rs and from_mir.rs #124545

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

Zalathar
Copy link
Contributor

Originally, spans.rs was mainly concerned with extracting and post-processing spans from MIR, so that they could be used for block coverage instrumentation.

Over time it has organically expanded to include more responsibilities, especially relating to branch coverage and MC/DC coverage, that don't really fit its current name.

This PR therefore takes all the extra code that is not part of the old span-refinement engine, and moves it out into a new mappings.rs file.


No functional changes. I have deliberately avoided doing any follow-up (such as renaming types or functions), because this particular change is very rot-prone, and I want it to be as simple and self-contained as possible.

@rustbot label +A-code-coverage

@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
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 Apr 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Apr 30, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Apr 30, 2024

In order to verify that the main patch consists only of moving two contiguous chunks of code (and adjusting imports), even after rebasing, I have manually verified it by doing the following:

  • Take a copy of the code segments that are deleted from spans.rs and from_mir.rs, and paste them over the top of the corresponding code in mappings.rs.
  • Verify that git reports no changes as a result of having done this.

(It's a shame that GitHub doesn't offer the option to show moved lines.)

Comment on lines +11 to +14
// FIXME(#124545) It's awkward that we have to re-export this, because it's an
// internal detail of `from_mir` that is also needed when handling branch and
// MC/DC spans. Ideally we would find a more natural home for it.
pub(super) use from_mir::unexpand_into_body_span_with_visible_macro;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little unhappy with having to re-export this here, but I don't want this whole split to be held up on such a relatively minor detail, so for now I've just left a FIXME note.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 30, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 30, 2024

📌 Commit ba87e5b has been approved by oli-obk

It is now in the queue for this repository.

@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 Apr 30, 2024
@bors
Copy link
Contributor

bors commented Apr 30, 2024

⌛ Testing commit ba87e5b with merge a743116...

extract_refined_covspans, unexpand_into_body_span_with_visible_macro,
};
use crate::coverage::ExtractedHirInfo;
use rustc_index::IndexVec;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops; I should clean up this misplaced import in one of my future PRs.

@bors
Copy link
Contributor

bors commented Apr 30, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 30, 2024
@bors bors merged commit a743116 into rust-lang:master Apr 30, 2024
10 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Apr 30, 2024
@Zalathar Zalathar deleted the mappings branch April 30, 2024 13:28
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a743116): 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)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 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: 672.522s -> 673.085s (0.08%)
Artifact size: 315.99 MiB -> 315.95 MiB (-0.01%)

bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
coverage: Split out MC/DC mappings from `BcbMappingKind`

These variants were added to `BcbMappingKind` as part of the [MC/DC coverage](https://en.wikipedia.org/wiki/Modified_Condition/Decision_Coverage) implementation in rust-lang#123409, because that was the path-of-least-resistance for integrating them into the existing code.

However, they ultimately represent complex concepts that the enum was not intended to handle, leading to more complexity in the code that processes them. This PR therefore follows in the footsteps of rust-lang#124545, and splits the MC/DC mappings out into their own dedicated vectors of structs.

After that, `BcbMappingKind` itself ends up having only one variant (`Code`), so this PR also flattens that enum into its enclosing struct, renamed to `mapping::CodeMapping`.

---

No functional changes.

This will conflict slightly with rust-lang#124571, but hopefully that should be easy to resolve either way.

`@rustbot` label +A-code-coverage
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) merged-by-bors This PR was explicitly merged by bors. 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