Skip to content

Commit c6a4922

Browse files
jhprattjhpratt
authored
Rollup merge of #122764 - Zalathar:loopy, r=oli-obk
coverage: Remove incorrect assertions from counter allocation These assertions detect situations where a BCB node (in the coverage graph) would have both a physical counter and one or more in-edge counters/expressions. For most BCBs that situation would indicate an implementation bug. However, it's perfectly fine in the case of a BCB having an edge that loops back to itself. Given the complexity and risk involved in fixing the assertions, and the fact that nothing relies on them actually being true, this patch just removes them instead. Fixes #122738. `````@rustbot````` label +A-code-coverage
2 parents 4e792df + 2f21e4f commit c6a4922

File tree

4 files changed

+102
-35
lines changed

4 files changed

+102
-35
lines changed

‎compiler/rustc_mir_transform/src/coverage/counters.rs

+4-35
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,12 @@
1+
use std::fmt::{self, Debug};
2+
13
use rustc_data_structures::captures::Captures;
24
use rustc_data_structures::fx::FxHashMap;
35
use rustc_data_structures::graph::WithNumNodes;
4-
use rustc_index::bit_set::BitSet;
56
use rustc_index::IndexVec;
6-
use rustc_middle::mir::coverage::*;
7+
use rustc_middle::mir::coverage::{CounterId, CovTerm, Expression, ExpressionId, Op};
78

8-
use super::graph::{BasicCoverageBlock, CoverageGraph, TraverseCoverageGraphWithLoops};
9-
10-
use std::fmt::{self, Debug};
9+
use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph, TraverseCoverageGraphWithLoops};
1110

1211
/// The coverage counter or counter expression associated with a particular
1312
/// BCB node or BCB edge.
@@ -18,10 +17,6 @@ pub(super) enum BcbCounter {
1817
}
1918

