Auto merge of #83774 - richkadel:zero-based-counters, r=tmandry

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`

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -250,13 +250,9 @@ fn add_unused_function_coverage(
             // Insert at least one real counter so the LLVM CoverageMappingReader will find expected
             // definitions.
             function_coverage.add_counter(UNUSED_FUNCTION_COUNTER_ID, code_region.clone());
+        } else {
+            function_coverage.add_unreachable_region(code_region.clone());
         }
-        // Add a Zero Counter for every code region.
-        //
-        // Even though the first coverage region already has an actual Counter, `llvm-cov` will not
-        // always report it. Re-adding an unreachable region (zero counter) for the same region
-        // seems to help produce the expected coverage.
-        function_coverage.add_unreachable_region(code_region.clone());
     }
 
     if let Some(coverage_context) = cx.coverage_context() {

compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs

@@ -24,21 +24,39 @@ pub enum CounterKind {
 pub struct Counter {
     // Important: The layout (order and types of fields) must match its C++ counterpart.
     pub kind: CounterKind,
-    pub id: u32,
+    id: u32,
 }
 
 impl Counter {
+    /// Constructs a new `Counter` of kind `Zero`. For this `CounterKind`, the
+    /// `id` is not used.
     pub fn zero() -> Self {
         Self { kind: CounterKind::Zero, id: 0 }
     }
 
+    /// Constructs a new `Counter` of kind `CounterValueReference`, and converts
+    /// the given 1-based counter_id to the required 0-based equivalent for
+    /// the `Counter` encoding.
     pub fn counter_value_reference(counter_id: CounterValueReference) -> Self {
-        Self { kind: CounterKind::CounterValueReference, id: counter_id.into() }
+        Self { kind: CounterKind::CounterValueReference, id: counter_id.zero_based_index() }
     }
 
+    /// Constructs a new `Counter` of kind `Expression`.
     pub fn expression(mapped_expression_index: MappedExpressionIndex) -> Self {
         Self { kind: CounterKind::Expression, id: mapped_expression_index.into() }
     }
+
+    /// Returns true if the `Counter` kind is `Zero`.
+    pub fn is_zero(&self) -> bool {
+        matches!(self.kind, CounterKind::Zero)
+    }
+
+    /// An explicitly-named function to get the ID value, making it more obvious
+    /// that the stored value is now 0-based.
+    pub fn zero_based_id(&self) -> u32 {
+        debug_assert!(!self.is_zero(), "`id` is undefined for CounterKind::Zero");
+        self.id
+    }
 }
 
 /// Aligns with [llvm::coverage::CounterExpression::ExprKind](https://github.com/rust-lang/llvm-project/blob/rustc/11.0-2020-10-12/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L147)

compiler/rustc_codegen_ssa/src/coverageinfo/map.rs

@@ -163,9 +163,7 @@ impl<'tcx> FunctionCoverage<'tcx> {
         self.counters.iter_enumerated().filter_map(|(index, entry)| {
             // Option::map() will return None to filter out missing counters. This may happen
             // if, for example, a MIR-instrumented counter is removed during an optimization.
-            entry.as_ref().map(|region| {
-                (Counter::counter_value_reference(index as CounterValueReference), region)
-            })
+            entry.as_ref().map(|region| (Counter::counter_value_reference(index), region))
         })
     }
 
@@ -206,9 +204,15 @@ impl<'tcx> FunctionCoverage<'tcx> {
             if id == ExpressionOperandId::ZERO {
                 Some(Counter::zero())
             } else if id.index() < self.counters.len() {
+                debug_assert!(
+                    id.index() > 0,
+                    "ExpressionOperandId indexes for counters are 1-based, but this id={}",
+                    id.index()
+                );
                 // Note: Some codegen-injected Counters may be only referenced by `Expression`s,
                 // and may not have their own `CodeRegion`s,
                 let index = CounterValueReference::from(id.index());
+                // Note, the conversion to LLVM `Counter` adjusts the index to be zero-based.
                 Some(Counter::counter_value_reference(index))
             } else {
                 let index = self.expression_index(u32::from(id));
@@ -233,19 +237,60 @@ impl<'tcx> FunctionCoverage<'tcx> {
             let optional_region = &expression.region;
             let Expression { lhs, op, rhs, .. } = *expression;
 
-            if let Some(Some((lhs_counter, rhs_counter))) =
-                id_to_counter(&new_indexes, lhs).map(|lhs_counter| {
+            if let Some(Some((lhs_counter, mut rhs_counter))) = id_to_counter(&new_indexes, lhs)
+                .map(|lhs_counter| {
                     id_to_counter(&new_indexes, rhs).map(|rhs_counter| (lhs_counter, rhs_counter))
                 })
             {
+                if lhs_counter.is_zero() && op.is_subtract() {
+                    // The left side of a subtraction was probably optimized out. As an example,
+                    // a branch condition might be evaluated as a constant expression, and the
+                    // branch could be removed, dropping unused counters in the process.
+                    //
+                    // Since counters are unsigned, we must assume the result of the expression
+                    // can be no more and no less than zero. An expression known to evaluate to zero
+                    // does not need to be added to the coverage map.
+                    //
+                    // Coverage test `loops_branches.rs` includes multiple variations of branches
+                    // based on constant conditional (literal `true` or `false`), and demonstrates
+                    // that the expected counts are still correct.
+                    debug!(
+                        "Expression subtracts from zero (assume unreachable): \
+                        original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}",
+                        original_index, lhs, op, rhs, optional_region,
+                    );
+                    rhs_counter = Counter::zero();
+                }
                 debug_assert!(
-                    (lhs_counter.id as usize)
-                        < usize::max(self.counters.len(), self.expressions.len())
+                    lhs_counter.is_zero()
+                        // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16`
+                        || ((lhs_counter.zero_based_id() as usize)
+                            <= usize::max(self.counters.len(), self.expressions.len())),
+                    "lhs id={} > both counters.len()={} and expressions.len()={}
+                    ({:?} {:?} {:?})",
+                    lhs_counter.zero_based_id(),
+                    self.counters.len(),
+                    self.expressions.len(),
+                    lhs_counter,
+                    op,
+                    rhs_counter,
                 );
