Rollup merge of rust-lang#144999 - Zalathar:remove-mcdc, r=oli-obk

coverage: Remove all unstable support for MC/DC instrumentation

Preliminary support for a partial implementation of “Modified Condition/Decision Coverage” instrumentation was added behind the unstable flag `-Zcoverage-options=mcdc` in 2024. These are the most substantial PRs involved:

- rust-lang#123409
- rust-lang#126733

At the time, I accepted these PRs with relatively modest scrutiny, because I did not want to stand in the way of independent work on MC/DC instrumentation. My hope was that ongoing work by interested contributors would lead to the code becoming clearer and more maintainable over time.

---

However, that MC/DC code has proven itself to be a major burden on overall maintenance of coverage instrumentation, and a major obstacle to other planned improvements, such as internal changes needed for proper support of macro expansion regions.

I have also become reluctant to accept any further MC/DC-related changes that would increase this burden.

That tension has resulted in an unhappy impasse. On one hand, the present MC/DC implementation is not yet complete, and shows little sign of being complete at an acceptable level of code quality in the foreseeable future. On the other hand, the continued existence of this partial MC/DC implementation is imposing serious maintenance burdens on every other aspect of coverage instrumentation, and is preventing some of the very improvements that would make it easier to accept expanded MC/DC support in the future.

While I know this will be disappointing to some, I think the healthy way forward is accept that I made the wrong call in accepting the current implementation, and to remove it entirely from the compiler.

Author: Zalathar

compiler/rustc_codegen_llvm/src/builder.rs

@@ -1886,48 +1886,4 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {
     ) {
         self.call_intrinsic("llvm.instrprof.increment", &[], &[fn_name, hash, num_counters, index]);
     }
-
-    /// Emits a call to `llvm.instrprof.mcdc.parameters`.
-    ///
-    /// This doesn't produce any code directly, but is used as input by
-    /// the LLVM pass that handles coverage instrumentation.
-    ///
-    /// (See clang's [`CodeGenPGO::emitMCDCParameters`] for comparison.)
-    ///
-    /// [`CodeGenPGO::emitMCDCParameters`]:
-    ///     https://github.com/rust-lang/llvm-project/blob/5399a24/clang/lib/CodeGen/CodeGenPGO.cpp#L1124
-    #[instrument(level = "debug", skip(self))]
-    pub(crate) fn mcdc_parameters(
-        &mut self,
-        fn_name: &'ll Value,
-        hash: &'ll Value,
-        bitmap_bits: &'ll Value,
-    ) {
-        self.call_intrinsic("llvm.instrprof.mcdc.parameters", &[], &[fn_name, hash, bitmap_bits]);
-    }
-
-    #[instrument(level = "debug", skip(self))]
-    pub(crate) fn mcdc_tvbitmap_update(
-        &mut self,
-        fn_name: &'ll Value,
-        hash: &'ll Value,
-        bitmap_index: &'ll Value,
-        mcdc_temp: &'ll Value,
-    ) {
-        let args = &[fn_name, hash, bitmap_index, mcdc_temp];
-        self.call_intrinsic("llvm.instrprof.mcdc.tvbitmap.update", &[], args);
-    }
-
-    #[instrument(level = "debug", skip(self))]
-    pub(crate) fn mcdc_condbitmap_reset(&mut self, mcdc_temp: &'ll Value) {
-        self.store(self.const_i32(0), mcdc_temp, self.tcx.data_layout.i32_align.abi);
-    }
-
-    #[instrument(level = "debug", skip(self))]
-    pub(crate) fn mcdc_condbitmap_update(&mut self, cond_index: &'ll Value, mcdc_temp: &'ll Value) {
-        let align = self.tcx.data_layout.i32_align.abi;
-        let current_tv_index = self.load(self.cx.type_i32(), mcdc_temp, align);
-        let new_tv_index = self.add(current_tv_index, cond_index);
-        self.store(new_tv_index, mcdc_temp, align);
-    }
 }

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs

@@ -73,48 +73,6 @@ pub(crate) struct CounterExpression {
     pub(crate) rhs: Counter,
 }
 
