diff options
11 files changed, 212 insertions, 96 deletions
diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 32f4fc76b3d..afc2bdbfd52 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -250,13 +250,9 @@ fn add_unused_function_coverage( // Insert at least one real counter so the LLVM CoverageMappingReader will find expected // definitions. function_coverage.add_counter(UNUSED_FUNCTION_COUNTER_ID, code_region.clone()); + } else { + function_coverage.add_unreachable_region(code_region.clone()); } - // Add a Zero Counter for every code region. - // - // Even though the first coverage region already has an actual Counter, `llvm-cov` will not - // always report it. Re-adding an unreachable region (zero counter) for the same region - // seems to help produce the expected coverage. - function_coverage.add_unreachable_region(code_region.clone()); } if let Some(coverage_context) = cx.coverage_context() { diff --git a/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs index af6c476292b..962c01c2ee7 100644 --- a/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs @@ -24,21 +24,39 @@ pub enum CounterKind { pub struct Counter { // Important: The layout (order and types of fields) must match its C++ counterpart. pub kind: CounterKind, - pub id: u32, + id: u32, } impl Counter { + /// Constructs a new `Counter` of kind `Zero`. For this `CounterKind`, the + /// `id` is not used. pub fn zero() -> Self { 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.into() } + Self { kind: CounterKind::CounterValueReference, id: counter_id.zero_based_index() } } + /// Constructs a new `Counter` of kind `Expression`. pub fn expression(mapped_expression_index: MappedExpressionIndex) -> Self { Self { kind: CounterKind::Expression, id: mapped_expression_index.into() } } + + /// Returns true if the `Counter` kind is `Zero`. + pub fn is_zero(&self) -> bool { + matches!(self.kind, CounterKind::Zero) + } + + /// An explicitly-named function to get the ID value, making it more obvious + /// that the stored value is now 0-based. + pub fn zero_based_id(&self) -> u32 { + debug_assert!(!self.is_zero(), "`id` is undefined for CounterKind::Zero"); + self.id + } } /// Aligns with [llvm::coverage::CounterExpression::ExprKind](https://github.com/rust-lang/llvm-project/blob/rustc/11.0-2020-10-12/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L147) diff --git a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs index 7a17bced1c0..4458fd68678 100644 --- a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs +++ b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs @@ -163,9 +163,7 @@ impl<'tcx> FunctionCoverage<'tcx> { self.counters.iter_enumerated().filter_map(|(index, entry)| { // Option::map() will return None to filter out missing counters. This may happen // if, for example, a MIR-instrumented counter is removed during an optimization. - entry.as_ref().map(|region| { - (Counter::counter_value_reference(index as CounterValueReference), region) - }) + entry.as_ref().map(|region| (Counter::counter_value_reference(index), region)) }) } @@ -206,9 +204,15 @@ impl<'tcx> FunctionCoverage<'tcx> { if id == ExpressionOperandId::ZERO { Some(Counter::zero()) } else if id.index() < self.counters.len() { + debug_assert!( + id.index() > 0, + "ExpressionOperandId 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)) } else { let index = self.expression_index(u32::from(id)); @@ -233,19 +237,60 @@ impl<'tcx> FunctionCoverage<'tcx> { let optional_region = &expression.region; let Expression { lhs, op, rhs, .. } = *expression; - if let Some(Some((lhs_counter, rhs_counter))) = - id_to_counter(&new_indexes, lhs).map(|lhs_counter| { + if let Some(Some((lhs_counter, mut rhs_counter))) = id_to_counter(&new_indexes, lhs) + .map(|lhs_counter| { id_to_counter(&new_indexes, rhs).map(|rhs_counter| (lhs_counter, rhs_counter)) }) { + if lhs_counter.is_zero() && op.is_subtract() { + // The left side of a subtraction was probably optimized out. As an example, + // a branch condition might be evaluated as a constant expression, and the + // branch could be removed, dropping unused counters in the process. + // + // Since counters are unsigned, we must assume the result of the expression + // can be no more and no less than zero. An expression known to evaluate to zero + // does not need to be added to the coverage map. + // + // Coverage test `loops_branches.rs` includes multiple variations of branches + // based on constant conditional (literal `true` or `false`), and demonstrates + // that the expected counts are still correct. + debug!( + "Expression subtracts from zero (assume unreachable): \ + original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", + original_index, lhs, op, rhs, optional_region, + ); + rhs_counter = Counter::zero(); + } debug_assert!( - (lhs_counter.id as usize) - < usize::max(self.counters.len(), self.expressions.len()) + lhs_counter.is_zero() + // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16` + || ((lhs_counter.zero_based_id() as usize) + <= usize::max(self.counters.len(), self.expressions.len())), + "lhs id={} > both counters.len()={} and expressions.len()={} + ({:?} {:?} {:?})", + lhs_counter.zero_based_id(), + self.counters.len(), + self.expressions.len(), + lhs_counter, + op, + rhs_counter, ); + debug_assert!( - (rhs_counter.id as usize) - < usize::max(self.counters.len(), self.expressions.len()) + rhs_counter.is_zero() + // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16` + || ((rhs_counter.zero_based_id() as usize) + <= usize::max(self.counters.len(), self.expressions.len())), + "rhs id={} > both counters.len()={} and expressions.len()={} + ({:?} {:?} {:?})", + rhs_counter.zero_based_id(), + self.counters.len(), + self.expressions.len(), + lhs_counter, + op, + rhs_counter, ); + // Both operands exist. `Expression` operands exist in `self.expressions` and have // been assigned a `new_index`. let mapped_expression_index = @@ -268,11 +313,15 @@ impl<'tcx> FunctionCoverage<'tcx> { expression_regions.push((Counter::expression(mapped_expression_index), region)); } } else { - debug!( - "Ignoring expression with one or more missing operands: \ - original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", - original_index, lhs, op, rhs, optional_region, - ) + bug!( + "expression has one or more missing operands \ + original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", + original_index, + lhs, + op, + rhs, + optional_region, + ); } } (counter_expressions, expression_regions.into_iter()) diff --git a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs index e2f75b2e337..621ec0519c9 100644 --- a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs @@ -36,7 +36,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { 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(u32::from(id)); + let index = bx.const_u32(id.zero_based_index()); debug!( "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})", fn_name, hash, num_counters, index, diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index ad48b236a9a..ddb1a84fe7b 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -41,8 +41,16 @@ rustc_index::newtype_index! { } impl CounterValueReference { - // Counters start at 1 to reserve 0 for ExpressionOperandId::ZERO. + /// Counters start at 1 to reserve 0 for ExpressionOperandId::ZERO. pub const START: Self = Self::from_u32(1); + + /// 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 + } } rustc_index::newtype_index! { @@ -175,3 +183,13 @@ pub enum Op { Subtract, Add, } + +impl Op { + pub fn is_add(&self) -> bool { + matches!(self, Self::Add) + } + + pub fn is_subtract(&self) -> bool { + matches!(self, Self::Subtract) + } +} diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async.txt index d9097bb50e5..e0a5937c246 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async.txt @@ -10,7 +10,7 @@ 10| | } 11| 1|} 12| | - 13| |async fn d() -> u8 { 1 } // should have a coverage count `0` (see below) + 13| 0|async fn d() -> u8 { 1 } 14| | 15| 0|async fn e() -> u8 { 1 } // unused function; executor does not block on `g()` 16| | @@ -66,7 +66,8 @@ 63| 1| 0 64| 1| } 65| 1| } - 66| 1| fn d() -> u8 { 1 } + 66| 1| fn d() -> u8 { 1 } // inner function is defined in-line, but the function is not executed + ^0 67| 1| fn f() -> u8 { 1 } 68| 1| match x { 69| 1| y if c(x) == y + 1 => { d(); } @@ -115,11 +116,14 @@ 109| | 110| 1| pub fn block_on<F: Future>(mut future: F) -> F::Output { 111| 1| let mut future = unsafe { Pin::new_unchecked(&mut future) }; - 112| 1| + 112| 1| use std::hint::unreachable_unchecked; 113| 1| static VTABLE: RawWakerVTable = RawWakerVTable::new( - 114| 1| |_| unimplemented!("clone"), - 115| 1| |_| unimplemented!("wake"), - 116| 1| |_| unimplemented!("wake_by_ref"), + 114| 1| |_| unsafe { unreachable_unchecked() }, // clone + ^0 + 115| 1| |_| unsafe { unreachable_unchecked() }, // wake + ^0 + 116| 1| |_| unsafe { unreachable_unchecked() }, // wake_by_ref + ^0 117| 1| |_| (), 118| 1| ); 119| 1| let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) }; @@ -132,15 +136,4 @@ 126| | } 127| 1| } 128| |} - 129| | - 130| |// `llvm-cov show` shows no coverage results for the `d()`, even though the - 131| |// crate's LLVM IR shows the function exists and has an InstrProf PGO counter, - 132| |// and appears to be registered like all other counted functions. - 133| |// - 134| |// `llvm-cov show --debug` output shows there is at least one `Counter` for this - 135| |// line, but counters do not appear in the `Combined regions` section (unlike - 136| |// `f()`, which is similar, but does appear in `Combined regions`, and does show - 137| |// coverage). The only difference is, `f()` is awaited, but the call to await - 138| |// `d()` is not reached. (Note: `d()` will appear in coverage if the test is - 139| |// modified to cause it to be awaited.) diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt index 7d2abe05973..1b6bb9ff889 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt @@ -56,7 +56,7 @@ 46| 1|//! println!("called some_func()"); 47| 1|//! } 48| |//! - 49| |//! #[derive(Debug)] + 49| 0|//! #[derive(Debug)] 50| |//! struct SomeError; 51| |//! 52| |//! extern crate doctest_crate; diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline.txt index 6148d89ed75..31d3ddea8d9 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline.txt @@ -47,7 +47,7 @@ 46| 6|} 47| | 48| |#[inline(always)] - 49| |fn error() { - 50| | panic!("error"); - 51| |} + 49| 0|fn error() { + 50| 0| panic!("error"); + 51| 0|} diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt index 474f02b7007..81d5c7d9034 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt @@ -1,7 +1,7 @@ 1| |#![allow(unused_assignments, unused_variables, while_true)] 2| | - 3| |// This test confirms an earlier problem was resolved, supporting the MIR graph generated by the - 4| |// structure of this `fmt` function. + 3| |// This test confirms that (1) unexecuted infinite loops are handled correctly by the + 4| |// InstrumentCoverage MIR pass; and (2) Counter Expressions that subtract from zero can be dropped. 5| | 6| |struct DebugTest; 7| | @@ -16,23 +16,51 @@ ^0 16| | } else { 17| | } - 18| 1| Ok(()) - 19| 1| } - 20| |} - 21| | - 22| 1|fn main() { - 23| 1| let debug_test = DebugTest; - 24| 1| println!("{:?}", debug_test); - 25| 1|} - 26| | - 27| |/* - 28| | - 29| |This is the error message generated, before the issue was fixed: - 30| | - 31| |error: internal compiler error: compiler/rustc_mir/src/transform/coverage/mod.rs:374:42: - 32| |Error processing: DefId(0:6 ~ bug_incomplete_cov_graph_traversal_simplified[317d]::{impl#0}::fmt): - 33| |Error { message: "`TraverseCoverageGraphWithLoops` missed some `BasicCoverageBlock`s: - 34| |[bcb6, bcb7, bcb9]" } - 35| | - 36| |*/ + 18| | + 19| 10| for i in 0..10 { + 20| 10| if true { + 21| 10| if false { + 22| | while true {} + 23| 10| } + 24| 10| write!(f, "error")?; + ^0 + 25| | } else { + 26| | } + 27| | } + 28| 1| Ok(()) + 29| 1| } + 30| |} + 31| | + 32| |struct DisplayTest; + 33| | + 34| |impl std::fmt::Display for DisplayTest { + 35| 1| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + 36| 1| if false { + 37| | } else { + 38| 1| if false { + 39| | while true {} + 40| 1| } + 41| 1| write!(f, "error")?; + ^0 + 42| | } + 43| 10| for i in 0..10 { + 44| 10| if false { + 45| | } else { + 46| 10| if false { + 47| | while true {} + 48| 10| } + 49| 10| write!(f, "error")?; + ^0 + 50| | } + 51| | } + 52| 1| Ok(()) + 53| 1| } + 54| |} + 55| | + 56| 1|fn main() { + 57| 1| let debug_test = DebugTest; + 58| 1| println!("{:?}", debug_test); + 59| 1| let display_test = DisplayTest; + 60| 1| println!("{}", display_test); + 61| 1|} diff --git a/src/test/run-make-fulldeps/coverage/async.rs b/src/test/run-make-fulldeps/coverage/async.rs index f08a7b44fbd..67bf696d072 100644 --- a/src/test/run-make-fulldeps/coverage/async.rs +++ b/src/test/run-make-fulldeps/coverage/async.rs @@ -10,7 +10,7 @@ async fn c(x: u8) -> u8 { } } -async fn d() -> u8 { 1 } // should have a coverage count `0` (see below) +async fn d() -> u8 { 1 } async fn e() -> u8 { 1 } // unused function; executor does not block on `g()` @@ -63,7 +63,7 @@ fn j(x: u8) { 0 } } - fn d() -> u8 { 1 } + fn d() -> u8 { 1 } // inner function is defined in-line, but the function is not executed fn f() -> u8 { 1 } match x { y if c(x) == y + 1 => { d(); } @@ -109,11 +109,11 @@ mod executor { pub fn block_on<F: Future>(mut future: F) -> F::Output { let mut future = unsafe { Pin::new_unchecked(&mut future) }; - + use std::hint::unreachable_unchecked; static VTABLE: RawWakerVTable = RawWakerVTable::new( - |_| unimplemented!("clone"), - |_| unimplemented!("wake"), - |_| unimplemented!("wake_by_ref"), + |_| unsafe { unreachable_unchecked() }, // clone + |_| unsafe { unreachable_unchecked() }, // wake + |_| unsafe { unreachable_unchecked() }, // wake_by_ref |_| (), ); let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) }; @@ -126,14 +126,3 @@ mod executor { } } } - -// `llvm-cov show` shows no coverage results for the `d()`, even though the -// crate's LLVM IR shows the function exists and has an InstrProf PGO counter, -// and appears to be registered like all other counted functions. -// -// `llvm-cov show --debug` output shows there is at least one `Counter` for this -// line, but counters do not appear in the `Combined regions` section (unlike -// `f()`, which is similar, but does appear in `Combined regions`, and does show -// coverage). The only difference is, `f()` is awaited, but the call to await -// `d()` is not reached. (Note: `d()` will appear in coverage if the test is -// modified to cause it to be awaited.) diff --git a/src/test/run-make-fulldeps/coverage/loops_branches.rs b/src/test/run-make-fulldeps/coverage/loops_branches.rs index 938421d32e7..4d9bbad3367 100644 --- a/src/test/run-make-fulldeps/coverage/loops_branches.rs +++ b/src/test/run-make-fulldeps/coverage/loops_branches.rs @@ -1,7 +1,7 @@ #![allow(unused_assignments, unused_variables, while_true)] -// This test confirms an earlier problem was resolved, supporting the MIR graph generated by the -// structure of this `fmt` function. +// This test confirms that (1) unexecuted infinite loops are handled correctly by the +// InstrumentCoverage MIR pass; and (2) Counter Expressions that subtract from zero can be dropped. struct DebugTest; @@ -15,6 +15,40 @@ impl std::fmt::Debug for DebugTest { write!(f, "error")?; } else { } + + for i in 0..10 { + if true { + if false { + while true {} + } + write!(f, "error")?; + } else { + } + } + Ok(()) + } +} + +struct DisplayTest; + +impl std::fmt::Display for DisplayTest { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + if false { + } else { + if false { + while true {} + } + write!(f, "error")?; + } + for i in 0..10 { + if false { + } else { + if false { + while true {} + } + write!(f, "error")?; + } + } Ok(()) } } @@ -22,15 +56,6 @@ impl std::fmt::Debug for DebugTest { fn main() { let debug_test = DebugTest; println!("{:?}", debug_test); + let display_test = DisplayTest; + println!("{}", display_test); } - -/* - -This is the error message generated, before the issue was fixed: - -error: internal compiler error: compiler/rustc_mir/src/transform/coverage/mod.rs:374:42: -Error processing: DefId(0:6 ~ bug_incomplete_cov_graph_traversal_simplified[317d]::{impl#0}::fmt): -Error { message: "`TraverseCoverageGraphWithLoops` missed some `BasicCoverageBlock`s: -[bcb6, bcb7, bcb9]" } - -*/ |