Make CodeRegions optional for Counters too

I relized the "hack" with GapRegions was completely unnecessary. It is
possible to inject counters (`llvm.instrprof.increment` intrinsic calls
without corresponding code regions in the coverage map. An expression
can still uses these counter values, solving the problem I thought I
needed GapRegions for.

Author: richkadel

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -126,33 +126,15 @@ impl CoverageMapGenerator {
                 let (filenames_index, _) = self.filenames.insert_full(c_filename);
                 virtual_file_mapping.push(filenames_index as u32);
             }
-            debug!("Adding counter {:?} to map for {:?}", counter, region,);
-            if start_line == end_line && start_col == end_col {
-                // The MIR `InstrumentCoverage` pass generates empty spans _ONLY_ for `BasicBlocks`
-                // that contribute to the counts of `CoverageKind::Expression`s, but don't represent
-                // specific source code to count.
-                //
-                // Make empty spans `GapRegion`s so their region executions counts are still
-                // available to any expressions that might reference them, but they don't affect
-                // the line execution count (as long as the line has at least one other counter).
-                mapping_regions.push(CounterMappingRegion::gap_region(
-                    counter,
-                    current_file_id,
-                    start_line,
-                    start_col,
-                    end_line,
-                    end_col,
-                ));
-            } else {
-                mapping_regions.push(CounterMappingRegion::code_region(
-                    counter,
-                    current_file_id,
-                    start_line,
-                    start_col,
-                    end_line,
-                    end_col,
-                ));
-            }
+            debug!("Adding counter {:?} to map for {:?}", counter, region);
+            mapping_regions.push(CounterMappingRegion::code_region(
+                counter,
+                current_file_id,
+                start_line,
+                start_col,
+                end_line,
+                end_col,
+            ));
         }
 
         // Encode and append the current function's coverage mapping data

compiler/rustc_codegen_ssa/src/coverageinfo/map.rs

