Make coverage counter IDs count up from 0, not 1

Operand types are now tracked explicitly, so there is no need to reserve ID 0 for the special always-zero counter. As part of the renumbering, this change fixes an off-by-one error in the way counters were counted by the `coverageinfo` query. As a result, functions should now have exactly the number of counters they actually need, instead of always having an extra counter that is never used.
rust-lang · Aug 1, 2023 · 3920e07 · 3920e07
1 parent f103db8
commit 3920e07
Show file tree

Hide file tree

Showing 12 changed files with 43 additions and 61 deletions.
diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs
@@ -1,4 +1,4 @@
-use rustc_middle::mir::coverage::{CounterValueReference, MappedExpressionIndex};
+use rustc_middle::mir::coverage::{CounterId, MappedExpressionIndex};
 
 /// Must match the layout of `LLVMRustCounterKind`.
 #[derive(Copy, Clone, Debug)]
@@ -36,11 +36,9 @@ impl Counter {
         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.zero_based_index() }
+    /// Constructs a new `Counter` of kind `CounterValueReference`.
+    pub fn counter_value_reference(counter_id: CounterId) -> Self {
+        Self { kind: CounterKind::CounterValueReference, id: counter_id.as_u32() }
     }
 
     /// Constructs a new `Counter` of kind `Expression`.

diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
@@ -3,7 +3,7 @@ pub use super::ffi::*;
 use rustc_index::{IndexSlice, IndexVec};
 use rustc_middle::bug;
 use rustc_middle::mir::coverage::{
-    CodeRegion, CounterValueReference, ExpressionId, MappedExpressionIndex, Op, Operand,
+    CodeRegion, CounterId, ExpressionId, MappedExpressionIndex, Op, Operand,
 };
 use rustc_middle::ty::Instance;
 use rustc_middle::ty::TyCtxt;
@@ -32,7 +32,7 @@ pub struct FunctionCoverage<'tcx> {
     instance: Instance<'tcx>,
     source_hash: u64,
     is_used: bool,
-    counters: IndexVec<CounterValueReference, Option<CodeRegion>>,
+    counters: IndexVec<CounterId, Option<CodeRegion>>,
     expressions: IndexVec<ExpressionId, Option<Expression>>,
     unreachable_regions: Vec<CodeRegion>,
 }
@@ -80,7 +80,7 @@ impl<'tcx> FunctionCoverage<'tcx> {
     }
 
     /// Adds a code region to be counted by an injected counter intrinsic.
