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.

Sign up for GitHub

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

Jump to bottom

coverage: Collect HIR info during MIR building #118237

Closed
wants to merge 8 commits into from
Closed

coverage: Collect HIR info during MIR building #118237

wants to merge 8 commits into from

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Nov 24, 2023 •
edited
Loading

The coverage instrumentor pass operates on MIR, but it needs access to some source-related information that is normally not present in MIR.

Historically, that information was obtained by looking back at HIR during MIR transformation. This PR arranges for the necessary information to be collected ahead of time during MIR building instead.

Fixes #79625.

In addition to general MIR-pass cleanliness, part of the motivation here is to make it easier to collect additional coverage information from HIR/THIR in the future, because much of the plumbing has been established by this PR.

@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2023

r? @wesleywiser

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

@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 Nov 24, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar
Zalathar commented Nov 24, 2023
View reviewed changes
compiler/rustc_mir_build/src/build/coverageinfo.rs Outdated
@@ -106,7 +104,6 @@ fn get_body_span<'tcx>(
}

fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, hir_body: &'tcx rustc_hir::Body<'tcx>) -> u64 {
// FIXME(cjgillot) Stop hashing HIR manually here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @cjgillot on removing this FIXME.

I believe it was mostly obsoleted by #95573, since this code now just fetches an already-computed hash, instead of computing a hash manually.

Now that this code runs during MIR building (instead of MIR transform), it looks like there's nothing left to fix here.

@Zalathar
Copy link
Contributor Author

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Nov 24, 2023
@Zalathar Zalathar mentioned this pull request Nov 26, 2023
Closed
@Zalathar Zalathar force-pushed the hir-info branch 9 times, most recently from 7f3f9d3 to e8381de Compare December 3, 2023 02:15
cjgillot
cjgillot reviewed Dec 3, 2023
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

In addition to general MIR-pass cleanliness, part of the motivation here is to make it easier to collect additional coverage information from HIR/THIR in the future, because much of the plumbing has been established by this PR.

There is a premise here that I can't quite agree with. Looking at HIR from a MIR pass is not an issue. What kind of information would you need to channel in the future?

compiler/rustc_mir_build/src/build/coverageinfo.rs Outdated Show resolved Hide resolved
@Zalathar
Copy link
Contributor Author

Zalathar commented Dec 4, 2023

In addition to general MIR-pass cleanliness, part of the motivation here is to make it easier to collect additional coverage information from HIR/THIR in the future, because much of the plumbing has been established by this PR.

There is a premise here that I can't quite agree with. Looking at HIR from a MIR pass is not an issue. What kind of information would you need to channel in the future?

The concrete motivation is branch coverage instrumentation (#118305).

I want to have an accurate association between the span of an if statement's boolean condition, and the two blocks that represent its then/else arms. Based on what I've tried so far, I don't think this can be accomplished through analysis of the existing MIR. The lowering process doesn't track this association precisely (because for every other purpose it doesn't need to), and I don't think it can be fully recovered from MIR statements/terminators and span contexts.

So the approach I've taken in my branch coverage draft is to add code to the lowering of if statements that injects marker statements into the then/else arms. It also builds up a side table (in mir::Body) associating the condition's span with two marker IDs, indicating which one is true (then) and which one is false (else). The coverage MIR pass can then use the table+markers to create accurate source code mappings for branches.

@Zalathar
Copy link
Contributor Author

Zalathar commented Dec 4, 2023

So the two main things I care about from this PR are:

  • Hoisting the is_eligible check out of MIR transforms and into MIR building, so that the branch coverage code will be able to avoid touching functions that aren't being instrumented for coverage (without having to duplicate the eligibility check).
  • Adding a field to mir::Body that coverage code can use to transmit information from MIR building to the coverage pass in MIR transforms.

Putting the signature/body span in that field is kind of secondary. I've done it so that this PR doesn't end up looking kind of pointless (in isolation), and also because the comments on that code gave me the impression that looking backwards at HIR from a MIR pass was gently discouraged. But in truth it's not really the central focus of this PR.

@Zalathar Zalathar force-pushed the hir-info branch 2 times, most recently from da6f75e to 1bc26f4 Compare December 5, 2023 03:13
@Zalathar
coverage: Check that the fn sig span is before the body span
3dbe59e 
If the function signature span comes after the body span (due to macro
expansion), expanding it towards the start of the body span would produce
nonsense.
@Zalathar
coverage: Collect HIR info during MIR building
65606bf 
The coverage instrumentor pass operates on MIR, but it needs access to some
source-related information that is normally not present in MIR.

Historically, that information was obtained by looking back at HIR during MIR
transformation. This patch arranges for the necessary information to be
collected ahead of time during MIR building instead.
@Zalathar
coverage: Remove some FIXMEs that are now fixed
8b62d93
@Zalathar
coverage: Don't use HIR to determine whether a def is fn-like
2106aaa
@Zalathar
Copy link
Contributor Author

Another thing I could do with this PR is remove the part that actually moves code into rustc_mir_build, and turn it into a purely-internal refactoring of the existing coverage code in rustc_mir_transform.

Then I could integrate the MIR building changes into #118305, where they would be easier to justify.

@bors
Copy link
Contributor

bors commented Dec 13, 2023

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

@Zalathar Zalathar mentioned this pull request Dec 14, 2023
Merged
@Zalathar
Copy link
Contributor Author

This needed some intensive manual rebasing, so I've instead reworked it into #118929.

@Zalathar Zalathar closed this Dec 14, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 15, 2023
@matthiaskrgr
Rollup merge of rust-lang#118929 - Zalathar:look-hir, r=cjgillot
6659b5e 
coverage: Tidy up early parts of the instrumentor pass

This is extracted from rust-lang#118237, which needed to be manually rebased anyway.

Unlike that PR, this one only affects the coverage instrumentor, and doesn't attempt to move any code into the MIR builder. That can be left to a future version of rust-lang#118305, which can still benefit from these improvements.

So this is now mostly a refactoring of some internal parts of the instrumentor.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2023
@rust-timer
Unrolled build for rust-lang#118929
83a528d 
Rollup merge of rust-lang#118929 - Zalathar:look-hir, r=cjgillot

coverage: Tidy up early parts of the instrumentor pass

This is extracted from rust-lang#118237, which needed to be manually rebased anyway.

Unlike that PR, this one only affects the coverage instrumentor, and doesn't attempt to move any code into the MIR builder. That can be left to a future version of rust-lang#118305, which can still benefit from these improvements.

So this is now mostly a refactoring of some internal parts of the instrumentor.
@Zalathar Zalathar deleted the hir-info branch July 1, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

@Swatinem Swatinem Swatinem approved these changes

Assignees

@wesleywiser wesleywiser

Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

InstrumentCoverage requires information from HIR that might need to be available via MIR
6 participants
@Zalathar @rustbot @bors @Swatinem @cjgillot @wesleywiser