Completes support for coverage in external crates

The prior PR corrected for errors encountered when trying to generate
the coverage map on source code inlined from external crates (including
macros and generics) by avoiding adding external DefIds to the coverage
map.

This made it possible to generate a coverage report including external
crates, but the external crate coverage was incomplete (did not include
coverage for the DefIds that were eliminated.

The root issue was that the coverage map was converting Span locations
to source file and locations, using the SourceMap for the current crate,
and this would not work for spans from external crates (compliled with a
different SourceMap).

The solution was to convert the Spans to filename and location during
MIR generation instead, so precompiled external crates would already
have the correct source code locations embedded in their MIR, when
imported into another crate.

3 files changed, 42 insertions, 158 deletions

diff --git a/src/librustc_codegen_ssa/coverageinfo/map.rs b/src/librustc_codegen_ssa/coverageinfo/map.rs
index 72138065a90..7f6841f9daa 100644
--- a/src/librustc_codegen_ssa/coverageinfo/map.rs
+++ b/src/librustc_codegen_ssa/coverageinfo/map.rs
@@ -3,12 +3,8 @@ pub use super::ffi::*;
 use rustc_index::vec::IndexVec;
 use rustc_middle::ty::Instance;
 use rustc_middle::ty::TyCtxt;
-use rustc_span::source_map::{Pos, SourceMap};
-use rustc_span::{BytePos, FileName, Loc, RealFileName};
 
-use std::cmp::{Ord, Ordering};
-use std::fmt;
-use std::path::PathBuf;
+use std::cmp::Ord;
 
 rustc_index::newtype_index! {
     pub struct ExpressionOperandId {
@@ -38,127 +34,35 @@ rustc_index::newtype_index! {
     }
 }
 
-#[derive(Clone, Debug)]
-pub struct Region {
-    start: Loc,
-    end: Loc,
-}
-
-impl Ord for Region {
-    fn cmp(&self, other: &Self) -> Ordering {
-        (&self.start.file.name, &self.start.line, &self.start.col, &self.end.line, &self.end.col)
-            .cmp(&(
-                &other.start.file.name,
-                &other.start.line,
-                &other.start.col,
-                &other.end.line,
-                &other.end.col,
-            ))
-    }
-}
-
-impl PartialOrd for Region {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-impl PartialEq for Region {
-    fn eq(&self, other: &Self) -> bool {
-        self.start.file.name == other.start.file.name
-            && self.start.line == other.start.line
-            && self.start.col == other.start.col
-            && self.end.line == other.end.line
-            && self.end.col == other.end.col
-    }
-}
-
-impl Eq for Region {}
-
-impl fmt::Display for Region {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let (file_path, start_line, start_col, end_line, end_col) = self.file_start_and_end();
-        write!(f, "{:?}:{}:{} - {}:{}", file_path, start_line, start_col, end_line, end_col)
-    }
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
+pub struct Region<'tcx> {
+    pub file_name: &'tcx str,
+    pub start_line: u32,
+    pub start_col: u32,
+    pub end_line: u32,
+    pub end_col: u32,
 }
 
-impl Region {
-    pub fn new(source_map: &SourceMap, start_byte_pos: u32, end_byte_pos: u32) -> Self {
-        let start = source_map.lookup_char_pos(BytePos::from_u32(start_byte_pos));
-        let end = source_map.lookup_char_pos(BytePos::from_u32(end_byte_pos));
-        assert_eq!(
-            start.file.name, end.file.name,
-            "Region start ({} -> {:?}) and end ({} -> {:?}) don't come from the same source file!",
-            start_byte_pos, start, end_byte_pos, end
-        );
-        Self { start, end }
-    }
-
-    pub fn file_start_and_end<'a>(&'a self) -> (&'a PathBuf, u32, u32, u32, u32) {
-        let start = &self.start;
-        let end = &self.end;
-        match &start.file.name {
-            FileName::Real(RealFileName::Named(path)) => (
-                path,
-                start.line as u32,
-                start.col.to_u32() + 1,
-                end.line as u32,
-                end.col.to_u32() + 1,
-            ),
-            _ => {
-                bug!("start.file.name should be a RealFileName, but it was: {:?}", start.file.name)
-            }
-        }
+impl<'tcx> Region<'tcx> {
+    pub fn new(
+        file_name: &'tcx str,
+        start_line: u32,
+        start_col: u32,
+        end_line: u32,
+        end_col: u32,
+    ) -> Self {
+        Self { file_name, start_line, start_col, end_line, end_col }
     }
 }
 
 #[derive(Clone, Debug)]
-pub struct ExpressionRegion {
+pub struct ExpressionRegion<'tcx> {
     lhs: ExpressionOperandId,
     op: ExprKind,
     rhs: ExpressionOperandId,
-    region: Region,
+    region: Region<'tcx>,
 }
 
-// FIXME(richkadel): There seems to be a problem computing the file location in
-// some cases. I need to investigate this more. When I generate and show coverage
-// for the example binary in the crates.io crate `json5format`, I had a couple of
-// notable problems:
-//
-//   1. I saw a lot of coverage spans in `llvm-cov show` highlighting regions in
-//      various comments (not corresponding to rustdoc code), indicating a possible
-//      problem with the byte_pos-to-source-map implementation.
-//
-//   2. And (perhaps not related) when I build the aforementioned example binary with:
-//      `RUST_FLAGS="-Zinstrument-coverage" cargo build --example formatjson5`
-//      and then run that binary with
-//      `LLVM_PROFILE_FILE="formatjson5.profraw" ./target/debug/examples/formatjson5 \
-//      some.json5` for some reason the binary generates *TWO* `.profraw` files. One
-//      named `default.profraw` and the other named `formatjson5.profraw` (the expected
-//      name, in this case).
-//
-//   3. I think that if I eliminate regions within a function, their region_ids,
-//      referenced in expressions, will be wrong? I think the ids are implied by their
-//      array position in the final coverage map output (IIRC).
-//
-//   4. I suspect a problem (if not the only problem) is the SourceMap is wrong for some
-//      region start/end byte positions. Just like I couldn't get the function hash at
-//      intrinsic codegen time for external crate functions, I think the SourceMap I
-//      have here only applies to the local crate, and I know I have coverages that
-//      reference external crates.
-//
-//          I still don't know if I fixed the hash problem correctly. If external crates
-//          implement the function, can't I use the coverage counters already compiled
-//          into those external crates? (Maybe not for generics and/or maybe not for
-//          macros... not sure. But I need to understand this better.)
-//
-// If the byte range conversion is wrong, fix it. But if it
-// is right, then it is possible for the start and end to be in different files.
-// Can I do something other than ignore coverages that span multiple files?
-//
-// If I can resolve this, remove the "Option<>" result type wrapper
-// `regions_in_file_order()` accordingly.
-
 /// Collects all of the coverage regions associated with (a) injected counters, (b) counter
 /// expressions (additions or subtraction), and (c) unreachable regions (always counted as zero),
 /// for a given Function. Counters and counter expressions have non-overlapping `id`s because they
@@ -171,19 +75,17 @@ pub struct ExpressionRegion {
 /// only whitespace or comments). According to LLVM Code Coverage Mapping documentation, "A count
 /// for a gap area is only used as the line execution count if there are no other regions on a
 /// line."
-pub struct FunctionCoverage<'a> {
-    source_map: &'a SourceMap,
+pub struct FunctionCoverage<'tcx> {
     source_hash: u64,
-    counters: IndexVec<CounterValueReference, Option<Region>>,
-    expressions: IndexVec<InjectedExpressionIndex, Option<ExpressionRegion>>,
-    unreachable_regions: Vec<Region>,
+    counters: IndexVec<CounterValueReference, Option<Region<'tcx>>>,
+    expressions: IndexVec<InjectedExpressionIndex, Option<ExpressionRegion<'tcx>>>,
+    unreachable_regions: Vec<Region<'tcx>>,
 }
 
-impl<'a> FunctionCoverage<'a> {
-    pub fn new<'tcx: 'a>(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Self {
+impl<'tcx> FunctionCoverage<'tcx> {
+    pub fn new(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Self {
         let coverageinfo = tcx.coverageinfo(instance.def_id());
         Self {
-            source_map: tcx.sess.source_map(),
             source_hash: 0, // will be set with the first `add_counter()`
             counters: IndexVec::from_elem_n(None, coverageinfo.num_counters as usize),
             expressions: IndexVec::from_elem_n(None, coverageinfo.num_expressions as usize),
@@ -194,20 +96,14 @@ impl<'a> FunctionCoverage<'a> {
     /// Adds a code region to be counted by an injected counter intrinsic.
     /// The source_hash (computed during coverage instrumentation) should also be provided, and
     /// should be the same for all counters in a given function.
-    pub fn add_counter(
-        &mut self,
-        source_hash: u64,
-        id: u32,
-        start_byte_pos: u32,
-        end_byte_pos: u32,
-    ) {
+    pub fn add_counter(&mut self, source_hash: u64, id: u32, region: Region<'tcx>) {
         if self.source_hash == 0 {
             self.source_hash = source_hash;
         } else {
             debug_assert_eq!(source_hash, self.source_hash);
         }
         self.counters[CounterValueReference::from(id)]
-            .replace(Region::new(self.source_map, start_byte_pos, end_byte_pos))
+            .replace(region)
             .expect_none("add_counter called with duplicate `id`");
     }
 
@@ -231,8 +127,7 @@ impl<'a> FunctionCoverage<'a> {
         lhs: u32,
         op: ExprKind,
         rhs: u32,
-        start_byte_pos: u32,
-        end_byte_pos: u32,
+        region: Region<'tcx>,
     ) {
         let expression_id = ExpressionOperandId::from(id_descending_from_max);
         let lhs = ExpressionOperandId::from(lhs);
@@ -240,18 +135,13 @@ impl<'a> FunctionCoverage<'a> {
 
         let expression_index = self.expression_index(expression_id);
         self.expressions[expression_index]
-            .replace(ExpressionRegion {
-                lhs,
-                op,
-                rhs,
-                region: Region::new(self.source_map, start_byte_pos, end_byte_pos),
-            })
+            .replace(ExpressionRegion { lhs, op, rhs, region })
             .expect_none("add_counter_expression called with duplicate `id_descending_from_max`");
     }
 
     /// Add a region that will be marked as "unreachable", with a constant "zero counter".
-    pub fn add_unreachable_region(&mut self, start_byte_pos: u32, end_byte_pos: u32) {
-        self.unreachable_regions.push(Region::new(self.source_map, start_byte_pos, end_byte_pos));
+    pub fn add_unreachable_region(&mut self, region: Region<'tcx>) {
+        self.unreachable_regions.push(region)
     }
 
     /// Return the source hash, generated from the HIR node structure, and used to indicate whether
@@ -264,8 +154,8 @@ impl<'a> FunctionCoverage<'a> {
     /// associated `Regions` (from which the LLVM-specific `CoverageMapGenerator` will create
     /// `CounterMappingRegion`s.
     pub fn get_expressions_and_counter_regions(
-        &'a self,
-    ) -> (Vec<CounterExpression>, impl Iterator<Item = (Counter, &'a Region)>) {
+        &'tcx self,
+    ) -> (Vec<CounterExpression>, impl Iterator<Item = (Counter, &'tcx Region<'tcx>)>) {
         assert!(self.source_hash != 0);
 
         let counter_regions = self.counter_regions();
@@ -277,7 +167,7 @@ impl<'a> FunctionCoverage<'a> {
         (counter_expressions, counter_regions)
     }
 
-    fn counter_regions(&'a self) -> impl Iterator<Item = (Counter, &'a Region)> {
+    fn counter_regions(&'tcx self) -> impl Iterator<Item = (Counter, &'tcx Region<'tcx>)> {
         self.counters.iter_enumerated().filter_map(|(index, entry)| {
             // Option::map() will return None to filter out missing counters. This may happen
             // if, for example, a MIR-instrumented counter is removed during an optimization.
@@ -288,8 +178,8 @@ impl<'a> FunctionCoverage<'a> {
     }
 
     fn expressions_with_regions(
-        &'a self,
-    ) -> (Vec<CounterExpression>, impl Iterator<Item = (Counter, &'a Region)>) {
+        &'tcx self,
+    ) -> (Vec<CounterExpression>, impl Iterator<Item = (Counter, &'tcx Region<'tcx>)>) {
         let mut counter_expressions = Vec::with_capacity(self.expressions.len());
         let mut expression_regions = Vec::with_capacity(self.expressions.len());
         let mut new_indexes =
@@ -350,7 +240,7 @@ impl<'a> FunctionCoverage<'a> {
         (counter_expressions, expression_regions.into_iter())
     }
 
-    fn unreachable_regions(&'a self) -> impl Iterator<Item = (Counter, &'a Region)> {
+    fn unreachable_regions(&'tcx self) -> impl Iterator<Item = (Counter, &'tcx Region<'tcx>)> {
         self.unreachable_regions.iter().map(|region| (Counter::zero(), region))
     }
 
diff --git a/src/librustc_codegen_ssa/coverageinfo/mod.rs b/src/librustc_codegen_ssa/coverageinfo/mod.rs
index 1f0ffd289b1..ff794a75c36 100644
--- a/src/librustc_codegen_ssa/coverageinfo/mod.rs
+++ b/src/librustc_codegen_ssa/coverageinfo/mod.rs
@@ -2,3 +2,4 @@ pub mod ffi;
 pub mod map;
 
 pub use map::ExprKind;
+pub use map::Region;
diff --git a/src/librustc_codegen_ssa/traits/coverageinfo.rs b/src/librustc_codegen_ssa/traits/coverageinfo.rs
index db1d86c974e..2b5878f46bc 100644
--- a/src/librustc_codegen_ssa/traits/coverageinfo.rs
+++ b/src/librustc_codegen_ssa/traits/coverageinfo.rs
@@ -1,5 +1,5 @@
 use super::BackendTypes;
-use crate::coverageinfo::ExprKind;
+use crate::coverageinfo::{ExprKind, Region};
 use rustc_middle::ty::Instance;
 
 pub trait CoverageInfoMethods: BackendTypes {
@@ -12,8 +12,7 @@ pub trait CoverageInfoBuilderMethods<'tcx>: BackendTypes {
         instance: Instance<'tcx>,
         function_source_hash: u64,
         index: u32,
-        start_byte_pos: u32,
-        end_byte_pos: u32,
+        region: Region<'tcx>,
     );
 
     fn add_counter_expression_region(
@@ -23,14 +22,8 @@ pub trait CoverageInfoBuilderMethods<'tcx>: BackendTypes {
         lhs: u32,
         op: ExprKind,
         rhs: u32,
-        start_byte_pos: u32,
-        end_byte_pos: u32,
+        region: Region<'tcx>,
     );
 
-    fn add_unreachable_region(
-        &mut self,
-        instance: Instance<'tcx>,
-        start_byte_pos: u32,
-        end_byte_pos: u32,
-    );
+    fn add_unreachable_region(&mut self, instance: Instance<'tcx>, region: Region<'tcx>);
 }