From 3f791cb616b4b60a4c86dd35f382db4717581711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Fri, 8 Mar 2024 11:52:43 +0000 Subject: [PATCH 01/21] mcdc-coverage: Add `mcdc` and `no-mcdc` options for `-Zcoverage-options` --- compiler/rustc_session/src/config.rs | 2 ++ compiler/rustc_session/src/options.rs | 3 ++- compiler/rustc_session/src/session.rs | 4 ++++ src/doc/rustc/src/instrument-coverage.md | 2 ++ 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index d51fcf693ed38..0b0a36791155c 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -148,6 +148,8 @@ pub enum InstrumentCoverage { pub struct CoverageOptions { /// Add branch coverage instrumentation. pub branch: bool, + /// Add MCDC coverage instrumentation. + pub mcdc: bool, } /// Settings for `-Z instrument-xray` flag. diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index 6204f86838548..b6d60379fdf38 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -396,7 +396,7 @@ mod desc { pub const parse_optimization_fuel: &str = "crate=integer"; pub const parse_dump_mono_stats: &str = "`markdown` (default) or `json`"; pub const parse_instrument_coverage: &str = parse_bool; - pub const parse_coverage_options: &str = "`branch` or `no-branch`"; + pub const parse_coverage_options: &str = "`branch`, `no-branch`, `mcdc`, `no-mcdc`"; pub const parse_instrument_xray: &str = "either a boolean (`yes`, `no`, `on`, `off`, etc), or a comma separated list of settings: `always` or `never` (mutually exclusive), `ignore-loops`, `instruction-threshold=N`, `skip-entry`, `skip-exit`"; pub const parse_unpretty: &str = "`string` or `string=string`"; pub const parse_treat_err_as_bug: &str = "either no value or a non-negative number"; @@ -950,6 +950,7 @@ mod parse { }; let slot = match option { "branch" => &mut slot.branch, + "mcdc" => &mut slot.mcdc, _ => return false, }; *slot = enabled; diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 55fff4421ae46..53740a8a6b1ea 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -352,6 +352,10 @@ impl Session { self.instrument_coverage() && self.opts.unstable_opts.coverage_options.branch } + pub fn instrument_coverage_mcdc(&self) -> bool { + self.instrument_coverage() && self.opts.unstable_opts.coverage_options.mcdc + } + pub fn is_sanitizer_cfi_enabled(&self) -> bool { self.opts.unstable_opts.sanitizer.contains(SanitizerSet::CFI) } diff --git a/src/doc/rustc/src/instrument-coverage.md b/src/doc/rustc/src/instrument-coverage.md index 185a3ba5dbd41..2a289bd4242dd 100644 --- a/src/doc/rustc/src/instrument-coverage.md +++ b/src/doc/rustc/src/instrument-coverage.md @@ -353,6 +353,8 @@ instrumentation. Pass one or more of the following values, separated by commas. - `branch` or `no-branch` - Enables or disables branch coverage instrumentation. +- `mcdc` or `no-mcdc` + - Enables or disables MCDC coverage instrumentation. ## Other references From 1782978b339516fab2793ca6c4bed94dd1ba73d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 13 Mar 2024 14:52:52 +0000 Subject: [PATCH 02/21] mcdc-coverage: Update ffi RegionKind to match LLVM's --- compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs | 9 +++++++++ .../rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp | 6 ++++++ 2 files changed, 15 insertions(+) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 2af28146a51ea..93bd192ed9dd1 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -99,6 +99,15 @@ pub enum RegionKind { /// associated with two counters, each representing the number of times the /// expression evaluates to true or false. BranchRegion = 4, + + /// A DecisionRegion represents a top-level boolean expression and is + /// associated with a variable length bitmap index and condition number. + #[allow(dead_code)] + MCDCDecisionRegion = 5, + + /// A Branch Region can be extended to include IDs to facilitate MC/DC. + #[allow(dead_code)] + MCDCBranchRegion = 6, } /// This struct provides LLVM's representation of a "CoverageMappingRegion", encoded into the diff --git a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp index 60789b07e54ea..2e3330511e4da 100644 --- a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp @@ -43,6 +43,8 @@ enum class LLVMRustCounterMappingRegionKind { SkippedRegion = 2, GapRegion = 3, BranchRegion = 4, + MCDCDecisionRegion = 5, + MCDCBranchRegion = 6, }; static coverage::CounterMappingRegion::RegionKind @@ -58,6 +60,10 @@ fromRust(LLVMRustCounterMappingRegionKind Kind) { return coverage::CounterMappingRegion::GapRegion; case LLVMRustCounterMappingRegionKind::BranchRegion: return coverage::CounterMappingRegion::BranchRegion; + case LLVMRustCounterMappingRegionKind::MCDCDecisionRegion: + return coverage::CounterMappingRegion::MCDCDecisionRegion; + case LLVMRustCounterMappingRegionKind::MCDCBranchRegion: + return coverage::CounterMappingRegion::MCDCBranchRegion; } report_fatal_error("Bad LLVMRustCounterMappingRegionKind!"); } From 5c5cd197b082a7a8b0077b4f64bbd03533415fa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 13 Mar 2024 15:43:46 +0000 Subject: [PATCH 03/21] mcdc-coverage: Add FFI for getting Bitmap IPSK, Bump CoverageMappingVersion --- compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 7 ++++--- compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs | 10 ++++++++++ compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 3 +++ .../rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp | 7 ++++++- 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 278db21b0a1f2..730a65e476488 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -31,10 +31,10 @@ use rustc_span::Symbol; pub fn finalize(cx: &CodegenCx<'_, '_>) { let tcx = cx.tcx; - // Ensure the installed version of LLVM supports Coverage Map Version 6 - // (encoded as a zero-based value: 5), which was introduced with LLVM 13. + // Ensure the installed version of LLVM supports Coverage Map Version 7 + // (encoded as a zero-based value: 6), which was introduced with LLVM 13. let version = coverageinfo::mapping_version(); - assert_eq!(version, 5, "The `CoverageMappingVersion` exposed by `llvm-wrapper` is out of sync"); + assert_eq!(version, 6, "The `CoverageMappingVersion` exposed by `llvm-wrapper` is out of sync"); debug!("Generating coverage map for CodegenUnit: `{}`", cx.codegen_unit.name()); @@ -276,6 +276,7 @@ fn encode_mappings_for_function( /// Construct coverage map header and the array of function records, and combine them into the /// coverage map. Save the coverage map data into the LLVM IR as a static global using a /// specific, well-known section and name. +/// https://llvm.org/docs/CoverageMappingFormat.html#llvm-ir-representation fn generate_coverage_map<'ll>( cx: &CodegenCx<'ll, '_>, version: u32, diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 68c1770c1d4a1..811895e9bb31f 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -278,3 +278,13 @@ pub(crate) fn covfun_section_name(cx: &CodegenCx<'_, '_>) -> String { }) .expect("Rust Coverage function record section name failed UTF-8 conversion") } + +/// Returns the section name for the MC/DC Bitmap section passed to the linker. +/// FIXME: Remove allow deadcode +#[allow(dead_code)] +pub(crate) fn prf_bits_section_name(cx: &CodegenCx<'_, '_>) -> String { + llvm::build_string(|s| unsafe { + llvm::LLVMRustCoverageWriteBitmapSectionNameToString(cx.llmod, s); + }) + .expect("Rust Coverage function record section name failed UTF-8 conversion") +} \ No newline at end of file diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index 284bc74d5c434..7348b4a686790 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1788,6 +1788,9 @@ extern "C" { #[allow(improper_ctypes)] pub fn LLVMRustCoverageWriteFuncSectionNameToString(M: &Module, Str: &RustString); + #[allow(improper_ctypes)] + pub fn LLVMRustCoverageWriteBitmapSectionNameToString(M: &Module, Str: &RustString); + #[allow(improper_ctypes)] pub fn LLVMRustCoverageWriteMappingVarNameToString(Str: &RustString); diff --git a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp index 2e3330511e4da..f357937063a06 100644 --- a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp @@ -204,6 +204,11 @@ extern "C" void LLVMRustCoverageWriteFuncSectionNameToString(LLVMModuleRef M, WriteSectionNameToString(M, IPSK_covfun, Str); } +extern "C" void LLVMRustCoverageWriteBitmapSectionNameToString(LLVMModuleRef M, + RustStringRef Str) { + WriteSectionNameToString(M, IPSK_bitmap, Str); +} + extern "C" void LLVMRustCoverageWriteMappingVarNameToString(RustStringRef Str) { auto name = getCoverageMappingVarName(); auto OS = RawRustStringOstream(Str); @@ -211,5 +216,5 @@ extern "C" void LLVMRustCoverageWriteMappingVarNameToString(RustStringRef Str) { } extern "C" uint32_t LLVMRustCoverageMappingVersion() { - return coverage::CovMapVersion::Version6; + return coverage::CovMapVersion::Version7; } From f7896169ca7344b0c69619d0fe016f25ddc36f2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Thu, 14 Mar 2024 15:31:31 +0000 Subject: [PATCH 04/21] mcdc-coverage: Add utils for instrprof.mcdc.parameters intrinsic --- compiler/rustc_codegen_llvm/src/builder.rs | 32 +++++++++++++++++++ compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 1 + .../rustc_codegen_ssa/src/traits/builder.rs | 7 ++++ .../rustc_llvm/llvm-wrapper/RustWrapper.cpp | 5 +++ 4 files changed, 45 insertions(+) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 1a32958d3627b..125354ab1f838 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -1238,6 +1238,38 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { } } + fn instrprof_mcdc_parameters( + &mut self, + fn_name: Self::Value, + hash: Self::Value, + num_bitmap_bytes: Self::Value, + ) { + debug!( + "instrprof_mcdc_parameters() with args ({:?}, {:?}, {:?})", + fn_name, hash, num_bitmap_bytes + ); + + let llfn = unsafe { llvm::LLVMRustGetInstrProfMCDCParametersIntrinsic(self.cx().llmod) }; + let llty = self.cx.type_func( + &[self.cx.type_ptr(), self.cx.type_i64(), self.cx.type_i32()], + self.cx.type_void(), + ); + let args = &[fn_name, hash, num_bitmap_bytes]; + let args = self.check_call("call", llty, llfn, args); + + unsafe { + let _ = llvm::LLVMRustBuildCall( + self.llbuilder, + llty, + llfn, + args.as_ptr() as *const &llvm::Value, + args.len() as c_uint, + [].as_ptr(), + 0 as c_uint, + ); + } + } + fn call( &mut self, llty: &'ll Type, diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index 7348b4a686790..3eff0bbb36a15 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1631,6 +1631,7 @@ extern "C" { // Miscellaneous instructions pub fn LLVMRustGetInstrProfIncrementIntrinsic(M: &Module) -> &Value; + pub fn LLVMRustGetInstrProfMCDCParametersIntrinsic(M: &Module) -> &Value; pub fn LLVMRustBuildCall<'a>( B: &Builder<'a>, Ty: &'a Type, diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 6c8dcc5b69001..fae2735bd3fe2 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -382,6 +382,13 @@ pub trait BuilderMethods<'a, 'tcx>: index: Self::Value, ); + fn instrprof_mcdc_parameters( + &mut self, + fn_name: Self::Value, + hash: Self::Value, + num_bitmap_bytes: Self::Value + ); + fn call( &mut self, llty: Self::Type, diff --git a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp index 8ec1f5a99e7ca..1e74f43328d9b 100644 --- a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp @@ -1528,6 +1528,11 @@ extern "C" LLVMValueRef LLVMRustGetInstrProfIncrementIntrinsic(LLVMModuleRef M) (llvm::Intrinsic::ID)llvm::Intrinsic::instrprof_increment)); } +extern "C" LLVMValueRef LLVMRustGetInstrProfMCDCParametersIntrinsic(LLVMModuleRef M) { + return wrap(llvm::Intrinsic::getDeclaration(unwrap(M), + (llvm::Intrinsic::ID)llvm::Intrinsic::instrprof_mcdc_parameters)); +} + extern "C" LLVMValueRef LLVMRustBuildMemCpy(LLVMBuilderRef B, LLVMValueRef Dst, unsigned DstAlign, LLVMValueRef Src, unsigned SrcAlign, From 7b321ca0916474b4f1ffbd2fd555cae002c91d50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Tue, 19 Mar 2024 10:47:28 +0000 Subject: [PATCH 05/21] mcdc-coverage: Add FFI MCDC Params for coverage mappings encoding --- .../src/coverageinfo/ffi.rs | 68 ++++++++++++++++++- .../llvm-wrapper/CoverageMappingWrapper.cpp | 27 +++++++- 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 93bd192ed9dd1..53ae3448150b4 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -102,6 +102,7 @@ pub enum RegionKind { /// A DecisionRegion represents a top-level boolean expression and is /// associated with a variable length bitmap index and condition number. + /// FIXME(dprn): Remove unused variables. #[allow(dead_code)] MCDCDecisionRegion = 5, @@ -110,6 +111,28 @@ pub enum RegionKind { MCDCBranchRegion = 6, } +/// This struct provides LLVM's representation of "MCDCParameters" that may be defined for a +/// Coverage Mapping Region. +/// +/// Correspond to struct `llvm::coverage::CounterMappingRegion::MCDCParameters` +/// +/// Must match The layout of `LLVMRustMCDCParameters` +#[derive(Copy, Clone, Debug, Default)] +#[repr(C)] +pub struct MCDCParameters { + /// Byte Index of Bitmap Coverage Object for a Decision Region. + bitmap_idx: u32, + + /// Number of Conditions used for a Decision Region. + num_conditions: u32, + + /// IDs used to represent a branch region and other branch regions + /// evaluated based on True and False branches. + id: u32, + true_id: u32, + false_id: u32, +} + /// This struct provides LLVM's representation of a "CoverageMappingRegion", encoded into the /// coverage map, in accordance with the /// [LLVM Code Coverage Mapping Format](https://github.com/rust-lang/llvm-project/blob/rustc/13.0-2021-09-30/llvm/docs/CoverageMappingFormat.rst#llvm-code-coverage-mapping-format). @@ -155,6 +178,8 @@ pub struct CounterMappingRegion { end_col: u32, kind: RegionKind, + + mcdc_params: MCDCParameters, } impl CounterMappingRegion { @@ -181,6 +206,7 @@ impl CounterMappingRegion { start_col, end_line, end_col, + None, ), } } @@ -203,9 +229,11 @@ impl CounterMappingRegion { end_line, end_col, kind: RegionKind::CodeRegion, + mcdc_params: Default::default(), } } + /// - `mcdc_params` should be None when MCDC is disabled. pub(crate) fn branch_region( counter: Counter, false_counter: Counter, @@ -214,7 +242,13 @@ impl CounterMappingRegion { start_col: u32, end_line: u32, end_col: u32, + mcdc_params: Option, ) -> Self { + let (kind, mcdc_params) = match mcdc_params { + None => (RegionKind::BranchRegion, Default::default()), + Some(params) => (RegionKind::MCDCBranchRegion, params), + }; + Self { counter, false_counter, @@ -224,7 +258,36 @@ impl CounterMappingRegion { start_col, end_line, end_col, - kind: RegionKind::BranchRegion, + kind, + mcdc_params, + } + } + + #[allow(dead_code)] + pub(crate) fn decision_region( + bitmap_idx: u32, + num_conditions: u32, + file_id: u32, + start_line: u32, + start_col: u32, + end_line: u32, + end_col: u32, + ) -> Self { + Self { + counter: Counter::ZERO, + false_counter: Counter::ZERO, + file_id, + expanded_file_id: 0, + start_line, + start_col, + end_line, + end_col, + kind: RegionKind::MCDCDecisionRegion, + mcdc_params: MCDCParameters { + bitmap_idx, + num_conditions, + .. Default::default() + }, } } @@ -249,6 +312,7 @@ impl CounterMappingRegion { end_line, end_col, kind: RegionKind::ExpansionRegion, + mcdc_params: Default::default(), } } @@ -272,6 +336,7 @@ impl CounterMappingRegion { end_line, end_col, kind: RegionKind::SkippedRegion, + mcdc_params: Default::default(), } } @@ -296,6 +361,7 @@ impl CounterMappingRegion { end_line, end_col: (1_u32 << 31) | end_col, kind: RegionKind::GapRegion, + mcdc_params: Default::default(), } } } diff --git a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp index f357937063a06..61ce73e14a75c 100644 --- a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp @@ -68,6 +68,30 @@ fromRust(LLVMRustCounterMappingRegionKind Kind) { report_fatal_error("Bad LLVMRustCounterMappingRegionKind!"); } +// FFI equivalent of struct `llvm::coverage::CounterMappingRegion::MCDCParameters` +// https://github.com/rust-lang/llvm-project/blob/66a2881a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L253-L263 +struct LLVMRustMCDCParameters { + /// Byte Index of Bitmap Coverage Object for a Decision Region. + uint32_t BitmapIdx = 0; + + /// Number of Conditions used for a Decision Region. + uint32_t NumConditions = 0; + + /// IDs used to represent a branch region and other branch regions + /// evaluated based on True and False branches. + uint32_t ID = 0; + uint32_t TrueID = 0; + uint32_t FalseID = 0; +}; + +static coverage::CounterMappingRegion::MCDCParameters +fromRust(LLVMRustMCDCParameters MCDCParams) { + return coverage::CounterMappingRegion::MCDCParameters{ + MCDCParams.BitmapIdx, MCDCParams.NumConditions, + MCDCParams.ID, MCDCParams.TrueID, MCDCParams.FalseID + }; +} + // FFI equivalent of struct `llvm::coverage::CounterMappingRegion` // https://github.com/rust-lang/llvm-project/blob/ea6fa9c2/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L211-L304 struct LLVMRustCounterMappingRegion { @@ -80,6 +104,7 @@ struct LLVMRustCounterMappingRegion { uint32_t LineEnd; uint32_t ColumnEnd; LLVMRustCounterMappingRegionKind Kind; + LLVMRustMCDCParameters MCDCParams; }; // FFI equivalent of enum `llvm::coverage::CounterExpression::ExprKind` @@ -146,7 +171,7 @@ extern "C" void LLVMRustCoverageWriteMappingToBuffer( MappingRegions.emplace_back( fromRust(Region.Count), fromRust(Region.FalseCount), #if LLVM_VERSION_GE(18, 0) && LLVM_VERSION_LT(19, 0) - coverage::CounterMappingRegion::MCDCParameters{}, + fromRust(Region.MCDCParams), #endif Region.FileID, Region.ExpandedFileID, Region.LineStart, Region.ColumnStart, Region.LineEnd, Region.ColumnEnd, From 7d1dc67d579d22ca1f165154dd0af4e22b4a3b0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Tue, 19 Mar 2024 15:30:54 +0000 Subject: [PATCH 06/21] mcdc-coverage: Add MCDCBlockMarker and MCDCDecisionMarker to the MIR --- .../src/coverageinfo/mod.rs | 5 +- compiler/rustc_middle/src/mir/coverage.rs | 27 ++++++++ compiler/rustc_middle/src/mir/pretty.rs | 2 +- .../rustc_middle/src/ty/structural_impls.rs | 1 + .../rustc_mir_build/src/build/coverageinfo.rs | 62 +++++++++++++++++-- .../rustc_mir_build/src/build/matches/mod.rs | 21 ++++++- .../src/cleanup_post_borrowck.rs | 9 ++- .../src/coverage/spans/from_mir.rs | 21 +++++-- 8 files changed, 131 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 811895e9bb31f..349b6fea0ac80 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -99,7 +99,10 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { .or_insert_with(|| FunctionCoverageCollector::new(instance, function_coverage_info)); match *kind { - CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } => unreachable!( + CoverageKind::SpanMarker + | CoverageKind::BlockMarker { .. } + | CoverageKind::MCDCBlockMarker { .. } + | CoverageKind::MCDCDecisionMarker { .. } => unreachable!( "marker statement {kind:?} should have been removed by CleanupPostBorrowck" ), CoverageKind::CounterIncrement { id } => { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 588aa1f40d79c..b0706e99f7eda 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -15,6 +15,13 @@ rustc_index::newtype_index! { pub struct BlockMarkerId {} } +rustc_index::newtype_index! { + #[derive(HashStable)] + #[encodable] + #[debug_format = "DecisionMarkerId({})"] + pub struct DecisionMarkerId {} +} + rustc_index::newtype_index! { /// ID of a coverage counter. Values ascend from 0. /// @@ -97,6 +104,18 @@ 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. + MCDCDecisionMarker { id: DecisionMarkerId }, + /// Marks the point in MIR control flow represented by a coverage counter. /// /// This is eventually lowered to `llvm.instrprof.increment` in LLVM IR. @@ -122,6 +141,10 @@ 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()) + } + MCDCDecisionMarker { id } => write!(fmt, "MCDCDecisionMarker({:?})", id.index()), CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()), ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()), } @@ -234,12 +257,16 @@ pub struct BranchInfo { /// data structures without having to scan the entire body first. pub num_block_markers: usize, pub branch_spans: Vec, + pub decisions: IndexVec, } #[derive(Clone, Debug)] #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub struct BranchSpan { + /// Source code region associated to the branch. pub span: Span, + /// Decision structure the branch is part of. Only used in the MCDC coverage. + pub decision_id: DecisionMarkerId, pub true_marker: BlockMarkerId, pub false_marker: BlockMarkerId, } diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index 41df2e3b5875a..bcfba1e8b0f37 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -477,7 +477,7 @@ fn write_coverage_branch_info( ) -> io::Result<()> { let coverage::BranchInfo { branch_spans, .. } = branch_info; - for coverage::BranchSpan { span, true_marker, false_marker } in branch_spans { + for coverage::BranchSpan { span, true_marker, false_marker, .. } in branch_spans { writeln!( w, "{INDENT}coverage branch {{ true: {true_marker:?}, false: {false_marker:?} }} => {span:?}", diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index a62379def534e..454e361cfc8f3 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -407,6 +407,7 @@ TrivialTypeTraversalImpls! { ::rustc_hir::MatchSource, ::rustc_target::asm::InlineAsmRegOrRegClass, crate::mir::coverage::BlockMarkerId, + crate::mir::coverage::DecisionMarkerId, crate::mir::coverage::CounterId, crate::mir::coverage::ExpressionId, crate::mir::Local, diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index ab0043906b19f..06a85a1c3ef03 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -2,11 +2,13 @@ use std::assert_matches::assert_matches; use std::collections::hash_map::Entry; use rustc_data_structures::fx::FxHashMap; -use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, CoverageKind}; +use rustc_index::IndexVec; +use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, CoverageKind, DecisionMarkerId}; use rustc_middle::mir::{self, BasicBlock, UnOp}; 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; @@ -16,6 +18,7 @@ pub(crate) struct BranchInfoBuilder { num_block_markers: usize, branch_spans: Vec, + decisions: IndexVec, } #[derive(Clone, Copy)] @@ -32,8 +35,15 @@ 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. pub(crate) fn new_if_enabled(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option { - if tcx.sess.instrument_coverage_branch() && tcx.is_eligible_for_coverage(def_id) { - Some(Self { nots: FxHashMap::default(), num_block_markers: 0, branch_spans: vec![] }) + if (tcx.sess.instrument_coverage_branch() || tcx.sess.instrument_coverage_mcdc()) + && tcx.is_eligible_for_coverage(def_id) + { + Some(Self { + nots: FxHashMap::default(), + num_block_markers: 0, + branch_spans: vec![], + decisions: IndexVec::new(), + }) } else { None } @@ -86,14 +96,14 @@ impl BranchInfoBuilder { } pub(crate) fn into_done(self) -> Option> { - let Self { nots: _, num_block_markers, branch_spans } = self; + let Self { nots: _, num_block_markers, branch_spans, decisions } = self; if num_block_markers == 0 { assert!(branch_spans.is_empty()); return None; } - Some(Box::new(mir::coverage::BranchInfo { num_block_markers, branch_spans })) + Some(Box::new(mir::coverage::BranchInfo { num_block_markers, branch_spans, decisions })) } } @@ -105,6 +115,7 @@ impl Builder<'_, '_> { mut expr_id: ExprId, mut then_block: BasicBlock, mut else_block: BasicBlock, + decision_id: Option, // 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 }; @@ -125,9 +136,15 @@ 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 { + CoverageKind::MCDCBlockMarker { id, decision_id } + } else { + CoverageKind::BlockMarker { id } + }; + let marker_statement = mir::Statement { source_info, - kind: mir::StatementKind::Coverage(CoverageKind::BlockMarker { id }), + kind: mir::StatementKind::Coverage(cov_kind), }; self.cfg.push(block, marker_statement); @@ -139,8 +156,41 @@ 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)), 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 { + // Early return if MCDC coverage is not enabled. + if !self.tcx.sess.instrument_coverage_mcdc() { + return None; + } + let Some(branch_info) = self.coverage_branch_info.as_mut() else { + return None; + }; + + let span = self.thir[expr_id].span; + let decision_id = branch_info.decisions.push(span); + + // Inject a decision marker + let source_info = self.source_info(span); + let marker_statement = mir::Statement { + source_info, + kind: mir::StatementKind::Coverage( + CoverageKind::MCDCDecisionMarker { id: decision_id }, + ), + }; + self.cfg.push(block, marker_statement); + + Some(decision_id) + } } diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index a6b513ce7d065..2cfcad3a9d00f 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -16,6 +16,7 @@ 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}; @@ -41,6 +42,9 @@ 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, } impl<'a, 'tcx> Builder<'a, 'tcx> { @@ -58,10 +62,18 @@ 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, expr_id, - ThenElseArgs { temp_scope_override, variable_source_info, declare_let_bindings }, + ThenElseArgs { + temp_scope_override, + variable_source_info, + declare_let_bindings, + decision_id, + }, ) } @@ -159,7 +171,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // Record branch coverage info for this condition. // (Does nothing if branch coverage is not enabled.) - this.visit_coverage_branch_condition(expr_id, then_block, else_block); + this.visit_coverage_branch_condition( + expr_id, + then_block, + else_block, + args.decision_id, + ); let source_info = this.source_info(expr_span); this.cfg.terminate(block, source_info, term); diff --git a/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs b/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs index da82f8de78147..d3ca67d45b7b3 100644 --- a/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs +++ b/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs @@ -5,7 +5,7 @@ //! - [`AscribeUserType`] //! - [`FakeRead`] //! - [`Assign`] statements with a [`Fake`] borrow -//! - [`Coverage`] statements of kind [`BlockMarker`] or [`SpanMarker`] +//! - [`Coverage`] statements of kind [`BlockMarker`], [`SpanMarker`], [`MCDCBlockMarker`], or [`MCDCDecisionMarker`] //! //! [`AscribeUserType`]: rustc_middle::mir::StatementKind::AscribeUserType //! [`Assign`]: rustc_middle::mir::StatementKind::Assign @@ -15,6 +15,8 @@ //! [`Coverage`]: rustc_middle::mir::StatementKind::Coverage //! [`BlockMarker`]: rustc_middle::mir::coverage::CoverageKind::BlockMarker //! [`SpanMarker`]: rustc_middle::mir::coverage::CoverageKind::SpanMarker +//! [`MCDCBlockMarker`]: rustc_middle::mir::coverage::CoverageKind::MCDCBlockMarker +//! [`MCDCDecisionMarker`]: rustc_middle::mir::coverage::CoverageKind::MCDCDecisionMarker use crate::MirPass; use rustc_middle::mir::coverage::CoverageKind; @@ -33,7 +35,10 @@ impl<'tcx> MirPass<'tcx> for CleanupPostBorrowck { | StatementKind::Coverage( // These kinds of coverage statements are markers inserted during // MIR building, and are not needed after InstrumentCoverage. - CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. }, + CoverageKind::BlockMarker { .. } + | CoverageKind::SpanMarker { .. } + | CoverageKind::MCDCBlockMarker { .. } + | CoverageKind::MCDCDecisionMarker { .. }, ) | StatementKind::FakeRead(..) => statement.make_nop(), _ => (), diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index adb0c9f1929d9..3e2e1b6e03cb2 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -222,8 +222,13 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option { | StatementKind::PlaceMention(..) | StatementKind::AscribeUserType(_, _) => Some(statement.source_info.span), - // Block markers are used for branch coverage, so ignore them here. - StatementKind::Coverage(CoverageKind::BlockMarker { .. }) => None, + StatementKind::Coverage( + // Block markers are used for branch coverage, so ignore them here. + CoverageKind::BlockMarker {..} + // Ignore MCDC markers as well + | CoverageKind::MCDCBlockMarker{ .. } + | CoverageKind::MCDCDecisionMarker{ .. } + ) => None, // These coverage statements should not exist prior to coverage instrumentation. StatementKind::Coverage( @@ -378,8 +383,14 @@ 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(CoverageKind::BlockMarker { id }) = statement.kind { - block_markers[id] = Some(bb); + if let StatementKind::Coverage(kind) = &statement.kind { + match kind { + CoverageKind::BlockMarker { id } + | CoverageKind::MCDCBlockMarker { id, decision_id: _ } => { + block_markers[*id] = Some(bb); + } + _ => (), + } } } } @@ -387,7 +398,7 @@ pub(super) fn extract_branch_mappings( branch_info .branch_spans .iter() - .filter_map(|&BranchSpan { span: raw_span, true_marker, false_marker }| { + .filter_map(|&BranchSpan { span: raw_span, decision_id: _, true_marker, false_marker }| { // For now, ignore any branch span that was introduced by // expansion. This makes things like assert macros less noisy. if !raw_span.ctxt().outer_expn_data().is_root() { From 5bdabd0e572fac800cb6cc122e67a25419263bb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Tue, 19 Mar 2024 17:02:00 +0000 Subject: [PATCH 07/21] mcdc-coverage: Add Data in BranchInfo for MCDC Tracability --- compiler/rustc_middle/src/mir/coverage.rs | 17 ++++++++++++-- .../rustc_mir_build/src/build/coverageinfo.rs | 23 +++++++++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index b0706e99f7eda..37ab84163664b 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -257,7 +257,19 @@ pub struct BranchInfo { /// data structures without having to scan the entire body first. pub num_block_markers: usize, pub branch_spans: Vec, - pub decisions: IndexVec, + + /// Associate a span for every decision in the function body. + /// Empty if MCDC coverage is disabled. + pub decision_spans: IndexVec, +} + +#[derive(Clone, Debug)] +#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] +pub struct DecisionSpan { + /// Source code region associated to the decision. + pub span: Span, + /// Number of conditions in the decision. + pub num_conditions: u32, } #[derive(Clone, Debug)] @@ -265,7 +277,8 @@ pub struct BranchInfo { pub struct BranchSpan { /// Source code region associated to the branch. pub span: Span, - /// Decision structure the branch is part of. Only used in the MCDC coverage. + /// ID of Decision structure the branch is part of. Only used in + /// the MCDC coverage. pub decision_id: DecisionMarkerId, pub true_marker: BlockMarkerId, pub false_marker: BlockMarkerId, diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 06a85a1c3ef03..25d70ffac1364 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -3,7 +3,9 @@ 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}; +use rustc_middle::mir::coverage::{ + BlockMarkerId, BranchSpan, CoverageKind, DecisionMarkerId, DecisionSpan, +}; use rustc_middle::mir::{self, BasicBlock, UnOp}; use rustc_middle::thir::{ExprId, ExprKind, Thir}; use rustc_middle::ty::TyCtxt; @@ -103,7 +105,24 @@ impl BranchInfoBuilder { return None; } - Some(Box::new(mir::coverage::BranchInfo { num_block_markers, branch_spans, decisions })) + let mut decision_spans = IndexVec::from_iter( + decisions + .into_iter() + .map(|span| DecisionSpan { span, num_conditions: 0 }), + ); + + // Count the number of conditions linked to each decision. + if !decision_spans.is_empty() { + for branch_span in branch_spans.iter() { + decision_spans[branch_span.decision_id].num_conditions += 1; + } + } + + Some(Box::new(mir::coverage::BranchInfo { + num_block_markers, + branch_spans, + decision_spans, + })) } } From e8b9b7c7ff53874bf5139bd1eebf0fd91fbc6cb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 20 Mar 2024 11:33:26 +0000 Subject: [PATCH 08/21] mcdc-coverage: Add CoverageKind::MCDCBitmapRequire --- .../src/coverageinfo/mod.rs | 7 +++++- compiler/rustc_middle/src/mir/coverage.rs | 8 ++++++ compiler/rustc_middle/src/mir/pretty.rs | 25 ++++++++++++++++--- .../src/coverage/spans/from_mir.rs | 3 ++- 4 files changed, 38 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 349b6fea0ac80..577e513ba0a32 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -105,6 +105,11 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { | CoverageKind::MCDCDecisionMarker { .. } => unreachable!( "marker statement {kind:?} should have been removed by CleanupPostBorrowck" ), + CoverageKind::MCDCBitmapRequire { needed_bytes } => { + // FIXME(dprn): TODO + let _ = needed_bytes; + warn!("call to correct intrinsic"); + } CoverageKind::CounterIncrement { id } => { func_coverage.mark_counter_id_seen(id); // We need to explicitly drop the `RefMut` before calling into `instrprof_increment`, @@ -290,4 +295,4 @@ pub(crate) fn prf_bits_section_name(cx: &CodegenCx<'_, '_>) -> String { llvm::LLVMRustCoverageWriteBitmapSectionNameToString(cx.llmod, s); }) .expect("Rust Coverage function record section name failed UTF-8 conversion") -} \ No newline at end of file +} diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 37ab84163664b..86e5f219a6b47 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -116,6 +116,13 @@ pub enum CoverageKind { /// Has no effect during codegen. MCDCDecisionMarker { id: DecisionMarkerId }, + /// Declares the number of bytes needed to store the test-vector bitmaps of + /// all the decisions in the function body. + /// + /// In LLVM backend, this is done by inserting a call to the + /// `instrprof.mcdc.parameters` intrinsic. + MCDCBitmapRequire { needed_bytes: u32 }, + /// Marks the point in MIR control flow represented by a coverage counter. /// /// This is eventually lowered to `llvm.instrprof.increment` in LLVM IR. @@ -145,6 +152,7 @@ impl Debug for CoverageKind { write!(fmt, "MCDCBlockMarker({:?}, {:?})", id.index(), decision_id.index()) } MCDCDecisionMarker { id } => write!(fmt, "MCDCDecisionMarker({:?})", id.index()), + MCDCBitmapRequire { needed_bytes } => write!(fmt, "MCDCBitmapRequire({needed_bytes} bytes)"), CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()), ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()), } diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index bcfba1e8b0f37..30d31de2667e5 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -475,14 +475,33 @@ fn write_coverage_branch_info( branch_info: &coverage::BranchInfo, w: &mut dyn io::Write, ) -> io::Result<()> { - let coverage::BranchInfo { branch_spans, .. } = branch_info; + let coverage::BranchInfo { branch_spans, decision_spans, .. } = branch_info; - for coverage::BranchSpan { span, true_marker, false_marker, .. } in branch_spans { + // Write MCDC stuff as well if present + for (id, coverage::DecisionSpan { span, num_conditions }) in decision_spans.iter_enumerated() { writeln!( w, - "{INDENT}coverage branch {{ true: {true_marker:?}, false: {false_marker:?} }} => {span:?}", + "{INDENT}coverage MCDC decision {{ id: {id:?}, num_conditions: {num_conditions:?} }} => {span:?}", )?; } + + if !decision_spans.is_empty() { + writeln!(w)?; + for coverage::BranchSpan { span, true_marker, false_marker, decision_id } in branch_spans { + writeln!( + w, + "{INDENT}coverage branch {{ decision: {decision_id:?}, true: {true_marker:?}, false: {false_marker:?} }} => {span:?}", + )?; + } + } else { + for coverage::BranchSpan { span, true_marker, false_marker, .. } in branch_spans { + writeln!( + w, + "{INDENT}coverage branch {{ true: {true_marker:?}, false: {false_marker:?} }} => {span:?}", + )?; + } + } + if !branch_spans.is_empty() { writeln!(w)?; } diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index 3e2e1b6e03cb2..8f5121c00feb2 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -232,7 +232,8 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option { // These coverage statements should not exist prior to coverage instrumentation. StatementKind::Coverage( - CoverageKind::CounterIncrement { .. } | CoverageKind::ExpressionUsed { .. }, + CoverageKind::CounterIncrement { .. } | CoverageKind::ExpressionUsed { .. } + | CoverageKind::MCDCBitmapRequire { .. }, ) => bug!( "Unexpected coverage statement found during coverage instrumentation: {statement:?}" ), From 76691546b8d28685047dfdd8801c2fbc073efff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Thu, 21 Mar 2024 09:11:54 +0000 Subject: [PATCH 09/21] mcdc-coverage: Generate `mcdc.parameters` intrinsic call at the beginning of a function --- .../src/coverageinfo/mod.rs | 15 ++++- compiler/rustc_mir_transform/messages.ftl | 2 + .../rustc_mir_transform/src/coverage/mcdc.rs | 60 +++++++++++++++++++ .../rustc_mir_transform/src/coverage/mod.rs | 5 ++ 4 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 compiler/rustc_mir_transform/src/coverage/mcdc.rs diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 577e513ba0a32..168a1caa12657 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -106,9 +106,18 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { "marker statement {kind:?} should have been removed by CleanupPostBorrowck" ), CoverageKind::MCDCBitmapRequire { needed_bytes } => { - // FIXME(dprn): TODO - let _ = needed_bytes; - warn!("call to correct intrinsic"); + // We need to explicitly drop the `RefMut` before calling into `instrprof_mcdc_parameters`, + // as that needs an exclusive borrow. + drop(coverage_map); + + let fn_name = bx.get_pgo_func_name_var(instance); + let hash = bx.const_u64(function_coverage_info.function_source_hash); + let num_bitmap_bytes = bx.const_u32(needed_bytes); + debug!( + "codegen intrinsic instrprof.mcdc.parameters(fn_name={:?}, hash={:?}, bitmap_bytes={:?})", + fn_name, hash, num_bitmap_bytes, + ); + bx.instrprof_mcdc_parameters(fn_name, hash, num_bitmap_bytes); } CoverageKind::CounterIncrement { id } => { func_coverage.mark_counter_id_seen(id); diff --git a/compiler/rustc_mir_transform/messages.ftl b/compiler/rustc_mir_transform/messages.ftl index b8dbdf18db3ea..1af73d1314258 100644 --- a/compiler/rustc_mir_transform/messages.ftl +++ b/compiler/rustc_mir_transform/messages.ftl @@ -34,6 +34,8 @@ mir_transform_mutation_layout_constrained_borrow_label = borrow of layout constr mir_transform_mutation_layout_constrained_borrow_note = references to fields of layout constrained fields lose the constraints. Coupled with interior mutability, the field can be changed to invalid values mir_transform_mutation_layout_constrained_label = mutation of layout constrained field mir_transform_mutation_layout_constrained_note = mutating layout constrained fields cannot statically be checked for valid values +mir_transform_mcdc_too_many_conditions = Unsupported MCDC Decision: Number of conditions ({$num_conditions}) exceeds max ({$max_conditions}). Expression will be skipped. + mir_transform_operation_will_panic = this operation will panic at runtime mir_transform_requires_unsafe = {$details} is unsafe and requires unsafe {$op_in_unsafe_fn_allowed -> diff --git a/compiler/rustc_mir_transform/src/coverage/mcdc.rs b/compiler/rustc_mir_transform/src/coverage/mcdc.rs new file mode 100644 index 0000000000000..7dce815e6ec3e --- /dev/null +++ b/compiler/rustc_mir_transform/src/coverage/mcdc.rs @@ -0,0 +1,60 @@ +use rustc_macros::Diagnostic; +use rustc_middle::{ + mir::{ + self, + coverage::{CoverageKind, DecisionSpan}, + Statement, + }, + ty::TyCtxt, +}; +use rustc_span::Span; + +#[derive(Diagnostic)] +#[diag(mir_transform_mcdc_too_many_conditions)] +pub(crate) struct MCDCTooManyConditions { + #[primary_span] + pub span: Span, + pub num_conditions: u32, + pub max_conditions: u32, +} + +const MAX_COND_DECISION: u32 = 6; + +/// If MCDC coverage is enabled, add MCDC instrumentation to the function. +/// Will do the following things : +/// 1. Add an instruction in the first basic block for the codegen to call +/// the `instrprof.mcdc.parameters` instrinsic. +pub fn instrument_function_mcdc<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir::Body<'tcx>) { + if !tcx.sess.instrument_coverage_mcdc() { + return; + } + + let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { + return; + }; + + // Compute the total sum of needed bytes for the current body. + let needed_bytes: u32 = branch_info + .decision_spans + .iter() + .filter_map(|DecisionSpan { span, num_conditions }| { + if *num_conditions > MAX_COND_DECISION { + tcx.dcx().emit_warn(MCDCTooManyConditions { + span: *span, + num_conditions: *num_conditions, + max_conditions: MAX_COND_DECISION, + }); + None + } else { + Some(1 << num_conditions) + } + }) + .sum(); + + let entry_bb = &mut mir_body.basic_blocks_mut()[mir::START_BLOCK]; + let mcdc_parameters_statement = Statement { + source_info: entry_bb.terminator().source_info, + kind: mir::StatementKind::Coverage(CoverageKind::MCDCBitmapRequire { needed_bytes }), + }; + entry_bb.statements.insert(0, mcdc_parameters_statement); +} diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index d382d2c03c24a..b7e3fb6c566be 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -3,6 +3,7 @@ pub mod query; mod counters; mod graph; mod spans; +mod mcdc; #[cfg(test)] mod tests; @@ -100,6 +101,10 @@ fn instrument_function_for_coverage<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir: &coverage_counters, ); + //////////////////////////////////////////////////// + // Add the MCDC instrumentation. This is a no-op if MCDC coverage is disabled. + mcdc::instrument_function_mcdc(tcx, mir_body); + mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo { function_source_hash: hir_info.function_source_hash, num_counters: coverage_counters.num_counters(), From 1e2afe6a935b8f4cdd46f7ef24f6ec14a7e27a62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Thu, 21 Mar 2024 16:56:34 +0000 Subject: [PATCH 10/21] mcdc-coverage: allocate a condition bitmap on the function stackframe upon MCDCBitmapRequire --- .../src/coverageinfo/mod.rs | 52 ++++++++++++++----- 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 168a1caa12657..66e79b48c740a 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -32,6 +32,10 @@ pub struct CrateCoverageContext<'ll, 'tcx> { pub(crate) function_coverage_map: RefCell, FunctionCoverageCollector<'tcx>>>, pub(crate) pgo_func_name_var_map: RefCell, &'ll llvm::Value>>, + + /// When MCDC is enabled, holds references to the stack-allocated function-wise + /// condition bitmaps. + pub(crate) mcdc_condbitmap_map: RefCell, &'ll llvm::Value>>, } impl<'ll, 'tcx> CrateCoverageContext<'ll, 'tcx> { @@ -39,6 +43,7 @@ impl<'ll, 'tcx> CrateCoverageContext<'ll, 'tcx> { Self { function_coverage_map: Default::default(), pgo_func_name_var_map: Default::default(), + mcdc_condbitmap_map: Default::default(), } } @@ -105,20 +110,6 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { | CoverageKind::MCDCDecisionMarker { .. } => unreachable!( "marker statement {kind:?} should have been removed by CleanupPostBorrowck" ), - CoverageKind::MCDCBitmapRequire { needed_bytes } => { - // We need to explicitly drop the `RefMut` before calling into `instrprof_mcdc_parameters`, - // as that needs an exclusive borrow. - drop(coverage_map); - - let fn_name = bx.get_pgo_func_name_var(instance); - let hash = bx.const_u64(function_coverage_info.function_source_hash); - let num_bitmap_bytes = bx.const_u32(needed_bytes); - debug!( - "codegen intrinsic instrprof.mcdc.parameters(fn_name={:?}, hash={:?}, bitmap_bytes={:?})", - fn_name, hash, num_bitmap_bytes, - ); - bx.instrprof_mcdc_parameters(fn_name, hash, num_bitmap_bytes); - } CoverageKind::CounterIncrement { id } => { func_coverage.mark_counter_id_seen(id); // We need to explicitly drop the `RefMut` before calling into `instrprof_increment`, @@ -150,6 +141,39 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { CoverageKind::ExpressionUsed { id } => { func_coverage.mark_expression_id_seen(id); } + CoverageKind::MCDCBitmapRequire { needed_bytes } => { + // We need to explicitly drop the `RefMut` before calling into `instrprof_mcdc_parameters`, + // as that needs an exclusive borrow. + drop(coverage_map); + + let fn_name = bx.get_pgo_func_name_var(instance); + let hash = bx.const_u64(function_coverage_info.function_source_hash); + let num_bitmap_bytes = bx.const_u32(needed_bytes); + debug!( + "codegen intrinsic instrprof.mcdc.parameters(fn_name={:?}, hash={:?}, bitmap_bytes={:?})", + fn_name, hash, num_bitmap_bytes, + ); + // Call the intrinsic to ask LLVM to allocate a global variable for the + // test vector bitmaps in the function body. + bx.instrprof_mcdc_parameters(fn_name, hash, num_bitmap_bytes); + + // Allocates an integer in the function stackframe that will be + // used for the condition bitmaps of the decisions. + let cond_bitmap_addr = bx.alloca( + bx.type_uint_from_ty(rustc_middle::ty::UintTy::U32), + Align::from_bytes(4).expect("4 bytes alignment failed"), + ); + + // Acquire a new handle to coverage_context. + let coverage_context = bx.coverage_context().expect("Presence checked"); + coverage_context + .mcdc_condbitmap_map + .borrow_mut() + .insert(instance, cond_bitmap_addr); + } + CoverageKind::MCDCCondBitmapReset => todo!(), + CoverageKind::MCDCCondBitmapUpdate { condition_id: _, bool_value: _ } => todo!(), + CoverageKind::MCDCTestBitmapUpdate { needed_bytes: _, decision_index: _ } => todo!(), } } } From a30b4bb1a466c499cbc80102916583a80f4bff92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Tue, 26 Mar 2024 10:09:43 +0000 Subject: [PATCH 11/21] mcdc-coverage: rename Decision marker to MCDCDecisionEntryMarker, add output marker --- .../src/coverageinfo/mod.rs | 3 ++- compiler/rustc_middle/src/mir/coverage.rs | 20 ++++++++++++++++--- .../rustc_mir_build/src/build/coverageinfo.rs | 16 ++++++--------- .../src/cleanup_post_borrowck.rs | 3 ++- .../src/coverage/spans/from_mir.rs | 3 ++- 5 files changed, 29 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 66e79b48c740a..5da0a5923d66f 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -107,7 +107,8 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } | CoverageKind::MCDCBlockMarker { .. } - | CoverageKind::MCDCDecisionMarker { .. } => unreachable!( + | CoverageKind::MCDCDecisionEntryMarker { .. } + | CoverageKind::MCDCDecisionOutputMarker { .. } => unreachable!( "marker statement {kind:?} should have been removed by CleanupPostBorrowck" ), CoverageKind::CounterIncrement { id } => { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 86e5f219a6b47..ed09dfc45137a 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -114,7 +114,14 @@ pub enum CoverageKind { /// conditions in the same decision will reference this id. /// /// Has no effect during codegen. - MCDCDecisionMarker { id: DecisionMarkerId }, + MCDCDecisionEntryMarker { id: DecisionMarkerId }, + + /// 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 }, /// Declares the number of bytes needed to store the test-vector bitmaps of /// all the decisions in the function body. @@ -151,8 +158,15 @@ impl Debug for CoverageKind { MCDCBlockMarker { id, decision_id } => { write!(fmt, "MCDCBlockMarker({:?}, {:?})", id.index(), decision_id.index()) } - MCDCDecisionMarker { id } => write!(fmt, "MCDCDecisionMarker({:?})", id.index()), - MCDCBitmapRequire { needed_bytes } => write!(fmt, "MCDCBitmapRequire({needed_bytes} bytes)"), + MCDCDecisionEntryMarker { id } => { + write!(fmt, "MCDCDecisionEntryMarker({:?})", id.index()) + } + &MCDCDecisionOutputMarker { id, outcome } => { + write!(fmt, "MCDCDecisionOutputMarker({:?}, {})", id.index(), outcome) + } + MCDCBitmapRequire { needed_bytes } => { + write!(fmt, "MCDCBitmapRequire({needed_bytes} bytes)") + } CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()), ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()), } diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 25d70ffac1364..4185eee30126d 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -106,9 +106,7 @@ impl BranchInfoBuilder { } let mut decision_spans = IndexVec::from_iter( - decisions - .into_iter() - .map(|span| DecisionSpan { span, num_conditions: 0 }), + decisions.into_iter().map(|span| DecisionSpan { span, num_conditions: 0 }), ); // Count the number of conditions linked to each decision. @@ -161,10 +159,8 @@ impl Builder<'_, '_> { CoverageKind::BlockMarker { id } }; - let marker_statement = mir::Statement { - source_info, - kind: mir::StatementKind::Coverage(cov_kind), - }; + let marker_statement = + mir::Statement { source_info, kind: mir::StatementKind::Coverage(cov_kind) }; self.cfg.push(block, marker_statement); id @@ -204,9 +200,9 @@ impl Builder<'_, '_> { let source_info = self.source_info(span); let marker_statement = mir::Statement { source_info, - kind: mir::StatementKind::Coverage( - CoverageKind::MCDCDecisionMarker { id: decision_id }, - ), + kind: mir::StatementKind::Coverage(CoverageKind::MCDCDecisionEntryMarker { + id: decision_id, + }), }; self.cfg.push(block, marker_statement); diff --git a/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs b/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs index d3ca67d45b7b3..47e315e4e39f3 100644 --- a/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs +++ b/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs @@ -38,7 +38,8 @@ impl<'tcx> MirPass<'tcx> for CleanupPostBorrowck { CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. } | CoverageKind::MCDCBlockMarker { .. } - | CoverageKind::MCDCDecisionMarker { .. }, + | CoverageKind::MCDCDecisionEntryMarker { .. } + | CoverageKind::MCDCDecisionOutputMarker { .. }, ) | StatementKind::FakeRead(..) => statement.make_nop(), _ => (), diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index 8f5121c00feb2..cdd0dd6fb09a4 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -227,7 +227,8 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option { CoverageKind::BlockMarker {..} // Ignore MCDC markers as well | CoverageKind::MCDCBlockMarker{ .. } - | CoverageKind::MCDCDecisionMarker{ .. } + | CoverageKind::MCDCDecisionEntryMarker{ .. } + | CoverageKind::MCDCDecisionOutputMarker { .. } ) => None, // These coverage statements should not exist prior to coverage instrumentation. From f8314960522a395e9e727e48079aaa5123dadbaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Tue, 26 Mar 2024 10:42:14 +0000 Subject: [PATCH 12/21] mcdc-coverage: Refactor Decision markers creation, add outcome markers creation --- compiler/rustc_mir_build/messages.ftl | 2 + .../rustc_mir_build/src/build/coverageinfo.rs | 126 +++++++++++++++--- .../rustc_mir_build/src/build/expr/into.rs | 13 ++ .../rustc_mir_build/src/build/matches/mod.rs | 8 -- compiler/rustc_mir_build/src/errors.rs | 7 + 5 files changed, 132 insertions(+), 24 deletions(-) diff --git a/compiler/rustc_mir_build/messages.ftl b/compiler/rustc_mir_build/messages.ftl index 1de691f32a70c..f332abf54208c 100644 --- a/compiler/rustc_mir_build/messages.ftl +++ b/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 diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 4185eee30126d..86b2f91ca1ac0 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -13,6 +13,7 @@ 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. @@ -20,6 +21,16 @@ pub(crate) struct BranchInfoBuilder { num_block_markers: usize, branch_spans: Vec, + + // 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, + /// 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, } @@ -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> { - 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 { + 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, // 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 { + /// 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 } } diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index b4eeeccc127b4..c6a105c8b1dd3 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/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`. then_block.and(else_block) }, diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 2cfcad3a9d00f..46508aea5b49e 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/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, } 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); diff --git a/compiler/rustc_mir_build/src/errors.rs b/compiler/rustc_mir_build/src/errors.rs index 848da56f98168..a31caa14243c0 100644 --- a/compiler/rustc_mir_build/src/errors.rs +++ b/compiler/rustc_mir_build/src/errors.rs @@ -850,6 +850,13 @@ pub(crate) struct PatternNotCovered<'s, 'tcx> { pub misc_suggestion: Option, } +#[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)] From 6e7e2c56ca0026095df665565c9f1437bb4b9e58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 27 Mar 2024 09:54:24 +0000 Subject: [PATCH 13/21] mcdc-coverage: Add CoverageKinds for Bitmap manipulations. --- compiler/rustc_middle/src/mir/coverage.rs | 38 ++++++++++++++----- .../src/coverage/spans/from_mir.rs | 8 +++- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index ed09dfc45137a..7f23a588f67c1 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -123,13 +123,6 @@ pub enum CoverageKind { /// Has no effect during codegen. MCDCDecisionOutputMarker { id: DecisionMarkerId, outcome: bool }, - /// Declares the number of bytes needed to store the test-vector bitmaps of - /// all the decisions in the function body. - /// - /// In LLVM backend, this is done by inserting a call to the - /// `instrprof.mcdc.parameters` intrinsic. - MCDCBitmapRequire { needed_bytes: u32 }, - /// Marks the point in MIR control flow represented by a coverage counter. /// /// This is eventually lowered to `llvm.instrprof.increment` in LLVM IR. @@ -147,6 +140,22 @@ pub enum CoverageKind { /// mappings. Intermediate expressions with no direct mappings are /// retained/zeroed based on whether they are transitively used.) ExpressionUsed { id: ExpressionId }, + + /// Declares the number of bytes needed to store the test-vector bitmaps of + /// all the decisions in the function body. + /// + /// In LLVM backend, this is done by inserting a call to the + /// `instrprof.mcdc.parameters` intrinsic. + MCDCBitmapRequire { needed_bytes: u32 }, + + /// Marks a point where the condition bitmap should be set to 0. + MCDCCondBitmapReset, + + /// Marks a point where a bit of the condition bitmap should be set. + MCDCCondBitmapUpdate { condition_id: u32, bool_value: bool }, + + /// Marks a point where a bit of the global Test Vector bitmap should be set to one. + MCDCTestBitmapUpdate { needed_bytes: u32, decision_index: u32 }, } impl Debug for CoverageKind { @@ -161,14 +170,23 @@ impl Debug for CoverageKind { MCDCDecisionEntryMarker { id } => { write!(fmt, "MCDCDecisionEntryMarker({:?})", id.index()) } - &MCDCDecisionOutputMarker { id, outcome } => { + MCDCDecisionOutputMarker { id, outcome } => { write!(fmt, "MCDCDecisionOutputMarker({:?}, {})", id.index(), outcome) } + CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()), + ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()), MCDCBitmapRequire { needed_bytes } => { write!(fmt, "MCDCBitmapRequire({needed_bytes} bytes)") } - CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()), - ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()), + MCDCCondBitmapReset => { + write!(fmt, "MCDCCondBitmapReset()") + } + MCDCCondBitmapUpdate { condition_id, bool_value } => { + write!(fmt, "MCDCCondBitmapUpdate({condition_id}, {bool_value})") + } + MCDCTestBitmapUpdate { needed_bytes, decision_index } => { + write!(fmt, "MCDCTVBitmapUpdate({needed_bytes} bytes, {decision_index})") + } } } } diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index cdd0dd6fb09a4..1bf0589f0e10b 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -233,8 +233,12 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option { // These coverage statements should not exist prior to coverage instrumentation. StatementKind::Coverage( - CoverageKind::CounterIncrement { .. } | CoverageKind::ExpressionUsed { .. } - | CoverageKind::MCDCBitmapRequire { .. }, + CoverageKind::CounterIncrement { .. } + | CoverageKind::ExpressionUsed { .. } + | CoverageKind::MCDCBitmapRequire { .. } + | CoverageKind::MCDCCondBitmapReset + | CoverageKind::MCDCCondBitmapUpdate { .. } + | CoverageKind::MCDCTestBitmapUpdate { .. } ) => bug!( "Unexpected coverage statement found during coverage instrumentation: {statement:?}" ), From 20cdc51cc497aad02bcb9e40f0d42eaa8f114e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 27 Mar 2024 11:43:10 +0000 Subject: [PATCH 14/21] mcdc-coverage: Add doc comments, move too many conditions error --- .../rustc_mir_transform/src/coverage/mcdc.rs | 133 +++++++++++++----- .../rustc_mir_transform/src/coverage/mod.rs | 2 +- compiler/rustc_mir_transform/src/errors.rs | 9 ++ 3 files changed, 109 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mcdc.rs b/compiler/rustc_mir_transform/src/coverage/mcdc.rs index 7dce815e6ec3e..aa2b5c1f84571 100644 --- a/compiler/rustc_mir_transform/src/coverage/mcdc.rs +++ b/compiler/rustc_mir_transform/src/coverage/mcdc.rs @@ -1,60 +1,125 @@ -use rustc_macros::Diagnostic; use rustc_middle::{ - mir::{ - self, - coverage::{CoverageKind, DecisionSpan}, - Statement, - }, + mir::{self, coverage::CoverageKind, Statement}, ty::TyCtxt, }; -use rustc_span::Span; - -#[derive(Diagnostic)] -#[diag(mir_transform_mcdc_too_many_conditions)] -pub(crate) struct MCDCTooManyConditions { - #[primary_span] - pub span: Span, - pub num_conditions: u32, - pub max_conditions: u32, -} + +use crate::coverage::graph::CoverageGraph; +use crate::errors::MCDCTooManyConditions; const MAX_COND_DECISION: u32 = 6; + /// If MCDC coverage is enabled, add MCDC instrumentation to the function. +/// +/// Assume a decision to be the following: +/// +/// if (A && B) || C { then(); } else { otherwise(); } +/// +/// The corresponding BDD (Binary Decision Diagram) will look like so: +/// +/// ``` +/// │ +/// ┌▼┐ +/// │A│ +/// ┌──┴─┴──┐ +/// │t f│ +/// │ │ +/// ┌▼┐ ┌▼┐ +/// │B├─────►C├───┐ +/// └┬┘ f └┬┘ f│ +/// │t │t │ +/// ┌──▼───┐ │ ┌─▼─────────┐ +/// │then()◄───┘ │otherwise()│ +/// └──────┘ └───────────┘ +/// ``` +/// +/// The coverage graph is similar, up to unwinding mechanics. The goal is to +/// instrument each edge of the BDD to update two bitmaps. +/// +/// The first bitmap (CBM) is updated upon the evaluation of each contidion. +/// It tracks the state of a condition at a given instant. +/// is given an index, such that when the decision is taken, CBM represents the +/// state of all conditions that were evaluated (1 for true, 0 for +/// false/skipped). +/// +/// The second bitmap (TVBM) is the test vector bitmap. It tracks all the test +/// vectors that were executed during the program's life. It is updated before +/// branching to `then()` or `otherwise()` by using CBM as an integer n and +/// setting the nth integer of TVBM to 1. +/// Basically, we do `TVBM |= 1 << CBM`. +/// +/// Note: This technique is very sub-optimal, as it implies that TVBM to have a +/// size of 2^n bits, (n being the number of conditions in the decision) to +/// account for every combination, even though only a subset of theses +/// combinations are actually achievable because of logical operators +/// short-circuit semantics. +/// An improvement to this instrumentation is being implemented in Clang and +/// shall be ported to Rustc in the future: +/// https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798 +/// +/// In the meantime, to follow the original implementation of Clang, we use this +/// suboptimal technique, while limiting the size of the decisions we can +/// instrument to 6 conditions. +/// /// Will do the following things : /// 1. Add an instruction in the first basic block for the codegen to call /// the `instrprof.mcdc.parameters` instrinsic. -pub fn instrument_function_mcdc<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir::Body<'tcx>) { +/// 2. Before each decision, add an instruction to reset CBM. +/// 3. Add an isntruction to update CBM upon condition evaluation. +/// 4. Add an instruction to update TVBM with the CBM value before jumping +/// to the `then` or `else` block. +/// 5. Build mappings for the reporting tools to get the results and transpose +/// it to the source code. +pub fn instrument_function_mcdc<'tcx>( + tcx: TyCtxt<'tcx>, + mir_body: &mut mir::Body<'tcx>, + coverage_graph: &CoverageGraph, +) { + let _ = coverage_graph; if !tcx.sess.instrument_coverage_mcdc() { return; } + debug!("Called instrument_function_mcdc()"); let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { return; }; - // Compute the total sum of needed bytes for the current body. - let needed_bytes: u32 = branch_info - .decision_spans - .iter() - .filter_map(|DecisionSpan { span, num_conditions }| { - if *num_conditions > MAX_COND_DECISION { - tcx.dcx().emit_warn(MCDCTooManyConditions { - span: *span, - num_conditions: *num_conditions, - max_conditions: MAX_COND_DECISION, - }); - None - } else { - Some(1 << num_conditions) - } - }) - .sum(); + let mut needed_bytes = 0; + let mut bitmap_indexes = vec![]; + + for dec_span in branch_info.decision_spans.iter() { + // Skip uninstrumentable conditions. + if dec_span.num_conditions > MAX_COND_DECISION { + tcx.dcx().emit_warn(MCDCTooManyConditions { + span: dec_span.span, + num_conditions: dec_span.num_conditions, + max_conditions: MAX_COND_DECISION, + }); + continue; + } + bitmap_indexes.push(needed_bytes); + needed_bytes += 1 << dec_span.num_conditions; + } + if needed_bytes == 0 { + // No decision to instrument + return; + } + + // In the first BB, specify that we need to allocate bitmaps. let entry_bb = &mut mir_body.basic_blocks_mut()[mir::START_BLOCK]; let mcdc_parameters_statement = Statement { source_info: entry_bb.terminator().source_info, kind: mir::StatementKind::Coverage(CoverageKind::MCDCBitmapRequire { needed_bytes }), }; entry_bb.statements.insert(0, mcdc_parameters_statement); + + // For each decision: + // - Find its 'root' basic block + // - Insert a 'reset CDM' instruction + // - for each branch, find the root condition, give it an index and + // call a condbitmapUpdate on it + // - Get the Output markers, and insert goto blocks before to put a + // tvbitmapupdate on it. } diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index b7e3fb6c566be..46d55be07a20a 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -103,7 +103,7 @@ fn instrument_function_for_coverage<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir: //////////////////////////////////////////////////// // Add the MCDC instrumentation. This is a no-op if MCDC coverage is disabled. - mcdc::instrument_function_mcdc(tcx, mir_body); + mcdc::instrument_function_mcdc(tcx, mir_body, &basic_coverage_blocks); mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo { function_source_hash: hir_info.function_source_hash, diff --git a/compiler/rustc_mir_transform/src/errors.rs b/compiler/rustc_mir_transform/src/errors.rs index 9297bc51fad99..5ef711ce64421 100644 --- a/compiler/rustc_mir_transform/src/errors.rs +++ b/compiler/rustc_mir_transform/src/errors.rs @@ -32,6 +32,15 @@ pub(crate) enum ConstMutate { }, } +#[derive(Diagnostic)] +#[diag(mir_transform_mcdc_too_many_conditions)] +pub(crate) struct MCDCTooManyConditions { + #[primary_span] + pub span: Span, + pub num_conditions: u32, + pub max_conditions: u32, +} + #[derive(Diagnostic)] #[diag(mir_transform_unaligned_packed_ref, code = E0793)] #[note] From cb55ae466dc45b2e9ac7f782cf49e353feea4544 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 27 Mar 2024 12:15:35 +0000 Subject: [PATCH 15/21] mcdc-coverage: Remove MCDCBlockMarker, Rename DecisionMarkerId, Add ConditionEntry/Output markers --- .../src/coverageinfo/mod.rs | 5 +- compiler/rustc_middle/src/mir/coverage.rs | 53 +++++++++++++------ .../rustc_middle/src/ty/structural_impls.rs | 3 +- .../rustc_mir_build/src/build/coverageinfo.rs | 20 +++---- .../src/cleanup_post_borrowck.rs | 5 +- .../src/coverage/spans/from_mir.rs | 13 ++--- 6 files changed, 58 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 5da0a5923d66f..3bc86597ddff1 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/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 } => { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 7f23a588f67c1..d5ff15700df0f 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/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, + pub decision_spans: IndexVec, } #[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, } diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 454e361cfc8f3..27478e3f91c59 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/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, diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 86b2f91ca1ac0..791012e327f20 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/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, + current_decision_id: Option, /// 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, + decisions: IndexVec, } #[derive(Clone, Copy)] @@ -158,7 +158,7 @@ impl BranchInfoBuilder { self.nested_decision_level > 1 } - pub fn current_decision_id(&self) -> Option { + pub fn current_decision_id(&self) -> Option { 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, }); diff --git a/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs b/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs index 47e315e4e39f3..f3e4b912da2e1 100644 --- a/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs +++ b/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(), _ => (), diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index 1bf0589f0e10b..9e9a195d7fe3c 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -226,9 +226,10 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option { // 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); } } } From 9fea26a7bb580e69981196fa28e7fc7ed8a948b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 27 Mar 2024 15:17:16 +0000 Subject: [PATCH 16/21] mcdc-coverage: Refactor the MCDC info building --- .../rustc_mir_build/src/build/coverageinfo.rs | 195 ++++++++++++------ .../rustc_mir_build/src/build/matches/mod.rs | 8 + 2 files changed, 143 insertions(+), 60 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 791012e327f20..ae97ec4810cf6 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/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, DecisionId, DecisionSpan, + BlockMarkerId, BranchSpan, ConditionId, CoverageKind, DecisionId, DecisionSpan, }; use rustc_middle::mir::{self, BasicBlock, UnOp}; use rustc_middle::thir::{ExprId, ExprKind, Thir}; @@ -15,14 +15,7 @@ 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, - - num_block_markers: usize, - branch_spans: Vec, - - // MCDC decision stuff +struct MCDCInfoBuilder { /// 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. @@ -31,7 +24,54 @@ pub(crate) struct BranchInfoBuilder { /// nested decisions. nested_decision_level: u32, /// Vector for storing all the decisions with their span - decisions: IndexVec, + decision_spans: IndexVec, + + next_condition_id: u32, +} + +impl MCDCInfoBuilder { + /// 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.decision_spans.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 { + if self.in_nested_condition() { None } else { self.current_decision_id } + } + + pub fn next_condition_id(&mut self) -> u32 { + let res = self.next_condition_id; + self.next_condition_id += 1; + res + } +} + +pub(crate) struct BranchInfoBuilder { + /// Maps condition expressions to their enclosing `!`, for better instrumentation. + nots: FxHashMap, + + num_block_markers: usize, + branch_spans: Vec, + + // MCDC decision stuff + mcdc_info: Option, } #[derive(Clone, Copy)] @@ -51,13 +91,21 @@ impl BranchInfoBuilder { if (tcx.sess.instrument_coverage_branch() || tcx.sess.instrument_coverage_mcdc()) && tcx.is_eligible_for_coverage(def_id) { + let mcdc_info = if tcx.sess.instrument_coverage_mcdc() { + Some(MCDCInfoBuilder { + current_decision_id: None, + nested_decision_level: 0, + decision_spans: IndexVec::new(), + next_condition_id: 0, + }) + } else { + None + }; Some(Self { nots: FxHashMap::default(), num_block_markers: 0, branch_spans: vec![], - current_decision_id: None, - nested_decision_level: 0, - decisions: IndexVec::new(), + mcdc_info, }) } else { None @@ -111,7 +159,7 @@ impl BranchInfoBuilder { } pub(crate) fn into_done(self) -> Option> { - let Self { nots: _, num_block_markers, branch_spans, decisions, .. } = self; + let Self { nots: _, num_block_markers, branch_spans, mcdc_info, .. } = self; if num_block_markers == 0 { assert!(branch_spans.is_empty()); @@ -119,7 +167,10 @@ impl BranchInfoBuilder { } let mut decision_spans = IndexVec::from_iter( - decisions.into_iter().map(|span| DecisionSpan { span, num_conditions: 0 }), + mcdc_info + .map_or(IndexVec::new(), |mcdc_info| mcdc_info.decision_spans) + .into_iter() + .map(|span| DecisionSpan { span, num_conditions: 0 }), ); // Count the number of conditions linked to each decision. @@ -135,32 +186,6 @@ 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 { - if self.in_nested_condition() { None } else { self.current_decision_id } - } } impl Builder<'_, '_> { @@ -206,7 +231,11 @@ 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(DecisionId::from_u32(0)), + decision_id: branch_info + .mcdc_info + .as_ref() + .and_then(|mcdc_info| mcdc_info.current_decision_id()) + .unwrap_or(DecisionId::from_u32(0)), true_marker, false_marker, }); @@ -215,25 +244,24 @@ impl Builder<'_, '_> { /// 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; - } - let Some(branch_info) = self.coverage_branch_info.as_mut() else { + // Get the MCDCInfoBuilder object, which existence implies that MCDC is enabled. + let Some(BranchInfoBuilder { mcdc_info: Some(mcdc_info), .. }) = + self.coverage_branch_info.as_mut() + else { return; }; let span = self.thir[expr_id].span; // enter_decision returns false if it detects nested decisions. - if !branch_info.enter_decision(span) { - // FIXME(dprn): do WARNING for nested decision. + if !mcdc_info.enter_decision(span) { + // FIXME(dprn): WARNING for nested decision does not seem to work properly 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."); + let decision_id = mcdc_info.current_decision_id().expect("Should have returned."); // Inject a decision marker let source_info = self.source_info(span); @@ -248,16 +276,15 @@ impl Builder<'_, '_> { /// 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 { + // Get the MCDCInfoBuilder object, which existence implies that MCDC is enabled. + let Some(BranchInfoBuilder { mcdc_info: Some(mcdc_info), .. }) = + self.coverage_branch_info.as_mut() + else { return; }; // Exit decision now so we can drop &mut to branch info - branch_info.exit_decision(); + mcdc_info.exit_decision(); } /// If MCDC is enabled and the current decision is being instrumented, @@ -268,16 +295,19 @@ impl Builder<'_, '_> { bb: BasicBlock, outcome: bool, ) -> BasicBlock { - let Some(branch_info) = self.coverage_branch_info.as_mut() else { - // Coverage instrumentation is not enabled. + // Get the MCDCInfoBuilder object, which existence implies that MCDC is enabled. + let Some(BranchInfoBuilder { mcdc_info: Some(mcdc_info), .. }) = + self.coverage_branch_info.as_mut() + else { return bb; }; - let Some(decision_id) = branch_info.current_decision_id() else { + + let Some(decision_id) = mcdc_info.current_decision_id() else { // Decision is not instrumented return bb; }; - let span = branch_info.decisions[decision_id]; + let span = mcdc_info.decision_spans[decision_id]; let source_info = self.source_info(span); let marker_statement = mir::Statement { source_info, @@ -298,4 +328,49 @@ impl Builder<'_, '_> { new_bb } + + /// Add markers on the condition's basic blocks to ease the later MCDC instrumentation. + pub(crate) fn visit_mcdc_condition( + &mut self, + condition_expr: ExprId, + condition_block: BasicBlock, + then_block: BasicBlock, + else_block: BasicBlock, + ) { + // Get the MCDCInfoBuilder object, which existence implies that MCDC is enabled. + let Some(BranchInfoBuilder { mcdc_info: Some(mcdc_info), .. }) = + self.coverage_branch_info.as_mut() + else { + return; + }; + + let Some(decision_id) = mcdc_info.current_decision_id() else { + // If current_decision_id() is None, the decision is not instrumented. + return; + }; + + + let id = ConditionId::from_u32(mcdc_info.next_condition_id()); + let span = self.thir[condition_expr].span; + let source_info = self.source_info(span); + + let mut inject_statement = |bb, cov_kind| { + let statement = + mir::Statement { source_info, kind: mir::StatementKind::Coverage(cov_kind) }; + self.cfg.basic_blocks[bb].statements.insert(0, statement); + }; + + inject_statement( + condition_block, + CoverageKind::MCDCConditionEntryMarker { decision_id, id }, + ); + inject_statement( + then_block, + CoverageKind::MCDCConditionOutputMarker { decision_id, id, outcome: true }, + ); + inject_statement( + else_block, + CoverageKind::MCDCConditionOutputMarker { decision_id, id, outcome: false }, + ); + } } diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 46508aea5b49e..d89c52731469a 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -169,6 +169,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { then_block, else_block, ); + // Record MCDC coverage info + // (Does nothing if mcdc coverage is not enabled.) + this.visit_mcdc_condition( + expr_id, + block, + then_block, + else_block, + ); let source_info = this.source_info(expr_span); this.cfg.terminate(block, source_info, term); From 25768f8e5d864f5fd15fd1da6fe13b010856676e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Thu, 28 Mar 2024 10:30:05 +0000 Subject: [PATCH 17/21] mcdc-coverage: Add FFI equivalents of MCDC bitmap update intrinsics --- compiler/rustc_codegen_llvm/src/builder.rs | 84 +++++++++++++++++++ compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 2 + .../rustc_codegen_ssa/src/traits/builder.rs | 18 ++++ .../rustc_llvm/llvm-wrapper/RustWrapper.cpp | 10 +++ 4 files changed, 114 insertions(+) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 125354ab1f838..e3b23f499a922 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -1270,6 +1270,90 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { } } + fn instrprof_mcdc_condbitmap_update( + &mut self, + fn_name: Self::Value, + hash: Self::Value, + cond_id: Self::Value, + cond_bitmap_addr: Self::Value, + cond_result: Self::Value, + ) { + debug!( + "instrprof_mcdc_condbitmap_update() with args ({:?}, {:?}, {:?}, {:?}, {:?})", + fn_name, hash, cond_id, cond_bitmap_addr, cond_result + ); + + let llfn = + unsafe { llvm::LLVMRustGetInstrProfMCDCCondBitmapUpdateIntrinsic(self.cx().llmod) }; + let llty = self.cx.type_func( + &[ + self.cx.type_ptr(), + self.cx.type_i64(), + self.cx.type_i32(), + self.cx.type_ptr(), + self.cx.type_i1(), + ], + self.cx.type_void(), + ); + + let args = &[fn_name, hash, cond_id, cond_bitmap_addr, cond_result]; + let args = self.check_call("call", llty, llfn, args); + + unsafe { + let _ = llvm::LLVMRustBuildCall( + self.llbuilder, + llty, + llfn, + args.as_ptr(), + args.len() as c_uint, + [].as_ptr(), + 0 as c_uint, + ); + } + } + + fn instrprof_mcdc_tvbitmap_update( + &mut self, + fn_name: Self::Value, + hash: Self::Value, + needed_bytes: Self::Value, + bitmap_idx: Self::Value, + cond_bitmap_addr: Self::Value, + ) { + debug!( + "instrprof_mcdc_tvbitmap_update() with args ({:?}, {:?}, {:?}, {:?}, {:?})", + fn_name, hash, needed_bytes, bitmap_idx, cond_bitmap_addr + ); + + let llfn = + unsafe { llvm::LLVMRustGetInstrProfMCDCTVBitmapUpdateIntrinsic(self.cx().llmod) }; + let llty = self.cx.type_func( + &[ + self.cx.type_ptr(), + self.cx.type_i64(), + self.cx.type_i32(), + self.cx.type_i32(), + self.cx.type_ptr(), + ], + self.cx.type_void(), + ); + + let args = &[fn_name, hash, needed_bytes, bitmap_idx, cond_bitmap_addr]; + let args = self.check_call("call", llty, llfn, args); + + unsafe { + let _ = llvm::LLVMRustBuildCall( + self.llbuilder, + llty, + llfn, + args.as_ptr(), + args.len() as c_uint, + [].as_ptr(), + 0 as c_uint, + ); + } + } + fn call( &mut self, llty: &'ll Type, diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index 3eff0bbb36a15..7f55339dd6c08 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1632,6 +1632,8 @@ extern "C" { // Miscellaneous instructions pub fn LLVMRustGetInstrProfIncrementIntrinsic(M: &Module) -> &Value; pub fn LLVMRustGetInstrProfMCDCParametersIntrinsic(M: &Module) -> &Value; + pub fn LLVMRustGetInstrProfMCDCCondBitmapUpdateIntrinsic(M: &Module) -> &Value; + pub fn LLVMRustGetInstrProfMCDCTVBitmapUpdateIntrinsic(M: &Module) -> &Value; pub fn LLVMRustBuildCall<'a>( B: &Builder<'a>, Ty: &'a Type, diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index fae2735bd3fe2..5333346fe629e 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -389,6 +389,24 @@ pub trait BuilderMethods<'a, 'tcx>: num_bitmap_bytes: Self::Value ); + fn instrprof_mcdc_condbitmap_update( + &mut self, + fn_name: Self::Value, + hash: Self::Value, + cond_id: Self::Value, + cond_bitmap_addr: Self::Value, + cond_result: Self::Value, + ); + + fn instrprof_mcdc_tvbitmap_update( + &mut self, + fn_name: Self::Value, + hash: Self::Value, + needed_bytes: Self::Value, + bitmap_idx: Self::Value, + cond_bitmap_addr: Self::Value, + ); + fn call( &mut self, llty: Self::Type, diff --git a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp index 1e74f43328d9b..25d54bd466753 100644 --- a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp @@ -1533,6 +1533,16 @@ extern "C" LLVMValueRef LLVMRustGetInstrProfMCDCParametersIntrinsic(LLVMModuleRe (llvm::Intrinsic::ID)llvm::Intrinsic::instrprof_mcdc_parameters)); } +extern "C" LLVMValueRef LLVMRustGetInstrProfMCDCCondBitmapUpdateIntrinsic(LLVMModuleRef M) { + return wrap(llvm::Intrinsic::getDeclaration(unwrap(M), + (llvm::Intrinsic::ID)llvm::Intrinsic::instrprof_mcdc_condbitmap_update)); +} + +extern "C" LLVMValueRef LLVMRustGetInstrProfMCDCTVBitmapUpdateIntrinsic(LLVMModuleRef M) { + return wrap(llvm::Intrinsic::getDeclaration(unwrap(M), + (llvm::Intrinsic::ID)llvm::Intrinsic::instrprof_mcdc_tvbitmap_update)); +} + extern "C" LLVMValueRef LLVMRustBuildMemCpy(LLVMBuilderRef B, LLVMValueRef Dst, unsigned DstAlign, LLVMValueRef Src, unsigned SrcAlign, From 83b8166dd631bedf77217e96eae9216239cdad97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Thu, 28 Mar 2024 13:58:48 +0000 Subject: [PATCH 18/21] mcdc-coverage(codegen): Add MCDCCondBitmap(Reset|Update) Handling in LLVM codegen --- .../src/coverageinfo/mod.rs | 50 ++++++++++++++++++- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 3bc86597ddff1..ace8c2427dbdc 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -173,8 +173,54 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { .borrow_mut() .insert(instance, cond_bitmap_addr); } - CoverageKind::MCDCCondBitmapReset => todo!(), - CoverageKind::MCDCCondBitmapUpdate { condition_id: _, bool_value: _ } => todo!(), + CoverageKind::MCDCCondBitmapReset => { + // Drop unused coverage_map, which prevents us to use bx. + drop(coverage_map); + + let cond_bitmap_addr = match coverage_context + .mcdc_condbitmap_map + .borrow() + .get(&instance) + { + Some(addr) => *addr, + None => bug!( + "Condition bitmap reset instruction was encountered without a condition bitmap being declared." + ), + }; + + bx.store( + bx.const_i32(0), + cond_bitmap_addr, + Align::from_bytes(4).expect("4 bytes alignement failed"), + ); + } + CoverageKind::MCDCCondBitmapUpdate { condition_id, bool_value } => { + // Drop unused coverage_map, which prevents us to use bx. + drop(coverage_map); + + let fn_name = bx.get_pgo_func_name_var(instance); + let hash = bx.const_u64(function_coverage_info.function_source_hash); + let cond_id = bx.const_u32(condition_id); + let cond_bitmap_addr = match coverage_context + .mcdc_condbitmap_map + .borrow() + .get(&instance) + { + Some(addr) => *addr, + None => bug!( + "Condition bitmap reset instruction was encountered without a condition bitmap being declared." + ), + }; + let cond_result = bx.const_bool(bool_value); + + bx.instrprof_mcdc_condbitmap_update( + fn_name, + hash, + cond_id, + cond_bitmap_addr, + cond_result, + ) + } CoverageKind::MCDCTestBitmapUpdate { needed_bytes: _, decision_index: _ } => todo!(), } } From 33de2bb9649580572baa08361607ea24264e7c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Fri, 29 Mar 2024 16:48:38 +0000 Subject: [PATCH 19/21] mcdc-coverage(instrumentation): Rename Id names, Start instrumentation implementation --- compiler/rustc_middle/src/mir/coverage.rs | 43 +++- .../rustc_middle/src/ty/structural_impls.rs | 2 + .../rustc_mir_build/src/build/coverageinfo.rs | 43 ++-- .../rustc_mir_build/src/build/expr/into.rs | 4 +- .../rustc_mir_transform/src/coverage/mcdc.rs | 214 +++++++++++++++++- 5 files changed, 257 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index d5ff15700df0f..52e672f8e4f81 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -15,6 +15,23 @@ rustc_index::newtype_index! { pub struct BlockMarkerId {} } +// DecisionMarkerId and ConditionMarkerId are used between THIR and MIR representations. +// DecisionId and ConditionId are used between MIR representation and LLVM-IR. + +rustc_index::newtype_index! { + #[derive(HashStable)] + #[encodable] + #[debug_format = "DecisionMarkerId({})"] + pub struct DecisionMarkerId {} +} + +rustc_index::newtype_index! { + #[derive(HashStable)] + #[encodable] + #[debug_format = "ConditionMarkerId({})"] + pub struct ConditionMarkerId {} +} + rustc_index::newtype_index! { #[derive(HashStable)] #[encodable] @@ -115,27 +132,31 @@ pub enum CoverageKind { /// conditions in the same decision will reference this id. /// /// Has no effect during codegen. - MCDCDecisionEntryMarker { id: DecisionId }, + MCDCDecisionEntryMarker { decm_id: DecisionMarkerId }, /// 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: DecisionId, outcome: bool }, + MCDCDecisionOutputMarker { decm_id: DecisionMarkerId, 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 }, + MCDCConditionEntryMarker { decm_id: DecisionMarkerId, condm_id: ConditionMarkerId }, /// 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 }, + MCDCConditionOutputMarker { + decm_id: DecisionMarkerId, + condm_id: ConditionMarkerId, + outcome: bool, + }, /// Marks the point in MIR control flow represented by a coverage counter. /// @@ -178,20 +199,20 @@ impl Debug for CoverageKind { match self { SpanMarker => write!(fmt, "SpanMarker"), BlockMarker { id } => write!(fmt, "BlockMarker({:?})", id.index()), - MCDCDecisionEntryMarker { id } => { + MCDCDecisionEntryMarker { decm_id: id } => { write!(fmt, "MCDCDecisionEntryMarker({:?})", id.index()) } - MCDCDecisionOutputMarker { id, outcome } => { + MCDCDecisionOutputMarker { decm_id: id, outcome } => { write!(fmt, "MCDCDecisionOutputMarker({:?}, {})", id.index(), outcome) } - MCDCConditionEntryMarker { decision_id, id } => { + MCDCConditionEntryMarker { decm_id: decision_id, condm_id: id } => { write!(fmt, "MCDCConditionEntryMarker({:?}, {:?})", decision_id.index(), id.index()) } - MCDCConditionOutputMarker { decision_id, id, outcome } => { + MCDCConditionOutputMarker { decm_id: decision_marker_id, condm_id: id, outcome } => { write!( fmt, "MCDCConditionOutcomeMarker({:?}, {:?}, {})", - decision_id.index(), + decision_marker_id.index(), id.index(), outcome ) @@ -323,7 +344,7 @@ pub struct BranchInfo { /// Associate a span for every decision in the function body. /// Empty if MCDC coverage is disabled. - pub decision_spans: IndexVec, + pub decision_spans: IndexVec, } #[derive(Clone, Debug)] @@ -342,7 +363,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: DecisionId, + pub decision_id: DecisionMarkerId, pub true_marker: BlockMarkerId, pub false_marker: BlockMarkerId, } diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 27478e3f91c59..6ba4ecfd2b560 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -407,6 +407,8 @@ TrivialTypeTraversalImpls! { ::rustc_hir::MatchSource, ::rustc_target::asm::InlineAsmRegOrRegClass, crate::mir::coverage::BlockMarkerId, + crate::mir::coverage::DecisionMarkerId, + crate::mir::coverage::ConditionMarkerId, crate::mir::coverage::DecisionId, crate::mir::coverage::ConditionId, crate::mir::coverage::CounterId, diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index ae97ec4810cf6..5da880edcd93e 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/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, ConditionId, CoverageKind, DecisionId, DecisionSpan, + BlockMarkerId, BranchSpan, ConditionMarkerId, CoverageKind, DecisionMarkerId, DecisionSpan, }; use rustc_middle::mir::{self, BasicBlock, UnOp}; use rustc_middle::thir::{ExprId, ExprKind, Thir}; @@ -19,12 +19,12 @@ struct MCDCInfoBuilder { /// 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, + current_decision_id: Option, /// 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 - decision_spans: IndexVec, + decision_spans: IndexVec, next_condition_id: u32, } @@ -52,7 +52,7 @@ impl MCDCInfoBuilder { self.nested_decision_level > 1 } - pub fn current_decision_id(&self) -> Option { + pub fn current_decision_id(&self) -> Option { if self.in_nested_condition() { None } else { self.current_decision_id } } @@ -235,7 +235,7 @@ impl Builder<'_, '_> { .mcdc_info .as_ref() .and_then(|mcdc_info| mcdc_info.current_decision_id()) - .unwrap_or(DecisionId::from_u32(0)), + .unwrap_or(DecisionMarkerId::from_u32(0)), true_marker, false_marker, }); @@ -268,7 +268,7 @@ impl Builder<'_, '_> { let marker_statement = mir::Statement { source_info, kind: mir::StatementKind::Coverage(CoverageKind::MCDCDecisionEntryMarker { - id: decision_id, + decm_id: decision_id, }), }; self.cfg.push(block, marker_statement); @@ -290,21 +290,17 @@ impl Builder<'_, '_> { /// 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 { + pub(crate) fn mcdc_decision_outcome_block(&mut self, bb: BasicBlock, outcome: bool) { // Get the MCDCInfoBuilder object, which existence implies that MCDC is enabled. let Some(BranchInfoBuilder { mcdc_info: Some(mcdc_info), .. }) = self.coverage_branch_info.as_mut() else { - return bb; + return; }; let Some(decision_id) = mcdc_info.current_decision_id() else { // Decision is not instrumented - return bb; + return; }; let span = mcdc_info.decision_spans[decision_id]; @@ -312,21 +308,13 @@ impl Builder<'_, '_> { let marker_statement = mir::Statement { source_info, kind: mir::StatementKind::Coverage(CoverageKind::MCDCDecisionOutputMarker { - id: decision_id, + decm_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); - - new_bb } /// Add markers on the condition's basic blocks to ease the later MCDC instrumentation. @@ -344,13 +332,12 @@ impl Builder<'_, '_> { return; }; - let Some(decision_id) = mcdc_info.current_decision_id() else { + let Some(decm_id) = mcdc_info.current_decision_id() else { // If current_decision_id() is None, the decision is not instrumented. return; }; - - let id = ConditionId::from_u32(mcdc_info.next_condition_id()); + let condm_id = ConditionMarkerId::from_u32(mcdc_info.next_condition_id()); let span = self.thir[condition_expr].span; let source_info = self.source_info(span); @@ -362,15 +349,15 @@ impl Builder<'_, '_> { inject_statement( condition_block, - CoverageKind::MCDCConditionEntryMarker { decision_id, id }, + CoverageKind::MCDCConditionEntryMarker { decm_id, condm_id }, ); inject_statement( then_block, - CoverageKind::MCDCConditionOutputMarker { decision_id, id, outcome: true }, + CoverageKind::MCDCConditionOutputMarker { decm_id, condm_id, outcome: true }, ); inject_statement( else_block, - CoverageKind::MCDCConditionOutputMarker { decision_id, id, outcome: false }, + CoverageKind::MCDCConditionOutputMarker { decm_id, condm_id, outcome: false }, ); } } diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index c6a105c8b1dd3..b7b00cdf903bb 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -92,7 +92,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { )); // If MCDC is enabled, inject an outcome marker. - let then_blk = this.mcdc_decision_outcome_block(then_blk, true); + this.mcdc_decision_outcome_block(then_blk, true); // Lower the `then` arm into its block. this.expr_into_dest(destination, then_blk, then) @@ -101,7 +101,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // 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.mcdc_decision_outcome_block(else_block, false); this.end_mcdc_decision_coverage(); // Pack `(then_block, else_block)` into `BlockAnd`. diff --git a/compiler/rustc_mir_transform/src/coverage/mcdc.rs b/compiler/rustc_mir_transform/src/coverage/mcdc.rs index aa2b5c1f84571..3585b7cb715a8 100644 --- a/compiler/rustc_mir_transform/src/coverage/mcdc.rs +++ b/compiler/rustc_mir_transform/src/coverage/mcdc.rs @@ -1,13 +1,215 @@ +use rustc_data_structures::fx::FxHashMap; +use rustc_index::{bit_set::BitSet, IndexVec}; use rustc_middle::{ - mir::{self, coverage::CoverageKind, Statement}, + mir::{ + self, + coverage::{self, ConditionId, CoverageKind, DecisionId, DecisionMarkerId}, + BasicBlock, StatementKind, + }, ty::TyCtxt, }; -use crate::coverage::graph::CoverageGraph; +use crate::coverage::{graph::CoverageGraph, inject_statement}; use crate::errors::MCDCTooManyConditions; const MAX_COND_DECISION: u32 = 6; +#[allow(dead_code)] + +struct Decisions { + data: IndexVec, + needed_bytes: u32, +} + +impl Decisions { + /// Use MIR markers and THIR extracted data to create the data we need for + /// MCDC instrumentation. + /// + /// Note: MCDC Instrumentation might skip some decisions that contains too + /// may conditions. + pub fn extract_decisions<'tcx>( + tcx: TyCtxt<'tcx>, + branch_info: &coverage::BranchInfo, + mir_body: &mir::Body<'_>, + ) -> Self { + let mut decisions_builder: IndexVec = IndexVec::new(); + let mut decm_id_to_dec_id: FxHashMap = Default::default(); + let mut ignored_decisions: BitSet = BitSet::new_empty( + branch_info.decision_spans.last_index().map(|i| i.as_usize()).unwrap_or(0) + 1, + ); + let mut needed_bytes: u32 = 0; + + // Start by gathering all the decisions. + for (bb, data) in mir_body.basic_blocks.iter_enumerated() { + for statement in &data.statements { + match &statement.kind { + StatementKind::Coverage(CoverageKind::MCDCDecisionEntryMarker { decm_id }) => { + assert!( + !decm_id_to_dec_id.contains_key(decm_id) + && !ignored_decisions.contains(*decm_id), + "Duplicated decision marker id {:?}.", + decm_id + ); + + // Skip uninstrumentable conditions and flag them + // as ignored for the rest of the process. + let dec_span = &branch_info.decision_spans[*decm_id]; + if dec_span.num_conditions > MAX_COND_DECISION { + tcx.dcx().emit_warn(MCDCTooManyConditions { + span: dec_span.span, + num_conditions: dec_span.num_conditions, + max_conditions: MAX_COND_DECISION, + }); + ignored_decisions.insert(*decm_id); + } else { + decm_id_to_dec_id.insert( + *decm_id, + decisions_builder.push(DecisionDataBuilder::new(bb, needed_bytes)), + ); + needed_bytes += 1 << dec_span.num_conditions; + } + } + _ => (), + } + } + } + + for (bb, data) in mir_body.basic_blocks.iter_enumerated() { + for statement in &data.statements { + let StatementKind::Coverage(cov_kind) = &statement.kind else { + continue; + }; + use CoverageKind::*; + match cov_kind { + // Handled above + // MCDCDecisionEntryMarker { decm_id } => { + // } + MCDCDecisionOutputMarker { decm_id, outcome } => { + if let Some(decision_id) = decm_id_to_dec_id.get(decm_id) { + if *outcome { + decisions_builder[*decision_id].set_then_bb(bb); + } else { + decisions_builder[*decision_id].set_else_bb(bb); + } + } else if !ignored_decisions.contains(*decm_id) { + // If id is not in the mapping vector nor in the ignored IDs bitset, + // It means we have not encountered the corresponding DecisionEntryMarker. + bug!( + "Decision output marker {:?} coming before its decision entry marker.", + decm_id + ); + } + } + MCDCConditionEntryMarker { decm_id, condm_id } => { + if let Some(decision_id) = decm_id_to_dec_id.get(decm_id) { + debug!("TODO MCDCConditionEntryMarker"); + let dec_data = &mut decisions_builder[*decision_id]; + dec_data.conditions.push(bb); + } else if !ignored_decisions.contains(*decm_id) { + // If id is not in the mapping vector nor in the ignored IDs bitset, + // It means we have not encountered the corresponding DecisionEntryMarker. + bug!( + "Condition marker {:?} references unknown decision entry marker.", + condm_id + ); + } + } + MCDCConditionOutputMarker { decm_id, condm_id, outcome: _ } => { + if let Some(_decision_id) = decm_id_to_dec_id.get(decm_id) { + debug!("TODO MCDCConditionOutcomeMarker"); + } else if !ignored_decisions.contains(*decm_id) { + // If id is not in the mapping vector nor in the ignored IDs bitset, + // It means we have not encountered the corresponding DecisionEntryMarker. + bug!( + "Condition marker {:?} references unknown decision entry marker.", + condm_id + ); + } + } + _ => (), // Ignore other marker kinds. + } + } + } + + Self { + data: IndexVec::from_iter(decisions_builder.into_iter().map(|b| b.into_done())), + needed_bytes, + } + } +} + +// FIXME(dprn): Remove allow dead code +#[allow(unused)] +struct DecisionData { + entry_bb: BasicBlock, + + /// Decision's offset in the global TV update table. + bitmap_idx: u32, + + conditions: IndexVec, + then_bb: BasicBlock, + else_bb: BasicBlock, +} + +// FIXME(dprn): Remove allow dead code +#[allow(dead_code)] +impl DecisionData { + pub fn new( + entry_bb: BasicBlock, + bitmap_idx: u32, + then_bb: BasicBlock, + else_bb: BasicBlock, + ) -> Self { + Self { entry_bb, bitmap_idx, conditions: IndexVec::new(), then_bb, else_bb } + } +} + +struct DecisionDataBuilder { + entry_bb: BasicBlock, + + /// Decision's offset in the global TV update table. + bitmap_idx: u32, + + conditions: IndexVec, + then_bb: Option, + else_bb: Option, +} + +// FIXME(dprn): Remove allow dead code +#[allow(dead_code)] +impl DecisionDataBuilder { + pub fn new(entry_bb: BasicBlock, bitmap_idx: u32) -> Self { + Self { + entry_bb, + bitmap_idx, + conditions: Default::default(), + then_bb: Default::default(), + else_bb: Default::default(), + } + } + + pub fn set_then_bb(&mut self, then_bb: BasicBlock) -> &mut Self { + self.then_bb = Some(then_bb); + self + } + + pub fn set_else_bb(&mut self, else_bb: BasicBlock) -> &mut Self { + self.else_bb = Some(else_bb); + self + } + + pub fn into_done(self) -> DecisionData { + assert!(!self.conditions.is_empty(), "Empty condition vector"); + + DecisionData { + entry_bb: self.entry_bb, + bitmap_idx: self.bitmap_idx, + conditions: self.conditions, + then_bb: self.then_bb.expect("Missing then_bb"), + else_bb: self.else_bb.expect("Missing else_bb"), + } + } +} /// If MCDC coverage is enabled, add MCDC instrumentation to the function. /// @@ -84,6 +286,7 @@ pub fn instrument_function_mcdc<'tcx>( let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { return; }; + let _decision_data = Decisions::extract_decisions(tcx, branch_info, mir_body); let mut needed_bytes = 0; let mut bitmap_indexes = vec![]; @@ -108,12 +311,7 @@ pub fn instrument_function_mcdc<'tcx>( } // In the first BB, specify that we need to allocate bitmaps. - let entry_bb = &mut mir_body.basic_blocks_mut()[mir::START_BLOCK]; - let mcdc_parameters_statement = Statement { - source_info: entry_bb.terminator().source_info, - kind: mir::StatementKind::Coverage(CoverageKind::MCDCBitmapRequire { needed_bytes }), - }; - entry_bb.statements.insert(0, mcdc_parameters_statement); + inject_statement(mir_body, CoverageKind::MCDCBitmapRequire { needed_bytes }, mir::START_BLOCK); // For each decision: // - Find its 'root' basic block From e00df5d47353ac56def8b0f661107340304eaa05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 3 Apr 2024 09:54:06 +0000 Subject: [PATCH 20/21] mcdc-coverage(codegen): Add MCDCTestVectorBitmapUpdate codegen implementation --- .../src/coverageinfo/mod.rs | 28 ++++++++++++++++++- compiler/rustc_middle/src/mir/coverage.rs | 4 +-- .../src/coverage/spans/from_mir.rs | 2 +- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index ace8c2427dbdc..75765e53461b7 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -221,7 +221,33 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { cond_result, ) } - CoverageKind::MCDCTestBitmapUpdate { needed_bytes: _, decision_index: _ } => todo!(), + CoverageKind::MCDCTestVectorBitmapUpdate { needed_bytes, decision_index } => { + // Drop unused coverage_map, which prevents us to use bx. + drop(coverage_map); + + let fn_name = bx.get_pgo_func_name_var(instance); + let hash = bx.const_u64(function_coverage_info.function_source_hash); + let needed_bytes = bx.const_u32(needed_bytes); + let bitmap_idx = bx.const_u32(decision_index); + let cond_bitmap_addr = match coverage_context + .mcdc_condbitmap_map + .borrow() + .get(&instance) + { + Some(addr) => *addr, + None => bug!( + "Condition bitmap reset instruction was encountered without a condition bitmap being declared." + ), + }; + + bx.instrprof_mcdc_tvbitmap_update( + fn_name, + hash, + needed_bytes, + bitmap_idx, + cond_bitmap_addr, + ) + } } } } diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 52e672f8e4f81..ca9d892de0aac 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -190,7 +190,7 @@ pub enum CoverageKind { MCDCCondBitmapUpdate { condition_id: u32, bool_value: bool }, /// Marks a point where a bit of the global Test Vector bitmap should be set to one. - MCDCTestBitmapUpdate { needed_bytes: u32, decision_index: u32 }, + MCDCTestVectorBitmapUpdate { needed_bytes: u32, decision_index: u32 }, } impl Debug for CoverageKind { @@ -228,7 +228,7 @@ impl Debug for CoverageKind { MCDCCondBitmapUpdate { condition_id, bool_value } => { write!(fmt, "MCDCCondBitmapUpdate({condition_id}, {bool_value})") } - MCDCTestBitmapUpdate { needed_bytes, decision_index } => { + MCDCTestVectorBitmapUpdate { needed_bytes, decision_index } => { write!(fmt, "MCDCTVBitmapUpdate({needed_bytes} bytes, {decision_index})") } } diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index 9e9a195d7fe3c..ab19fd4264751 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -239,7 +239,7 @@ fn filtered_statement_span(statement: &Statement<'_>) -> Option { | CoverageKind::MCDCBitmapRequire { .. } | CoverageKind::MCDCCondBitmapReset | CoverageKind::MCDCCondBitmapUpdate { .. } - | CoverageKind::MCDCTestBitmapUpdate { .. } + | CoverageKind::MCDCTestVectorBitmapUpdate { .. } ) => bug!( "Unexpected coverage statement found during coverage instrumentation: {statement:?}" ), From e112dc8cd6d200d168a148a3fee8a635c670dbb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 3 Apr 2024 10:20:32 +0000 Subject: [PATCH 21/21] mcdc-coverage(mappings): Add variants for MCDCDecision and MCDCBranch in MappingKind and BcbMappingKind --- .../src/coverageinfo/ffi.rs | 35 ++++++++++++++----- compiler/rustc_middle/src/mir/coverage.rs | 12 +++++-- .../rustc_mir_transform/src/coverage/mod.rs | 3 ++ .../rustc_mir_transform/src/coverage/spans.rs | 12 ++++++- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 53ae3448150b4..0e1d6af42820f 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -102,8 +102,6 @@ pub enum RegionKind { /// A DecisionRegion represents a top-level boolean expression and is /// associated with a variable length bitmap index and condition number. - /// FIXME(dprn): Remove unused variables. - #[allow(dead_code)] MCDCDecisionRegion = 5, /// A Branch Region can be extended to include IDs to facilitate MC/DC. @@ -208,6 +206,32 @@ impl CounterMappingRegion { end_col, None, ), + MappingKind::MCDCBranch { true_term, false_term, id, true_id, false_id } => { + Self::branch_region( + Counter::from_term(true_term), + Counter::from_term(false_term), + local_file_id, + start_line, + start_col, + end_line, + end_col, + Some(MCDCParameters { + id, + true_id, + false_id, + .. Default::default() + }), + ) + } + MappingKind::MCDCDecision { bitmap_idx, num_conditions } => Self::decision_region( + bitmap_idx, + num_conditions, + local_file_id, + start_line, + start_col, + end_line, + end_col, + ), } } @@ -263,7 +287,6 @@ impl CounterMappingRegion { } } - #[allow(dead_code)] pub(crate) fn decision_region( bitmap_idx: u32, num_conditions: u32, @@ -283,11 +306,7 @@ impl CounterMappingRegion { end_line, end_col, kind: RegionKind::MCDCDecisionRegion, - mcdc_params: MCDCParameters { - bitmap_idx, - num_conditions, - .. Default::default() - }, + mcdc_params: MCDCParameters { bitmap_idx, num_conditions, ..Default::default() }, } } diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index ca9d892de0aac..1ce7372d94a20 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -287,16 +287,23 @@ pub enum MappingKind { Code(CovTerm), /// Associates a branch region with separate counters for true and false. Branch { true_term: CovTerm, false_term: CovTerm }, + /// FIXME(dprn): doc + MCDCBranch { true_term: CovTerm, false_term: CovTerm, id: u32, true_id: u32, false_id: u32 }, + /// Associates an MCDC Decision with + MCDCDecision { bitmap_idx: u32, num_conditions: u32 }, } impl MappingKind { /// Iterator over all coverage terms in this mapping kind. pub fn terms(&self) -> impl Iterator { - let one = |a| std::iter::once(a).chain(None); - let two = |a, b| std::iter::once(a).chain(Some(b)); + let zero = || std::iter::empty().chain(None).chain(None); + let one = |a| std::iter::empty().chain(Some(a)).chain(None); + let two = |a, b| std::iter::empty().chain(Some(a)).chain(Some(b)); match *self { Self::Code(term) => one(term), Self::Branch { true_term, false_term } => two(true_term, false_term), + Self::MCDCBranch { .. } => zero(), + Self::MCDCDecision { .. } => zero(), } } @@ -308,6 +315,7 @@ impl MappingKind { Self::Branch { true_term, false_term } => { Self::Branch { true_term: map_fn(true_term), false_term: map_fn(false_term) } } + Self::MCDCBranch { .. } | Self::MCDCDecision { .. } => self.clone(), } } } diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 46d55be07a20a..739feeec71ed2 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -150,6 +150,9 @@ fn create_mappings<'tcx>( true_term: term_for_bcb(true_bcb), false_term: term_for_bcb(false_bcb), }, + BcbMappingKind::MCDCDecision { bitmap_idx, num_conditions } => { + MappingKind::MCDCDecision { bitmap_idx, num_conditions } + } }; let code_region = make_code_region(source_map, file_name, span, body_span)?; Some(Mapping { kind, code_region }) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index 672de1fbe60fd..24f3f0efd2f0b 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -9,12 +9,21 @@ use crate::coverage::ExtractedHirInfo; mod from_mir; +// FIXME(dprn): Remove allow dead code +#[allow(dead_code)] #[derive(Clone, Copy, Debug)] pub(super) enum BcbMappingKind { /// Associates an ordinary executable code span with its corresponding BCB. Code(BasicCoverageBlock), /// Associates a branch span with BCBs for its true and false arms. - Branch { true_bcb: BasicCoverageBlock, false_bcb: BasicCoverageBlock }, + Branch { + true_bcb: BasicCoverageBlock, + false_bcb: BasicCoverageBlock, + }, + MCDCDecision { + bitmap_idx: u32, + num_conditions: u32, + }, } #[derive(Debug)] @@ -92,6 +101,7 @@ pub(super) fn generate_coverage_spans( insert(true_bcb); insert(false_bcb); } + BcbMappingKind::MCDCDecision { .. } => (), } }