2019
impl BcbCounter {
21-
fn is_expression(&self) -> bool {
22-
matches!(self, Self::Expression { .. })
23-
}
24-
2520
pub(super) fn as_term(&self) -> CovTerm {
2621
match *self {
2722
BcbCounter::Counter { id, .. } => CovTerm::Counter(id),
@@ -60,10 +55,6 @@ pub(super) struct CoverageCounters {
6055
/// We currently don't iterate over this map, but if we do in the future,
6156
/// switch it back to `FxIndexMap` to avoid query stability hazards.
6257
bcb_edge_counters: FxHashMap<(BasicCoverageBlock, BasicCoverageBlock), BcbCounter>,
63-
/// Tracks which BCBs have a counter associated with some incoming edge.
64-
/// Only used by assertions, to verify that BCBs with incoming edge
65-
/// counters do not have their own physical counters (expressions are allowed).
66-
bcb_has_incoming_edge_counters: BitSet<BasicCoverageBlock>,
6758
/// Table of expression data, associating each expression ID with its
6859
/// corresponding operator (+ or -) and its LHS/RHS operands.
6960
expressions: IndexVec<ExpressionId, Expression>,
@@ -83,7 +74,6 @@ impl CoverageCounters {
8374
counter_increment_sites: IndexVec::new(),
8475
bcb_counters: IndexVec::from_elem_n(None, num_bcbs),
8576
bcb_edge_counters: FxHashMap::default(),
86-
bcb_has_incoming_edge_counters: BitSet::new_empty(num_bcbs),
8777
expressions: IndexVec::new(),
8878
};
8979

@@ -122,14 +112,6 @@ impl CoverageCounters {
122112
}
123113

124114
fn set_bcb_counter(&mut self, bcb: BasicCoverageBlock, counter_kind: BcbCounter) -> BcbCounter {
125-
assert!(
126-
// If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also
127-
// have an expression (to be injected into an existing `BasicBlock` represented by this
128-
// `BasicCoverageBlock`).
129-
counter_kind.is_expression() || !self.bcb_has_incoming_edge_counters.contains(bcb),
130-
"attempt to add a `Counter` to a BCB target with existing incoming edge counters"
131-
);
132-
133115
if let Some(replaced) = self.bcb_counters[bcb].replace(counter_kind) {
134116
bug!(
135117
"attempt to set a BasicCoverageBlock coverage counter more than once; \
@@ -146,19 +128,6 @@ impl CoverageCounters {
146128
to_bcb: BasicCoverageBlock,
147129
counter_kind: BcbCounter,
148130
) -> BcbCounter {
149-
// If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also
150-
// have an expression (to be injected into an existing `BasicBlock` represented by this
151-
// `BasicCoverageBlock`).
152-
if let Some(node_counter) = self.bcb_counter(to_bcb)
153-
&& !node_counter.is_expression()
154-
{
155-
bug!(
156-
"attempt to add an incoming edge counter from {from_bcb:?} \
157-
when the target BCB already has {node_counter:?}"
158-
);
159-
}
160-
161-
self.bcb_has_incoming_edge_counters.insert(to_bcb);
162131
if let Some(replaced) = self.bcb_edge_counters.insert((from_bcb, to_bcb), counter_kind) {
163132
bug!(
164133
"attempt to set an edge counter more than once; from_bcb: \

‎tests/coverage/let_else_loop.cov-map

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
Function name: let_else_loop::_if (unused)
2+
Raw bytes (19): 0x[01, 01, 00, 03, 00, 16, 01, 01, 0c, 00, 02, 09, 00, 10, 00, 02, 09, 00, 10]
3+
Number of files: 1
4+
- file 0 => global file 1
5+
Number of expressions: 0
6+
Number of file 0 mappings: 3
7+
- Code(Zero) at (prev + 22, 1) to (start + 1, 12)
8+
- Code(Zero) at (prev + 2, 9) to (start + 0, 16)
9+
- Code(Zero) at (prev + 2, 9) to (start + 0, 16)
10+
11+
Function name: let_else_loop::_loop_either_way (unused)
12+
Raw bytes (19): 0x[01, 01, 00, 03, 00, 0f, 01, 01, 14, 00, 01, 1c, 00, 23, 00, 01, 05, 00, 0c]
13+
Number of files: 1
14+
- file 0 => global file 1
15+
Number of expressions: 0
16+
Number of file 0 mappings: 3
17+
- Code(Zero) at (prev + 15, 1) to (start + 1, 20)
18+
- Code(Zero) at (prev + 1, 28) to (start + 0, 35)
19+
- Code(Zero) at (prev + 1, 5) to (start + 0, 12)
20+
21+
Function name: let_else_loop::loopy
22+
Raw bytes (19): 0x[01, 01, 00, 03, 01, 09, 01, 01, 14, 00, 01, 1c, 00, 23, 05, 01, 01, 00, 02]
23+
Number of files: 1
24+
- file 0 => global file 1
25+
Number of expressions: 0
26+
Number of file 0 mappings: 3
27+
- Code(Counter(0)) at (prev + 9, 1) to (start + 1, 20)
28+
- Code(Zero) at (prev + 1, 28) to (start + 0, 35)
29+
- Code(Counter(1)) at (prev + 1, 1) to (start + 0, 2)
30+

‎tests/coverage/let_else_loop.coverage

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
LL| |#![feature(coverage_attribute)]
2+
LL| |//@ edition: 2021
3+
LL| |
4+
LL| |// Regression test for <https://github.com/rust-lang/rust/issues/122738>.
5+
LL| |// These code patterns should not trigger an ICE when allocating a physical
6+
LL| |// counter to a node and also one of its in-edges, because that is allowed
7+
LL| |// when the node contains a tight loop to itself.
8+
LL| |
9+
LL| 1|fn loopy(cond: bool) {
10+
LL| 1| let true = cond else { loop {} };
11+
^0
12+
LL| 1|}
13+
LL| |
14+
LL| |// Variant that also has `loop {}` on the success path.
15+
LL| |// This isn't needed to catch the original ICE, but might help detect regressions.
16+
LL| 0|fn _loop_either_way(cond: bool) {
17+
LL| 0| let true = cond else { loop {} };
18+
LL| 0| loop {}
19+
LL| |}
20+
LL| |
21+
LL| |// Variant using regular `if` instead of let-else.
22+
LL| |// This doesn't trigger the original ICE, but might help detect regressions.
23+
LL| 0|fn _if(cond: bool) {
24+
LL| 0| if cond {
25+
LL| 0| loop {}
26+
LL| | } else {
27+
LL| 0| loop {}
28+
LL| | }
29+
LL| |}
30+
LL| |
31+
LL| |#[coverage(off)]
32+
LL| |fn main() {
33+
LL| | loopy(true);
34+
LL| |}
35+

‎tests/coverage/let_else_loop.rs

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
#![feature(coverage_attribute)]
2+
//@ edition: 2021
3+
4+
// Regression test for <https://github.com/rust-lang/rust/issues/122738>.
5+
// These code patterns should not trigger an ICE when allocating a physical
6+
// counter to a node and also one of its in-edges, because that is allowed
7+
// when the node contains a tight loop to itself.
8+
9+
fn loopy(cond: bool) {
10+
let true = cond else { loop {} };
11+
}
12+
13+
// Variant that also has `loop {}` on the success path.
14+
// This isn't needed to catch the original ICE, but might help detect regressions.
15+
fn _loop_either_way(cond: bool) {
16+
let true = cond else { loop {} };
17+
loop {}
18+
}
19+
20+
// Variant using regular `if` instead of let-else.
21+
// This doesn't trigger the original ICE, but might help detect regressions.
22+
fn _if(cond: bool) {
23+
if cond {
24+
loop {}
25+
} else {
26+
loop {}
27+
}
28+
}
29+
30+
#[coverage(off)]
31+
fn main() {
32+
loopy(true);
33+
}

0 commit comments

Comments
 (0)