mcdc-coverage: Remove MCDCBlockMarker, Rename DecisionMarkerId, Add ConditionEntry/Output markers

Author: RenjiSann

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -106,9 +106,10 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
         match *kind {
             CoverageKind::SpanMarker
             | CoverageKind::BlockMarker { .. }
-            | CoverageKind::MCDCBlockMarker { .. }
             | CoverageKind::MCDCDecisionEntryMarker { .. }
-            | CoverageKind::MCDCDecisionOutputMarker { .. } => unreachable!(
+            | CoverageKind::MCDCDecisionOutputMarker { .. }
+            | CoverageKind::MCDCConditionEntryMarker { .. }
+            | CoverageKind::MCDCConditionOutputMarker { .. } => unreachable!(
                 "marker statement {kind:?} should have been removed by CleanupPostBorrowck"
             ),
             CoverageKind::CounterIncrement { id } => {

compiler/rustc_middle/src/mir/coverage.rs

@@ -18,8 +18,15 @@ rustc_index::newtype_index! {
 rustc_index::newtype_index! {
     #[derive(HashStable)]
     #[encodable]
-    #[debug_format = "DecisionMarkerId({})"]
-    pub struct DecisionMarkerId {}
+    #[debug_format = "DecisionId({})"]
+    pub struct DecisionId {}
+}
+
+rustc_index::newtype_index! {
+    #[derive(HashStable)]
+    #[encodable]
+    #[debug_format = "ConditionId({})"]
+    pub struct ConditionId {}
 }
 
 rustc_index::newtype_index! {
@@ -104,24 +111,31 @@ pub enum CoverageKind {
     /// Should be erased before codegen (at some point after `InstrumentCoverage`).
     BlockMarker { id: BlockMarkerId },
 
-    /// Same as BlockMarker, but carries a reference to its parent decision for
-    /// MCDC coverage.
-    ///
-    /// Has no effect during codegen.
-    MCDCBlockMarker { id: BlockMarkerId, decision_id: DecisionMarkerId },
-
     /// Marks the first condition of a decision (boolean expression). All
     /// conditions in the same decision will reference this id.
     ///
     /// Has no effect during codegen.
-    MCDCDecisionEntryMarker { id: DecisionMarkerId },
+    MCDCDecisionEntryMarker { id: DecisionId },
 
     /// Marks one of the basic blocks following the decision referenced with `id`.
     /// the outcome bool designates which branch of the decision it is:
     /// `true` for the then block, `false` for the else block.
     ///
     /// Has no effect during codegen.
-    MCDCDecisionOutputMarker { id: DecisionMarkerId, outcome: bool },
+    MCDCDecisionOutputMarker { id: DecisionId, outcome: bool },
+
+    /// Marks a basic block where a condition evaluation occurs
+    /// The block may end with a SwitchInt where the 2 successors BBs have a
+    /// MCDCConditionOutcomeMarker statement with a matching ID.
+    ///
+    /// Has no effect during codegen.
+    MCDCConditionEntryMarker { decision_id: DecisionId, id: ConditionId },
+
+    /// Marks a basic block that is branched from a condition evaluation.
+    /// The block may have a predecessor with a matching ID.
+    ///
+    /// Has no effect during codegen.
+    MCDCConditionOutputMarker { decision_id: DecisionId, id: ConditionId, outcome: bool },
 
     /// Marks the point in MIR control flow represented by a coverage counter.
     ///
@@ -164,15 +178,24 @@ impl Debug for CoverageKind {
         match self {
             SpanMarker => write!(fmt, "SpanMarker"),
             BlockMarker { id } => write!(fmt, "BlockMarker({:?})", id.index()),
-            MCDCBlockMarker { id, decision_id } => {
-                write!(fmt, "MCDCBlockMarker({:?}, {:?})", id.index(), decision_id.index())
-            }
             MCDCDecisionEntryMarker { id } => {
                 write!(fmt, "MCDCDecisionEntryMarker({:?})", id.index())
             }
             MCDCDecisionOutputMarker { id, outcome } => {
                 write!(fmt, "MCDCDecisionOutputMarker({:?}, {})", id.index(), outcome)
             }
+            MCDCConditionEntryMarker { decision_id, id } => {
+                write!(fmt, "MCDCConditionEntryMarker({:?}, {:?})", decision_id.index(), id.index())
+            }
+            MCDCConditionOutputMarker { decision_id, id, outcome } => {
+                write!(
+                    fmt,
+                    "MCDCConditionOutcomeMarker({:?}, {:?}, {})",
+                    decision_id.index(),
+                    id.index(),
+                    outcome
+                )
+            }
             CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()),
             ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()),
             MCDCBitmapRequire { needed_bytes } => {
@@ -300,7 +323,7 @@ pub struct BranchInfo {
 
     /// Associate a span for every decision in the function body.
     /// Empty if MCDC coverage is disabled.
-    pub decision_spans: IndexVec<DecisionMarkerId, DecisionSpan>,
+    pub decision_spans: IndexVec<DecisionId, DecisionSpan>,
 }
 
 #[derive(Clone, Debug)]
@@ -319,7 +342,7 @@ pub struct BranchSpan {
     pub span: Span,
     /// ID of Decision structure the branch is part of. Only used in
     /// the MCDC coverage.
-    pub decision_id: DecisionMarkerId,
+    pub decision_id: DecisionId,
     pub true_marker: BlockMarkerId,
     pub false_marker: BlockMarkerId,
 }

compiler/rustc_middle/src/ty/structural_impls.rs

@@ -407,7 +407,8 @@ TrivialTypeTraversalImpls! {
     ::rustc_hir::MatchSource,
     ::rustc_target::asm::InlineAsmRegOrRegClass,
     crate::mir::coverage::BlockMarkerId,
-    crate::mir::coverage::DecisionMarkerId,
+    crate::mir::coverage::DecisionId,
+    crate::mir::coverage::ConditionId,
     crate::mir::coverage::CounterId,
     crate::mir::coverage::ExpressionId,
     crate::mir::Local,

compiler/rustc_mir_build/src/build/coverageinfo.rs

@@ -4,7 +4,7 @@ use std::collections::hash_map::Entry;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::IndexVec;
 use rustc_middle::mir::coverage::{
-    BlockMarkerId, BranchSpan, CoverageKind, DecisionMarkerId, DecisionSpan,
+    BlockMarkerId, BranchSpan, CoverageKind, DecisionId, DecisionSpan,
 };
 use rustc_middle::mir::{self, BasicBlock, UnOp};
 use rustc_middle::thir::{ExprId, ExprKind, Thir};
@@ -26,12 +26,12 @@ pub(crate) struct BranchInfoBuilder {
     /// ID of the current decision.
     /// Do not use directly. Use the function instead, as it will hide
     /// the decision in the scope of nested decisions.
-    current_decision_id: Option<DecisionMarkerId>,
+    current_decision_id: Option<DecisionId>,
     /// Track the nesting level of decision to avoid MCDC instrumentation of
     /// nested decisions.
     nested_decision_level: u32,
     /// Vector for storing all the decisions with their span
-    decisions: IndexVec<DecisionMarkerId, Span>,
+    decisions: IndexVec<DecisionId, Span>,
 }
 
 #[derive(Clone, Copy)]
@@ -158,7 +158,7 @@ impl BranchInfoBuilder {
         self.nested_decision_level > 1
     }
 
-    pub fn current_decision_id(&self) -> Option<DecisionMarkerId> {
+    pub fn current_decision_id(&self) -> Option<DecisionId> {
         if self.in_nested_condition() { None } else { self.current_decision_id }
     }
 }
@@ -191,14 +191,10 @@ impl Builder<'_, '_> {
         let mut inject_branch_marker = |block: BasicBlock| {
             let id = branch_info.next_block_marker_id();
 
-            let cov_kind = if let Some(decision_id) = branch_info.current_decision_id() {
-                CoverageKind::MCDCBlockMarker { id, decision_id }
-            } else {
-                CoverageKind::BlockMarker { id }
+            let marker_statement = mir::Statement {
+                source_info,
+                kind: mir::StatementKind::Coverage(CoverageKind::BlockMarker { id }),
             };
-
-            let marker_statement =
-                mir::Statement { source_info, kind: mir::StatementKind::Coverage(cov_kind) };
             self.cfg.push(block, marker_statement);
 
             id
@@ -210,7 +206,7 @@ impl Builder<'_, '_> {
         branch_info.branch_spans.push(BranchSpan {
             span: source_info.span,
             // FIXME(dprn): Handle case when MCDC is disabled better than just putting 0.
-            decision_id: branch_info.current_decision_id.unwrap_or(DecisionMarkerId::from_u32(0)),
+            decision_id: branch_info.current_decision_id.unwrap_or(DecisionId::from_u32(0)),
             true_marker,
             false_marker,
         });

compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs

@@ -37,9 +37,10 @@ impl<'tcx> MirPass<'tcx> for CleanupPostBorrowck {
                         // MIR building, and are not needed after InstrumentCoverage.
                         CoverageKind::BlockMarker { .. }
                         | CoverageKind::SpanMarker { .. }
-                        | CoverageKind::MCDCBlockMarker { .. }
                         | CoverageKind::MCDCDecisionEntryMarker { .. }
-                        | CoverageKind::MCDCDecisionOutputMarker { .. },
+                        | CoverageKind::MCDCDecisionOutputMarker { .. }
+                        | CoverageKind::MCDCConditionEntryMarker { .. }
+                        | CoverageKind::MCDCConditionOutputMarker { .. },
                     )
                     | StatementKind::FakeRead(..) => statement.make_nop(),
                     _ => (),

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

@@ -226,9 +226,10 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option<Span> {
             // Block markers are used for branch coverage, so ignore them here.
             CoverageKind::BlockMarker {..}
             // Ignore MCDC markers as well
-            | CoverageKind::MCDCBlockMarker{ .. }
             | CoverageKind::MCDCDecisionEntryMarker{ .. }
             | CoverageKind::MCDCDecisionOutputMarker { .. }
+            | CoverageKind::MCDCConditionEntryMarker { .. }
+            | CoverageKind::MCDCConditionOutputMarker { .. }
         ) => None,
 
         // These coverage statements should not exist prior to coverage instrumentation.
@@ -389,14 +390,8 @@ 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(kind) = &statement.kind {
-                match kind {
-                    CoverageKind::BlockMarker { id }
-                    | CoverageKind::MCDCBlockMarker { id, decision_id: _ } => {
-                        block_markers[*id] = Some(bb);
-                    }
-                    _ => (),
-                }
+            if let StatementKind::Coverage(CoverageKind::BlockMarker { id }) = &statement.kind {
+                block_markers[*id] = Some(bb);
             }
         }
     }