Skip to content

Commit

Permalink
Auto merge of #124603 - Zalathar:mcdc-mappings, r=Nadrieril
Browse files Browse the repository at this point in the history
coverage: Split out MC/DC mappings from `BcbMappingKind`

These variants were added to `BcbMappingKind` as part of the [MC/DC coverage](https://en.wikipedia.org/wiki/Modified_Condition/Decision_Coverage) implementation in #123409, because that was the path-of-least-resistance for integrating them into the existing code.

However, they ultimately represent complex concepts that the enum was not intended to handle, leading to more complexity in the code that processes them. This PR therefore follows in the footsteps of #124545, and splits the MC/DC mappings out into their own dedicated vectors of structs.

After that, `BcbMappingKind` itself ends up having only one variant (`Code`), so this PR also flattens that enum into its enclosing struct, renamed to `mapping::CodeMapping`.

---

No functional changes.

This will conflict slightly with #124571, but hopefully that should be easy to resolve either way.

`@rustbot` label +A-code-coverage
  • Loading branch information
bors committed May 5, 2024
2 parents 872a856 + 6968123 commit 9c9b568
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 157 deletions.
213 changes: 109 additions & 104 deletions compiler/rustc_mir_transform/src/coverage/mappings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ use std::collections::BTreeSet;

use rustc_data_structures::graph::DirectedGraph;
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::coverage::{
BlockMarkerId, BranchSpan, ConditionInfo, CoverageKind, MCDCBranchSpan, MCDCDecisionSpan,
};
use rustc_index::IndexVec;
use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, ConditionInfo, CoverageKind};
use rustc_middle::mir::{self, BasicBlock, StatementKind};
use rustc_span::Span;

Expand All @@ -13,55 +12,53 @@ use crate::coverage::spans::{
extract_refined_covspans, unexpand_into_body_span_with_visible_macro,
};
use crate::coverage::ExtractedHirInfo;
use rustc_index::IndexVec;

#[derive(Clone, Debug)]
pub(super) enum BcbMappingKind {
/// Associates an ordinary executable code span with its corresponding BCB.
Code(BasicCoverageBlock),

// Ordinary branch mappings are stored separately, so they don't have a
// variant in this enum.
//
/// Associates a mcdc branch span with condition info besides fields for normal branch.
MCDCBranch {
true_bcb: BasicCoverageBlock,
false_bcb: BasicCoverageBlock,
/// If `None`, this actually represents a normal branch mapping inserted
/// for code that was too complex for MC/DC.
condition_info: Option<ConditionInfo>,
decision_depth: u16,
},
/// Associates a mcdc decision with its join BCB.
MCDCDecision {
end_bcbs: BTreeSet<BasicCoverageBlock>,
bitmap_idx: u32,
conditions_num: u16,
decision_depth: u16,
},
}

