Skip to content
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

coverage: Clean up mcdc_bitmap_bytes and conditions_num #124571

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ pub mod mcdc {
#[derive(Clone, Copy, Debug, Default)]
pub struct DecisionParameters {
bitmap_idx: u32,
conditions_num: u16,
num_conditions: u16,
}

// ConditionId in llvm is `unsigned int` at 18 while `int16_t` at [19](https://github.com/llvm/llvm-project/pull/81257)
Expand Down Expand Up @@ -178,7 +178,7 @@ pub mod mcdc {

impl From<DecisionInfo> for DecisionParameters {
fn from(value: DecisionInfo) -> Self {
Self { bitmap_idx: value.bitmap_idx, conditions_num: value.conditions_num }
Self { bitmap_idx: value.bitmap_idx, num_conditions: value.num_conditions }
}
}
}
Expand Down
7 changes: 1 addition & 6 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,8 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
let cond_bitmap = coverage_context
.try_get_mcdc_condition_bitmap(&instance, decision_depth)
.expect("mcdc cond bitmap should have been allocated for merging into the global bitmap");
let bitmap_bytes = bx.tcx().coverage_ids_info(instance.def).mcdc_bitmap_bytes;
let bitmap_bytes = function_coverage_info.mcdc_bitmap_bytes;
assert!(bitmap_idx < bitmap_bytes, "bitmap index of the decision out of range");
assert!(
bitmap_bytes <= function_coverage_info.mcdc_bitmap_bytes,
"bitmap length disagreement: query says {bitmap_bytes} but function info only has {}",
function_coverage_info.mcdc_bitmap_bytes
);

let fn_name = bx.get_pgo_func_name_var(instance);
let hash = bx.const_u64(function_coverage_info.function_source_hash);
Expand Down
10 changes: 2 additions & 8 deletions compiler/rustc_middle/src/mir/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,11 @@ pub enum CoverageKind {
/// Marks the point in MIR control flow represented by a evaluated condition.
///
/// This is eventually lowered to `llvm.instrprof.mcdc.condbitmap.update` in LLVM IR.
///
/// If this statement does not survive MIR optimizations, the condition would never be
/// taken as evaluated.
CondBitmapUpdate { id: ConditionId, value: bool, decision_depth: u16 },

/// Marks the point in MIR control flow represented by a evaluated decision.
///
/// This is eventually lowered to `llvm.instrprof.mcdc.tvbitmap.update` in LLVM IR.
///
/// If this statement does not survive MIR optimizations, the decision would never be
/// taken as evaluated.
TestVectorBitmapUpdate { bitmap_idx: u32, decision_depth: u16 },
}

Expand Down Expand Up @@ -335,14 +329,14 @@ pub struct MCDCBranchSpan {
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub struct DecisionInfo {
pub bitmap_idx: u32,
pub conditions_num: u16,
pub num_conditions: u16,
}

#[derive(Clone, Debug)]
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub struct MCDCDecisionSpan {
pub span: Span,
pub conditions_num: usize,
pub num_conditions: usize,
pub end_markers: Vec<BlockMarkerId>,
pub decision_depth: u16,
}
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 @@ -511,12 +511,12 @@ fn write_coverage_branch_info(
)?;
}

for coverage::MCDCDecisionSpan { span, conditions_num, end_markers, decision_depth } in
for coverage::MCDCDecisionSpan { span, num_conditions, end_markers, decision_depth } in
mcdc_decision_spans
{
writeln!(
w,
"{INDENT}coverage mcdc decision {{ conditions_num: {conditions_num:?}, end: {end_markers:?}, depth: {decision_depth:?} }} => {span:?}"
"{INDENT}coverage mcdc decision {{ num_conditions: {num_conditions:?}, end: {end_markers:?}, depth: {decision_depth:?} }} => {span:?}"
)?;
}

Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,4 @@ pub struct CoverageIdsInfo {
/// InstrumentCoverage MIR pass, if the highest-numbered counter increments
/// were removed by MIR optimizations.
pub max_counter_id: mir::coverage::CounterId,

/// Coverage codegen for mcdc needs to know the size of the global bitmap so that it can
/// set the `bytemap-bytes` argument of the `llvm.instrprof.mcdc.tvbitmap.update` intrinsic.
pub mcdc_bitmap_bytes: u32,
}
4 changes: 3 additions & 1 deletion compiler/rustc_mir_build/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ mir_build_deref_raw_pointer_requires_unsafe_unsafe_op_in_unsafe_fn_allowed =
.note = raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior
.label = dereference of raw pointer

mir_build_exceeds_mcdc_condition_num_limit = Conditions number of the decision ({$conditions_num}) exceeds limit ({$max_conditions_num}). MCDC analysis will not count this expression.
mir_build_exceeds_mcdc_condition_limit =
number of conditions in decision ({$num_conditions}) exceeds limit ({$limit})
.note = this decision will not be instrumented for MC/DC coverage

mir_build_extern_static_requires_unsafe =
use of extern static is unsafe and requires unsafe block
Expand Down
30 changes: 15 additions & 15 deletions compiler/rustc_mir_build/src/build/coverageinfo/mcdc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ use rustc_middle::ty::TyCtxt;
use rustc_span::Span;

use crate::build::Builder;
use crate::errors::MCDCExceedsConditionNumLimit;
use crate::errors::MCDCExceedsConditionLimit;

/// 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;
/// So LLVM imposes a limit to prevent the bitmap footprint from growing too large without the user's knowledge.
/// This limit may be relaxed if [upstream change #82448](https://github.com/llvm/llvm-project/pull/82448) is merged.
const MAX_CONDITIONS_IN_DECISION: usize = 6;

#[derive(Default)]
struct MCDCDecisionCtx {
Expand Down Expand Up @@ -99,22 +99,22 @@ impl MCDCState {
}
None => decision_ctx.processing_decision.insert(MCDCDecisionSpan {
span,
conditions_num: 0,
num_conditions: 0,
end_markers: vec![],
decision_depth,
}),
};

let parent_condition = decision_ctx.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)
decision.num_conditions += 1;
ConditionId::from(decision.num_conditions)
} else {
parent_condition.condition_id
};

