Auto merge of #117123 - Zalathar:bad-counter-ids, r=petrochenkov

coverage: Consistently remove unused counter IDs from expressions/mappings

If some coverage counters were removed by MIR optimizations, we need to take care not to refer to those counter IDs in coverage mappings, and instead replace them with a constant zero value. If we don't, `llvm-cov` might see a too-large counter ID and silently discard the entire function from its coverage reports.

Fixes #117012.

Author: bors

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

@@ -102,7 +102,7 @@ impl<'tcx> FunctionCoverageCollector<'tcx> {
         // have zero as both of their operands, and will therefore always have
         // a value of zero. Other expressions that refer to these as operands
         // can have those operands replaced with `CovTerm::Zero`.
-        let mut zero_expressions = FxIndexSet::default();
+        let mut zero_expressions = ZeroExpressions::default();
 
         // Simplify a copy of each expression based on lower-numbered expressions,
         // and then update the set of always-zero expressions if necessary.
@@ -131,16 +131,16 @@ impl<'tcx> FunctionCoverageCollector<'tcx> {
                 )
             };
 
-            // If an operand refers to an expression that is always zero, then
-            // that operand can be replaced with `CovTerm::Zero`.
-            let maybe_set_operand_to_zero = |operand: &mut CovTerm| match *operand {
-                CovTerm::Expression(id) => {
+            // If an operand refers to a counter or expression that is always
+            // zero, then that operand can be replaced with `CovTerm::Zero`.
+            let maybe_set_operand_to_zero = |operand: &mut CovTerm| {
+                if let CovTerm::Expression(id) = *operand {
                     assert_operand_expression_is_lower(id);
-                    if zero_expressions.contains(&id) {
-                        *operand = CovTerm::Zero;
-                    }
                 }
-                _ => (),
+
+                if is_zero_term(&self.counters_seen, &zero_expressions, *operand) {
+                    *operand = CovTerm::Zero;
+                }
             };
             maybe_set_operand_to_zero(&mut lhs);
             maybe_set_operand_to_zero(&mut rhs);
@@ -159,7 +159,7 @@ impl<'tcx> FunctionCoverageCollector<'tcx> {
             }
         }
 
-        ZeroExpressions(zero_expressions)
+        zero_expressions
     }
 
     pub(crate) fn into_finished(self) -> FunctionCoverage<'tcx> {
@@ -205,19 +205,14 @@ impl<'tcx> FunctionCoverage<'tcx> {
         // thing on the Rust side unless we're confident we can do much better.
         // (See `CounterExpressionsMinimizer` in `CoverageMappingWriter.cpp`.)
 
-        let counter_from_operand = |operand: CovTerm| match operand {
-            CovTerm::Expression(id) if self.zero_expressions.contains(id) => Counter::ZERO,
-            _ => Counter::from_term(operand),
-        };
-
         self.function_coverage_info.expressions.iter().map(move |&Expression { lhs, op, rhs }| {
             CounterExpression {
-                lhs: counter_from_operand(lhs),
+                lhs: self.counter_for_term(lhs),
                 kind: match op {
                     Op::Add => ExprKind::Add,
                     Op::Subtract => ExprKind::Subtract,
                 },
-                rhs: counter_from_operand(rhs),
+                rhs: self.counter_for_term(rhs),
             }
         })
     }
@@ -227,34 +222,49 @@ impl<'tcx> FunctionCoverage<'tcx> {
     pub(crate) fn counter_regions(
         &self,
     ) -> impl Iterator<Item = (Counter, &CodeRegion)> + ExactSizeIterator {
-        // Historically, mappings were stored directly in counter/expression
-        // statements in MIR, and MIR optimizations would sometimes remove them.
-        // That's mostly no longer true, so now we detect cases where that would
-        // have happened, and zero out the corresponding mappings here instead.
-        let counter_for_term = move |term: CovTerm| {
-            let force_to_zero = match term {
-                CovTerm::Counter(id) => !self.counters_seen.contains(id),
-                CovTerm::Expression(id) => self.zero_expressions.contains(id),
-                CovTerm::Zero => false,
-            };
-            if force_to_zero { Counter::ZERO } else { Counter::from_term(term) }
-        };
-
         self.function_coverage_info.mappings.iter().map(move |mapping| {
             let &Mapping { term, ref code_region } = mapping;
-            let counter = counter_for_term(term);
+            let counter = self.counter_for_term(term);
             (counter, code_region)
         })
     }
+
+    fn counter_for_term(&self, term: CovTerm) -> Counter {
+        if is_zero_term(&self.counters_seen, &self.zero_expressions, term) {
+            Counter::ZERO
+        } else {
+            Counter::from_term(term)
+        }
+    }
 }
 
 /// Set of expression IDs that are known to always evaluate to zero.
 /// Any mapping or expression operand that refers to these expressions can have
 /// that reference replaced with a constant zero value.
