Skip to content

coverage: Use SpanMarker to improve coverage spans for if ! expressions #118198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 9, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
coverage: Add CoverageKind::SpanMarker for including extra spans in…
… MIR

There are cases where coverage instrumentation wants to show a span for some
syntax element, but there is no MIR node that naturally carries that span, so
the instrumentor can't see it.

MIR building can now use this new kind of coverage statement to deliberately
include those spans in MIR, attached to a dummy statement that has no other
effect.
  • Loading branch information
Zalathar committed Dec 8, 2023
commit 44b47aa976d6a6bdd7eb1f99a8ee5270afbe993e
3 changes: 3 additions & 0 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {

let Coverage { kind } = coverage;
match *kind {
// Span markers are only meaningful during MIR instrumentation,
// and have no effect during codegen.
CoverageKind::SpanMarker => {}
CoverageKind::CounterIncrement { id } => {
func_coverage.mark_counter_id_seen(id);
// We need to explicitly drop the `RefMut` before calling into `instrprof_increment`,
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_middle/src/mir/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@ impl Debug for CovTerm {

#[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub enum CoverageKind {
/// Marks a span that might otherwise not be represented in MIR, so that
/// coverage instrumentation can associate it with its enclosing block/BCB.
///
/// Only used by the `InstrumentCoverage` pass, and has no effect during
/// codegen.
SpanMarker,

/// Marks the point in MIR control flow represented by a coverage counter.
///
/// This is eventually lowered to `llvm.instrprof.increment` in LLVM IR.
Expand All @@ -99,6 +106,7 @@ impl Debug for CoverageKind {
fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
use CoverageKind::*;
match self {
SpanMarker => write!(fmt, "SpanMarker"),
CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()),
ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()),
}
Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_mir_build/src/build/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,19 @@ impl<'tcx> CFG<'tcx> {
self.push(block, stmt);
}

/// Adds a dummy statement whose only role is to associate a span with its
/// enclosing block for the purposes of coverage instrumentation.
///
/// This results in more accurate coverage reports for certain kinds of
/// syntax (e.g. `continue` or `if !`) that would otherwise not appear in MIR.
pub(crate) fn push_coverage_span_marker(&mut self, block: BasicBlock, source_info: SourceInfo) {
let kind = StatementKind::Coverage(Box::new(Coverage {
kind: coverage::CoverageKind::SpanMarker,
}));
let stmt = Statement { source_info, kind };
self.push(block, stmt);
}

pub(crate) fn terminate(
&mut self,
block: BasicBlock,
Expand Down
15 changes: 12 additions & 3 deletions compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,13 @@ fn is_closure(statement: &Statement<'_>) -> bool {
/// If the MIR `Statement` has a span contributive to computing coverage spans,
/// return it; otherwise return `None`.
fn filtered_statement_span(statement: &Statement<'_>) -> Option<Span> {
use mir::coverage::CoverageKind;

match statement.kind {
// These statements have spans that are often outside the scope of the executed source code
// for their parent `BasicBlock`.
StatementKind::StorageLive(_)
| StatementKind::StorageDead(_)
// Coverage should not be encountered, but don't inject coverage coverage
| StatementKind::Coverage(_)
// Ignore `ConstEvalCounter`s
| StatementKind::ConstEvalCounter
// Ignore `Nop`s
Expand All @@ -122,9 +122,13 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option<Span> {
// If and when the Issue is resolved, remove this special case match pattern:
StatementKind::FakeRead(box (FakeReadCause::ForGuardBinding, _)) => None,

// Retain spans from all other statements
// Retain spans from most other statements.
StatementKind::FakeRead(box (_, _)) // Not including `ForGuardBinding`
| StatementKind::Intrinsic(..)
| StatementKind::Coverage(box mir::Coverage {
// The purpose of `SpanMarker` is to be matched and accepted here.
kind: CoverageKind::SpanMarker
})
| StatementKind::Assign(_)
| StatementKind::SetDiscriminant { .. }
| StatementKind::Deinit(..)
Expand All @@ -133,6 +137,11 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option<Span> {
| StatementKind::AscribeUserType(_, _) => {
Some(statement.source_info.span)
}

StatementKind::Coverage(box mir::Coverage {
// These coverage statements should not exist prior to coverage instrumentation.
kind: CoverageKind::CounterIncrement { .. } | CoverageKind::ExpressionUsed { .. }
}) => bug!("Unexpected coverage statement found during coverage instrumentation: {statement:?}"),
}
}

Expand Down