Skip to content

Commit

Permalink
Auto merge of #117123 - Zalathar:bad-counter-ids, r=petrochenkov
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bors committed Oct 28, 2023
2 parents 6a66ca2 + 230dd5b commit 6b78377
Show file tree
Hide file tree
Showing 12 changed files with 474 additions and 165 deletions.
72 changes: 41 additions & 31 deletions compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand All @@ -159,7 +159,7 @@ impl<'tcx> FunctionCoverageCollector<'tcx> {
}
}

ZeroExpressions(zero_expressions)
zero_expressions
}

pub(crate) fn into_finished(self) -> FunctionCoverage<'tcx> {
Expand Down Expand Up @@ -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),
}
})
}
Expand All @@ -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),
}
}
30 changes: 15 additions & 15 deletions tests/coverage-map/fn_sig_into_try.cov-map
Original file line number Diff line number Diff line change
Expand Up @@ -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))

98 changes: 98 additions & 0 deletions tests/coverage-map/status-quo/bad_counter_ids.cov-map
Original file line number Diff line number Diff line change
@@ -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)

66 changes: 66 additions & 0 deletions tests/coverage-map/status-quo/bad_counter_ids.rs
Original file line number Diff line number Diff line change
@@ -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());
}
Loading

0 comments on commit 6b78377

Please sign in to comment.