From f61f34f4b8dbbcee2e0cb793d0f79ac04e4667b7 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 28 Aug 2024 22:01:10 +1000 Subject: [PATCH 1/3] coverage: `CodeRegion` is never stored in an arena This might have been left over when coverage regions were stored in individual MIR statements, instead of a separate table attached to the MIR body. --- compiler/rustc_middle/src/arena.rs | 1 - compiler/rustc_middle/src/ty/codec.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/compiler/rustc_middle/src/arena.rs b/compiler/rustc_middle/src/arena.rs index 37c10b14054c5..7050a06b8dc22 100644 --- a/compiler/rustc_middle/src/arena.rs +++ b/compiler/rustc_middle/src/arena.rs @@ -35,7 +35,6 @@ macro_rules! arena_types { )>, [] crate_for_resolver: rustc_data_structures::steal::Steal<(rustc_ast::Crate, rustc_ast::AttrVec)>, [] resolutions: rustc_middle::ty::ResolverGlobalCtxt, - [decode] code_region: rustc_middle::mir::coverage::CodeRegion, [] const_allocs: rustc_middle::mir::interpret::Allocation, [] region_scope_tree: rustc_middle::middle::region::ScopeTree, // Required for the incremental on-disk cache diff --git a/compiler/rustc_middle/src/ty/codec.rs b/compiler/rustc_middle/src/ty/codec.rs index 401f6da6526aa..46203ee150f64 100644 --- a/compiler/rustc_middle/src/ty/codec.rs +++ b/compiler/rustc_middle/src/ty/codec.rs @@ -462,7 +462,6 @@ impl_decodable_via_ref! { &'tcx traits::ImplSource<'tcx, ()>, &'tcx mir::Body<'tcx>, &'tcx mir::BorrowCheckResult<'tcx>, - &'tcx mir::coverage::CodeRegion, &'tcx ty::List, &'tcx ty::ListWithCachedTypeInfo>, &'tcx ty::List, From 5e162a8f484dcaeeb094d3f7ef5c60c0ec467626 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 28 Aug 2024 22:07:57 +1000 Subject: [PATCH 2/3] coverage: Simplify some debug logging --- compiler/rustc_mir_transform/src/coverage/mod.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 96ca3b43d5c2a..a3ba0deba26f1 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -362,19 +362,13 @@ fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb /// but it's hard to rule out entirely (especially in the presence of complex macros /// or other expansions), and if it does happen then skipping a span or function is /// better than an ICE or `llvm-cov` failure that the user might have no way to avoid. +#[instrument(level = "debug", skip(source_map))] fn make_code_region( source_map: &SourceMap, file_name: Symbol, span: Span, body_span: Span, ) -> Option { - debug!( - "Called make_code_region(file_name={}, span={}, body_span={})", - file_name, - source_map.span_to_diagnostic_string(span), - source_map.span_to_diagnostic_string(body_span) - ); - let lo = span.lo(); let hi = span.hi(); From 46e1b5b6dd4454b52566efa8ddfa800daa20f067 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 28 Aug 2024 18:46:49 +1000 Subject: [PATCH 3/3] coverage: Rename `CodeRegion` to `SourceRegion` LLVM uses the word "code" to refer to a particular kind of coverage mapping. This unrelated usage of the word is confusing, and makes it harder to introduce types whose names correspond to the LLVM classification of coverage kinds. --- .../src/coverageinfo/ffi.rs | 7 ++-- .../src/coverageinfo/map_data.rs | 12 +++--- compiler/rustc_middle/src/mir/coverage.rs | 6 +-- compiler/rustc_middle/src/mir/pretty.rs | 4 +- .../rustc_mir_transform/src/coverage/mod.rs | 40 +++++++++---------- 5 files changed, 35 insertions(+), 34 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index cabdd310d1fc8..d7a4f105f3c41 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -1,5 +1,5 @@ use rustc_middle::mir::coverage::{ - CodeRegion, ConditionInfo, CounterId, CovTerm, DecisionInfo, ExpressionId, MappingKind, + ConditionInfo, CounterId, CovTerm, DecisionInfo, ExpressionId, MappingKind, SourceRegion, }; /// Must match the layout of `LLVMRustCounterKind`. @@ -236,9 +236,10 @@ impl CounterMappingRegion { pub(crate) fn from_mapping( mapping_kind: &MappingKind, local_file_id: u32, - code_region: &CodeRegion, + source_region: &SourceRegion, ) -> Self { - let &CodeRegion { file_name: _, start_line, start_col, end_line, end_col } = code_region; + let &SourceRegion { file_name: _, start_line, start_col, end_line, end_col } = + source_region; match *mapping_kind { MappingKind::Code(term) => Self::code_region( Counter::from_term(term), diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 44eafab6060b3..5ed640b840e76 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -2,8 +2,8 @@ use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxIndexSet; use rustc_index::bit_set::BitSet; use rustc_middle::mir::coverage::{ - CodeRegion, CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping, - MappingKind, Op, + CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping, MappingKind, Op, + SourceRegion, }; use rustc_middle::ty::Instance; use rustc_span::Symbol; @@ -201,7 +201,7 @@ impl<'tcx> FunctionCoverage<'tcx> { /// Returns an iterator over all filenames used by this function's mappings. pub(crate) fn all_file_names(&self) -> impl Iterator + Captures<'_> { - self.function_coverage_info.mappings.iter().map(|mapping| mapping.code_region.file_name) + self.function_coverage_info.mappings.iter().map(|mapping| mapping.source_region.file_name) } /// Convert this function's coverage expression data into a form that can be @@ -230,12 +230,12 @@ impl<'tcx> FunctionCoverage<'tcx> { /// that will be used by `mapgen` when preparing for FFI. pub(crate) fn counter_regions( &self, - ) -> impl Iterator + ExactSizeIterator { + ) -> impl Iterator + ExactSizeIterator { self.function_coverage_info.mappings.iter().map(move |mapping| { - let Mapping { kind, code_region } = mapping; + let Mapping { kind, source_region } = mapping; let kind = kind.map_terms(|term| if self.is_zero_term(term) { CovTerm::Zero } else { term }); - (kind, code_region) + (kind, source_region) }) } diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index b11c523cfe964..bfe2a2c2cb3db 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -163,7 +163,7 @@ impl Debug for CoverageKind { #[derive(Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq, Eq, PartialOrd, Ord)] #[derive(TypeFoldable, TypeVisitable)] -pub struct CodeRegion { +pub struct SourceRegion { pub file_name: Symbol, pub start_line: u32, pub start_col: u32, @@ -171,7 +171,7 @@ pub struct CodeRegion { pub end_col: u32, } -impl Debug for CodeRegion { +impl Debug for SourceRegion { fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result { write!( fmt, @@ -242,7 +242,7 @@ impl MappingKind { #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub struct Mapping { pub kind: MappingKind, - pub code_region: CodeRegion, + pub source_region: SourceRegion, } /// Stores per-function coverage information attached to a `mir::Body`, diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index efdc0c710ba7e..6785805c27df3 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -547,8 +547,8 @@ fn write_function_coverage_info( for (id, expression) in expressions.iter_enumerated() { writeln!(w, "{INDENT}coverage {id:?} => {expression:?};")?; } - for coverage::Mapping { kind, code_region } in mappings { - writeln!(w, "{INDENT}coverage {kind:?} => {code_region:?};")?; + for coverage::Mapping { kind, source_region } in mappings { + writeln!(w, "{INDENT}coverage {kind:?} => {source_region:?};")?; } writeln!(w)?; diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index a3ba0deba26f1..af0f1e38a754e 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -13,7 +13,7 @@ use rustc_hir::intravisit::{walk_expr, Visitor}; use rustc_middle::hir::map::Map; use rustc_middle::hir::nested_filter; use rustc_middle::mir::coverage::{ - CodeRegion, CoverageKind, DecisionInfo, FunctionCoverageInfo, Mapping, MappingKind, + CoverageKind, DecisionInfo, FunctionCoverageInfo, Mapping, MappingKind, SourceRegion, }; use rustc_middle::mir::{ self, BasicBlock, BasicBlockData, SourceInfo, Statement, StatementKind, Terminator, @@ -159,7 +159,7 @@ fn create_mappings<'tcx>( .expect("all BCBs with spans were given counters") .as_term() }; - let region_for_span = |span: Span| make_code_region(source_map, file_name, span, body_span); + let region_for_span = |span: Span| make_source_region(source_map, file_name, span, body_span); // Fully destructure the mappings struct to make sure we don't miss any kinds. let ExtractedMappings { @@ -175,9 +175,9 @@ fn create_mappings<'tcx>( mappings.extend(code_mappings.iter().filter_map( // Ordinary code mappings are the simplest kind. |&mappings::CodeMapping { span, bcb }| { - let code_region = region_for_span(span)?; + let source_region = region_for_span(span)?; let kind = MappingKind::Code(term_for_bcb(bcb)); - Some(Mapping { kind, code_region }) + Some(Mapping { kind, source_region }) }, )); @@ -186,29 +186,29 @@ fn create_mappings<'tcx>( let true_term = term_for_bcb(true_bcb); let false_term = term_for_bcb(false_bcb); let kind = MappingKind::Branch { true_term, false_term }; - let code_region = region_for_span(span)?; - Some(Mapping { kind, code_region }) + let source_region = region_for_span(span)?; + Some(Mapping { kind, source_region }) }, )); mappings.extend(mcdc_branches.iter().filter_map( |&mappings::MCDCBranch { span, true_bcb, false_bcb, condition_info, decision_depth: _ }| { - let code_region = region_for_span(span)?; + let source_region = region_for_span(span)?; let true_term = term_for_bcb(true_bcb); let false_term = term_for_bcb(false_bcb); let kind = match condition_info { Some(mcdc_params) => MappingKind::MCDCBranch { true_term, false_term, mcdc_params }, None => MappingKind::Branch { true_term, false_term }, }; - Some(Mapping { kind, code_region }) + Some(Mapping { kind, source_region }) }, )); mappings.extend(mcdc_decisions.iter().filter_map( |&mappings::MCDCDecision { span, bitmap_idx, num_conditions, .. }| { - let code_region = region_for_span(span)?; + let source_region = region_for_span(span)?; let kind = MappingKind::MCDCDecision(DecisionInfo { bitmap_idx, num_conditions }); - Some(Mapping { kind, code_region }) + Some(Mapping { kind, source_region }) }, )); @@ -363,12 +363,12 @@ fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb /// or other expansions), and if it does happen then skipping a span or function is /// better than an ICE or `llvm-cov` failure that the user might have no way to avoid. #[instrument(level = "debug", skip(source_map))] -fn make_code_region( +fn make_source_region( source_map: &SourceMap, file_name: Symbol, span: Span, body_span: Span, -) -> Option { +) -> Option { let lo = span.lo(); let hi = span.hi(); @@ -418,7 +418,7 @@ fn make_code_region( start_line = source_map.doctest_offset_line(&file.name, start_line); end_line = source_map.doctest_offset_line(&file.name, end_line); - check_code_region(CodeRegion { + check_source_region(SourceRegion { file_name, start_line: start_line as u32, start_col: start_col as u32, @@ -427,12 +427,12 @@ fn make_code_region( }) } -/// If `llvm-cov` sees a code region that is improperly ordered (end < start), +/// If `llvm-cov` sees a source region that is improperly ordered (end < start), /// it will immediately exit with a fatal error. To prevent that from happening, /// discard regions that are improperly ordered, or might be interpreted in a /// way that makes them improperly ordered. -fn check_code_region(code_region: CodeRegion) -> Option { - let CodeRegion { file_name: _, start_line, start_col, end_line, end_col } = code_region; +fn check_source_region(source_region: SourceRegion) -> Option { + let SourceRegion { file_name: _, start_line, start_col, end_line, end_col } = source_region; // Line/column coordinates are supposed to be 1-based. If we ever emit // coordinates of 0, `llvm-cov` might misinterpret them. @@ -445,17 +445,17 @@ fn check_code_region(code_region: CodeRegion) -> Option { let is_ordered = (start_line, start_col) <= (end_line, end_col); if all_nonzero && end_col_has_high_bit_unset && is_ordered { - Some(code_region) + Some(source_region) } else { debug!( - ?code_region, + ?source_region, ?all_nonzero, ?end_col_has_high_bit_unset, ?is_ordered, - "Skipping code region that would be misinterpreted or rejected by LLVM" + "Skipping source region that would be misinterpreted or rejected by LLVM" ); // If this happens in a debug build, ICE to make it easier to notice. - debug_assert!(false, "Improper code region: {code_region:?}"); + debug_assert!(false, "Improper source region: {source_region:?}"); None } }