coverage: Collect HIR info during MIR building by Zalathar · Pull Request #118237 · rust-lang/rust

Zalathar · 2023-11-24T10:00:01Z

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 · 2023-11-24T10:00:11Z

r? @wesleywiser

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

rustbot · 2023-11-24T10:00:13Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-11-24T10:03:44Z

compiler/rustc_mir_build/src/build/coverageinfo.rs

 }

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


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 · 2023-11-24T10:04:14Z

@rustbot label +A-code-coverage

cjgillot

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

Zalathar · 2023-12-04T01:11:11Z

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 · 2023-12-04T01:18:26Z

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.

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.

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 · 2023-12-11T01:16:54Z

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 · 2023-12-13T13:17:45Z

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

Zalathar · 2023-12-14T03:21:42Z

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

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.

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.

rustbot assigned wesleywiser Nov 24, 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 Nov 24, 2023

Zalathar commented Nov 24, 2023

View reviewed changes

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

Zalathar force-pushed the hir-info branch from 3337372 to f3e8d51 Compare November 25, 2023 01:37

Swatinem approved these changes Nov 25, 2023

View reviewed changes

Zalathar force-pushed the hir-info branch from f3e8d51 to d29d7f1 Compare November 26, 2023 02:22

Zalathar mentioned this pull request Nov 26, 2023

Draft of branch coverage support #118305

Closed

Zalathar force-pushed the hir-info branch 9 times, most recently from 7f3f9d3 to e8381de Compare December 3, 2023 02:15

cjgillot reviewed Dec 3, 2023

View reviewed changes

compiler/rustc_mir_build/src/build/coverageinfo.rs Outdated Show resolved Hide resolved

Zalathar force-pushed the hir-info branch 2 times, most recently from da6f75e to 1bc26f4 Compare December 5, 2023 03:13

Zalathar added 4 commits December 10, 2023 09:24

coverage: Assert that the instrumentor never sees promoted MIR

5adf145

coverage: Extract is_eligible_for_coverage

3babb38

coverage: Extract helper for getting HIR info for coverage

6421752

coverage: Simplify make_coverage_hir_info

623ef37

Zalathar added 4 commits December 10, 2023 09:24

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.

coverage: Remove some FIXMEs that are now fixed

8b62d93

coverage: Don't use HIR to determine whether a def is fn-like

2106aaa

Zalathar force-pushed the hir-info branch from 1bc26f4 to 2106aaa Compare December 9, 2023 22:44

Zalathar mentioned this pull request Dec 14, 2023

coverage: Tidy up early parts of the instrumentor pass #118929

Merged

Zalathar closed this Dec 14, 2023

Zalathar deleted the hir-info branch July 1, 2024 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

coverage: Collect HIR info during MIR building #118237

coverage: Collect HIR info during MIR building #118237
Zalathar wants to merge 8 commits intorust-lang:masterfrom
Zalathar:hir-info

Zalathar commented Nov 24, 2023 •

edited

Loading

Uh oh!

rustbot commented Nov 24, 2023

Uh oh!

rustbot commented Nov 24, 2023

Uh oh!

Zalathar Nov 24, 2023

Uh oh!

Zalathar commented Nov 24, 2023

Uh oh!

cjgillot left a comment

Uh oh!

Uh oh!

Zalathar commented Dec 4, 2023

Uh oh!

Zalathar commented Dec 4, 2023

Uh oh!

Zalathar commented Dec 11, 2023

Uh oh!

bors commented Dec 13, 2023

Uh oh!

Zalathar commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

Zalathar commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 24, 2023

Uh oh!

rustbot commented Nov 24, 2023

Uh oh!

Zalathar Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Nov 24, 2023

Uh oh!

cjgillot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zalathar commented Dec 4, 2023

Uh oh!

Zalathar commented Dec 4, 2023

Uh oh!

Zalathar commented Dec 11, 2023

Uh oh!

bors commented Dec 13, 2023

Uh oh!

Zalathar commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Zalathar commented Nov 24, 2023 •

edited

Loading