@@ -149,12 +149,10 @@ impl FunctionCoverage {
                 if id == ExpressionOperandId::ZERO {
                     Some(Counter::zero())
                 } else if id.index() < self.counters.len() {
+                    // Note: Some codegen-injected Counters may be only referenced by `Expression`s,
+                    // and may not have their own `CodeRegion`s,
                     let index = CounterValueReference::from(id.index());
-                    self.counters
-                        .get(index)
-                        .expect("counter id is out of range")
-                        .as_ref()
-                        .map(|_| Counter::counter_value_reference(index))
+                    Some(Counter::counter_value_reference(index))
                 } else {
                     let index = self.expression_index(u32::from(id));
                     self.expressions

compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs

@@ -10,24 +10,23 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         let Coverage { kind, code_region } = coverage;
         match kind {
             CoverageKind::Counter { function_source_hash, id } => {
-                bx.add_coverage_counter(
-                    self.instance,
-                    function_source_hash,
-                    id,
-                    code_region.expect("counters always have code regions"),
-                );
-
                 let coverageinfo = bx.tcx().coverageinfo(self.instance.def_id());
 
                 let fn_name = bx.create_pgo_func_name_var(self.instance);
                 let hash = bx.const_u64(function_source_hash);
                 let num_counters = bx.const_u32(coverageinfo.num_counters);
-                let id = bx.const_u32(u32::from(id));
+                let index = bx.const_u32(u32::from(id));
                 debug!(
                     "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})",
-                    fn_name, hash, num_counters, id,
+                    fn_name, hash, num_counters, index,
                 );
-                bx.instrprof_increment(fn_name, hash, num_counters, id);
+                bx.instrprof_increment(fn_name, hash, num_counters, index);
+
+                if let Some(code_region) = code_region {
+                    bx.add_coverage_counter(self.instance, function_source_hash, id, code_region);
+                }
+                // Note: Some counters do not have code regions, but may still be referenced from
+                // expressions.
             }
             CoverageKind::Expression { id, lhs, op, rhs } => {
                 bx.add_coverage_counter_expression(self.instance, id, lhs, op, rhs, code_region);

compiler/rustc_mir/src/transform/instrument_coverage.rs

@@ -1244,12 +1244,6 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
             } else {
                 bug!("Every BasicCoverageBlock should have a Counter or Expression");
             };
-            debug!(
-                "Injecting {} at: {:?}:\n{}\n==========",
-                self.format_counter(&counter_kind),
-                span,
-                source_map.span_to_snippet(span).expect("Error getting source for span"),
-            );
             if let Some(bcb_to_coverage_spans_with_counters) =
                 debug_bcb_to_coverage_spans_with_counters.as_mut()
             {
@@ -1258,25 +1252,12 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
                     .or_insert_with(|| Vec::new())
                     .push((covspan.clone(), counter_kind.clone()));
             }
-            let mut code_region = None;
-            if span.hi() == body_span.hi() {
-                // All functions execute a `Return`-terminated `BasicBlock`, regardless of how the
-                // function returns; but only some functions also _can_ return after a `Goto` block
-                // that ends on the closing brace of the function (with the `Return`). When this
-                // happens, the last character is counted 2 (or possibly more) times, when we know
-                // the function returned only once (of course). By giving all `Goto` terminators at
-                // the end of a function a `non-reportable` code region, they are still counted
-                // if appropriate, but they don't increment the line counter, as long as their is
-                // also a `Return` on that last line.
-                if let TerminatorKind::Goto { .. } = self.bcb_terminator(bcb).kind {
-                    code_region =
-                        Some(make_non_reportable_code_region(file_name, &source_file, span));
-                }
-            }
-            if code_region.is_none() {
-                code_region = Some(make_code_region(file_name, &source_file, span, body_span));
+            let some_code_region = if self.is_code_region_redundant(bcb, span, body_span) {
+                None
+            } else {
+                Some(make_code_region(file_name, &source_file, span, body_span))
             };
-            self.inject_statement(counter_kind, self.bcb_last_bb(bcb), code_region.unwrap());
+            self.inject_statement(counter_kind, self.bcb_last_bb(bcb), some_code_region);
         }
 
         // The previous step looped through the `CoverageSpan`s and injected the counter from the
@@ -1423,18 +1404,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
                         target_bb
                     };
 
-                    let span = self.mir_body[inject_to_bb].terminator().source_info.span;
-                    debug!(
-                        "make_non_reportable_code_region for {:?} at span={:?}, counter={}",
-                        inject_to_bb,
-                        span,
-                        self.format_counter(&counter_kind)
-                    );
-                    self.inject_statement(
-                        counter_kind,
-                        inject_to_bb,
-                        make_non_reportable_code_region(file_name, &source_file, span),
-                    );
+                    self.inject_statement(counter_kind, inject_to_bb, None);
                 }
                 CoverageKind::Expression { .. } => {
                     self.inject_intermediate_expression(counter_kind)
@@ -1583,6 +1553,28 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
         }
     }
 
+    fn is_code_region_redundant(
+        &self,
+        bcb: BasicCoverageBlock,
+        span: Span,
+        body_span: Span,
+    ) -> bool {
+        if span.hi() == body_span.hi() {
+            // All functions execute a `Return`-terminated `BasicBlock`, regardless of how the
+            // function returns; but only some functions also _can_ return after a `Goto` block
+            // that ends on the closing brace of the function (with the `Return`). When this
+            // happens, the last character is counted 2 (or possibly more) times, when we know
+            // the function returned only once (of course). By giving all `Goto` terminators at
+            // the end of a function a `non-reportable` code region, they are still counted
+            // if appropriate, but they don't increment the line counter, as long as their is
+            // also a `Return` on that last line.
+            if let TerminatorKind::Goto { .. } = self.bcb_terminator(bcb).kind {
+                return true;
+            }
+        }
+        false
+    }
+
     /// Traverse the BCB CFG and add either a `Counter` or `Expression` to ever BCB, to be
     /// injected with `CoverageSpan`s. `Expressions` have no runtime overhead, so if a viable
     /// expression (adding or subtracting two other counters or expressions) can compute the same
@@ -2139,16 +2131,19 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
         &mut self,
         counter_kind: CoverageKind,
         bb: BasicBlock,