-pub(crate) mod mcdc {
-    use rustc_middle::mir::coverage::{ConditionId, ConditionInfo, DecisionInfo};
-
-    /// Must match the layout of `LLVMRustMCDCDecisionParameters`.
-    #[repr(C)]
-    #[derive(Clone, Copy, Debug, Default)]
-    pub(crate) struct DecisionParameters {
-        bitmap_idx: u32,
-        num_conditions: u16,
-    }
-
-    type LLVMConditionId = i16;
-
-    /// Must match the layout of `LLVMRustMCDCBranchParameters`.
-    #[repr(C)]
-    #[derive(Clone, Copy, Debug, Default)]
-    pub(crate) struct BranchParameters {
-        condition_id: LLVMConditionId,
-        condition_ids: [LLVMConditionId; 2],
-    }
-
-    impl From<ConditionInfo> for BranchParameters {
-        fn from(value: ConditionInfo) -> Self {
-            let to_llvm_cond_id = |cond_id: Option<ConditionId>| {
-                cond_id.and_then(|id| LLVMConditionId::try_from(id.as_usize()).ok()).unwrap_or(-1)
-            };
-            let ConditionInfo { condition_id, true_next_id, false_next_id } = value;
-            Self {
-                condition_id: to_llvm_cond_id(Some(condition_id)),
-                condition_ids: [to_llvm_cond_id(false_next_id), to_llvm_cond_id(true_next_id)],
-            }
-        }
-    }
-
-    impl From<DecisionInfo> for DecisionParameters {
-        fn from(info: DecisionInfo) -> Self {
-            let DecisionInfo { bitmap_idx, num_conditions } = info;
-            Self { bitmap_idx, num_conditions }
-        }
-    }
-}
-
 /// A span of source code coordinates to be embedded in coverage metadata.
 ///
 /// Must match the layout of `LLVMRustCoverageSpan`.
@@ -148,26 +106,14 @@ pub(crate) struct Regions {
     pub(crate) code_regions: Vec<CodeRegion>,
     pub(crate) expansion_regions: Vec<ExpansionRegion>,
     pub(crate) branch_regions: Vec<BranchRegion>,
-    pub(crate) mcdc_branch_regions: Vec<MCDCBranchRegion>,
-    pub(crate) mcdc_decision_regions: Vec<MCDCDecisionRegion>,
 }
 
 impl Regions {
     /// Returns true if none of this structure's tables contain any regions.
     pub(crate) fn has_no_regions(&self) -> bool {
-        let Self {
-            code_regions,
-            expansion_regions,
-            branch_regions,
-            mcdc_branch_regions,
-            mcdc_decision_regions,
-        } = self;
-
-        code_regions.is_empty()
-            && expansion_regions.is_empty()
-            && branch_regions.is_empty()
-            && mcdc_branch_regions.is_empty()
-            && mcdc_decision_regions.is_empty()
+        let Self { code_regions, expansion_regions, branch_regions } = self;
+
+        code_regions.is_empty() && expansion_regions.is_empty() && branch_regions.is_empty()
     }
 }
 
@@ -195,21 +141,3 @@ pub(crate) struct BranchRegion {
     pub(crate) true_counter: Counter,
     pub(crate) false_counter: Counter,
 }
-
-/// Must match the layout of `LLVMRustCoverageMCDCBranchRegion`.
-#[derive(Clone, Debug)]
-#[repr(C)]
-pub(crate) struct MCDCBranchRegion {
-    pub(crate) cov_span: CoverageSpan,
-    pub(crate) true_counter: Counter,
-    pub(crate) false_counter: Counter,
-    pub(crate) mcdc_branch_params: mcdc::BranchParameters,
-}
-
-/// Must match the layout of `LLVMRustCoverageMCDCDecisionRegion`.
-#[derive(Clone, Debug)]
-#[repr(C)]
-pub(crate) struct MCDCDecisionRegion {
-    pub(crate) cov_span: CoverageSpan,
-    pub(crate) mcdc_decision_params: mcdc::DecisionParameters,
-}

compiler/rustc_codegen_llvm/src/coverageinfo/llvm_cov.rs

@@ -63,13 +63,7 @@ pub(crate) fn write_function_mappings_to_buffer(
     expressions: &[ffi::CounterExpression],
     regions: &ffi::Regions,
 ) -> Vec<u8> {
-    let ffi::Regions {
-        code_regions,
-        expansion_regions,
-        branch_regions,
-        mcdc_branch_regions,
-        mcdc_decision_regions,
-    } = regions;
+    let ffi::Regions { code_regions, expansion_regions, branch_regions } = regions;
 
     // SAFETY:
     // - All types are FFI-compatible and have matching representations in Rust/C++.
@@ -87,10 +81,6 @@ pub(crate) fn write_function_mappings_to_buffer(
             expansion_regions.len(),
             branch_regions.as_ptr(),
             branch_regions.len(),
-            mcdc_branch_regions.as_ptr(),
-            mcdc_branch_regions.len(),
-            mcdc_decision_regions.as_ptr(),
-            mcdc_decision_regions.len(),
             buffer,
         )
     })

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs

@@ -140,8 +140,6 @@ fn fill_region_tables<'tcx>(
         code_regions,
         expansion_regions: _, // FIXME(Zalathar): Fill out support for expansion regions
         branch_regions,
-        mcdc_branch_regions,
-        mcdc_decision_regions,
     } = &mut covfun.regions;
 
     // For each counter/region pair in this function+file, convert it to a
