From a3d737a08641b20408fd70d580b2dec93448b469 Mon Sep 17 00:00:00 2001 From: zhuyunxing Date: Fri, 26 Apr 2024 18:37:51 +0800 Subject: [PATCH] coverage. Split mcdc code into a sub module of coverageingo --- .../rustc_mir_build/src/build/coverageinfo.rs | 304 ++++-------------- .../src/build/coverageinfo/mcdc.rs | 231 +++++++++++++ 2 files changed, 288 insertions(+), 247 deletions(-) create mode 100644 compiler/rustc_mir_build/src/build/coverageinfo/mcdc.rs diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 9e9ccd3dc2d01..32e4eeadbbe0c 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -1,31 +1,24 @@ +mod mcdc; +use mcdc::MCDCInfoBuilder; use std::assert_matches::assert_matches; use std::collections::hash_map::Entry; -use std::collections::VecDeque; use rustc_data_structures::fx::FxHashMap; -use rustc_middle::mir::coverage::{ - BlockMarkerId, BranchSpan, ConditionId, ConditionInfo, CoverageKind, MCDCBranchSpan, - MCDCDecisionSpan, -}; +use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, CoverageKind}; use rustc_middle::mir::{self, BasicBlock, SourceInfo, UnOp}; -use rustc_middle::thir::{ExprId, ExprKind, LogicalOp, Thir}; +use rustc_middle::thir::{ExprId, ExprKind, Thir}; use rustc_middle::ty::TyCtxt; use rustc_span::def_id::LocalDefId; -use rustc_span::Span; use crate::build::{Builder, CFG}; -use crate::errors::MCDCExceedsConditionNumLimit; pub(crate) struct BranchInfoBuilder { /// Maps condition expressions to their enclosing `!`, for better instrumentation. nots: FxHashMap, - - num_block_markers: usize, + block_marker_generator: BlockMarkerGenerator, branch_spans: Vec, - mcdc_branch_spans: Vec, - mcdc_decision_spans: Vec, - mcdc_state: Option, + mcdc_builder: Option, } #[derive(Clone, Copy)] @@ -38,6 +31,36 @@ struct NotInfo { is_flipped: bool, } +#[derive(Default)] +struct BlockMarkerGenerator { + num_block_markers: usize, +} + +impl BlockMarkerGenerator { + fn next_block_marker_id(&mut self) -> BlockMarkerId { + let id = BlockMarkerId::from_usize(self.num_block_markers); + self.num_block_markers += 1; + id + } + + fn inject_block_marker( + &mut self, + cfg: &mut CFG<'_>, + source_info: SourceInfo, + block: BasicBlock, + ) -> BlockMarkerId { + let id = self.next_block_marker_id(); + + let marker_statement = mir::Statement { + source_info, + kind: mir::StatementKind::Coverage(CoverageKind::BlockMarker { id }), + }; + cfg.push(block, marker_statement); + + id + } +} + impl BranchInfoBuilder { /// Creates a new branch info builder, but only if branch coverage instrumentation /// is enabled and `def_id` represents a function that is eligible for coverage. @@ -45,11 +68,9 @@ impl BranchInfoBuilder { if tcx.sess.instrument_coverage_branch() && tcx.is_eligible_for_coverage(def_id) { Some(Self { nots: FxHashMap::default(), - num_block_markers: 0, + block_marker_generator: BlockMarkerGenerator::default(), branch_spans: vec![], - mcdc_branch_spans: vec![], - mcdc_decision_spans: vec![], - mcdc_state: MCDCState::new_if_enabled(tcx), + mcdc_builder: tcx.sess.instrument_coverage_mcdc().then(MCDCInfoBuilder::new), }) } else { None @@ -96,44 +117,6 @@ impl BranchInfoBuilder { } } - fn fetch_mcdc_condition_info( - &mut self, - tcx: TyCtxt<'_>, - true_marker: BlockMarkerId, - false_marker: BlockMarkerId, - ) -> Option { - let mcdc_state = self.mcdc_state.as_mut()?; - let (mut condition_info, decision_result) = - mcdc_state.take_condition(true_marker, false_marker); - if let Some(decision) = decision_result { - match decision.conditions_num { - 0 => { - unreachable!("Decision with no condition is not expected"); - } - 1..=MAX_CONDITIONS_NUM_IN_DECISION => { - self.mcdc_decision_spans.push(decision); - } - _ => { - // Do not generate mcdc mappings and statements for decisions with too many conditions. - let rebase_idx = self.mcdc_branch_spans.len() - decision.conditions_num + 1; - for branch in &mut self.mcdc_branch_spans[rebase_idx..] { - branch.condition_info = None; - } - - // ConditionInfo of this branch shall also be reset. - condition_info = None; - - tcx.dcx().emit_warn(MCDCExceedsConditionNumLimit { - span: decision.span, - conditions_num: decision.conditions_num, - max_conditions_num: MAX_CONDITIONS_NUM_IN_DECISION, - }); - } - } - } - condition_info - } - fn add_two_way_branch<'tcx>( &mut self, cfg: &mut CFG<'tcx>, @@ -141,43 +124,20 @@ impl BranchInfoBuilder { true_block: BasicBlock, false_block: BasicBlock, ) { - let true_marker = self.inject_block_marker(cfg, source_info, true_block); - let false_marker = self.inject_block_marker(cfg, source_info, false_block); + let true_marker = + self.block_marker_generator.inject_block_marker(cfg, source_info, true_block); + let false_marker = + self.block_marker_generator.inject_block_marker(cfg, source_info, false_block); self.branch_spans.push(BranchSpan { span: source_info.span, true_marker, false_marker }); } - fn next_block_marker_id(&mut self) -> BlockMarkerId { - let id = BlockMarkerId::from_usize(self.num_block_markers); - self.num_block_markers += 1; - id - } - - fn inject_block_marker( - &mut self, - cfg: &mut CFG<'_>, - source_info: SourceInfo, - block: BasicBlock, - ) -> BlockMarkerId { - let id = self.next_block_marker_id(); - - let marker_statement = mir::Statement { - source_info, - kind: mir::StatementKind::Coverage(CoverageKind::BlockMarker { id }), - }; - cfg.push(block, marker_statement); - - id - } - pub(crate) fn into_done(self) -> Option> { let Self { nots: _, - num_block_markers, + block_marker_generator: BlockMarkerGenerator { num_block_markers }, branch_spans, - mcdc_branch_spans, - mcdc_decision_spans, - mcdc_state: _, + mcdc_builder, } = self; if num_block_markers == 0 { @@ -185,156 +145,18 @@ impl BranchInfoBuilder { return None; } + let (mcdc_decision_spans, mcdc_branch_spans) = + mcdc_builder.map(MCDCInfoBuilder::into_done).unwrap_or_default(); + Some(Box::new(mir::coverage::BranchInfo { num_block_markers, branch_spans, - mcdc_branch_spans, mcdc_decision_spans, + mcdc_branch_spans, })) } } -/// The MCDC bitmap scales exponentially (2^n) based on the number of conditions seen, -/// So llvm sets a maximum value prevents the bitmap footprint from growing too large without the user's knowledge. -/// This limit may be relaxed if the [upstream change](https://github.com/llvm/llvm-project/pull/82448) is merged. -const MAX_CONDITIONS_NUM_IN_DECISION: usize = 6; - -struct MCDCState { - /// To construct condition evaluation tree. - decision_stack: VecDeque, - processing_decision: Option, -} - -impl MCDCState { - fn new_if_enabled(tcx: TyCtxt<'_>) -> Option { - tcx.sess - .instrument_coverage_mcdc() - .then(|| Self { decision_stack: VecDeque::new(), processing_decision: None }) - } - - // At first we assign ConditionIds for each sub expression. - // If the sub expression is composite, re-assign its ConditionId to its LHS and generate a new ConditionId for its RHS. - // - // Example: "x = (A && B) || (C && D) || (D && F)" - // - // Visit Depth1: - // (A && B) || (C && D) || (D && F) - // ^-------LHS--------^ ^-RHS--^ - // ID=1 ID=2 - // - // Visit LHS-Depth2: - // (A && B) || (C && D) - // ^-LHS--^ ^-RHS--^ - // ID=1 ID=3 - // - // Visit LHS-Depth3: - // (A && B) - // LHS RHS - // ID=1 ID=4 - // - // Visit RHS-Depth3: - // (C && D) - // LHS RHS - // ID=3 ID=5 - // - // Visit RHS-Depth2: (D && F) - // LHS RHS - // ID=2 ID=6 - // - // Visit Depth1: - // (A && B) || (C && D) || (D && F) - // ID=1 ID=4 ID=3 ID=5 ID=2 ID=6 - // - // A node ID of '0' always means MC/DC isn't being tracked. - // - // If a "next" node ID is '0', it means it's the end of the test vector. - // - // As the compiler tracks expression in pre-order, we can ensure that condition info of parents are always properly assigned when their children are visited. - // - If the op is AND, the "false_next" of LHS and RHS should be the parent's "false_next". While "true_next" of the LHS is the RHS, the "true next" of RHS is the parent's "true_next". - // - If the op is OR, the "true_next" of LHS and RHS should be the parent's "true_next". While "false_next" of the LHS is the RHS, the "false next" of RHS is the parent's "false_next". - fn record_conditions(&mut self, op: LogicalOp, span: Span) { - let decision = match self.processing_decision.as_mut() { - Some(decision) => { - decision.span = decision.span.to(span); - decision - } - None => self.processing_decision.insert(MCDCDecisionSpan { - span, - conditions_num: 0, - end_markers: vec![], - }), - }; - - let parent_condition = self.decision_stack.pop_back().unwrap_or_default(); - let lhs_id = if parent_condition.condition_id == ConditionId::NONE { - decision.conditions_num += 1; - ConditionId::from(decision.conditions_num) - } else { - parent_condition.condition_id - }; - - decision.conditions_num += 1; - let rhs_condition_id = ConditionId::from(decision.conditions_num); - - let (lhs, rhs) = match op { - LogicalOp::And => { - let lhs = ConditionInfo { - condition_id: lhs_id, - true_next_id: rhs_condition_id, - false_next_id: parent_condition.false_next_id, - }; - let rhs = ConditionInfo { - condition_id: rhs_condition_id, - true_next_id: parent_condition.true_next_id, - false_next_id: parent_condition.false_next_id, - }; - (lhs, rhs) - } - LogicalOp::Or => { - let lhs = ConditionInfo { - condition_id: lhs_id, - true_next_id: parent_condition.true_next_id, - false_next_id: rhs_condition_id, - }; - let rhs = ConditionInfo { - condition_id: rhs_condition_id, - true_next_id: parent_condition.true_next_id, - false_next_id: parent_condition.false_next_id, - }; - (lhs, rhs) - } - }; - // We visit expressions tree in pre-order, so place the left-hand side on the top. - self.decision_stack.push_back(rhs); - self.decision_stack.push_back(lhs); - } - - fn take_condition( - &mut self, - true_marker: BlockMarkerId, - false_marker: BlockMarkerId, - ) -> (Option, Option) { - let Some(condition_info) = self.decision_stack.pop_back() else { - return (None, None); - }; - let Some(decision) = self.processing_decision.as_mut() else { - bug!("Processing decision should have been created before any conditions are taken"); - }; - if condition_info.true_next_id == ConditionId::NONE { - decision.end_markers.push(true_marker); - } - if condition_info.false_next_id == ConditionId::NONE { - decision.end_markers.push(false_marker); - } - - if self.decision_stack.is_empty() { - (Some(condition_info), self.processing_decision.take()) - } else { - (Some(condition_info), None) - } - } -} - impl Builder<'_, '_> { /// If branch coverage is enabled, inject marker statements into `then_block` /// and `else_block`, and record their IDs in the table of branch spans. @@ -359,30 +181,18 @@ impl Builder<'_, '_> { let source_info = SourceInfo { span: self.thir[expr_id].span, scope: self.source_scope }; // Separate path for handling branches when MC/DC is enabled. - if branch_info.mcdc_state.is_some() { - let mut inject_block_marker = - |block| branch_info.inject_block_marker(&mut self.cfg, source_info, block); - let true_marker = inject_block_marker(then_block); - let false_marker = inject_block_marker(else_block); - let condition_info = - branch_info.fetch_mcdc_condition_info(self.tcx, true_marker, false_marker); - branch_info.mcdc_branch_spans.push(MCDCBranchSpan { - span: source_info.span, - condition_info, - true_marker, - false_marker, - }); + if let Some(mcdc_builder) = branch_info.mcdc_builder.as_mut() { + mcdc_builder.visit_evaluated_condition( + self.tcx, + &mut self.cfg, + source_info, + then_block, + else_block, + &mut branch_info.block_marker_generator, + ); return; } branch_info.add_two_way_branch(&mut self.cfg, source_info, then_block, else_block); } - - pub(crate) fn visit_coverage_branch_operation(&mut self, logical_op: LogicalOp, span: Span) { - if let Some(branch_info) = self.coverage_branch_info.as_mut() - && let Some(mcdc_state) = branch_info.mcdc_state.as_mut() - { - mcdc_state.record_conditions(logical_op, span); - } - } } diff --git a/compiler/rustc_mir_build/src/build/coverageinfo/mcdc.rs b/compiler/rustc_mir_build/src/build/coverageinfo/mcdc.rs new file mode 100644 index 0000000000000..429abecdeb377 --- /dev/null +++ b/compiler/rustc_mir_build/src/build/coverageinfo/mcdc.rs @@ -0,0 +1,231 @@ +use std::collections::VecDeque; + +use rustc_middle::{ + mir::{ + coverage::{BlockMarkerId, ConditionId, ConditionInfo, MCDCBranchSpan, MCDCDecisionSpan}, + BasicBlock, SourceInfo, + }, + thir::LogicalOp, + ty::TyCtxt, +}; +use rustc_span::Span; + +use crate::{ + build::{Builder, CFG}, + errors::MCDCExceedsConditionNumLimit, +}; + +use super::BlockMarkerGenerator; + +/// The MCDC bitmap scales exponentially (2^n) based on the number of conditions seen, +/// So llvm sets a maximum value prevents the bitmap footprint from growing too large without the user's knowledge. +/// This limit may be relaxed if the [upstream change](https://github.com/llvm/llvm-project/pull/82448) is merged. +const MAX_CONDITIONS_NUM_IN_DECISION: usize = 6; + +#[derive(Default)] +pub(super) struct MCDCInfoBuilder { + branch_spans: Vec, + decision_spans: Vec, + state: MCDCState, +} + +impl MCDCInfoBuilder { + pub(super) fn new() -> Self { + Self::default() + } + + pub(super) fn visit_evaluated_condition( + &mut self, + tcx: TyCtxt<'_>, + cfg: &mut CFG<'_>, + source_info: SourceInfo, + true_blk: BasicBlock, + false_blk: BasicBlock, + block_marker_generator: &mut BlockMarkerGenerator, + ) { + let true_marker = block_marker_generator.inject_block_marker(cfg, source_info, true_blk); + let false_marker = block_marker_generator.inject_block_marker(cfg, source_info, false_blk); + + let (mut condition_info, decision_result) = + self.state.take_condition(true_marker, false_marker); + if let Some(decision) = decision_result { + match decision.conditions_num { + 0 => { + unreachable!("Decision with no condition is not expected"); + } + 1..=MAX_CONDITIONS_NUM_IN_DECISION => { + self.decision_spans.push(decision); + } + _ => { + // Do not generate mcdc mappings and statements for decisions with too many conditions. + let rebase_idx = self.branch_spans.len() + 1 - decision.conditions_num; + for branch in &mut self.branch_spans[rebase_idx..] { + branch.condition_info = None; + } + + // ConditionInfo of this branch shall also be reset. + condition_info = None; + + tcx.dcx().emit_warn(MCDCExceedsConditionNumLimit { + span: decision.span, + conditions_num: decision.conditions_num, + max_conditions_num: MAX_CONDITIONS_NUM_IN_DECISION, + }); + } + } + } + self.branch_spans.push(MCDCBranchSpan { + span: source_info.span, + condition_info, + true_marker, + false_marker, + }); + } + + pub(super) fn into_done(self) -> (Vec, Vec) { + (self.decision_spans, self.branch_spans) + } +} + +#[derive(Default)] +pub struct MCDCState { + /// To construct condition evaluation tree. + decision_stack: VecDeque, + processing_decision: Option, +} + +impl MCDCState { + // At first we assign ConditionIds for each sub expression. + // If the sub expression is composite, re-assign its ConditionId to its LHS and generate a new ConditionId for its RHS. + // + // Example: "x = (A && B) || (C && D) || (D && F)" + // + // Visit Depth1: + // (A && B) || (C && D) || (D && F) + // ^-------LHS--------^ ^-RHS--^ + // ID=1 ID=2 + // + // Visit LHS-Depth2: + // (A && B) || (C && D) + // ^-LHS--^ ^-RHS--^ + // ID=1 ID=3 + // + // Visit LHS-Depth3: + // (A && B) + // LHS RHS + // ID=1 ID=4 + // + // Visit RHS-Depth3: + // (C && D) + // LHS RHS + // ID=3 ID=5 + // + // Visit RHS-Depth2: (D && F) + // LHS RHS + // ID=2 ID=6 + // + // Visit Depth1: + // (A && B) || (C && D) || (D && F) + // ID=1 ID=4 ID=3 ID=5 ID=2 ID=6 + // + // A node ID of '0' always means MC/DC isn't being tracked. + // + // If a "next" node ID is '0', it means it's the end of the test vector. + // + // As the compiler tracks expression in pre-order, we can ensure that condition info of parents are always properly assigned when their children are visited. + // - If the op is AND, the "false_next" of LHS and RHS should be the parent's "false_next". While "true_next" of the LHS is the RHS, the "true next" of RHS is the parent's "true_next". + // - If the op is OR, the "true_next" of LHS and RHS should be the parent's "true_next". While "false_next" of the LHS is the RHS, the "false next" of RHS is the parent's "false_next". + fn record_conditions(&mut self, op: LogicalOp, span: Span) { + let decision = match self.processing_decision.as_mut() { + Some(decision) => { + decision.span = decision.span.to(span); + decision + } + None => self.processing_decision.insert(MCDCDecisionSpan { + span, + conditions_num: 0, + end_markers: vec![], + }), + }; + + let parent_condition = self.decision_stack.pop_back().unwrap_or_default(); + let lhs_id = if parent_condition.condition_id == ConditionId::NONE { + decision.conditions_num += 1; + ConditionId::from(decision.conditions_num) + } else { + parent_condition.condition_id + }; + + decision.conditions_num += 1; + let rhs_condition_id = ConditionId::from(decision.conditions_num); + + let (lhs, rhs) = match op { + LogicalOp::And => { + let lhs = ConditionInfo { + condition_id: lhs_id, + true_next_id: rhs_condition_id, + false_next_id: parent_condition.false_next_id, + }; + let rhs = ConditionInfo { + condition_id: rhs_condition_id, + true_next_id: parent_condition.true_next_id, + false_next_id: parent_condition.false_next_id, + }; + (lhs, rhs) + } + LogicalOp::Or => { + let lhs = ConditionInfo { + condition_id: lhs_id, + true_next_id: parent_condition.true_next_id, + false_next_id: rhs_condition_id, + }; + let rhs = ConditionInfo { + condition_id: rhs_condition_id, + true_next_id: parent_condition.true_next_id, + false_next_id: parent_condition.false_next_id, + }; + (lhs, rhs) + } + }; + // We visit expressions tree in pre-order, so place the left-hand side on the top. + self.decision_stack.push_back(rhs); + self.decision_stack.push_back(lhs); + } + + fn take_condition( + &mut self, + true_marker: BlockMarkerId, + false_marker: BlockMarkerId, + ) -> (Option, Option) { + let Some(condition_info) = self.decision_stack.pop_back() else { + return (None, None); + }; + let Some(decision) = self.processing_decision.as_mut() else { + bug!("Processing decision should have been created before any conditions are taken"); + }; + if condition_info.true_next_id == ConditionId::NONE { + decision.end_markers.push(true_marker); + } + if condition_info.false_next_id == ConditionId::NONE { + decision.end_markers.push(false_marker); + } + + if self.decision_stack.is_empty() { + (Some(condition_info), self.processing_decision.take()) + } else { + (Some(condition_info), None) + } + } +} + +impl Builder<'_, '_> { + pub(crate) fn visit_coverage_branch_operation(&mut self, logical_op: LogicalOp, span: Span) { + if let Some(mcdc_builder) = self + .coverage_branch_info + .as_mut() + .and_then(|branch_info| branch_info.mcdc_builder.as_mut()) + { + mcdc_builder.state.record_conditions(logical_op, span); + } + } +}