From e3b83dfe5f63e99046d3e60cb00f2bada1ecea17 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 4 Oct 2023 14:21:11 +1100 Subject: [PATCH 1/3] coverage: Force `-Copt-level=2` in the unreachable-code test This test is mainly for detecting problems triggered by MIR optimizations, but the run-coverage tests don't enable optimization by default. (The coverage-map copy has also been updated, to keep the two tests in sync.) --- tests/coverage-map/unreachable.rs | 2 +- tests/run-coverage/unreachable.coverage | 2 +- tests/run-coverage/unreachable.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/coverage-map/unreachable.rs b/tests/coverage-map/unreachable.rs index 6385bfa160d7d..61728a3c298e5 100644 --- a/tests/coverage-map/unreachable.rs +++ b/tests/coverage-map/unreachable.rs @@ -1,6 +1,6 @@ #![feature(core_intrinsics)] #![feature(coverage_attribute)] -// compile-flags: --edition=2021 +// compile-flags: --edition=2021 -Copt-level=2 // // If we instrument a function for coverage, but all of its counter-increment diff --git a/tests/run-coverage/unreachable.coverage b/tests/run-coverage/unreachable.coverage index fa0ac9ccfa1c8..da52f0522d4a3 100644 --- a/tests/run-coverage/unreachable.coverage +++ b/tests/run-coverage/unreachable.coverage @@ -1,6 +1,6 @@ LL| |#![feature(core_intrinsics)] LL| |#![feature(coverage_attribute)] - LL| |// compile-flags: --edition=2021 + LL| |// compile-flags: --edition=2021 -Copt-level=2 LL| | LL| |// LL| |// If we instrument a function for coverage, but all of its counter-increment diff --git a/tests/run-coverage/unreachable.rs b/tests/run-coverage/unreachable.rs index 6385bfa160d7d..61728a3c298e5 100644 --- a/tests/run-coverage/unreachable.rs +++ b/tests/run-coverage/unreachable.rs @@ -1,6 +1,6 @@ #![feature(core_intrinsics)] #![feature(coverage_attribute)] -// compile-flags: --edition=2021 +// compile-flags: --edition=2021 -Copt-level=2 // // If we instrument a function for coverage, but all of its counter-increment From e6b282820d4447f99ad7c0f9e933d5f20e295e76 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 26 Sep 2023 21:45:21 +1000 Subject: [PATCH 2/3] coverage: Repair instrumented functions that have lost all their counters If a function has been instrumented for coverage, but MIR optimizations subsequently remove all of its counter-increment statements, then we won't emit LLVM counter-increment intrinsics. LLVM will think the function is not instrumented, and it will disappear from coverage mappings and coverage reports. This new MIR pass detects when that has happened, and re-inserts a dummy counter-increment statement so that LLVM knows to treat the function as instrumented. --- .../rustc_mir_transform/src/coverage/mod.rs | 5 +- .../src/coverage/repair.rs | 58 +++++++++++++++++++ compiler/rustc_mir_transform/src/lib.rs | 3 + 3 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 compiler/rustc_mir_transform/src/coverage/repair.rs diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 6fdaff6b4c021..ed0fe2c9a2e6e 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -1,9 +1,8 @@ -pub mod query; - mod counters; mod graph; +pub mod query; +pub(crate) mod repair; mod spans; - #[cfg(test)] mod tests; diff --git a/compiler/rustc_mir_transform/src/coverage/repair.rs b/compiler/rustc_mir_transform/src/coverage/repair.rs new file mode 100644 index 0000000000000..9c9d3416c16a7 --- /dev/null +++ b/compiler/rustc_mir_transform/src/coverage/repair.rs @@ -0,0 +1,58 @@ +use rustc_middle::mir::coverage::{CounterId, CoverageKind}; +use rustc_middle::mir::{ + self, Coverage, MirPass, SourceInfo, Statement, StatementKind, START_BLOCK, +}; +use rustc_middle::ty::TyCtxt; +use rustc_span::DUMMY_SP; + +/// If a function has been [instrumented for coverage](super::InstrumentCoverage), +/// but MIR optimizations subsequently remove all of its [`CoverageKind::CounterIncrement`] +/// statements (e.g. because bb0 is unreachable), then we won't generate any +/// `llvm.instrprof.increment` intrinsics. LLVM will think the function is not +/// instrumented, and it will disappear from coverage mappings and coverage reports. +/// +/// This pass detects when that has happened, and re-inserts a dummy counter-increment +/// statement so that LLVM knows to treat the function as instrumented. +pub struct RepairCoverage; + +impl<'tcx> MirPass<'tcx> for RepairCoverage { + fn is_enabled(&self, sess: &rustc_session::Session) -> bool { + sess.instrument_coverage() + } + + fn run_pass(&self, _tcx: TyCtxt<'tcx>, body: &mut mir::Body<'tcx>) { + // If a function wasn't instrumented for coverage in the first place, + // then there's no need to repair anything. + if body.function_coverage_info.is_none() { + return; + } + + // If the body still contains one or more counter-increment statements, + // there's no need to repair anything. + let has_counter_increment = body + .basic_blocks + .iter() + .flat_map(|bb_data| &bb_data.statements) + .filter_map(|statement| match statement.kind { + StatementKind::Coverage(box ref coverage) => Some(coverage), + _ => None, + }) + .any(|coverage| matches!(coverage.kind, CoverageKind::CounterIncrement { .. })); + if has_counter_increment { + return; + } + + debug!( + "all counter-increments were removed after instrumentation; restoring one counter in {:?}", + body.source.def_id(), + ); + + let statement = Statement { + source_info: SourceInfo::outermost(DUMMY_SP), + kind: StatementKind::Coverage(Box::new(Coverage { + kind: CoverageKind::CounterIncrement { id: CounterId::START }, + })), + }; + body[START_BLOCK].statements.insert(0, statement); + } +} diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index d579420ecb835..9fabcf8ffb7a3 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -585,6 +585,9 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { &large_enums::EnumSizeOpt { discrepancy: 128 }, // Some cleanup necessary at least for LLVM and potentially other codegen backends. &add_call_guards::CriticalCallEdges, + // If MIR optimizations removed all coverage-increment statements + // from an instrumented function, add another one to avoid problems. + &coverage::repair::RepairCoverage, // Cleanup for human readability, off by default. &prettify::ReorderBasicBlocks, &prettify::ReorderLocals, From 2a95ee6a261c2d1fc5f60e7fd7fa3ea7e1c09cf8 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 27 Sep 2023 17:41:45 +1000 Subject: [PATCH 3/3] Allow MIR pass `UnreachableProp` when coverage is enabled Now that coverage has a dedicated MIR pass to detect and repair instrumented functions that have lost all their counters, it should be OK to permit this pass in coverage builds. --- compiler/rustc_mir_transform/src/unreachable_prop.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/rustc_mir_transform/src/unreachable_prop.rs b/compiler/rustc_mir_transform/src/unreachable_prop.rs index ea7aafd866b15..26398b36693d6 100644 --- a/compiler/rustc_mir_transform/src/unreachable_prop.rs +++ b/compiler/rustc_mir_transform/src/unreachable_prop.rs @@ -13,11 +13,7 @@ pub struct UnreachablePropagation; impl MirPass<'_> for UnreachablePropagation { fn is_enabled(&self, sess: &rustc_session::Session) -> bool { // Enable only under -Zmir-opt-level=2 as this can make programs less debuggable. - - // FIXME(#116171) Coverage gets confused by MIR passes that can remove all - // coverage statements from an instrumented function. This pass can be - // re-enabled when coverage codegen is robust against that happening. - sess.mir_opt_level() >= 2 && !sess.instrument_coverage() + sess.mir_opt_level() >= 2 } fn run_pass<'tcx>(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {