mcdc-coverage: Refactor Decision markers creation, add outcome markers creation

Author: RenjiSann

compiler/rustc_mir_build/messages.ftl

@@ -200,6 +200,8 @@ mir_build_lower_range_bound_must_be_less_than_or_equal_to_upper =
 
 mir_build_lower_range_bound_must_be_less_than_upper = lower range bound must be less than upper
 
+mir_build_mcdc_nested_decision = Unsupported MCDC Decision: Nested decision. Expression will not be corevered.
+
 mir_build_more_information = for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html
 
 mir_build_moved = value is moved into `{$name}` here

compiler/rustc_mir_build/src/build/coverageinfo.rs

@@ -13,13 +13,24 @@ use rustc_span::def_id::LocalDefId;
 use rustc_span::Span;
 
 use crate::build::Builder;
+use crate::errors::MCDCNestedDecision;
 
 pub(crate) struct BranchInfoBuilder {
     /// Maps condition expressions to their enclosing `!`, for better instrumentation.
     nots: FxHashMap<ExprId, NotInfo>,
 
     num_block_markers: usize,
     branch_spans: Vec<BranchSpan>,
+
+    // MCDC decision stuff
+    /// 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>,
+    /// 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>,
 }
 
@@ -44,6 +55,8 @@ impl BranchInfoBuilder {
                 nots: FxHashMap::default(),
                 num_block_markers: 0,
                 branch_spans: vec![],
+                current_decision_id: None,
+                nested_decision_level: 0,
                 decisions: IndexVec::new(),
             })
         } else {
@@ -98,7 +111,7 @@ impl BranchInfoBuilder {
     }
 
     pub(crate) fn into_done(self) -> Option<Box<mir::coverage::BranchInfo>> {
-        let Self { nots: _, num_block_markers, branch_spans, decisions } = self;
+        let Self { nots: _, num_block_markers, branch_spans, decisions, .. } = self;
 
         if num_block_markers == 0 {
             assert!(branch_spans.is_empty());
@@ -122,6 +135,32 @@ impl BranchInfoBuilder {
             decision_spans,
         }))
     }
+
+    /// Increase the nested decision level and return true if the
+    /// decision can be instrumented (not in a nested condition).
+    pub fn enter_decision(&mut self, span: Span) -> bool {
+        self.nested_decision_level += 1;
+        let can_mcdc = !self.in_nested_condition();
+
+        if can_mcdc {
+            self.current_decision_id = Some(self.decisions.push(span));
+        }
+
+        can_mcdc
+    }
+
+    pub fn exit_decision(&mut self) {
+        self.nested_decision_level -= 1;
+    }
+
+    /// Return true if the current decision is located inside another decision.
+    pub fn in_nested_condition(&self) -> bool {
+        self.nested_decision_level > 1
+    }
+
+    pub fn current_decision_id(&self) -> Option<DecisionMarkerId> {
+        if self.in_nested_condition() { None } else { self.current_decision_id }
+    }
 }
 
 impl Builder<'_, '_> {
@@ -132,7 +171,6 @@ impl Builder<'_, '_> {
         mut expr_id: ExprId,
         mut then_block: BasicBlock,
         mut else_block: BasicBlock,
-        decision_id: Option<DecisionMarkerId>, // If MCDC is enabled
     ) {
         // Bail out if branch coverage is not enabled for this function.
         let Some(branch_info) = self.coverage_branch_info.as_ref() else { return };
@@ -153,7 +191,7 @@ 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) = decision_id {
+            let cov_kind = if let Some(decision_id) = branch_info.current_decision_id() {
                 CoverageKind::MCDCBlockMarker { id, decision_id }
             } else {
                 CoverageKind::BlockMarker { id }
@@ -171,30 +209,35 @@ impl Builder<'_, '_> {
 
         branch_info.branch_spans.push(BranchSpan {
             span: source_info.span,
-            // FIXME: Handle case when MCDC is disabled better than just putting 0.
-            decision_id: decision_id.unwrap_or(DecisionMarkerId::from_u32(0)),
+            // 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)),
             true_marker,
             false_marker,
         });
     }
 
-    /// If MCDC coverage is enabled, inject a marker in all the decisions
-    /// (boolean expressions)
-    pub(crate) fn visit_coverage_decision(
-        &mut self,
-        expr_id: ExprId,
-        block: BasicBlock,
-    ) -> Option<DecisionMarkerId> {
+    /// If MCDC coverage is enabled, inject a decision entry marker in the given decision.
+    /// return true
+    pub(crate) fn begin_mcdc_decision_coverage(&mut self, expr_id: ExprId, block: BasicBlock) {
         // Early return if MCDC coverage is not enabled.
         if !self.tcx.sess.instrument_coverage_mcdc() {
-            return None;
+            return;
         }
         let Some(branch_info) = self.coverage_branch_info.as_mut() else {
-            return None;
+            return;
         };
 
         let span = self.thir[expr_id].span;
-        let decision_id = branch_info.decisions.push(span);
+
+        // enter_decision returns false if it detects nested decisions.
+        if !branch_info.enter_decision(span) {
+            // FIXME(dprn): do WARNING for nested decision.
+            debug!("MCDC: Unsupported nested decision");
+            self.tcx.dcx().emit_warn(MCDCNestedDecision { span });
+            return;
+        }
+
+        let decision_id = branch_info.current_decision_id().expect("Should have returned.");
 
         // Inject a decision marker
         let source_info = self.source_info(span);
@@ -205,7 +248,58 @@ impl Builder<'_, '_> {
             }),
         };
         self.cfg.push(block, marker_statement);
+    }
+
+    /// If MCDC is enabled, and function is instrumented,
+    pub(crate) fn end_mcdc_decision_coverage(&mut self) {
+        // Early return if MCDC coverage is not enabled.
+        if !self.tcx.sess.instrument_coverage_mcdc() {
+            return;
+        }
+        let Some(branch_info) = self.coverage_branch_info.as_mut() else {
+            return;
+        };
+
+        // Exit decision now so we can drop &mut to branch info
+        branch_info.exit_decision();
+    }
+
+    /// If MCDC is enabled and the current decision is being instrumented,
+    /// inject an `MCDCDecisionOutputMarker` to the given basic block.
+    /// `outcome` should be true for the then block and false for the else block.
+    pub(crate) fn mcdc_decision_outcome_block(
+        &mut self,
+        bb: BasicBlock,
+        outcome: bool,
+    ) -> BasicBlock {
+        let Some(branch_info) = self.coverage_branch_info.as_mut() else {
+            // Coverage instrumentation is not enabled.
+            return bb;
+        };
+        let Some(decision_id) = branch_info.current_decision_id() else {
+            // Decision is not instrumented
+            return bb;
+        };
+
+        let span = branch_info.decisions[decision_id];
+        let source_info = self.source_info(span);
+        let marker_statement = mir::Statement {
+            source_info,
+            kind: mir::StatementKind::Coverage(CoverageKind::MCDCDecisionOutputMarker {
+                id: decision_id,
+                outcome,
+            }),
+        };
+
+        // Insert statements at the beginning of the following basic block
+        self.cfg.block_data_mut(bb).statements.insert(0, marker_statement);
+
+        // Create a new block to return
+        let new_bb = self.cfg.start_new_block();
+
+        // Set bb -> new_bb
+        self.cfg.goto(bb, source_info, new_bb);
 
-        Some(decision_id)
+        new_bb
     }
 }

compiler/rustc_mir_build/src/build/expr/into.rs

@@ -76,6 +76,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             this.source_info(then_span)
                         };
 