-    pub fn add_counter(&mut self, id: CounterValueReference, region: CodeRegion) {
+    pub fn add_counter(&mut self, id: CounterId, region: CodeRegion) {
         if let Some(previous_region) = self.counters[id].replace(region.clone()) {
             assert_eq!(previous_region, region, "add_counter: code region for id changed");
         }
@@ -192,18 +192,7 @@ impl<'tcx> FunctionCoverage<'tcx> {
         type NewIndexes = IndexSlice<ExpressionId, Option<MappedExpressionIndex>>;
         let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand {
             Operand::Zero => Some(Counter::zero()),
-            Operand::Counter(id) => {
-                debug_assert!(
-                    id.index() > 0,
-                    "Operand::Counter 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))
-            }
+            Operand::Counter(id) => Some(Counter::counter_value_reference(id)),
             Operand::Expression(id) => {
                 self.expressions
                     .get(id)

diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
@@ -16,9 +16,7 @@ use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
 use rustc_llvm::RustString;
 use rustc_middle::bug;
-use rustc_middle::mir::coverage::{
-    CodeRegion, CounterValueReference, CoverageKind, ExpressionId, Op, Operand,
-};
+use rustc_middle::mir::coverage::{CodeRegion, CounterId, CoverageKind, ExpressionId, Op, Operand};
 use rustc_middle::mir::Coverage;
 use rustc_middle::ty;
 use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt};
@@ -33,7 +31,7 @@ mod ffi;
 pub(crate) mod map_data;
 pub mod mapgen;
 
-const UNUSED_FUNCTION_COUNTER_ID: CounterValueReference = CounterValueReference::START;
+const UNUSED_FUNCTION_COUNTER_ID: CounterId = CounterId::START;
 
 const VAR_ALIGN_BYTES: usize = 8;
 
@@ -125,7 +123,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
                     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(id.zero_based_index());
+                    let index = bx.const_u32(id.as_u32());
                     debug!(
                         "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})",
                         fn_name, hash, num_counters, index,
@@ -178,7 +176,7 @@ impl<'tcx> Builder<'_, '_, 'tcx> {
     fn add_coverage_counter(
         &mut self,
         instance: Instance<'tcx>,
-        id: CounterValueReference,
+        id: CounterId,
         region: CodeRegion,
     ) -> bool {
         if let Some(coverage_context) = self.coverage_context() {

diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs
@@ -6,22 +6,22 @@ use rustc_span::Symbol;
 use std::fmt::{self, Debug, Formatter};
 
 rustc_index::newtype_index! {
+    /// ID of a coverage counter. Values ascend from 0.
+    ///
+    /// Note that LLVM handles counter IDs as `uint32_t`, so there is no need
+    /// to use a larger representation on the Rust side.
     #[derive(HashStable)]
     #[max = 0xFFFF_FFFF]
-    #[debug_format = "CounterValueReference({})"]
-    pub struct CounterValueReference {}
+    #[debug_format = "CounterId({})"]
+    pub struct CounterId {}
 }
 
-impl CounterValueReference {
-    /// Counters start at 1 for historical reasons.
-    pub const START: Self = Self::from_u32(1);
+impl CounterId {
+    pub const START: Self = Self::from_u32(0);
 
-    /// 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
+    #[inline(always)]
+    pub fn next_id(self) -> Self {
+        Self::from_u32(self.as_u32() + 1)
     }
 }
 
@@ -63,7 +63,7 @@ rustc_index::newtype_index! {
 #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
 pub enum Operand {
     Zero,
-    Counter(CounterValueReference),
+    Counter(CounterId),
     Expression(ExpressionId),
 }
 
@@ -83,7 +83,7 @@ pub enum CoverageKind {
         function_source_hash: u64,
         /// ID of this counter within its enclosing function.
         /// Expressions in the same function can refer to it as an operand.
-        id: CounterValueReference,
+        id: CounterId,
     },
     Expression {
         /// ID of this coverage-counter expression within its enclosing function.

diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs
@@ -470,7 +470,7 @@ TrivialTypeTraversalAndLiftImpls! {
     ::rustc_hir::Unsafety,
     ::rustc_target::asm::InlineAsmRegOrRegClass,
     ::rustc_target::spec::abi::Abi,
-    crate::mir::coverage::CounterValueReference,
+    crate::mir::coverage::CounterId,
     crate::mir::coverage::ExpressionId,
     crate::mir::coverage::MappedExpressionIndex,
     crate::mir::Local,

diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs
@@ -16,7 +16,7 @@ use rustc_middle::mir::coverage::*;
 /// `Coverage` statements.
 pub(super) struct CoverageCounters {
     function_source_hash: u64,
-    next_counter_id: u32,
+    next_counter_id: CounterId,
     next_expression_id: ExpressionId,
     pub debug_counters: DebugCounters,
 }
@@ -25,7 +25,7 @@ impl CoverageCounters {
     pub fn new(function_source_hash: u64) -> Self {
         Self {
             function_source_hash,
-            next_counter_id: CounterValueReference::START.as_u32(),
+            next_counter_id: CounterId::START,
             next_expression_id: ExpressionId::START,
             debug_counters: DebugCounters::new(),
         }
@@ -93,10 +93,10 @@ impl CoverageCounters {
     }
 
     /// Counter IDs start from one and go up.
-    fn next_counter(&mut self) -> CounterValueReference {
+    fn next_counter(&mut self) -> CounterId {
         let next = self.next_counter_id;
-        self.next_counter_id += 1;
-        CounterValueReference::from(next)
+        self.next_counter_id = next.next_id();
+        next
     }
 
     /// Expression IDs start from 0 and go up.

diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs
@@ -43,11 +43,9 @@ struct CoverageVisitor {
 }
 
 impl CoverageVisitor {
-    /// Updates `num_counters` to the maximum encountered zero-based counter_id plus 1. Note the
-    /// final computed number of counters should be the number of all `CoverageKind::Counter`
-    /// statements in the MIR *plus one* for the implicit `ZERO` counter.
+    /// Updates `num_counters` to the maximum encountered counter ID plus 1.
     #[inline(always)]
-    fn update_num_counters(&mut self, counter_id: CounterValueReference) {
+    fn update_num_counters(&mut self, counter_id: CounterId) {
         let counter_id = counter_id.as_u32();
         self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1);
     }
@@ -103,8 +101,7 @@ fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) ->
     let mir_body = tcx.instance_mir(instance_def);
 
     let mut coverage_visitor = CoverageVisitor {
-        // num_counters always has at least the `ZERO` counter.
-        info: CoverageInfo { num_counters: 1, num_expressions: 0 },
+        info: CoverageInfo { num_counters: 0, num_expressions: 0 },
         add_missing_operands: false,
     };
 

diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs
@@ -683,7 +683,7 @@ fn test_make_bcb_counters() {
 
         let_bcb!(1);
         assert_eq!(
-            1, // coincidentally, bcb1 has a `Counter` with id = 1
+            0, // bcb1 has a `Counter` with id = 0
             match basic_coverage_blocks[bcb1].counter().expect("should have a counter") {
                 CoverageKind::Counter { id, .. } => id,
                 _ => panic!("expected a Counter"),
@@ -693,7 +693,7 @@ fn test_make_bcb_counters() {
 
         let_bcb!(2);
         assert_eq!(
-            2, // coincidentally, bcb2 has a `Counter` with id = 2
+            1, // bcb2 has a `Counter` with id = 1
             match basic_coverage_blocks[bcb2].counter().expect("should have a counter") {
                 CoverageKind::Counter { id, .. } => id,
                 _ => panic!("expected a Counter"),

diff --git a/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot
@@ -2,5 +2,5 @@ digraph Cov_0_4 {
     graph [fontname="Courier, monospace"];
     node [fontname="Courier, monospace"];
     edge [fontname="Courier, monospace"];
-    bcb0__Cov_0_4 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 18:1-20:2<br align="left"/>    19:5-19:9: @0[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:18:1 - 20:2<br align="left"/>    20:2-20:2: @0.Return: return</td></tr><tr><td align="left" balign="left">bb0: Return</td></tr></table>>];
+    bcb0__Cov_0_4 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 18:1-20:2<br align="left"/>    19:5-19:9: @0[0]: Coverage::Counter(0) for $DIR/coverage_graphviz.rs:18:1 - 20:2<br align="left"/>    20:2-20:2: @0.Return: return</td></tr><tr><td align="left" balign="left">bb0: Return</td></tr></table>>];
 }
diff --git a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot
@@ -2,7 +2,7 @@ digraph Cov_0_3 {
     graph [fontname="Courier, monospace"];
     node [fontname="Courier, monospace"];
     edge [fontname="Courier, monospace"];
-    bcb3__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb3</td></tr><tr><td align="left" balign="left">Counter(bcb3) at 13:10-13:10<br align="left"/>    13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11</td></tr><tr><td align="left" balign="left">bb5: Goto</td></tr></table>>];
+    bcb3__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb3</td></tr><tr><td align="left" balign="left">Counter(bcb3) at 13:10-13:10<br align="left"/>    13:10-13:10: @5[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:13:10 - 13:11</td></tr><tr><td align="left" balign="left">bb5: Goto</td></tr></table>>];
     bcb2__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb2</td></tr><tr><td align="left" balign="left">Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18<br align="left"/>    12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2<br align="left"/>Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2<br align="left"/>    15:2-15:2: @4.Return: return</td></tr><tr><td align="left" balign="left">bb4: Return</td></tr></table>>];
     bcb1__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb1</td></tr><tr><td align="left" balign="left">Expression(bcb0 + bcb3) at 10:5-11:17<br align="left"/>    11:12-11:17: @2.Call: _2 = bar() -&gt; [return: bb3, unwind: bb6]</td></tr><tr><td align="left" balign="left">bb1: FalseUnwind<br align="left"/>bb2: Call</td></tr><tr><td align="left" balign="left">bb3: SwitchInt</td></tr></table>>];
     bcb0__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 9:1-9:11<br align="left"/>    </td></tr><tr><td align="left" balign="left">bb0: Goto</td></tr></table>>];

diff --git a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff
@@ -5,7 +5,7 @@
       let mut _0: bool;
 
       bb0: {
-+         Coverage::Counter(1) for /the/src/instrument_coverage.rs:20:1 - 22:2;
++         Coverage::Counter(0) for /the/src/instrument_coverage.rs:20:1 - 22:2;
           _0 = const true;
           return;
       }

diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff
@@ -8,12 +8,12 @@
       let mut _3: !;
 
       bb0: {
-+         Coverage::Counter(1) for /the/src/instrument_coverage.rs:11:1 - 11:11;
++         Coverage::Counter(0) for /the/src/instrument_coverage.rs:11:1 - 11:11;
           goto -> bb1;
       }
 
       bb1: {
-+         Coverage::Expression(0) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17;
++         Coverage::Expression(0) = Counter(0) + Counter(1) for /the/src/instrument_coverage.rs:12:5 - 13:17;
           falseUnwind -> [real: bb2, unwind: bb6];
       }
 
@@ -28,14 +28,14 @@
 
       bb4: {
 +         Coverage::Expression(2) = Expression(1) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2;
-+         Coverage::Expression(1) = Expression(0) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18;
++         Coverage::Expression(1) = Expression(0) - Counter(1) for /the/src/instrument_coverage.rs:14:13 - 14:18;
           _0 = const ();
           StorageDead(_2);
           return;
       }
 
       bb5: {
-+         Coverage::Counter(2) for /the/src/instrument_coverage.rs:15:10 - 15:11;
++         Coverage::Counter(1) for /the/src/instrument_coverage.rs:15:10 - 15:11;
           _1 = const ();
           StorageDead(_2);
           goto -> bb1;