@@ -161,20 +159,6 @@ fn fill_region_tables<'tcx>(
                     false_counter: counter_for_bcb(false_bcb),
                 });
             }
-            MappingKind::MCDCBranch { true_bcb, false_bcb, mcdc_params } => {
-                mcdc_branch_regions.push(ffi::MCDCBranchRegion {
-                    cov_span,
-                    true_counter: counter_for_bcb(true_bcb),
-                    false_counter: counter_for_bcb(false_bcb),
-                    mcdc_branch_params: ffi::mcdc::BranchParameters::from(mcdc_params),
-                });
-            }
-            MappingKind::MCDCDecision(mcdc_decision_params) => {
-                mcdc_decision_regions.push(ffi::MCDCDecisionRegion {
-                    cov_span,
-                    mcdc_decision_params: ffi::mcdc::DecisionParameters::from(mcdc_decision_params),
-                });
-            }
         }
     }
 }

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -1,11 +1,10 @@
 use std::cell::{OnceCell, RefCell};
 use std::ffi::{CStr, CString};
 
-use rustc_abi::Size;
 use rustc_codegen_ssa::traits::{
-    BuilderMethods, ConstCodegenMethods, CoverageInfoBuilderMethods, MiscCodegenMethods,
+    ConstCodegenMethods, CoverageInfoBuilderMethods, MiscCodegenMethods,
 };
-use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
+use rustc_data_structures::fx::FxIndexMap;
 use rustc_middle::mir::coverage::CoverageKind;
 use rustc_middle::ty::Instance;
 use tracing::{debug, instrument};
@@ -28,34 +27,13 @@ pub(crate) struct CguCoverageContext<'ll, 'tcx> {
     /// symbol name, and `llvm-cov` will exit fatally if it can't resolve that
     /// hash back to an entry in the binary's `__llvm_prf_names` linker section.
     pub(crate) pgo_func_name_var_map: RefCell<FxIndexMap<Instance<'tcx>, &'ll llvm::Value>>,
-    pub(crate) mcdc_condition_bitmap_map: RefCell<FxHashMap<Instance<'tcx>, Vec<&'ll llvm::Value>>>,
 
     covfun_section_name: OnceCell<CString>,
 }
 
 impl<'ll, 'tcx> CguCoverageContext<'ll, 'tcx> {
     pub(crate) fn new() -> Self {
-        Self {
-            pgo_func_name_var_map: Default::default(),
-            mcdc_condition_bitmap_map: Default::default(),
-            covfun_section_name: Default::default(),
-        }
-    }
-
-    /// LLVM use a temp value to record evaluated mcdc test vector of each decision, which is
-    /// called condition bitmap. In order to handle nested decisions, several condition bitmaps can
-    /// be allocated for a function body. These values are named `mcdc.addr.{i}` and are a 32-bit
-    /// integers. They respectively hold the condition bitmaps for decisions with a depth of `i`.
-    fn try_get_mcdc_condition_bitmap(
-        &self,
-        instance: &Instance<'tcx>,
-        decision_depth: u16,
-    ) -> Option<&'ll llvm::Value> {
-        self.mcdc_condition_bitmap_map
-            .borrow()
-            .get(instance)
-            .and_then(|bitmap_map| bitmap_map.get(decision_depth as usize))
-            .copied() // Dereference Option<&&Value> to Option<&Value>
+        Self { pgo_func_name_var_map: Default::default(), covfun_section_name: Default::default() }
     }
 
     /// Returns the list of instances considered "used" in this CGU, as
@@ -105,38 +83,6 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
 }
 
 impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
