Report coverage `0` of dead blocks #84797

richkadel · 2021-05-01T22:07:06Z

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 Statements and adds their code_regions as
Unreachable coverage Statements 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).

r? @tmandry
cc: @wesleywiser

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.

tmandry · 2021-05-04T01:45:33Z

@bors r+

bors · 2021-05-04T01:45:34Z

📌 Commit 3fca198 has been approved by tmandry

…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`

…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``

…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```

…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````

…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`````

…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``````

…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```````

richkadel · 2021-05-06T15:43:23Z

This looks like it could be an important difference between run-make and run-make-fulldeps.

rust/src/bootstrap/test.rs

Lines 1380 to 1387 in 3fca198

    
           // Tests that use compiler libraries may inherit the `-lLLVM` link 
        
           // requirement, but the `-L` library path is not propagated across 
        
           // separate compilations. We can add LLVM's library path to the 
        
           // platform-specific environment variable as a workaround. 
        
           if !builder.config.dry_run && suite.ends_with("fulldeps") { 
        
               let llvm_libdir = output(Command::new(&llvm_config).arg("--libdir")); 
        
               add_link_lib_path(vec![llvm_libdir.trim().into()], &mut cmd); 
        
           }

@cuviper - It looks like you may have added this in the last year or so. Is there any reason not to also add LLVM's library path to run-make?

Do you think this would explain the error on MUSL target(s)?

failed to execute command: "musl-g++" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m32" "-march=i686" "-Wl,-melf_i386" "-static" "-Wa,-mrelax-relocations=no" "-print-file-name=libstdc++.a"
error: No such file or directory (os error 2)

richkadel · 2021-05-06T15:46:12Z

By the way, I also just realized this would be the first run-make test to include # needs-profiler-support.

