Rollup merge of #141650 - Zalathar:revert-unused-local-file, r=Zalathar

coverage: Revert "unused local file IDs" due to empty function names

The changes to coverage metadata generation in #140847 appear to be the most likely cause of the `function name is empty` errors reported in #141577.

If that guess is correct, great. If not, no big deal.

---

This reverts commit 3b22c21, reversing changes made to 5f292ee.

r? ghost

Author: matthiaskrgr

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs

@@ -155,20 +155,6 @@ pub(crate) struct Regions {
 impl Regions {
     /// Returns true if none of this structure's tables contain any regions.
     pub(crate) fn has_no_regions(&self) -> bool {
-        // Every region has a span, so if there are no spans then there are no regions.
-        self.all_cov_spans().next().is_none()
-    }
-
-    pub(crate) fn all_cov_spans(&self) -> impl Iterator<Item = &CoverageSpan> {
-        macro_rules! iter_cov_spans {
-            ( $( $regions:expr ),* $(,)? ) => {
-                std::iter::empty()
-                $(
-                    .chain( $regions.iter().map(|region| &region.cov_span) )
-                )*
-            }
-        }
-
         let Self {
             code_regions,
             expansion_regions,
@@ -177,13 +163,11 @@ impl Regions {
             mcdc_decision_regions,
         } = self;
 
-        iter_cov_spans!(
-            code_regions,
-            expansion_regions,
-            branch_regions,
-            mcdc_branch_regions,
-            mcdc_decision_regions,
-        )
+        code_regions.is_empty()
+            && expansion_regions.is_empty()
+            && branch_regions.is_empty()
+            && mcdc_branch_regions.is_empty()
+            && mcdc_decision_regions.is_empty()
     }
 }
 

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

@@ -11,7 +11,6 @@ use rustc_abi::Align;
 use rustc_codegen_ssa::traits::{
     BaseTypeCodegenMethods as _, ConstCodegenMethods, StaticCodegenMethods,
 };
-use rustc_index::IndexVec;
 use rustc_middle::mir::coverage::{
     BasicCoverageBlock, CovTerm, CoverageIdsInfo, Expression, FunctionCoverageInfo, Mapping,
     MappingKind, Op,
@@ -105,16 +104,6 @@ fn fill_region_tables<'tcx>(
     ids_info: &'tcx CoverageIdsInfo,
     covfun: &mut CovfunRecord<'tcx>,
 ) {
-    // If this function is unused, replace all counters with zero.
-    let counter_for_bcb = |bcb: BasicCoverageBlock| -> ffi::Counter {
-        let term = if covfun.is_used {
-            ids_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term")
-        } else {
-            CovTerm::Zero
-        };
-        ffi::Counter::from_term(term)
-    };
-
     // Currently a function's mappings must all be in the same file, so use the
     // first mapping's span to determine the file.
     let source_map = tcx.sess.source_map();
@@ -126,12 +115,6 @@ fn fill_region_tables<'tcx>(
 
     let local_file_id = covfun.virtual_file_mapping.push_file(&source_file);
 
-    // If this testing flag is set, add an extra unused entry to the local
-    // file table, to help test the code for detecting unused file IDs.
-    if tcx.sess.coverage_inject_unused_local_file() {
-        covfun.virtual_file_mapping.push_file(&source_file);
-    }
-
     // In rare cases, _all_ of a function's spans are discarded, and coverage
     // codegen needs to handle that gracefully to avoid #133606.
     // It's hard for tests to trigger this organically, so instead we set
@@ -152,6 +135,16 @@ fn fill_region_tables<'tcx>(
     // For each counter/region pair in this function+file, convert it to a
     // form suitable for FFI.
     for &Mapping { ref kind, span } in &fn_cov_info.mappings {
+        // If this function is unused, replace all counters with zero.
+        let counter_for_bcb = |bcb: BasicCoverageBlock| -> ffi::Counter {
+            let term = if covfun.is_used {
+                ids_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term")
+            } else {
+                CovTerm::Zero
+            };
+            ffi::Counter::from_term(term)
+        };
+
         let Some(coords) = make_coords(span) else { continue };
         let cov_span = coords.make_coverage_span(local_file_id);
 
@@ -184,19 +177,6 @@ fn fill_region_tables<'tcx>(
     }
 }
 
-/// LLVM requires all local file IDs to have at least one mapping region.
-/// If that's not the case, skip this function, to avoid an assertion failure
-/// (or worse) in LLVM.
-fn check_local_file_table(covfun: &CovfunRecord<'_>) -> bool {
-    let mut local_file_id_seen =
-        IndexVec::<u32, _>::from_elem_n(false, covfun.virtual_file_mapping.local_file_table.len());
-    for cov_span in covfun.regions.all_cov_spans() {
-        local_file_id_seen[cov_span.file_id] = true;
-    }
-
-    local_file_id_seen.into_iter().all(|seen| seen)
-}
-
 /// Generates the contents of the covfun record for this function, which
 /// contains the function's coverage mapping data. The record is then stored
 /// as a global variable in the `__llvm_covfun` section.
@@ -205,10 +185,6 @@ pub(crate) fn generate_covfun_record<'tcx>(
     global_file_table: &GlobalFileTable,
     covfun: &CovfunRecord<'tcx>,
 ) {
-    if !check_local_file_table(covfun) {
-        return;
-    }
-
     let &CovfunRecord {
         mangled_function_name,
         source_hash,

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

@@ -39,10 +39,7 @@ impl Coords {
 /// 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.
 pub(crate) fn make_coords(source_map: &SourceMap, file: &SourceFile, span: Span) -> Option<Coords> {
-    if span.is_empty() {
-        debug_assert!(false, "can't make coords from empty span: {span:?}");
-        return None;
-    }
+    let span = ensure_non_empty_span(source_map, span)?;
 
     let lo = span.lo();
     let hi = span.hi();
@@ -73,6 +70,29 @@ pub(crate) fn make_coords(source_map: &SourceMap, file: &SourceFile, span: Span)
     })
 }
 
+fn ensure_non_empty_span(source_map: &SourceMap, span: Span) -> Option<Span> {
+    if !span.is_empty() {
+        return Some(span);
+    }
+
+    // The span is empty, so try to enlarge it to cover an adjacent '{' or '}'.
+    source_map
+        .span_to_source(span, |src, start, end| try {
+            // Adjusting span endpoints by `BytePos(1)` is normally a bug,
+            // but in this case we have specifically checked that the character
+            // we're skipping over is one of two specific ASCII characters, so
+            // adjusting by exactly 1 byte is correct.
+            if src.as_bytes().get(end).copied() == Some(b'{') {
+                Some(span.with_hi(span.hi() + BytePos(1)))
+            } else if start > 0 && src.as_bytes()[start - 1] == b'}' {
+                Some(span.with_lo(span.lo() - BytePos(1)))
+            } else {
+                None
+            }
+        })
+        .ok()?
+}
+
 /// 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

compiler/rustc_interface/src/tests.rs

@@ -776,8 +776,7 @@ fn test_unstable_options_tracking_hash() {
         CoverageOptions {
             level: CoverageLevel::Mcdc,
             no_mir_spans: true,
-            discard_all_spans_in_codegen: true,
-            inject_unused_local_file: true,
+            discard_all_spans_in_codegen: true
         }
     );
     tracked!(crate_attr, vec!["abc".to_string()]);

compiler/rustc_mir_transform/src/coverage/spans.rs

@@ -1,8 +1,7 @@
 use rustc_data_structures::fx::FxHashSet;
 use rustc_middle::mir;
 use rustc_middle::ty::TyCtxt;
-use rustc_span::source_map::SourceMap;
-use rustc_span::{BytePos, DesugaringKind, ExpnKind, MacroKind, Span};
+use rustc_span::{DesugaringKind, ExpnKind, MacroKind, Span};
 use tracing::instrument;
 
 use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph};
@@ -84,18 +83,8 @@ pub(super) fn extract_refined_covspans<'tcx>(
     // Discard any span that overlaps with a hole.
     discard_spans_overlapping_holes(&mut covspans, &holes);
 
-    // Discard spans that overlap in unwanted ways.
+    // Perform more refinement steps after holes have been dealt with.
     let mut covspans = remove_unwanted_overlapping_spans(covspans);
-
-    // For all empty spans, either enlarge them to be non-empty, or discard them.
-    let source_map = tcx.sess.source_map();
-    covspans.retain_mut(|covspan| {
-        let Some(span) = ensure_non_empty_span(source_map, covspan.span) else { return false };
-        covspan.span = span;
-        true
-    });
-
-    // Merge covspans that can be merged.
     covspans.dedup_by(|b, a| a.merge_if_eligible(b));
 
     code_mappings.extend(covspans.into_iter().map(|Covspan { span, bcb }| {
@@ -241,26 +230,3 @@ fn compare_spans(a: Span, b: Span) -> std::cmp::Ordering {
         // - Both have the same start and span A extends further right
         .then_with(|| Ord::cmp(&a.hi(), &b.hi()).reverse())
 }
-
-fn ensure_non_empty_span(source_map: &SourceMap, span: Span) -> Option<Span> {
-    if !span.is_empty() {
-        return Some(span);
-    }
-
-    // The span is empty, so try to enlarge it to cover an adjacent '{' or '}'.
-    source_map
-        .span_to_source(span, |src, start, end| try {
-            // Adjusting span endpoints by `BytePos(1)` is normally a bug,
-            // but in this case we have specifically checked that the character
-            // we're skipping over is one of two specific ASCII characters, so
-            // adjusting by exactly 1 byte is correct.
-            if src.as_bytes().get(end).copied() == Some(b'{') {
-                Some(span.with_hi(span.hi() + BytePos(1)))
-            } else if start > 0 && src.as_bytes()[start - 1] == b'}' {
-                Some(span.with_lo(span.lo() - BytePos(1)))
-            } else {
-                None
-            }
-        })
-        .ok()?
-}

compiler/rustc_session/src/config.rs

@@ -195,11 +195,6 @@ pub struct CoverageOptions {
     /// regression tests for #133606, because we don't have an easy way to
     /// reproduce it from actual source code.
     pub discard_all_spans_in_codegen: bool,
-
-    /// `-Zcoverage-options=inject-unused-local-file`: During codegen, add an
-    /// extra dummy entry to each function's local file table, to exercise the
-    /// code that checks for local file IDs with no mapping regions.
-    pub inject_unused_local_file: bool,
 }
 
 /// Controls whether branch coverage or MC/DC coverage is enabled.

compiler/rustc_session/src/options.rs

@@ -1413,7 +1413,6 @@ pub mod parse {
                 "mcdc" => slot.level = CoverageLevel::Mcdc,
                 "no-mir-spans" => slot.no_mir_spans = true,
                 "discard-all-spans-in-codegen" => slot.discard_all_spans_in_codegen = true,
-                "inject-unused-local-file" => slot.inject_unused_local_file = true,
                 _ => return false,
             }
         }

compiler/rustc_session/src/session.rs

@@ -371,11 +371,6 @@ impl Session {
         self.opts.unstable_opts.coverage_options.discard_all_spans_in_codegen
     }
 
-    /// True if testing flag `-Zcoverage-options=inject-unused-local-file` was passed.
-    pub fn coverage_inject_unused_local_file(&self) -> bool {
-        self.opts.unstable_opts.coverage_options.inject_unused_local_file
-    }
-
     pub fn is_sanitizer_cfi_enabled(&self) -> bool {
         self.opts.unstable_opts.sanitizer.contains(SanitizerSet::CFI)
     }

tests/coverage/async_closure.cov-map

@@ -37,29 +37,32 @@ Number of file 0 mappings: 8
 Highest counter ID seen: c0
 
 Function name: async_closure::main::{closure#0}
-Raw bytes (9): 0x[01, 01, 00, 01, 01, 0b, 22, 00, 24]
+Raw bytes (14): 0x[01, 01, 00, 02, 01, 0b, 22, 00, 23, 01, 00, 23, 00, 24]
 Number of files: 1
 - file 0 => $DIR/async_closure.rs
 Number of expressions: 0
-Number of file 0 mappings: 1
-- Code(Counter(0)) at (prev + 11, 34) to (start + 0, 36)
+Number of file 0 mappings: 2
+- Code(Counter(0)) at (prev + 11, 34) to (start + 0, 35)
+- Code(Counter(0)) at (prev + 0, 35) to (start + 0, 36)
 Highest counter ID seen: c0
 
 Function name: async_closure::main::{closure#0}
-Raw bytes (9): 0x[01, 01, 00, 01, 01, 0b, 22, 00, 24]
+Raw bytes (14): 0x[01, 01, 00, 02, 01, 0b, 22, 00, 23, 01, 00, 23, 00, 24]
 Number of files: 1
 - file 0 => $DIR/async_closure.rs
 Number of expressions: 0
-Number of file 0 mappings: 1
-- Code(Counter(0)) at (prev + 11, 34) to (start + 0, 36)
+Number of file 0 mappings: 2
+- Code(Counter(0)) at (prev + 11, 34) to (start + 0, 35)
+- Code(Counter(0)) at (prev + 0, 35) to (start + 0, 36)
 Highest counter ID seen: c0
 
 Function name: async_closure::main::{closure#0}::{closure#0}::<i16>
-Raw bytes (9): 0x[01, 01, 00, 01, 01, 0b, 22, 00, 24]
+Raw bytes (14): 0x[01, 01, 00, 02, 01, 0b, 22, 00, 23, 01, 00, 23, 00, 24]
 Number of files: 1
 - file 0 => $DIR/async_closure.rs
 Number of expressions: 0
-Number of file 0 mappings: 1
-- Code(Counter(0)) at (prev + 11, 34) to (start + 0, 36)
+Number of file 0 mappings: 2
+- Code(Counter(0)) at (prev + 11, 34) to (start + 0, 35)
+- Code(Counter(0)) at (prev + 0, 35) to (start + 0, 36)
 Highest counter ID seen: c0