decision.conditions_num += 1;
let rhs_condition_id = ConditionId::from(decision.conditions_num);
decision.num_conditions += 1;
let rhs_condition_id = ConditionId::from(decision.num_conditions);

let (lhs, rhs) = match op {
LogicalOp::And => {
Expand Down Expand Up @@ -207,27 +207,27 @@ impl MCDCInfoBuilder {
// is empty, i.e. when all the conditions of the decision were instrumented,
// and the decision is "complete".
if let Some(decision) = decision_result {
match decision.conditions_num {
match decision.num_conditions {
0 => {
unreachable!("Decision with no condition is not expected");
}
1..=MAX_CONDITIONS_NUM_IN_DECISION => {
1..=MAX_CONDITIONS_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() - decision.conditions_num + 1;
let rebase_idx = self.branch_spans.len() - decision.num_conditions + 1;
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 {
tcx.dcx().emit_warn(MCDCExceedsConditionLimit {
span: decision.span,
conditions_num: decision.conditions_num,
max_conditions_num: MAX_CONDITIONS_NUM_IN_DECISION,
num_conditions: decision.num_conditions,
limit: MAX_CONDITIONS_IN_DECISION,
});
}
}
Expand Down
9 changes: 5 additions & 4 deletions compiler/rustc_mir_build/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -819,12 +819,13 @@ pub struct NontrivialStructuralMatch<'tcx> {
}

#[derive(Diagnostic)]
#[diag(mir_build_exceeds_mcdc_condition_num_limit)]
pub(crate) struct MCDCExceedsConditionNumLimit {
#[diag(mir_build_exceeds_mcdc_condition_limit)]
#[note]
pub(crate) struct MCDCExceedsConditionLimit {
#[primary_span]
pub span: Span,
pub conditions_num: usize,
pub max_conditions_num: usize,
pub num_conditions: usize,
pub limit: usize,
}

#[derive(Diagnostic)]
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_mir_transform/src/coverage/mappings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ 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) num_conditions: u16,
pub(super) decision_depth: u16,
}

Expand Down Expand Up @@ -136,8 +136,8 @@ pub(super) fn generate_coverage_spans(
// 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)
.map(|&MCDCDecision { bitmap_idx, num_conditions, .. }| {
bitmap_idx + (1_u32 << u32::from(num_conditions)).div_ceil(8)
})
.max()
.unwrap_or(0);
Expand Down Expand Up @@ -266,13 +266,13 @@ pub(super) fn extract_mcdc_mappings(
.collect::<Option<_>>()?;

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

Some(MCDCDecision {
span,
end_bcbs,
bitmap_idx,
conditions_num: decision.conditions_num as u16,
num_conditions: decision.num_conditions as u16,
decision_depth: decision.decision_depth,
})
},
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,9 @@ fn create_mappings<'tcx>(
));

mappings.extend(coverage_spans.mcdc_decisions.iter().filter_map(
|&mappings::MCDCDecision { span, bitmap_idx, conditions_num, .. }| {
|&mappings::MCDCDecision { span, bitmap_idx, num_conditions, .. }| {
let code_region = region_for_span(span)?;
let kind = MappingKind::MCDCDecision(DecisionInfo { bitmap_idx, conditions_num });
let kind = MappingKind::MCDCDecision(DecisionInfo { bitmap_idx, num_conditions });
Some(Mapping { kind, code_region })
},
));
Expand Down Expand Up @@ -260,7 +260,7 @@ fn inject_mcdc_statements<'tcx>(
span: _,
ref end_bcbs,
bitmap_idx,
conditions_num: _,
num_conditions: _,
decision_depth,
} in &coverage_spans.mcdc_decisions
{
Expand Down
12 changes: 1 addition & 11 deletions compiler/rustc_mir_transform/src/coverage/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,7 @@ fn coverage_ids_info<'tcx>(
.max()
.unwrap_or(CounterId::ZERO);

let mcdc_bitmap_bytes = mir_body
.coverage_branch_info
.as_deref()
.map(|info| {
info.mcdc_decision_spans
.iter()
.fold(0, |acc, decision| acc + (1_u32 << decision.conditions_num).div_ceil(8))
})
.unwrap_or_default();

CoverageIdsInfo { max_counter_id, mcdc_bitmap_bytes }
CoverageIdsInfo { max_counter_id }
}

fn all_coverage_in_mir_body<'a, 'tcx>(
Expand Down
Loading
Loading