+                        // If MCDC is enabled AND we are not in a nested decision,
+                        // Mark the decision's root, true and false outcomes.
+                        this.begin_mcdc_decision_coverage(expr_id, block);
+
                         // Lower the condition, and have it branch into `then` and `else` blocks.
                         let (then_block, else_block) =
                             this.in_if_then_scope(condition_scope, then_span, |this| {
@@ -87,10 +91,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                                     true, // Declare `let` bindings normally
                                 ));
 
+                                // If MCDC is enabled, inject an outcome marker.
+                                let then_blk = this.mcdc_decision_outcome_block(then_blk, true);
+
                                 // Lower the `then` arm into its block.
                                 this.expr_into_dest(destination, then_blk, then)
                             });
 
+                        // If MCDC is enabled and decision was instrumented, exit the
+                        // decision scope and inject an MCDC decision output marker
+                        // in the then and else blocks.
+                        let else_block = this.mcdc_decision_outcome_block(else_block, false);
+                        this.end_mcdc_decision_coverage();
+
                         // Pack `(then_block, else_block)` into `BlockAnd<BasicBlock>`.
                         then_block.and(else_block)
                     },

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -16,7 +16,6 @@ use rustc_data_structures::{
 };
 use rustc_hir::{BindingAnnotation, ByRef};
 use rustc_middle::middle::region;
-use rustc_middle::mir::coverage::DecisionMarkerId;
 use rustc_middle::mir::{self, *};
 use rustc_middle::thir::{self, *};
 use rustc_middle::ty::{self, CanonicalUserTypeAnnotation, Ty};
@@ -42,9 +41,6 @@ struct ThenElseArgs {
     /// Forwarded to [`Builder::lower_let_expr`] when lowering [`ExprKind::Let`].
     /// When false (for match guards), `let` bindings won't be declared.
     declare_let_bindings: bool,
-
-    /// Id of the parent decision id MCDC is enabled
-    decision_id: Option<DecisionMarkerId>,
 }
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
@@ -62,8 +58,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         variable_source_info: SourceInfo,
         declare_let_bindings: bool,
     ) -> BlockAnd<()> {
-        // Get an ID for the decision if MCDC is enabled.
-        let decision_id = self.visit_coverage_decision(expr_id, block);
 
         self.then_else_break_inner(
             block,
@@ -72,7 +66,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 temp_scope_override,
                 variable_source_info,
                 declare_let_bindings,
-                decision_id,
             },
         )
     }
@@ -175,7 +168,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     expr_id,
                     then_block,
                     else_block,
-                    args.decision_id,
                 );
 
                 let source_info = this.source_info(expr_span);

compiler/rustc_mir_build/src/errors.rs

@@ -850,6 +850,13 @@ pub(crate) struct PatternNotCovered<'s, 'tcx> {
     pub misc_suggestion: Option<MiscPatternSuggestion>,
 }
 
+#[derive(Diagnostic)]
+#[diag(mir_build_mcdc_nested_decision)]
+pub(crate) struct MCDCNestedDecision {
+    #[primary_span]
+    pub span: Span,
+}
+
 #[derive(Subdiagnostic)]
 #[note(mir_build_inform_irrefutable)]
 #[note(mir_build_more_information)]