From 70206f06ca5cff08d3b84240074ec89e705ab61b Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 20 Mar 2024 18:00:12 +1100 Subject: [PATCH 1/3] coverage: Regression test for ICE triggered by self-loops --- tests/coverage/let_else_loop.cov-map | 30 +++++++++++++++++++++++ tests/coverage/let_else_loop.coverage | 35 +++++++++++++++++++++++++++ tests/coverage/let_else_loop.rs | 33 +++++++++++++++++++++++++ 3 files changed, 98 insertions(+) create mode 100644 tests/coverage/let_else_loop.cov-map create mode 100644 tests/coverage/let_else_loop.coverage create mode 100644 tests/coverage/let_else_loop.rs diff --git a/tests/coverage/let_else_loop.cov-map b/tests/coverage/let_else_loop.cov-map new file mode 100644 index 0000000000000..b0cee30052200 --- /dev/null +++ b/tests/coverage/let_else_loop.cov-map @@ -0,0 +1,30 @@ +Function name: let_else_loop::_if (unused) +Raw bytes (19): 0x[01, 01, 00, 03, 00, 16, 01, 01, 0c, 00, 02, 09, 00, 10, 00, 02, 09, 00, 10] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 0 +Number of file 0 mappings: 3 +- Code(Zero) at (prev + 22, 1) to (start + 1, 12) +- Code(Zero) at (prev + 2, 9) to (start + 0, 16) +- Code(Zero) at (prev + 2, 9) to (start + 0, 16) + +Function name: let_else_loop::_loop_either_way (unused) +Raw bytes (19): 0x[01, 01, 00, 03, 00, 0f, 01, 01, 14, 00, 01, 1c, 00, 23, 00, 01, 05, 00, 0c] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 0 +Number of file 0 mappings: 3 +- Code(Zero) at (prev + 15, 1) to (start + 1, 20) +- Code(Zero) at (prev + 1, 28) to (start + 0, 35) +- Code(Zero) at (prev + 1, 5) to (start + 0, 12) + +Function name: let_else_loop::loopy +Raw bytes (19): 0x[01, 01, 00, 03, 01, 09, 01, 01, 14, 00, 01, 1c, 00, 23, 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 + 9, 1) to (start + 1, 20) +- Code(Zero) at (prev + 1, 28) to (start + 0, 35) +- Code(Counter(1)) at (prev + 1, 1) to (start + 0, 2) + diff --git a/tests/coverage/let_else_loop.coverage b/tests/coverage/let_else_loop.coverage new file mode 100644 index 0000000000000..3b3c39ff30180 --- /dev/null +++ b/tests/coverage/let_else_loop.coverage @@ -0,0 +1,35 @@ + LL| |#![feature(coverage_attribute)] + LL| |//@ edition: 2021 + LL| |//@ ignore-test + LL| |// Regression test for . + LL| |// These code patterns should not trigger an ICE when allocating a physical + LL| |// counter to a node and also one of its in-edges, because that is allowed + LL| |// when the node contains a tight loop to itself. + LL| | + LL| 1|fn loopy(cond: bool) { + LL| 1| let true = cond else { loop {} }; + ^0 + LL| 1|} + LL| | + LL| |// Variant that also has `loop {}` on the success path. + LL| |// This isn't needed to catch the original ICE, but might help detect regressions. + LL| 0|fn _loop_either_way(cond: bool) { + LL| 0| let true = cond else { loop {} }; + LL| 0| loop {} + LL| |} + LL| | + LL| |// Variant using regular `if` instead of let-else. + LL| |// This doesn't trigger the original ICE, but might help detect regressions. + LL| 0|fn _if(cond: bool) { + LL| 0| if cond { + LL| 0| loop {} + LL| | } else { + LL| 0| loop {} + LL| | } + LL| |} + LL| | + LL| |#[coverage(off)] + LL| |fn main() { + LL| | loopy(true); + LL| |} + diff --git a/tests/coverage/let_else_loop.rs b/tests/coverage/let_else_loop.rs new file mode 100644 index 0000000000000..92a252cbbdc99 --- /dev/null +++ b/tests/coverage/let_else_loop.rs @@ -0,0 +1,33 @@ +#![feature(coverage_attribute)] +//@ edition: 2021 +//@ ignore-test +// Regression test for . +// These code patterns should not trigger an ICE when allocating a physical +// counter to a node and also one of its in-edges, because that is allowed +// when the node contains a tight loop to itself. + +fn loopy(cond: bool) { + let true = cond else { loop {} }; +} + +// Variant that also has `loop {}` on the success path. +// This isn't needed to catch the original ICE, but might help detect regressions. +fn _loop_either_way(cond: bool) { + let true = cond else { loop {} }; + loop {} +} + +// Variant using regular `if` instead of let-else. +// This doesn't trigger the original ICE, but might help detect regressions. +fn _if(cond: bool) { + if cond { + loop {} + } else { + loop {} + } +} + +#[coverage(off)] +fn main() { + loopy(true); +} From 85bec7a50c9a8ab466f8fbafa7b0c4eb05e6c721 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 20 Mar 2024 18:17:23 +1100 Subject: [PATCH 2/3] coverage: Remove incorrect assertions from counter allocation These assertions detect situations where a BCB node 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. --- .../src/coverage/counters.rs | 31 ------------------- tests/coverage/let_else_loop.coverage | 2 +- tests/coverage/let_else_loop.rs | 2 +- 3 files changed, 2 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 9a1d8bae6b410..402ce6ba8d9a9 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -1,7 +1,6 @@ use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::graph::WithNumNodes; -use rustc_index::bit_set::BitSet; use rustc_index::IndexVec; use rustc_middle::mir::coverage::*; @@ -18,10 +17,6 @@ pub(super) enum BcbCounter { } impl BcbCounter { - fn is_expression(&self) -> bool { - matches!(self, Self::Expression { .. }) - } - pub(super) fn as_term(&self) -> CovTerm { match *self { BcbCounter::Counter { id, .. } => CovTerm::Counter(id), @@ -60,10 +55,6 @@ pub(super) struct CoverageCounters { /// We currently don't iterate over this map, but if we do in the future, /// switch it back to `FxIndexMap` to avoid query stability hazards. bcb_edge_counters: FxHashMap<(BasicCoverageBlock, BasicCoverageBlock), BcbCounter>, - /// Tracks which BCBs have a counter associated with some incoming edge. - /// Only used by assertions, to verify that BCBs with incoming edge - /// counters do not have their own physical counters (expressions are allowed). - bcb_has_incoming_edge_counters: BitSet, /// Table of expression data, associating each expression ID with its /// corresponding operator (+ or -) and its LHS/RHS operands. expressions: IndexVec, @@ -83,7 +74,6 @@ impl CoverageCounters { counter_increment_sites: IndexVec::new(), bcb_counters: IndexVec::from_elem_n(None, num_bcbs), bcb_edge_counters: FxHashMap::default(), - bcb_has_incoming_edge_counters: BitSet::new_empty(num_bcbs), expressions: IndexVec::new(), }; @@ -122,14 +112,6 @@ impl CoverageCounters { } fn set_bcb_counter(&mut self, bcb: BasicCoverageBlock, counter_kind: BcbCounter) -> BcbCounter { - assert!( - // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also - // have an expression (to be injected into an existing `BasicBlock` represented by this - // `BasicCoverageBlock`). - counter_kind.is_expression() || !self.bcb_has_incoming_edge_counters.contains(bcb), - "attempt to add a `Counter` to a BCB target with existing incoming edge counters" - ); - if let Some(replaced) = self.bcb_counters[bcb].replace(counter_kind) { bug!( "attempt to set a BasicCoverageBlock coverage counter more than once; \ @@ -146,19 +128,6 @@ impl CoverageCounters { to_bcb: BasicCoverageBlock, counter_kind: BcbCounter, ) -> BcbCounter { - // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also - // have an expression (to be injected into an existing `BasicBlock` represented by this - // `BasicCoverageBlock`). - if let Some(node_counter) = self.bcb_counter(to_bcb) - && !node_counter.is_expression() - { - bug!( - "attempt to add an incoming edge counter from {from_bcb:?} \ - when the target BCB already has {node_counter:?}" - ); - } - - self.bcb_has_incoming_edge_counters.insert(to_bcb); if let Some(replaced) = self.bcb_edge_counters.insert((from_bcb, to_bcb), counter_kind) { bug!( "attempt to set an edge counter more than once; from_bcb: \ diff --git a/tests/coverage/let_else_loop.coverage b/tests/coverage/let_else_loop.coverage index 3b3c39ff30180..d193c8ca1b514 100644 --- a/tests/coverage/let_else_loop.coverage +++ b/tests/coverage/let_else_loop.coverage @@ -1,6 +1,6 @@ LL| |#![feature(coverage_attribute)] LL| |//@ edition: 2021 - LL| |//@ ignore-test + LL| | LL| |// Regression test for . LL| |// These code patterns should not trigger an ICE when allocating a physical LL| |// counter to a node and also one of its in-edges, because that is allowed diff --git a/tests/coverage/let_else_loop.rs b/tests/coverage/let_else_loop.rs index 92a252cbbdc99..12e0aeabcab95 100644 --- a/tests/coverage/let_else_loop.rs +++ b/tests/coverage/let_else_loop.rs @@ -1,6 +1,6 @@ #![feature(coverage_attribute)] //@ edition: 2021 -//@ ignore-test + // Regression test for . // These code patterns should not trigger an ICE when allocating a physical // counter to a node and also one of its in-edges, because that is allowed From 2f21e4f8bb86e1877d20bf74289272ecbe2bf103 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 20 Mar 2024 18:25:06 +1100 Subject: [PATCH 3/3] coverage: Tidy imports in `rustc_mir_transform::coverage::counters` --- compiler/rustc_mir_transform/src/coverage/counters.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 402ce6ba8d9a9..69dc4f2ddea71 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -1,12 +1,12 @@ +use std::fmt::{self, Debug}; + use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::graph::WithNumNodes; use rustc_index::IndexVec; -use rustc_middle::mir::coverage::*; +use rustc_middle::mir::coverage::{CounterId, CovTerm, Expression, ExpressionId, Op}; -use super::graph::{BasicCoverageBlock, CoverageGraph, TraverseCoverageGraphWithLoops}; - -use std::fmt::{self, Debug}; +use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph, TraverseCoverageGraphWithLoops}; /// The coverage counter or counter expression associated with a particular /// BCB node or BCB edge.