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

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|}