+
                 debug_assert!(
-                    (rhs_counter.id as usize)
-                        < usize::max(self.counters.len(), self.expressions.len())
+                    rhs_counter.is_zero()
+                        // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16`
+                        || ((rhs_counter.zero_based_id() as usize)
+                            <= usize::max(self.counters.len(), self.expressions.len())),
+                    "rhs id={} > both counters.len()={} and expressions.len()={}
+                    ({:?} {:?} {:?})",
+                    rhs_counter.zero_based_id(),
+                    self.counters.len(),
+                    self.expressions.len(),
+                    lhs_counter,
+                    op,
+                    rhs_counter,
                 );
+
                 // Both operands exist. `Expression` operands exist in `self.expressions` and have
                 // been assigned a `new_index`.
                 let mapped_expression_index =
@@ -268,11 +313,15 @@ impl<'tcx> FunctionCoverage<'tcx> {
                     expression_regions.push((Counter::expression(mapped_expression_index), region));
                 }
             } else {
-                debug!(
-                    "Ignoring expression with one or more missing operands: \
-                    original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}",
-                    original_index, lhs, op, rhs, optional_region,
-                )
+                bug!(
+                    "expression has one or more missing operands \
+                      original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}",
+                    original_index,
+                    lhs,
+                    op,
+                    rhs,
+                    optional_region,
+                );
             }
         }
         (counter_expressions, expression_regions.into_iter())

compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs

@@ -36,7 +36,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     let fn_name = bx.get_pgo_func_name_var(instance);
                     let hash = bx.const_u64(function_source_hash);
                     let num_counters = bx.const_u32(coverageinfo.num_counters);
-                    let index = bx.const_u32(u32::from(id));
+                    let index = bx.const_u32(id.zero_based_index());
                     debug!(
                         "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})",
                         fn_name, hash, num_counters, index,

compiler/rustc_middle/src/mir/coverage.rs

@@ -41,8 +41,16 @@ rustc_index::newtype_index! {
 }
 
 impl CounterValueReference {
-    // Counters start at 1 to reserve 0 for ExpressionOperandId::ZERO.
+    /// Counters start at 1 to reserve 0 for ExpressionOperandId::ZERO.
     pub const START: Self = Self::from_u32(1);
+
+    /// Returns explicitly-requested zero-based version of the counter id, used
+    /// 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);
+        one_based_index - 1
+    }
 }
 
 rustc_index::newtype_index! {
@@ -175,3 +183,13 @@ pub enum Op {
     Subtract,
     Add,
 }
+
+impl Op {
+    pub fn is_add(&self) -> bool {
+        matches!(self, Self::Add)
+    }
+
+    pub fn is_subtract(&self) -> bool {
+        matches!(self, Self::Subtract)
+    }
+}

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

@@ -10,7 +10,7 @@
    10|       |    }
    11|      1|}
    12|       |
-   13|       |async fn d() -> u8 { 1 } // should have a coverage count `0` (see below)
+   13|      0|async fn d() -> u8 { 1 }
    14|       |
    15|      0|async fn e() -> u8 { 1 } // unused function; executor does not block on `g()`
    16|       |
@@ -66,7 +66,8 @@
    63|      1|            0
    64|      1|        }
    65|      1|    }
-   66|      1|    fn d() -> u8 { 1 }
+   66|      1|    fn d() -> u8 { 1 } // inner function is defined in-line, but the function is not executed
+                  ^0
    67|      1|    fn f() -> u8 { 1 }
    68|      1|    match x {
    69|      1|        y if c(x) == y + 1 => { d(); }
@@ -115,11 +116,14 @@
   109|       |
   110|      1|    pub fn block_on<F: Future>(mut future: F) -> F::Output {
   111|      1|        let mut future = unsafe { Pin::new_unchecked(&mut future) };
-  112|      1|
+  112|      1|        use std::hint::unreachable_unchecked;
   113|      1|        static VTABLE: RawWakerVTable = RawWakerVTable::new(
-  114|      1|            |_| unimplemented!("clone"),
-  115|      1|            |_| unimplemented!("wake"),
-  116|      1|            |_| unimplemented!("wake_by_ref"),
+  114|      1|            |_| unsafe { unreachable_unchecked() }, // clone
+                              ^0
+  115|      1|            |_| unsafe { unreachable_unchecked() }, // wake
+                              ^0
+  116|      1|            |_| unsafe { unreachable_unchecked() }, // wake_by_ref
+                              ^0
   117|      1|            |_| (),
   118|      1|        );
   119|      1|        let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
@@ -132,15 +136,4 @@
   126|       |        }
   127|      1|    }
   128|       |}
