Skip to content

Commit

Permalink
Rollup merge of #122937 - Zalathar:unbox, r=oli-obk
Browse files Browse the repository at this point in the history
Unbox and unwrap the contents of `StatementKind::Coverage`

The payload of coverage statements was historically a structure with several fields, so it was boxed to avoid bloating `StatementKind`.

Now that the payload is a single relatively-small enum, we can replace `Box<Coverage>` with just `CoverageKind`.

This patch also adds a size assertion for `StatementKind`, to avoid accidentally bloating it in the future.

``@rustbot`` label +A-code-coverage
  • Loading branch information
matthiaskrgr authored Mar 24, 2024
2 parents 702261d + ab92699 commit 19d3827
Show file tree
Hide file tree
Showing 15 changed files with 44 additions and 68 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_gcc/src/coverageinfo.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use rustc_codegen_ssa::traits::CoverageInfoBuilderMethods;
use rustc_middle::mir::Coverage;
use rustc_middle::mir::coverage::CoverageKind;
use rustc_middle::ty::Instance;

use crate::builder::Builder;

impl<'a, 'gcc, 'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'a, 'gcc, 'tcx> {
fn add_coverage(&mut self, _instance: Instance<'tcx>, _coverage: &Coverage) {
fn add_coverage(&mut self, _instance: Instance<'tcx>, _kind: &CoverageKind) {
// TODO(antoyo)
}
}
4 changes: 1 addition & 3 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
use rustc_llvm::RustString;
use rustc_middle::bug;
use rustc_middle::mir::coverage::CoverageKind;
use rustc_middle::mir::Coverage;
use rustc_middle::ty::layout::HasTyCtxt;
use rustc_middle::ty::Instance;

Expand Down Expand Up @@ -75,7 +74,7 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {

impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
#[instrument(level = "debug", skip(self))]
fn add_coverage(&mut self, instance: Instance<'tcx>, coverage: &Coverage) {
fn add_coverage(&mut self, instance: Instance<'tcx>, kind: &CoverageKind) {
// Our caller should have already taken care of inlining subtleties,
// so we can assume that counter/expression IDs in this coverage
// statement are meaningful for the given instance.
Expand All @@ -98,7 +97,6 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
.entry(instance)
.or_insert_with(|| FunctionCoverageCollector::new(instance, function_coverage_info));

let Coverage { kind } = coverage;
match *kind {
CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } => unreachable!(
"marker statement {kind:?} should have been removed by CleanupPostBorrowck"
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::traits::*;

use rustc_middle::mir::Coverage;
use rustc_middle::mir::coverage::CoverageKind;
use rustc_middle::mir::SourceScope;

use super::FunctionCx;

impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
pub fn codegen_coverage(&self, bx: &mut Bx, coverage: &Coverage, scope: SourceScope) {
pub fn codegen_coverage(&self, bx: &mut Bx, kind: &CoverageKind, scope: SourceScope) {
// Determine the instance that coverage data was originally generated for.
let instance = if let Some(inlined) = scope.inlined_instance(&self.mir.source_scopes) {
self.monomorphize(inlined)
Expand All @@ -15,6 +15,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
};

// Handle the coverage info in a backend-specific way.
bx.add_coverage(instance, coverage);
bx.add_coverage(instance, kind);
}
}
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
cg_indirect_place.storage_dead(bx);
}
}
mir::StatementKind::Coverage(box ref coverage) => {
self.codegen_coverage(bx, coverage, statement.source_info.scope);
mir::StatementKind::Coverage(ref kind) => {
self.codegen_coverage(bx, kind, statement.source_info.scope);
}
mir::StatementKind::Intrinsic(box NonDivergingIntrinsic::Assume(ref op)) => {
if !matches!(bx.tcx().sess.opts.optimize, OptLevel::No | OptLevel::Less) {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use super::BackendTypes;
use rustc_middle::mir::Coverage;
use rustc_middle::mir::coverage::CoverageKind;
use rustc_middle::ty::Instance;

pub trait CoverageInfoBuilderMethods<'tcx>: BackendTypes {
/// Handle the MIR coverage info in a backend-specific way.
///
/// This can potentially be a no-op in backends that don't support
/// coverage instrumentation.
fn add_coverage(&mut self, instance: Instance<'tcx>, coverage: &Coverage);
fn add_coverage(&mut self, instance: Instance<'tcx>, kind: &CoverageKind);
}
3 changes: 1 addition & 2 deletions compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,7 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
self.fail(location, format!("explicit `{kind:?}` is forbidden"));
}
}
StatementKind::Coverage(coverage) => {
let kind = &coverage.kind;
StatementKind::Coverage(kind) => {
if self.mir_phase >= MirPhase::Analysis(AnalysisPhase::PostCleanup)
&& let CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. } = kind
{
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use rustc_middle::mir::interpret::{
Provenance,
};
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::{self, *};
use rustc_middle::mir::*;
use rustc_target::abi::Size;

const INDENT: &str = " ";
Expand Down Expand Up @@ -711,7 +711,7 @@ impl Debug for Statement<'_> {
AscribeUserType(box (ref place, ref c_ty), ref variance) => {
write!(fmt, "AscribeUserType({place:?}, {variance:?}, {c_ty:?})")
}
Coverage(box mir::Coverage { ref kind }) => write!(fmt, "Coverage::{kind:?}"),
Coverage(ref kind) => write!(fmt, "Coverage::{kind:?}"),
Intrinsic(box ref intrinsic) => write!(fmt, "{intrinsic}"),
ConstEvalCounter => write!(fmt, "ConstEvalCounter"),
Nop => write!(fmt, "nop"),
Expand Down
9 changes: 2 additions & 7 deletions compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ pub enum StatementKind<'tcx> {
///
/// Interpreters and codegen backends that don't support coverage instrumentation
/// can usually treat this as a no-op.
Coverage(Box<Coverage>),
Coverage(CoverageKind),

/// Denotes a call to an intrinsic that does not require an unwind path and always returns.
/// This avoids adding a new block and a terminator for simple intrinsics.
Expand Down Expand Up @@ -517,12 +517,6 @@ pub enum FakeReadCause {
ForIndex,
}

#[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)]
#[derive(TypeFoldable, TypeVisitable)]
pub struct Coverage {
pub kind: CoverageKind,
}

#[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)]
#[derive(TypeFoldable, TypeVisitable)]
pub struct CopyNonOverlapping<'tcx> {
Expand Down Expand Up @@ -1458,5 +1452,6 @@ mod size_asserts {
static_assert_size!(Place<'_>, 16);
static_assert_size!(PlaceElem<'_>, 24);
static_assert_size!(Rvalue<'_>, 40);
static_assert_size!(StatementKind<'_>, 16);
// tidy-alphabetical-end
}
6 changes: 3 additions & 3 deletions compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,10 @@ macro_rules! make_mir_visitor {

fn visit_coverage(
&mut self,
coverage: & $($mutability)? Coverage,
kind: & $($mutability)? coverage::CoverageKind,
location: Location,
) {
self.super_coverage(coverage, location);
self.super_coverage(kind, location);
}

fn visit_retag(
Expand Down Expand Up @@ -803,7 +803,7 @@ macro_rules! make_mir_visitor {
}

fn super_coverage(&mut self,
_coverage: & $($mutability)? Coverage,
_kind: & $($mutability)? coverage::CoverageKind,
_location: Location) {
}

Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_mir_build/src/build/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,7 @@ impl<'tcx> CFG<'tcx> {
/// 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 kind = StatementKind::Coverage(coverage::CoverageKind::SpanMarker);
let stmt = Statement { source_info, kind };
self.push(block, stmt);
}
Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_mir_build/src/build/coverageinfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,7 @@ impl Builder<'_, '_> {

let marker_statement = mir::Statement {
source_info,
kind: mir::StatementKind::Coverage(Box::new(mir::Coverage {
kind: CoverageKind::BlockMarker { id },
})),
kind: mir::StatementKind::Coverage(CoverageKind::BlockMarker { id }),
};
self.cfg.push(block, marker_statement);

Expand Down
9 changes: 4 additions & 5 deletions compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

use crate::MirPass;
use rustc_middle::mir::coverage::CoverageKind;
use rustc_middle::mir::{Body, BorrowKind, Coverage, Rvalue, StatementKind, TerminatorKind};
use rustc_middle::mir::{Body, BorrowKind, Rvalue, StatementKind, TerminatorKind};
use rustc_middle::ty::TyCtxt;

pub struct CleanupPostBorrowck;
Expand All @@ -30,12 +30,11 @@ impl<'tcx> MirPass<'tcx> for CleanupPostBorrowck {
match statement.kind {
StatementKind::AscribeUserType(..)
| StatementKind::Assign(box (_, Rvalue::Ref(_, BorrowKind::Fake, _)))
| StatementKind::Coverage(box Coverage {
| StatementKind::Coverage(
// These kinds of coverage statements are markers inserted during
// MIR building, and are not needed after InstrumentCoverage.
kind: CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. },
..
})
CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. },
)
| StatementKind::FakeRead(..) => statement.make_nop(),
_ => (),
}
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::MirPass;

use rustc_middle::mir::coverage::*;
use rustc_middle::mir::{
self, BasicBlock, BasicBlockData, Coverage, SourceInfo, Statement, StatementKind, Terminator,
self, BasicBlock, BasicBlockData, SourceInfo, Statement, StatementKind, Terminator,
TerminatorKind,
};
use rustc_middle::ty::TyCtxt;
Expand Down Expand Up @@ -230,10 +230,7 @@ fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb
debug!(" injecting statement {counter_kind:?} for {bb:?}");
let data = &mut mir_body[bb];
let source_info = data.terminator().source_info;
let statement = Statement {
source_info,
kind: StatementKind::Coverage(Box::new(Coverage { kind: counter_kind })),
};
let statement = Statement { source_info, kind: StatementKind::Coverage(counter_kind) };
data.statements.insert(0, statement);
}

Expand Down
10 changes: 4 additions & 6 deletions compiler/rustc_mir_transform/src/coverage/query.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use rustc_data_structures::captures::Captures;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir::coverage::{CounterId, CoverageKind};
use rustc_middle::mir::{Body, Coverage, CoverageIdsInfo, Statement, StatementKind};
use rustc_middle::mir::{Body, CoverageIdsInfo, Statement, StatementKind};
use rustc_middle::query::TyCtxtAt;
use rustc_middle::ty::{self, TyCtxt};
use rustc_middle::util::Providers;
Expand Down Expand Up @@ -54,7 +54,7 @@ fn coverage_ids_info<'tcx>(
let mir_body = tcx.instance_mir(instance_def);

let max_counter_id = all_coverage_in_mir_body(mir_body)
.filter_map(|coverage| match coverage.kind {
.filter_map(|kind| match *kind {
CoverageKind::CounterIncrement { id } => Some(id),
_ => None,
})
Expand All @@ -66,12 +66,10 @@ fn coverage_ids_info<'tcx>(

fn all_coverage_in_mir_body<'a, 'tcx>(
body: &'a Body<'tcx>,
) -> impl Iterator<Item = &'a Coverage> + Captures<'tcx> {
) -> impl Iterator<Item = &'a CoverageKind> + Captures<'tcx> {
body.basic_blocks.iter().flat_map(|bb_data| &bb_data.statements).filter_map(|statement| {
match statement.kind {
StatementKind::Coverage(box ref coverage) if !is_inlined(body, statement) => {
Some(coverage)
}
StatementKind::Coverage(ref kind) if !is_inlined(body, statement) => Some(kind),
_ => None,
}
})
Expand Down
34 changes: 14 additions & 20 deletions compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,7 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option<Span> {
// for their parent `BasicBlock`.
StatementKind::StorageLive(_)
| StatementKind::StorageDead(_)
// Ignore `ConstEvalCounter`s
| StatementKind::ConstEvalCounter
// Ignore `Nop`s
| StatementKind::Nop => None,

// FIXME(#78546): MIR InstrumentCoverage - Can the source_info.span for `FakeRead`
Expand All @@ -211,30 +209,28 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option<Span> {
StatementKind::FakeRead(box (FakeReadCause::ForGuardBinding, _)) => None,

// Retain spans from most other statements.
StatementKind::FakeRead(box (_, _)) // Not including `ForGuardBinding`
StatementKind::FakeRead(_)
| StatementKind::Intrinsic(..)
| StatementKind::Coverage(box mir::Coverage {
| StatementKind::Coverage(
// The purpose of `SpanMarker` is to be matched and accepted here.
kind: CoverageKind::SpanMarker
})
CoverageKind::SpanMarker,
)
| StatementKind::Assign(_)
| StatementKind::SetDiscriminant { .. }
| StatementKind::Deinit(..)
| StatementKind::Retag(_, _)
| StatementKind::PlaceMention(..)
| StatementKind::AscribeUserType(_, _) => {
Some(statement.source_info.span)
}
| StatementKind::AscribeUserType(_, _) => Some(statement.source_info.span),

StatementKind::Coverage(box mir::Coverage {
// Block markers are used for branch coverage, so ignore them here.
kind: CoverageKind::BlockMarker {..}
}) => None,
// Block markers are used for branch coverage, so ignore them here.
StatementKind::Coverage(CoverageKind::BlockMarker { .. }) => None,

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:?}"),
// These coverage statements should not exist prior to coverage instrumentation.
StatementKind::Coverage(
CoverageKind::CounterIncrement { .. } | CoverageKind::ExpressionUsed { .. },
) => bug!(
"Unexpected coverage statement found during coverage instrumentation: {statement:?}"
),
}
}

Expand Down Expand Up @@ -382,9 +378,7 @@ pub(super) fn extract_branch_mappings(
// Fill out the mapping from block marker IDs to their enclosing blocks.
for (bb, data) in mir_body.basic_blocks.iter_enumerated() {
for statement in &data.statements {
if let StatementKind::Coverage(coverage) = &statement.kind
&& let CoverageKind::BlockMarker { id } = coverage.kind
{
if let StatementKind::Coverage(CoverageKind::BlockMarker { id }) = statement.kind {
block_markers[id] = Some(bb);
}
}
Expand Down

0 comments on commit 19d3827

Please sign in to comment.