So moving these tests to run-make is not quite as cut and dry as I was led to believe. :-(

richkadel · 2021-05-06T16:31:06Z

I removed the commit that moved the tests to run-make.

Can someone please re-approve/send to bors?

Thanks!

ghost · 2021-05-06T16:32:06Z

@richkadel You might want to remove "fixes #83830" from the PR description.

richkadel · 2021-05-06T16:33:21Z

Good catch! Done.

Dylan-DPC-zz · 2021-05-06T16:34:42Z

@bors r=tmandry rollup=iffy

bors · 2021-05-06T16:34:43Z

📌 Commit 0b0d293 has been approved by tmandry

bors · 2021-05-07T10:06:45Z

⌛ Testing commit 0b0d293 with merge e5f83d2...

bors · 2021-05-07T12:47:46Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing e5f83d2 to master...

Fixes: rust-lang#83830 The first commit was migrated from another PR that failed because CI had errors likely resulting from trying to run the coverage tests in run-make. (See: rust-lang#84797 (comment)) So moving the tests should be done as it's own separate PR. To attempt to resolve those CI errors, this PR also updates bootstrap to add LLVM library link path to run-make. When moving coverage tests from run-make-fulldeps to run-make, some targets failed in CI with an obscure message: failed to execute command: "musl-g++" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m32" "-march=i686" "-Wl,-melf_i386" "-static" "-Wa,-mrelax-relocations=no" "-print-file-name=libstdc++.a" error: No such file or directory (os error 2) The coverage tests include # needs-profiler-support and these are the first run-make tests to require it (as far as I can tell). There is a special case in bootstrap for adding the LLVM library link path, and it applies to run-make-fulldeps. This commit adds it for run-make as well.

Fixes: rust-lang#85081 An `assert!()` in `FunctionCoverage` failed, because PR rust-lang#84797 replaces unreachable `Counter`s with `Unreachable` code regions to be added to the function's coverage map. To fix it, and still generate valid LLVM coverage IR, convert one unreachable to a `Counter`.

richkadel · 2021-05-11T19:44:06Z

It looks like this commit is causing two issues, an ICE (as explained in #85081 ) and (with the ICE fixed in proposed PR #85082) malformed coverage data that results in an llvm-cov error when trying to generate a report.

With the proposed ICE fix, now I see the llvm-cov errors, and I confirmed these errors do not occur if I revert this PR #84797.

I have a strong theory (based on a conversation with llvm-cov maintainers) that I was on the right track to retain Coverage statements when dead blocks are eliminated, but I think I do not want to replace them with CoverageKind::Unreachable. Instead, I probably want to keep the Counter and Expression data, but indicate that the counter statements should not be codeegenned. (I'm confirming this last part with the llvm-cov guys.)

I'm going to revert this commit since I'm still waiting for answers, and in the mean time, -Zinstrument-coverage is probably failing for most users, for non-trivial code bases.

…tatements, r=tmandry" This reverts commit e5f83d2, reversing changes made to ac888e8.

…-statements, r=tmandry Revert "Auto merge of rust-lang#84797 - richkadel:cover-unreachable-statements… This reverts commit e5f83d2, reversing changes made to ac888e8. See rust-lang#84797 (comment) r? `@tmandry`

Rollup of 8 pull requests Successful merges: - rust-lang#83501 (rustdoc: Add unstable CLI option to show basic type layout information) - rust-lang#85018 (shrinking the deprecated method span) - rust-lang#85124 (rustdoc: remove explicit boolean comparisons.) - rust-lang#85136 (Change param name (k to key and v to value) in std::env module) - rust-lang#85162 (Fix typo in variable name) - rust-lang#85187 (Use .name_str() to format primitive types in error messages) - rust-lang#85191 (Improve rustdoc gui tester) - rust-lang#85196 (Revert "Auto merge of rust-lang#84797 - richkadel:cover-unreachable-statements…) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

...by saving dead counters/expressions. Unfortunately, I'm still getting the same result, when trying to get coverage from Fuchsia apps (at least those using fuchsia_async, but that may not be the specific cause): When running `llvm-cov` to get the coverage report, I get the well known and still useless message: ``` Failed to load coverage: Malformed instrumentation profile data ``` I can change CoverageMappingReader.cpp to avoid failing and I will see some coverage results (maybe most of the coverage results) but there is missing coverage as well. I don't know why the function name is not found. The "hack" is, change this: ```c++ if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName)) return Err; if (FuncName.empty()) return make_error<InstrProfError>(instrprof_error::malformed); ++CovMapNumUsedRecords; ``` to this: ```c++ if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName)) return Err; if (FuncName.empty()) FuncName = "MissingFuncNameForCoverage"; ++CovMapNumUsedRecords; ``` I did learn that the original implementation (replacing counters and expressions with Zero/unreachable counters), so this PR addresses that issue. I hoped that it would fix the problem, but it didn't. Specifically regarding the LLVM coverage adjustment (improvement?) made in this version, compared to the reverted PR (rust-lang#84797): LLVM requires all counters with code regions be added to the coverage map. The original dead block fix replaced the counters and expressions with a Zero (unreachable) code region. This commit saves the original counter or expression, without adding a statement to increment the counter for the eliminated dead block.

rust-highfive assigned tmandry May 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2021

This was referenced May 1, 2021

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

Closed

Update coverage docs rust-lang/rustc-dev-guide#1122

Merged

Removes unneeded check of #[no_coverage] in mapgen #84875

Merged

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 May 4, 2021

richkadel mentioned this pull request May 4, 2021

Coverage instruments closure bodies in macros (not the macro body) #84897

Merged

Dylan-DPC-zz mentioned this pull request May 4, 2021

Rollup of 11 pull requests #84899

Closed

Dylan-DPC-zz mentioned this pull request May 4, 2021

Rollup of 11 pull requests #84907

Closed

Dylan-DPC-zz mentioned this pull request May 4, 2021

Rollup of 12 pull requests #84912

Closed

Dylan-DPC-zz mentioned this pull request May 4, 2021

Rollup of 12 pull requests #84924

Closed

Dylan-DPC-zz mentioned this pull request May 4, 2021

Rollup of 15 pull requests #84928

Closed

Dylan-DPC-zz mentioned this pull request May 4, 2021

Rollup of 15 pull requests #84932

Closed

Dylan-DPC-zz mentioned this pull request May 5, 2021

Rollup of 11 pull requests #84951

Closed

richkadel force-pushed the cover-unreachable-statements branch from 3fca198 to 0b0d293 Compare May 6, 2021 16:07

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 May 6, 2021

richkadel mentioned this pull request May 6, 2021

Move coverage tests from run-make-fulldeps to run-make #85007

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2021

bors merged commit e5f83d2 into rust-lang:master May 7, 2021

rustbot added this to the 1.54.0 milestone May 7, 2021

richkadel mentioned this pull request May 8, 2021

PR #84797 causes an ICE in some situations #85081

Closed

richkadel mentioned this pull request May 8, 2021

Fix coverage ICEs (unreachable-cov-only, spanview wrong body) #85082

Closed

richkadel added a commit to richkadel/rust that referenced this pull request May 11, 2021

Revert "Auto merge of rust-lang#84797 - richkadel:cover-unreachable-s…

bea112b

…tatements, r=tmandry" This reverts commit e5f83d2, reversing changes made to ac888e8.

richkadel mentioned this pull request May 11, 2021

Revert "Auto merge of #84797 - richkadel:cover-unreachable-statements… #85196

Merged

JohnTitor mentioned this pull request May 11, 2021

Rollup of 8 pull requests #85199

Merged

richkadel mentioned this pull request May 12, 2021

WIP - Changes to try to fix and reland cover unreachable statements #85210

Closed

matthiaskrgr mentioned this pull request May 12, 2021

ICE: instrument-coverage on generator-storage-dead-unwind.rs #85213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report coverage `0` of dead blocks #84797

Report coverage `0` of dead blocks #84797

richkadel commented May 1, 2021 •

edited

Loading

tmandry commented May 4, 2021

bors commented May 4, 2021

richkadel commented May 6, 2021

richkadel commented May 6, 2021

richkadel commented May 6, 2021

ghost commented May 6, 2021 •

edited by ghost

Loading

richkadel commented May 6, 2021

Dylan-DPC-zz commented May 6, 2021

bors commented May 6, 2021

bors commented May 7, 2021

bors commented May 7, 2021

richkadel commented May 11, 2021

Report coverage 0 of dead blocks #84797

Report coverage 0 of dead blocks #84797

Conversation

richkadel commented May 1, 2021 • edited Loading

tmandry commented May 4, 2021

bors commented May 4, 2021

richkadel commented May 6, 2021

richkadel commented May 6, 2021

richkadel commented May 6, 2021

ghost commented May 6, 2021 • edited by ghost Loading

richkadel commented May 6, 2021

Dylan-DPC-zz commented May 6, 2021

bors commented May 6, 2021

bors commented May 7, 2021

bors commented May 7, 2021

richkadel commented May 11, 2021

Report coverage `0` of dead blocks #84797

Report coverage `0` of dead blocks #84797

richkadel commented May 1, 2021 •

edited

Loading

ghost commented May 6, 2021 •

edited by ghost

Loading