-        code_region: CodeRegion,
+        some_code_region: Option<CodeRegion>,
     ) {
-        debug!("  injecting statement {:?} covering {:?}", counter_kind, code_region);
+        debug!(
+            "  injecting statement {:?} for {:?} at code region: {:?}",
+            counter_kind, bb, some_code_region
+        );
         let data = &mut self.mir_body[bb];
         let source_info = data.terminator().source_info;
         let statement = Statement {
             source_info,
             kind: StatementKind::Coverage(box Coverage {
                 kind: counter_kind,
-                code_region: Some(code_region),
+                code_region: some_code_region,
             }),
         };
         data.statements.push(statement);
@@ -2762,8 +2757,8 @@ fn filtered_terminator_span(terminator: &'a Terminator<'tcx>, body_span: Span) -
         //
         // However, in other cases, a visible `CoverageSpan` is not wanted, but the `Goto`
         // block must still be counted (for example, to contribute its count to an `Expression`
-        // that reports the execution count for some other block). The code region for the `Goto`
-        // counters, in these cases, is created using `make_non_reportable_code_region()`.
+        // that reports the execution count for some other block). In these cases, the code region
+        // is set to `None`.
         TerminatorKind::Goto { .. } => {
             Some(function_source_span(terminator.source_info.span.shrink_to_hi(), body_span))
         }
@@ -2788,35 +2783,6 @@ fn function_source_span(span: Span, body_span: Span) -> Span {
     if body_span.contains(span) { span } else { body_span }
 }
 
-/// Make a non-reportable code region. (Used coverage-generated "edge counters", and for some
-/// `Goto`-terminated blocks.)
-///
-/// Only the `Span`s `hi()` position is used, and an empty `Span` at that location is generated,
-/// for coverage counting.
-///
-/// The coverage map generation phase (part of codegen) knows to report any `Counter` or
-/// `Expression` with an empty span as what LLVM's Coverage Mapping Specification calls a
-/// `GapRegion`, rather than a `CodeRegion` (used in all other cases). Counters associated with a
-/// `GapRegion` still contribute their counter values to other expressions, but if the `GapRegion`s
-/// line has any other `CodeRegion` covering the same line, the `GapRegion` will not be counted
-/// toward the line execution count. This prevents double-counting line execution, which would
-/// otherwise occur.
-fn make_non_reportable_code_region(
-    file_name: Symbol,
-    source_file: &Lrc<SourceFile>,
-    span: Span,
-) -> CodeRegion {
-    let (start_line, start_col) = source_file.lookup_file_pos(span.hi());
-    let (end_line, end_col) = (start_line, start_col);
-    CodeRegion {
-        file_name,
-        start_line: start_line as u32,
-        start_col: start_col.to_u32() + 1,
-        end_line: end_line as u32,
-        end_col: end_col.to_u32() + 1,
-    }
-}
-
 /// Convert the Span into its file name, start line and column, and end line and column
 fn make_code_region(
     file_name: Symbol,

src/test/run-make-fulldeps/coverage-reports-base/expected_export_coverage.lazy_boolean.json

@@ -17,8 +17,8 @@
             },
             "lines": {
               "count": 40,
-              "covered": 29,
-              "percent": 72.5
+              "covered": 30,
+              "percent": 75
             },
             "regions": {
               "count": 37,
@@ -42,8 +42,8 @@
         },
         "lines": {
           "count": 40,
-          "covered": 29,
-          "percent": 72.5
+          "covered": 30,
+          "percent": 75
         },
         "regions": {
           "count": 37,

src/test/run-make-fulldeps/coverage-reports-base/expected_export_coverage.various_conditions.json

@@ -17,8 +17,8 @@
             },
             "lines": {
               "count": 49,
-              "covered": 24,
-              "percent": 48.97959183673469
+              "covered": 23,
+              "percent": 46.93877551020408
             },
             "regions": {
               "count": 69,
@@ -42,8 +42,8 @@
         },
         "lines": {
           "count": 49,
-          "covered": 24,
-          "percent": 48.97959183673469
+          "covered": 23,
+          "percent": 46.93877551020408
         },
         "regions": {
           "count": 69,

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.lazy_boolean.txt

@@ -18,7 +18,8 @@
    17|       |        =
    18|      1|            a < b
    19|       |        ||
-   20|      0|            b < c
+   20|      1|            b < c
+                          ^0
    21|       |    ;
    22|       |    let
    23|      1|        somebool

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.various_conditions.txt

@@ -27,8 +27,7 @@
    27|       |
    28|      1|    if countdown > 7 {
    29|      1|        countdown -= 4;
-   30|      1|    } else if countdown > 2 {
-                            ^0
+   30|      0|    } else if countdown > 2 {
    31|      0|        if countdown < 1 || countdown > 5 || countdown != 9 {
    32|      0|            countdown = 0;
    33|      0|        }