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

Show zero coverage for unreachable blocks (e.g., dropped after const evaluation) #84018

Closed
richkadel opened this issue Apr 8, 2021 · 3 comments · Fixed by #84797 or #85385
Closed

Show zero coverage for unreachable blocks (e.g., dropped after const evaluation) #84018

richkadel opened this issue Apr 8, 2021 · 3 comments · Fixed by #84797 or #85385

Comments

@richkadel
Copy link
Contributor

Source-based code coverage (via -Z instrument-coverage) does not show coverage for unreachable blocks that were optimized out during MIR transformations. For example:

if false {
    println!("never executed");
}

Ideally the second line would show a coverage execution count of 0, but, assuming this snippet is executed, it would instead show something like:

1   if false {
        println!("never executed");
1   }

The coverage percentage for this block would be computed as 100%, but should be about 66% (by lines covered).

The MIR InstrumentCoverage pass is performed before MIR optimizations, so the coverage instrumentation is available in MIR for line 2. However, subsequent optimizations drop the MIR block, including the coverage Statement before the codegen phase. The coverage map is generated during codegen, and can use tcx queries to extract data from the optimized_mir(), but that version of the MIR has already been purged of the unreachable blocks.

As far as I can tell, tcx queries for intermediate MIR representations (for example, tcx.mir_promoted()) are not available at codegen (they have already been "stolen" by the optimization passes).

To solve this problem, I believe I need to cache at least the coverage statements (or their data at least) for the pre-optimized MIR, before those statements are potentially dropped by subsequent MIR passes.

cc: @tmandry @wesleywiser

@richkadel
Copy link
Contributor Author

I think the easiest way to resolve this would be to just disable simplification of unreachable blocks (due to const eval), if -Zinstrument-coverage.

Is there an option for that? I haven't looked yet.

@richkadel
Copy link
Contributor Author

I think the easiest way to resolve this would be to just disable simplification of unreachable blocks (due to const eval), if -Zinstrument-coverage.

Replying to my own question:

This would not have worked. I noticed (and found in-code comments to confirm) that dead code cannot be left in the MIR.

But I solved this in a better way (see PR #84797) by simply saving coverage code regions from dropped BasicBlocks as Unreachable coverage, adding them to the START_BLOCK. This seems to work really well.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? `@tmandry`
cc: `@wesleywiser`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? ``@tmandry``
cc: ``@wesleywiser``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? ```@tmandry```
cc: ```@wesleywiser```
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? ````@tmandry````
cc: ````@wesleywiser````
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? `````@tmandry`````
cc: `````@wesleywiser`````
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? ``````@tmandry``````
cc: ``````@wesleywiser``````
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 5, 2021
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? ```````@tmandry```````
cc: ```````@wesleywiser```````
bors added a commit to rust-lang-ci/rust that referenced this issue May 7, 2021
…s, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR (in [commit 1](rust-lang@0b0d293)).

r? `@tmandry`
cc: `@wesleywiser`
@bors bors closed this as completed in 0b0d293 May 7, 2021
richkadel added a commit to richkadel/rust that referenced this issue May 12, 2021
Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
finds all dropped coverage `Statement`s and adds their `code_region`s as
`Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are
still included in the coverage map.

Check out the resulting changes in the test coverage reports in this PR.
richkadel added a commit to richkadel/rust that referenced this issue May 16, 2021
Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

Note, this PR relands an earlier, reverted PR that failed when compiling
generators. Generators clone blocks, so CFG simplification is used to
remove the original `BasicBlocks`; and since they were cloned, the
original blocks should not be saved. (Saving them resulted in duplicate
coverage counters, and `llvm-cov` failed with a "Malformed coverage
data" error.

If `instrument-coverage` is enabled,
`simplify::remove_dead_blocks_with_coverage()`,
with `DroppedCoverage::SaveUnreachable`, finds all dropped coverage
`Statement`s and adds their `code_region`s as `Unreachable` coverage
`Statement`s to the `START_BLOCK`, so they are still included in the
coverage map.

Check out the resulting changes in the test coverage reports in this PR.
@richkadel
Copy link
Contributor Author

@tmandry - This issue should be re-opened, since the PR that closed it was reverted.

The new fix in #85385 is not quite working (as I just found out today) so it is still a work in progress and marked "Draft" for now.

@wesleywiser wesleywiser reopened this May 20, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 3, 2021
…erage, r=wesleywiser

Reland - Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

Note, this PR relands an earlier, reverted PR that failed when compiling
generators. The prior issues with generators has been resolved and a new
test was added to prevent future regressions.

Check out the resulting changes to test coverage of dead blocks in the
test coverage reports in this PR.

r? `@tmandry`
fyi: `@wesleywiser`
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 4, 2021
…age, r=wesleywiser

Reland - Report coverage `0` of dead blocks

Fixes: rust-lang#84018

With `-Z instrument-coverage`, coverage reporting of dead blocks
(for example, blocks dropped because a conditional branch is dropped,
based on const evaluation) is now supported.

Note, this PR relands an earlier, reverted PR that failed when compiling
generators. The prior issues with generators has been resolved and a new
test was added to prevent future regressions.

Check out the resulting changes to test coverage of dead blocks in the
test coverage reports in this PR.

r? `@tmandry`
fyi: `@wesleywiser`
@bors bors closed this as completed in f4f76e6 Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants