Auto merge of #74959 - richkadel:llvm-coverage-map-gen-5.1, r=tmandry

Rust function-level coverage now works on external crates

Follow-up to a known issue discussed (post-merge) in #74733:

Resolves a known issue in the coverage map where some regions had nonsensical source code locations.

External crate functions are already included in their own coverage maps, per library, and don't need to also
be added to the importing crate's coverage map. (In fact, their source start and end byte positions are not relevant to the importing crate's SourceMap.)

The fix was to simply skip trying to add imported coverage info to the coverage map if the instrumented function is not "local".

The injected counters are still relevant, however, and the LLVM `instrprof.increment` intrinsic call parameters will map those counters to the external crates' coverage maps, when generating runtime coverage data.

Now Rust Coverage can cleanly instrument and analyze coverage on an entire crate and its dependencies.

Example (instrumenting https://github.com/google/json5format):

```bash
$ ./x.py build rust-demangler  # make sure the demangler is built
$ cd ~/json5format
$ RUSTC=$HOME/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc \
   RUSTFLAGS="-Zinstrument-coverage" \
   cargo build --example formatjson5
$ LLVM_PROFILE_FILE="formatjson5.profraw" \
   ./target/debug/examples/formatjson5 session_manager.cml
$ ~/rust/build/x86_64-unknown-linux-gnu/llvm/bin/llvm-profdata merge \
   -sparse formatjson5.profraw -o formatjson5.profdata
$ ~/rust/build/x86_64-unknown-linux-gnu/llvm/bin/llvm-cov show --use-color \
   --instr-profile=formatjson5.profdata target/debug/examples/formatjson5 \
   --show-line-counts-or-regions  \
   --Xdemangler=$HOME/rust/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-demangler \
   --show-instantiations \
   2>&1 | less -R
```

(Scan forward for some of the non-zero coverage results, with `/^....[0-9]\|  *[^ |0]`.)

<img width="1071" alt="Screen Shot 2020-07-30 at 1 21 01 PM" src="https://user-images.githubusercontent.com/3827298/88970627-97e43000-d267-11ea-8e4d-fe40a091f756.png">

src/librustc_codegen_llvm/intrinsic.rs

@@ -90,64 +90,69 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
         args: &Vec<Operand<'tcx>>,
         caller_instance: ty::Instance<'tcx>,
     ) -> bool {
+        let mut is_codegen_intrinsic = true;
+        // Set `is_codegen_intrinsic` to `false` to bypass `codegen_intrinsic_call()`.
+
         if self.tcx.sess.opts.debugging_opts.instrument_coverage {
-            // Add the coverage information from the MIR to the Codegen context. Some coverage
-            // intrinsics are used only to pass along the coverage information (returns `false`
-            // for `is_codegen_intrinsic()`), but `count_code_region` is also converted into an
-            // LLVM intrinsic to increment a coverage counter.
-            match intrinsic {
-                sym::count_code_region => {
-                    use coverage::count_code_region_args::*;
-                    self.add_counter_region(
-                        caller_instance,
-                        op_to_u64(&args[FUNCTION_SOURCE_HASH]),
-                        op_to_u32(&args[COUNTER_ID]),
-                        op_to_u32(&args[START_BYTE_POS]),
-                        op_to_u32(&args[END_BYTE_POS]),
-                    );
-                    return true; // Also inject the counter increment in the backend
-                }
-                sym::coverage_counter_add | sym::coverage_counter_subtract => {
-                    use coverage::coverage_counter_expression_args::*;
-                    self.add_counter_expression_region(
-                        caller_instance,
-                        op_to_u32(&args[EXPRESSION_ID]),
-                        op_to_u32(&args[LEFT_ID]),
-                        if intrinsic == sym::coverage_counter_add {
-                            ExprKind::Add
-                        } else {
-                            ExprKind::Subtract
-                        },
-                        op_to_u32(&args[RIGHT_ID]),
-                        op_to_u32(&args[START_BYTE_POS]),
-                        op_to_u32(&args[END_BYTE_POS]),
-                    );
-                    return false; // Does not inject backend code
+            // If the intrinsic is from the local MIR, add the coverage information to the Codegen
+            // context, to be encoded into the local crate's coverage map.
+            if caller_instance.def_id().is_local() {
+                // FIXME(richkadel): Make sure to add coverage analysis tests on a crate with
+                // external crate dependencies, where:
+                //   1. Both binary and dependent crates are compiled with `-Zinstrument-coverage`
+                //   2. Only binary is compiled with `-Zinstrument-coverage`
+                //   3. Only dependent crates are compiled with `-Zinstrument-coverage`
+                match intrinsic {
+                    sym::count_code_region => {
+                        use coverage::count_code_region_args::*;
+                        self.add_counter_region(
+                            caller_instance,
+                            op_to_u64(&args[FUNCTION_SOURCE_HASH]),
+                            op_to_u32(&args[COUNTER_ID]),
+                            op_to_u32(&args[START_BYTE_POS]),
+                            op_to_u32(&args[END_BYTE_POS]),
+                        );
+                    }
+                    sym::coverage_counter_add | sym::coverage_counter_subtract => {
+                        use coverage::coverage_counter_expression_args::*;
+                        self.add_counter_expression_region(
+                            caller_instance,
+                            op_to_u32(&args[EXPRESSION_ID]),
+                            op_to_u32(&args[LEFT_ID]),
+                            if intrinsic == sym::coverage_counter_add {
+                                ExprKind::Add
+                            } else {
+                                ExprKind::Subtract
+                            },
+                            op_to_u32(&args[RIGHT_ID]),
+                            op_to_u32(&args[START_BYTE_POS]),
+                            op_to_u32(&args[END_BYTE_POS]),
+                        );
+                    }
+                    sym::coverage_unreachable => {
+                        use coverage::coverage_unreachable_args::*;
+                        self.add_unreachable_region(
+                            caller_instance,
+                            op_to_u32(&args[START_BYTE_POS]),
+                            op_to_u32(&args[END_BYTE_POS]),
+                        );
+                    }
+                    _ => {}
                 }
-                sym::coverage_unreachable => {
-                    use coverage::coverage_unreachable_args::*;
-                    self.add_unreachable_region(
-                        caller_instance,
-                        op_to_u32(&args[START_BYTE_POS]),
-                        op_to_u32(&args[END_BYTE_POS]),
-                    );
-                    return false; // Does not inject backend code
+            }
+
+            // Only the `count_code_region` coverage intrinsic is translated into an actual LLVM
+            // intrinsic call (local or not); otherwise, set `is_codegen_intrinsic` to `false`.
+            match intrinsic {
+                sym::coverage_counter_add
+                | sym::coverage_counter_subtract
+                | sym::coverage_unreachable => {
+                    is_codegen_intrinsic = false;
                 }
                 _ => {}
             }
-        } else {
-            // NOT self.tcx.sess.opts.debugging_opts.instrument_coverage
-            if intrinsic == sym::count_code_region {
-                // An external crate may have been pre-compiled with coverage instrumentation, and
-                // some references from the current crate to the external crate might carry along
-                // the call terminators to coverage intrinsics, like `count_code_region` (for
-                // example, when instantiating a generic function). If the current crate has
-                // `instrument_coverage` disabled, the `count_code_region` call terminators should
-                // be ignored.
-                return false; // Do not inject coverage counters inlined from external crates
-            }
         }
-        true // Unhandled intrinsics should be passed to `codegen_intrinsic_call()`
+        is_codegen_intrinsic
     }
 
     fn codegen_intrinsic_call(

src/librustc_codegen_ssa/coverageinfo/map.rs

@@ -86,7 +86,11 @@ 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);
+        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 }
     }