-  129|       |
-  130|       |// `llvm-cov show` shows no coverage results for the `d()`, even though the
-  131|       |// crate's LLVM IR shows the function exists and has an InstrProf PGO counter,
-  132|       |// and appears to be registered like all other counted functions.
-  133|       |//
-  134|       |// `llvm-cov show --debug` output shows there is at least one `Counter` for this
-  135|       |// line, but counters do not appear in the `Combined regions` section (unlike
-  136|       |// `f()`, which is similar, but does appear in `Combined regions`, and does show
-  137|       |// coverage). The only difference is, `f()` is awaited, but the call to await
-  138|       |// `d()` is not reached. (Note: `d()` will appear in coverage if the test is
-  139|       |// modified to cause it to be awaited.)
 

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

@@ -56,7 +56,7 @@
    46|      1|//!     println!("called some_func()");
    47|      1|//! }
    48|       |//!
-   49|       |//! #[derive(Debug)]
+   49|      0|//! #[derive(Debug)]
    50|       |//! struct SomeError;
    51|       |//!
    52|       |//! extern crate doctest_crate;

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

@@ -47,7 +47,7 @@
    46|      6|}
    47|       |
    48|       |#[inline(always)]
-   49|       |fn error() {
-   50|       |    panic!("error");
-   51|       |}
+   49|      0|fn error() {
+   50|      0|    panic!("error");
+   51|      0|}
 

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

@@ -1,7 +1,7 @@
     1|       |#![allow(unused_assignments, unused_variables, while_true)]
     2|       |
-    3|       |// This test confirms an earlier problem was resolved, supporting the MIR graph generated by the
-    4|       |// structure of this `fmt` function.
+    3|       |// This test confirms that (1) unexecuted infinite loops are handled correctly by the
+    4|       |// InstrumentCoverage MIR pass; and (2) Counter Expressions that subtract from zero can be dropped.
     5|       |
     6|       |struct DebugTest;
     7|       |
@@ -16,23 +16,51 @@
                                             ^0
    16|       |        } else {
    17|       |        }
-   18|      1|        Ok(())
-   19|      1|    }
-   20|       |}
-   21|       |
-   22|      1|fn main() {
-   23|      1|    let debug_test = DebugTest;
-   24|      1|    println!("{:?}", debug_test);
-   25|      1|}
-   26|       |
-   27|       |/*
-   28|       |
-   29|       |This is the error message generated, before the issue was fixed:
-   30|       |
-   31|       |error: internal compiler error: compiler/rustc_mir/src/transform/coverage/mod.rs:374:42:
-   32|       |Error processing: DefId(0:6 ~ bug_incomplete_cov_graph_traversal_simplified[317d]::{impl#0}::fmt):
-   33|       |Error { message: "`TraverseCoverageGraphWithLoops` missed some `BasicCoverageBlock`s:
-   34|       |[bcb6, bcb7, bcb9]" }
-   35|       |
-   36|       |*/
+   18|       |
+   19|     10|        for i in 0..10 {
+   20|     10|            if true {
+   21|     10|                if false {
+   22|       |                    while true {}
+   23|     10|                }
+   24|     10|                write!(f, "error")?;
+                                                ^0
+   25|       |            } else {
+   26|       |            }
+   27|       |        }
+   28|      1|        Ok(())
+   29|      1|    }
+   30|       |}
+   31|       |
+   32|       |struct DisplayTest;
+   33|       |
+   34|       |impl std::fmt::Display for DisplayTest {
+   35|      1|    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+   36|      1|        if false {
+   37|       |        } else {
+   38|      1|            if false {
+   39|       |                while true {}
+   40|      1|            }
+   41|      1|            write!(f, "error")?;
+                                            ^0
+   42|       |        }
+   43|     10|        for i in 0..10 {
+   44|     10|            if false {
+   45|       |            } else {
+   46|     10|                if false {
+   47|       |                    while true {}
+   48|     10|                }
+   49|     10|                write!(f, "error")?;
+                                                ^0
+   50|       |            }
+   51|       |        }
+   52|      1|        Ok(())
+   53|      1|    }
+   54|       |}
+   55|       |
+   56|      1|fn main() {
+   57|      1|    let debug_test = DebugTest;
+   58|      1|    println!("{:?}", debug_test);
+   59|      1|    let display_test = DisplayTest;
+   60|      1|    println!("{}", display_test);
+   61|      1|}