Translate counters from Rust 1-based to LLVM 0-based counter ids #83774

richkadel · 2021-04-02T07:15:29Z

A colleague contacted me and asked why Rust's counters start at 1, when
Clangs appear to start at 0. There is a reason why Rust's internal
counters start at 1 (see the docs), and I tried to keep them consistent
when codegenned to LLVM's coverage mapping format. LLVM should be
tolerant of missing counters, but as my colleague pointed out,
llvm-cov will silently fail to generate a coverage report for a
function based on LLVM's assumption that the counters are 0-based.

See:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp#L170

Apparently, if, for example, a function has no branches, it would have
exactly 1 counter. CounterValues.size() would be 1, and (with the
1-based index), the counter ID would be 1. This would fail the check
and abort reporting coverage for the function.

It turns out that by correcting for this during coverage map generation,
by subtracting 1 from the Rust Counter ID (both when generating the
counter increment intrinsic call, and when adding counters to the map),
some uncovered functions (including in tests) now appear covered! This
corrects the coverage for a few tests!

r? @tmandry
FYI: @wesleywiser

richkadel · 2021-04-02T07:18:23Z

I'll rebase this, and it will only change 10 files. I need to wrap up #83755 first though.

These are the changed files:

compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs
compiler/rustc_codegen_ssa/src/coverageinfo/map.rs
compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs
compiler/rustc_middle/src/mir/coverage.rs
src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async.txt
src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt
src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline.txt
src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt
src/test/run-make-fulldeps/coverage/async.rs
src/test/run-make-fulldeps/coverage/loops_branches.rs

richkadel · 2021-04-02T18:54:36Z

I went ahead and rebased on upstream/master. All of the spanview changed files (8, I think), will go away once #83755 lands.

Really cool that this fix resolved another mystery in compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs. Now I know why I had to add both a Counter and an Unreachable for the same region, in add_unused_function_coverage(). I no longer need to do that.

@tmandry - I'm looking into making one more change relevant to this PR and will probably add it as a second commit, if it's not too complex.

compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs

compiler/rustc_middle/src/mir/coverage.rs

nagisa · 2021-04-02T22:12:48Z

compiler/rustc_codegen_ssa/src/coverageinfo/map.rs