+#[derive(Default)]
 struct ZeroExpressions(FxIndexSet<ExpressionId>);
 
 impl ZeroExpressions {
+    fn insert(&mut self, id: ExpressionId) {
+        self.0.insert(id);
+    }
+
     fn contains(&self, id: ExpressionId) -> bool {
         self.0.contains(&id)
     }
 }
+
+/// Returns `true` if the given term is known to have a value of zero, taking
+/// into account knowledge of which counters are unused and which expressions
+/// are always zero.
+fn is_zero_term(
+    counters_seen: &BitSet<CounterId>,
+    zero_expressions: &ZeroExpressions,
+    term: CovTerm,
+) -> bool {
+    match term {
+        CovTerm::Zero => true,
+        CovTerm::Counter(id) => !counters_seen.contains(id),
+        CovTerm::Expression(id) => zero_expressions.contains(id),
+    }
+}

tests/coverage-map/fn_sig_into_try.cov-map

@@ -7,47 +7,47 @@ Number of file 0 mappings: 1
 - Code(Counter(0)) at (prev + 10, 1) to (start + 4, 2)
 
 Function name: fn_sig_into_try::b
-Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 10, 01, 02, 0f, 00, 02, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
+Raw bytes (28): 0x[01, 01, 02, 01, 00, 00, 02, 04, 01, 10, 01, 02, 0f, 00, 02, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
-- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
+- expression 0 operands: lhs = Counter(0), rhs = Zero
+- expression 1 operands: lhs = Zero, rhs = Expression(0, Sub)
 Number of file 0 mappings: 4
 - Code(Counter(0)) at (prev + 16, 1) to (start + 2, 15)
 - Code(Zero) at (prev + 2, 15) to (start + 0, 16)
 - Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
-    = (c0 - c1)
+    = (c0 - Zero)
 - Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
-    = (c1 + (c0 - c1))
+    = (Zero + (c0 - Zero))
 
 Function name: fn_sig_into_try::c
-Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 16, 01, 02, 17, 00, 02, 17, 00, 18, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
+Raw bytes (28): 0x[01, 01, 02, 01, 00, 00, 02, 04, 01, 16, 01, 02, 17, 00, 02, 17, 00, 18, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
-- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
+- expression 0 operands: lhs = Counter(0), rhs = Zero
+- expression 1 operands: lhs = Zero, rhs = Expression(0, Sub)
 Number of file 0 mappings: 4
 - Code(Counter(0)) at (prev + 22, 1) to (start + 2, 23)
 - Code(Zero) at (prev + 2, 23) to (start + 0, 24)
 - Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
-    = (c0 - c1)
+    = (c0 - Zero)
 - Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
-    = (c1 + (c0 - c1))
+    = (Zero + (c0 - Zero))
 
 Function name: fn_sig_into_try::d
-Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 1c, 01, 03, 0f, 00, 03, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
+Raw bytes (28): 0x[01, 01, 02, 01, 00, 00, 02, 04, 01, 1c, 01, 03, 0f, 00, 03, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
-- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
+- expression 0 operands: lhs = Counter(0), rhs = Zero
+- expression 1 operands: lhs = Zero, rhs = Expression(0, Sub)
 Number of file 0 mappings: 4
 - Code(Counter(0)) at (prev + 28, 1) to (start + 3, 15)
 - Code(Zero) at (prev + 3, 15) to (start + 0, 16)
 - Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
-    = (c0 - c1)
+    = (c0 - Zero)
 - Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
-    = (c1 + (c0 - c1))
+    = (Zero + (c0 - Zero))
 

tests/coverage-map/status-quo/bad_counter_ids.cov-map

@@ -0,0 +1,98 @@
+Function name: <bad_counter_ids::Foo as core::cmp::PartialEq>::eq
+Raw bytes (9): 0x[01, 01, 00, 01, 01, 0c, 11, 00, 1a]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 1
+- Code(Counter(0)) at (prev + 12, 17) to (start + 0, 26)
+
+Function name: <bad_counter_ids::Foo as core::fmt::Debug>::fmt
+Raw bytes (9): 0x[01, 01, 00, 01, 01, 0c, 0a, 00, 0f]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 1
+- Code(Counter(0)) at (prev + 12, 10) to (start + 0, 15)
+
+Function name: bad_counter_ids::eq_bad
+Raw bytes (14): 0x[01, 01, 00, 02, 01, 23, 01, 02, 1f, 00, 03, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 2
+- Code(Counter(0)) at (prev + 35, 1) to (start + 2, 31)
+- Code(Zero) at (prev + 3, 1) to (start + 0, 2)
+
+Function name: bad_counter_ids::eq_bad_message
+Raw bytes (21): 0x[01, 01, 01, 01, 00, 03, 01, 28, 01, 02, 0f, 02, 02, 20, 00, 2b, 00, 01, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 1
+- expression 0 operands: lhs = Counter(0), rhs = Zero
+Number of file 0 mappings: 3
+- Code(Counter(0)) at (prev + 40, 1) to (start + 2, 15)
+- Code(Expression(0, Sub)) at (prev + 2, 32) to (start + 0, 43)
+    = (c0 - Zero)
+- Code(Zero) at (prev + 1, 1) to (start + 0, 2)
+
+Function name: bad_counter_ids::eq_good
+Raw bytes (14): 0x[01, 01, 00, 02, 01, 0f, 01, 02, 1f, 05, 03, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 2
+- Code(Counter(0)) at (prev + 15, 1) to (start + 2, 31)
+- Code(Counter(1)) at (prev + 3, 1) to (start + 0, 2)
+
+Function name: bad_counter_ids::eq_good_message
+Raw bytes (19): 0x[01, 01, 00, 03, 01, 14, 01, 02, 0f, 00, 02, 20, 00, 2b, 05, 01, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 3
+- Code(Counter(0)) at (prev + 20, 1) to (start + 2, 15)
+- Code(Zero) at (prev + 2, 32) to (start + 0, 43)
+- Code(Counter(1)) at (prev + 1, 1) to (start + 0, 2)
+
+Function name: bad_counter_ids::ne_bad
+Raw bytes (14): 0x[01, 01, 00, 02, 01, 2d, 01, 02, 1f, 00, 03, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 2
+- Code(Counter(0)) at (prev + 45, 1) to (start + 2, 31)
+- Code(Zero) at (prev + 3, 1) to (start + 0, 2)
+
+Function name: bad_counter_ids::ne_bad_message
+Raw bytes (19): 0x[01, 01, 00, 03, 01, 32, 01, 02, 0f, 05, 02, 20, 00, 2b, 00, 01, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 3
+- Code(Counter(0)) at (prev + 50, 1) to (start + 2, 15)
+- Code(Counter(1)) at (prev + 2, 32) to (start + 0, 43)
+- Code(Zero) at (prev + 1, 1) to (start + 0, 2)
+
+Function name: bad_counter_ids::ne_good
+Raw bytes (16): 0x[01, 01, 01, 01, 00, 02, 01, 19, 01, 02, 1f, 02, 03, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 1
+- expression 0 operands: lhs = Counter(0), rhs = Zero
+Number of file 0 mappings: 2
+- Code(Counter(0)) at (prev + 25, 1) to (start + 2, 31)
+- Code(Expression(0, Sub)) at (prev + 3, 1) to (start + 0, 2)
+    = (c0 - Zero)
+
+Function name: bad_counter_ids::ne_good_message
+Raw bytes (21): 0x[01, 01, 01, 01, 00, 03, 01, 1e, 01, 02, 0f, 00, 02, 20, 00, 2b, 02, 01, 01, 00, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 1
+- expression 0 operands: lhs = Counter(0), rhs = Zero
+Number of file 0 mappings: 3
+- Code(Counter(0)) at (prev + 30, 1) to (start + 2, 15)
+- Code(Zero) at (prev + 2, 32) to (start + 0, 43)
+- Code(Expression(0, Sub)) at (prev + 1, 1) to (start + 0, 2)
+    = (c0 - Zero)
+

tests/coverage-map/status-quo/bad_counter_ids.rs

@@ -0,0 +1,66 @@
+#![feature(coverage_attribute)]
+// compile-flags: --edition=2021 -Copt-level=0 -Zmir-opt-level=3
+
+// Regression test for <https://github.com/rust-lang/rust/issues/117012>.
+//
+// If some coverage counters were removed by MIR optimizations, we need to take
+// care not to refer to those counter IDs in coverage mappings, and instead
+// replace them with a constant zero value. If we don't, `llvm-cov` might see
+// a too-large counter ID and silently discard the entire function from its
+// coverage reports.
+
+#[derive(Debug, PartialEq, Eq)]
+struct Foo(u32);
+
+fn eq_good() {
+    println!("a");
+    assert_eq!(Foo(1), Foo(1));
+}
+
+fn eq_good_message() {
+    println!("b");
+    assert_eq!(Foo(1), Foo(1), "message b");
+}
+
+fn ne_good() {
+    println!("c");
+    assert_ne!(Foo(1), Foo(3));
+}
+
+fn ne_good_message() {
+    println!("d");
+    assert_ne!(Foo(1), Foo(3), "message d");
+}
+
+fn eq_bad() {
+    println!("e");
+    assert_eq!(Foo(1), Foo(3));
+}
+
+fn eq_bad_message() {
+    println!("f");
+    assert_eq!(Foo(1), Foo(3), "message f");
+}
+
+fn ne_bad() {
+    println!("g");
+    assert_ne!(Foo(1), Foo(1));
+}
+
+fn ne_bad_message() {
+    println!("h");
+    assert_ne!(Foo(1), Foo(1), "message h");
+}
+
+#[coverage(off)]
+fn main() {
+    eq_good();
+    eq_good_message();
+    ne_good();
+    ne_good_message();
+
+    assert!(std::panic::catch_unwind(eq_bad).is_err());
+    assert!(std::panic::catch_unwind(eq_bad_message).is_err());
+    assert!(std::panic::catch_unwind(ne_bad).is_err());
+    assert!(std::panic::catch_unwind(ne_bad_message).is_err());
+}