-    fn init_coverage(&mut self, instance: Instance<'tcx>) {
-        let Some(function_coverage_info) =
-            self.tcx.instance_mir(instance.def).function_coverage_info.as_deref()
-        else {
-            return;
-        };
-
-        // If there are no MC/DC bitmaps to set up, return immediately.
-        if function_coverage_info.mcdc_bitmap_bits == 0 {
-            return;
-        }
-
-        let fn_name = self.ensure_pgo_func_name_var(instance);
-        let hash = self.const_u64(function_coverage_info.function_source_hash);
-        let bitmap_bits = self.const_u32(function_coverage_info.mcdc_bitmap_bits as u32);
-        self.mcdc_parameters(fn_name, hash, bitmap_bits);
-
-        // Create pointers named `mcdc.addr.{i}` to stack-allocated condition bitmaps.
-        let mut cond_bitmaps = vec![];
-        for i in 0..function_coverage_info.mcdc_num_condition_bitmaps {
-            // MC/DC intrinsics will perform loads/stores that use the ABI default
-            // alignment for i32, so our variable declaration should match.
-            let align = self.tcx.data_layout.i32_align.abi;
-            let cond_bitmap = self.alloca(Size::from_bytes(4), align);
-            llvm::set_value_name(cond_bitmap, format!("mcdc.addr.{i}").as_bytes());
-            self.store(self.const_i32(0), cond_bitmap, align);
-            cond_bitmaps.push(cond_bitmap);
-        }
-
-        self.coverage_cx().mcdc_condition_bitmap_map.borrow_mut().insert(instance, cond_bitmaps);
-    }
-
     #[instrument(level = "debug", skip(self))]
     fn add_coverage(&mut self, instance: Instance<'tcx>, kind: &CoverageKind) {
         // Our caller should have already taken care of inlining subtleties,
@@ -153,7 +99,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
         // When that happens, we currently just discard those statements, so
         // the corresponding code will be undercounted.
         // FIXME(Zalathar): Find a better solution for mixed-coverage builds.
-        let Some(coverage_cx) = &bx.cx.coverage_cx else { return };
+        let Some(_coverage_cx) = &bx.cx.coverage_cx else { return };
 
         let Some(function_coverage_info) =
             bx.tcx.instance_mir(instance.def).function_coverage_info.as_deref()
@@ -185,30 +131,6 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
             }
             // If a BCB doesn't have an associated physical counter, there's nothing to codegen.
             CoverageKind::VirtualCounter { .. } => {}
-            CoverageKind::CondBitmapUpdate { index, decision_depth } => {
-                let cond_bitmap = coverage_cx
-                    .try_get_mcdc_condition_bitmap(&instance, decision_depth)
-                    .expect("mcdc cond bitmap should have been allocated for updating");
-                let cond_index = bx.const_i32(index as i32);
-                bx.mcdc_condbitmap_update(cond_index, cond_bitmap);
-            }
-            CoverageKind::TestVectorBitmapUpdate { bitmap_idx, decision_depth } => {
-                let cond_bitmap =
-                    coverage_cx.try_get_mcdc_condition_bitmap(&instance, decision_depth).expect(
-                        "mcdc cond bitmap should have been allocated for merging \
-                        into the global bitmap",
-                    );
-                assert!(
-                    bitmap_idx as usize <= function_coverage_info.mcdc_bitmap_bits,
-                    "bitmap index of the decision out of range"
-                );
-
-                let fn_name = bx.ensure_pgo_func_name_var(instance);
-                let hash = bx.const_u64(function_coverage_info.function_source_hash);
-                let bitmap_index = bx.const_u32(bitmap_idx);
-                bx.mcdc_tvbitmap_update(fn_name, hash, bitmap_index, cond_bitmap);
-                bx.mcdc_condbitmap_reset(cond_bitmap);
-            }
         }
     }
 }

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -2056,10 +2056,6 @@ unsafe extern "C" {
         NumExpansionRegions: size_t,
         BranchRegions: *const crate::coverageinfo::ffi::BranchRegion,
         NumBranchRegions: size_t,
-        MCDCBranchRegions: *const crate::coverageinfo::ffi::MCDCBranchRegion,
-        NumMCDCBranchRegions: size_t,
-        MCDCDecisionRegions: *const crate::coverageinfo::ffi::MCDCDecisionRegion,
-        NumMCDCDecisionRegions: size_t,
         BufferOut: &RustString,
     );
 

compiler/rustc_codegen_ssa/src/mir/mod.rs

@@ -296,10 +296,6 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
     // Apply debuginfo to the newly allocated locals.
     fx.debug_introduce_locals(&mut start_bx, consts_debug_info.unwrap_or_default());
 
-    // If the backend supports coverage, and coverage is enabled for this function,
-    // do any necessary start-of-function codegen (e.g. locals for MC/DC bitmaps).
-    start_bx.init_coverage(instance);
-
     // The builders will be created separately for each basic block at `codegen_block`.
     // So drop the builder of `start_llbb` to avoid having two at the same time.
     drop(start_bx);

compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs

@@ -2,11 +2,6 @@ use rustc_middle::mir::coverage::CoverageKind;
 use rustc_middle::ty::Instance;
 
 pub trait CoverageInfoBuilderMethods<'tcx> {
-    /// Performs any start-of-function codegen needed for coverage instrumentation.
-    ///
-    /// Can be a no-op in backends that don't support coverage instrumentation.
-    fn init_coverage(&mut self, _instance: Instance<'tcx>) {}
-
     /// Handle the MIR coverage info in a backend-specific way.
     ///
     /// This can potentially be a no-op in backends that don't support

compiler/rustc_interface/src/tests.rs

@@ -777,7 +777,7 @@ fn test_unstable_options_tracking_hash() {
     tracked!(
         coverage_options,
         CoverageOptions {
-            level: CoverageLevel::Mcdc,
+            level: CoverageLevel::Branch,
             // (don't collapse test-only options onto the same line)
             discard_all_spans_in_codegen: true,
         }