/// Associates an ordinary executable code span with its corresponding BCB.
#[derive(Debug)]
pub(super) struct BcbMapping {
pub(super) kind: BcbMappingKind,
pub(super) struct CodeMapping {
pub(super) span: Span,
pub(super) bcb: BasicCoverageBlock,
}

/// This is separate from [`BcbMappingKind`] to help prepare for larger changes
/// This is separate from [`MCDCBranch`] to help prepare for larger changes
/// that will be needed for improved branch coverage in the future.
/// (See <https://github.com/rust-lang/rust/pull/124217>.)
#[derive(Debug)]
pub(super) struct BcbBranchPair {
pub(super) struct BranchPair {
pub(super) span: Span,
pub(super) true_bcb: BasicCoverageBlock,
pub(super) false_bcb: BasicCoverageBlock,
}

/// Associates an MC/DC branch span with condition info besides fields for normal branch.
#[derive(Debug)]
pub(super) struct MCDCBranch {
pub(super) span: Span,
pub(super) true_bcb: BasicCoverageBlock,
pub(super) false_bcb: BasicCoverageBlock,
/// If `None`, this actually represents a normal branch mapping inserted
/// for code that was too complex for MC/DC.
pub(super) condition_info: Option<ConditionInfo>,
pub(super) decision_depth: u16,
}

/// Associates an MC/DC decision with its join BCBs.
#[derive(Debug)]
pub(super) struct MCDCDecision {
pub(super) span: Span,
pub(super) end_bcbs: BTreeSet<BasicCoverageBlock>,
pub(super) bitmap_idx: u32,
pub(super) conditions_num: u16,
pub(super) decision_depth: u16,
}

pub(super) struct CoverageSpans {
bcb_has_mappings: BitSet<BasicCoverageBlock>,
pub(super) mappings: Vec<BcbMapping>,
pub(super) branch_pairs: Vec<BcbBranchPair>,
pub(super) code_mappings: Vec<CodeMapping>,
pub(super) branch_pairs: Vec<BranchPair>,
test_vector_bitmap_bytes: u32,
pub(super) mcdc_branches: Vec<MCDCBranch>,
pub(super) mcdc_decisions: Vec<MCDCDecision>,
}

impl CoverageSpans {
Expand All @@ -83,26 +80,38 @@ pub(super) fn generate_coverage_spans(
hir_info: &ExtractedHirInfo,
basic_coverage_blocks: &CoverageGraph,
) -> Option<CoverageSpans> {
let mut mappings = vec![];
let mut code_mappings = vec![];
let mut branch_pairs = vec![];
let mut mcdc_branches = vec![];
let mut mcdc_decisions = vec![];

if hir_info.is_async_fn {
// An async function desugars into a function that returns a future,
// with the user code wrapped in a closure. Any spans in the desugared
// outer function will be unhelpful, so just keep the signature span
// and ignore all of the spans in the MIR body.
if let Some(span) = hir_info.fn_sig_span_extended {
mappings.push(BcbMapping { kind: BcbMappingKind::Code(START_BCB), span });
code_mappings.push(CodeMapping { span, bcb: START_BCB });
}
} else {
extract_refined_covspans(mir_body, hir_info, basic_coverage_blocks, &mut mappings);
extract_refined_covspans(mir_body, hir_info, basic_coverage_blocks, &mut code_mappings);

branch_pairs.extend(extract_branch_pairs(mir_body, hir_info, basic_coverage_blocks));

mappings.extend(extract_mcdc_mappings(mir_body, hir_info.body_span, basic_coverage_blocks));
extract_mcdc_mappings(
mir_body,
hir_info.body_span,
basic_coverage_blocks,
&mut mcdc_branches,
&mut mcdc_decisions,
);
}

if mappings.is_empty() && branch_pairs.is_empty() {
if code_mappings.is_empty()
&& branch_pairs.is_empty()
&& mcdc_branches.is_empty()
&& mcdc_decisions.is_empty()
{
return None;
}

Expand All @@ -111,29 +120,36 @@ pub(super) fn generate_coverage_spans(
let mut insert = |bcb| {
bcb_has_mappings.insert(bcb);
};
let mut test_vector_bitmap_bytes = 0;
for BcbMapping { kind, span: _ } in &mappings {
match *kind {
BcbMappingKind::Code(bcb) => insert(bcb),
BcbMappingKind::MCDCBranch { true_bcb, false_bcb, .. } => {
insert(true_bcb);
insert(false_bcb);
}
BcbMappingKind::MCDCDecision { bitmap_idx, conditions_num, .. } => {
// `bcb_has_mappings` is used for inject coverage counters
// but they are not needed for decision BCBs.
// While the length of test vector bitmap should be calculated here.
test_vector_bitmap_bytes = test_vector_bitmap_bytes
.max(bitmap_idx + (1_u32 << conditions_num as u32).div_ceil(8));
}
}

for &CodeMapping { span: _, bcb } in &code_mappings {
insert(bcb);
}
for &BranchPair { true_bcb, false_bcb, .. } in &branch_pairs {
insert(true_bcb);
insert(false_bcb);
}
for &BcbBranchPair { true_bcb, false_bcb, .. } in &branch_pairs {
for &MCDCBranch { true_bcb, false_bcb, .. } in &mcdc_branches {
insert(true_bcb);
insert(false_bcb);
}

Some(CoverageSpans { bcb_has_mappings, mappings, branch_pairs, test_vector_bitmap_bytes })
// Determine the length of the test vector bitmap.
let test_vector_bitmap_bytes = mcdc_decisions
.iter()
.map(|&MCDCDecision { bitmap_idx, conditions_num, .. }| {
bitmap_idx + (1_u32 << u32::from(conditions_num)).div_ceil(8)
})
.max()
.unwrap_or(0);

Some(CoverageSpans {
bcb_has_mappings,
code_mappings,
branch_pairs,
test_vector_bitmap_bytes,
mcdc_branches,
mcdc_decisions,
})
}

fn resolve_block_markers(
Expand Down Expand Up @@ -167,7 +183,7 @@ pub(super) fn extract_branch_pairs(
mir_body: &mir::Body<'_>,
hir_info: &ExtractedHirInfo,
basic_coverage_blocks: &CoverageGraph,
) -> Vec<BcbBranchPair> {
) -> Vec<BranchPair> {
let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { return vec![] };

let block_markers = resolve_block_markers(branch_info, mir_body);
Expand All @@ -190,7 +206,7 @@ pub(super) fn extract_branch_pairs(
let true_bcb = bcb_from_marker(true_marker)?;
let false_bcb = bcb_from_marker(false_marker)?;

Some(BcbBranchPair { span, true_bcb, false_bcb })
Some(BranchPair { span, true_bcb, false_bcb })
})
.collect::<Vec<_>>()
}
Expand All @@ -199,10 +215,10 @@ pub(super) fn extract_mcdc_mappings(
mir_body: &mir::Body<'_>,
body_span: Span,
basic_coverage_blocks: &CoverageGraph,
) -> Vec<BcbMapping> {
let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else {
return vec![];
};
mcdc_branches: &mut impl Extend<MCDCBranch>,
mcdc_decisions: &mut impl Extend<MCDCDecision>,
) {
let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { return };

let block_markers = resolve_block_markers(branch_info, mir_body);

Expand All @@ -223,53 +239,42 @@ pub(super) fn extract_mcdc_mappings(
Some((span, true_bcb, false_bcb))
};

let mcdc_branch_filter_map = |&MCDCBranchSpan {
span: raw_span,
true_marker,
false_marker,
condition_info,
decision_depth,
}| {
check_branch_bcb(raw_span, true_marker, false_marker).map(|(span, true_bcb, false_bcb)| {
BcbMapping {
kind: BcbMappingKind::MCDCBranch {
true_bcb,
false_bcb,
condition_info,
decision_depth,
},
span,
}
})
};
mcdc_branches.extend(branch_info.mcdc_branch_spans.iter().filter_map(
|&mir::coverage::MCDCBranchSpan {
span: raw_span,
condition_info,
true_marker,
false_marker,
decision_depth,
}| {
let (span, true_bcb, false_bcb) =
check_branch_bcb(raw_span, true_marker, false_marker)?;
Some(MCDCBranch { span, true_bcb, false_bcb, condition_info, decision_depth })
},
));

let mut next_bitmap_idx = 0;

let decision_filter_map = |decision: &MCDCDecisionSpan| {
let (span, _) = unexpand_into_body_span_with_visible_macro(decision.span, body_span)?;
mcdc_decisions.extend(branch_info.mcdc_decision_spans.iter().filter_map(
|decision: &mir::coverage::MCDCDecisionSpan| {
let (span, _) = unexpand_into_body_span_with_visible_macro(decision.span, body_span)?;

let end_bcbs = decision
.end_markers
.iter()
.map(|&marker| bcb_from_marker(marker))
.collect::<Option<_>>()?;
let end_bcbs = decision
.end_markers
.iter()
.map(|&marker| bcb_from_marker(marker))
.collect::<Option<_>>()?;

let bitmap_idx = next_bitmap_idx;
next_bitmap_idx += (1_u32 << decision.conditions_num).div_ceil(8);
let bitmap_idx = next_bitmap_idx;
next_bitmap_idx += (1_u32 << decision.conditions_num).div_ceil(8);

Some(BcbMapping {
kind: BcbMappingKind::MCDCDecision {
Some(MCDCDecision {
span,
end_bcbs,
bitmap_idx,
conditions_num: decision.conditions_num as u16,
decision_depth: decision.decision_depth,
},
span,
})
};

std::iter::empty()
.chain(branch_info.mcdc_branch_spans.iter().filter_map(mcdc_branch_filter_map))
.chain(branch_info.mcdc_decision_spans.iter().filter_map(decision_filter_map))
.collect::<Vec<_>>()
})
},
));
}
Loading

0 comments on commit 9c9b568

Please sign in to comment.