+                if !(lhs_counter.is_zero() && op.is_subtract()) {
+                    debug_assert!(
+                        lhs_counter.is_zero()
+                            || ((lhs_counter.zero_based_id() as usize)


Pedantic nit: This can wrap for usize = u16, but unclear if the compiler built for a platform where this holds true is going be at all useful.

I would perhaps write this as:

usize::max(self.counters.len(), self.expressions.len())) .try_into::<u32>() .map(|v| lhs_counter.zero_based_id() <= v) .unwrap_or(true)

Though, I guess, if self.counters.len() >= u32::MAX things are probably already going very wrong everywhere anyway.

I think I get it, but this is a lot harder to understand what's going on, and why. I think if usize = u16 and the zero_based_id() > usize::MAX then a lot of things are probably breaking/broken.

Plus this is in a debug_assert!(), so unless we're going to write a test case that creates thousands of branches to catch this, we're unlikely to even execute this check (in production code, particularly when targeting a small machine like this).

I'm tempted to leave this as-is to save others the mental exercise, but I can add a comment.

@nagisa - You said it's a "nit", but do let me know if you strongly disagree.

FYI, I just added a comment above each as usize so at least the concern is captured there.

richkadel · 2021-04-02T23:06:34Z

@tmandry - I'm looking into making one more change relevant to this PR and will probably add it as a second commit, if it's not too complex.

@tmandry I'm not going to add anything else to this PR. I thought I had an easy solution to adding coverage for branches that are optimized out (e..g., MIR const eval-based), but I forgot that, currently, I can only get the coverage from the optimized_mir() (where the missing coverage has already been dropped).

I want to get a version of the MIR prior to optimizations, but calls like mir_promoted and promoted_mir only return results that have already been "stolen" (I think that means I can't access them anymore).

So let's land this PR as-is and look at what to do to get the dropped coverage at a later time.

tmandry · 2021-04-02T23:22:45Z

compiler/rustc_middle/src/mir/coverage.rs

+    /// during codegen. LLVM expects zero-based indexes.
+    pub fn zero_based_index(&self) -> u32 {
+        let one_based_index = self.as_u32();
+        debug_assert!(one_based_index > 0);


fyi: not necessary since underflow already panics in debug mode

ack... I think I'll leave it though, just for the readability of the message (looks less like an unexpected bug and more like an assertion)

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt

compiler/rustc_codegen_ssa/src/coverageinfo/map.rs

A colleague contacted me and asked why Rust's counters start at 1, when Clangs appear to start at 0. There is a reason why Rust's internal counters start at 1 (see the docs), and I tried to keep them consistent when codegenned to LLVM's coverage mapping format. LLVM should be tolerant of missing counters, but as my colleague pointed out, `llvm-cov` will silently fail to generate a coverage report for a function based on LLVM's assumption that the counters are 0-based. See: https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp#L170 Apparently, if, for example, a function has no branches, it would have exactly 1 counter. `CounterValues.size()` would be 1, and (with the 1-based index), the counter ID would be 1. This would fail the check and abort reporting coverage for the function. It turns out that by correcting for this during coverage map generation, by subtracting 1 from the Rust Counter ID (both when generating the counter increment intrinsic call, and when adding counters to the map), some uncovered functions (including in tests) now appear covered! This corrects the coverage for a few tests!

tmandry · 2021-04-03T00:17:15Z

@bors r+

bors · 2021-04-03T00:17:16Z

📌 Commit 7ceff68 has been approved by tmandry

…andry Translate counters from Rust 1-based to LLVM 0-based counter ids A colleague contacted me and asked why Rust's counters start at 1, when Clangs appear to start at 0. There is a reason why Rust's internal counters start at 1 (see the docs), and I tried to keep them consistent when codegenned to LLVM's coverage mapping format. LLVM should be tolerant of missing counters, but as my colleague pointed out, `llvm-cov` will silently fail to generate a coverage report for a function based on LLVM's assumption that the counters are 0-based. See: https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp#L170 Apparently, if, for example, a function has no branches, it would have exactly 1 counter. `CounterValues.size()` would be 1, and (with the 1-based index), the counter ID would be 1. This would fail the check and abort reporting coverage for the function. It turns out that by correcting for this during coverage map generation, by subtracting 1 from the Rust Counter ID (both when generating the counter increment intrinsic call, and when adding counters to the map), some uncovered functions (including in tests) now appear covered! This corrects the coverage for a few tests! r? `@tmandry` FYI: `@wesleywiser`

bors · 2021-04-03T06:27:07Z

⌛ Testing commit 7ceff68 with merge 836c317...

bors · 2021-04-03T08:45:58Z

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

For source-based coverage, the frontend sets the counter IDs and the constraints of counter IDs is not defined. For e.g., the Rust frontend until recently had a reserved counter #0 (rust-lang/rust#83774). Rust coverage instrumentation also creates counters on edges in addition to basic blocks. Some functions may have more counters than regions. This breaks an assumption in CoverageMapping.cpp where the number of counters in a function is assumed to be bounded by the number of regions: Counts.assign(Record.MappingRegions.size(), 0); This assumption causes CounterMappingContext::evaluate() to fail since there are not enough counter values created in the above call to `Counts.assign`. Consequently, some uncovered functions are not reported in coverage reports. This change walks a Function's CoverageMappingRecord to find the maximum counter ID, and uses it to initialize the counter array when instrprof records are missing for a function in sparse profiles. Differential Revision: https://reviews.llvm.org/D101780

rust-highfive assigned tmandry Apr 2, 2021

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

richkadel force-pushed the zero-based-counters branch 4 times, most recently from e947b8b to 0cade41 Compare April 2, 2021 18:52

nagisa reviewed Apr 2, 2021

View reviewed changes

compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs Outdated Show resolved Hide resolved

nagisa reviewed Apr 2, 2021

View reviewed changes

compiler/rustc_middle/src/mir/coverage.rs Outdated Show resolved Hide resolved

nagisa reviewed Apr 2, 2021

View reviewed changes

richkadel force-pushed the zero-based-counters branch from 0cade41 to 1da70a0 Compare April 2, 2021 22:59

richkadel force-pushed the zero-based-counters branch from 1da70a0 to df55114 Compare April 2, 2021 23:14

tmandry reviewed Apr 2, 2021

View reviewed changes

richkadel force-pushed the zero-based-counters branch from df55114 to 6ef77e0 Compare April 3, 2021 00:02

tmandry reviewed Apr 3, 2021

View reviewed changes

compiler/rustc_codegen_ssa/src/coverageinfo/map.rs Outdated Show resolved Hide resolved

richkadel force-pushed the zero-based-counters branch from 6ef77e0 to 7ceff68 Compare April 3, 2021 00:16

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 3, 2021

Dylan-DPC-zz mentioned this pull request Apr 3, 2021

Rollup of 6 pull requests #83798

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 3, 2021

bors merged commit 836c317 into rust-lang:master Apr 3, 2021

rustbot added this to the 1.53.0 milestone Apr 3, 2021

This was referenced Apr 7, 2021

Source-code based coverage is failing to identify some executable lines #83985

Closed

Some Rust functions still missing coverage (-Z instrument-coverage) #79617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate counters from Rust 1-based to LLVM 0-based counter ids #83774

Translate counters from Rust 1-based to LLVM 0-based counter ids #83774

richkadel commented Apr 2, 2021 •

edited

Loading

richkadel commented Apr 2, 2021

richkadel commented Apr 2, 2021

nagisa Apr 2, 2021 •

edited

Loading

richkadel Apr 2, 2021

richkadel Apr 2, 2021

richkadel commented Apr 2, 2021

tmandry Apr 2, 2021

richkadel Apr 3, 2021

tmandry commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

Translate counters from Rust 1-based to LLVM 0-based counter ids #83774

Translate counters from Rust 1-based to LLVM 0-based counter ids #83774

Conversation

richkadel commented Apr 2, 2021 • edited Loading

richkadel commented Apr 2, 2021

richkadel commented Apr 2, 2021

nagisa Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

richkadel Apr 2, 2021

Choose a reason for hiding this comment

richkadel Apr 2, 2021

Choose a reason for hiding this comment

richkadel commented Apr 2, 2021

tmandry Apr 2, 2021

Choose a reason for hiding this comment

richkadel Apr 3, 2021

Choose a reason for hiding this comment

tmandry commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

richkadel commented Apr 2, 2021 •

edited

Loading

nagisa Apr 2, 2021 •

edited

Loading