Rust function-level coverage now works on external crates

[richkadel]

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):

$ ./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].)

[rust-highfive]

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

[richkadel]

r? @tmandry

[richkadel]

@wesleywiser FYI, here's the fix.

[richkadel]

@wesleywiser @tmandry As you can see in the example below, the external crate coverage maps are working, even for generics and inlined functions.

My explanation in the initial PR comment is, to some degree, my "theory" on how this is working, but to be honest, I'm not entirely sure how counters called from monomorphized generics with types in an importing crate would correctly map the counter data to the right generic function in the dependent crate's coverage map.

It might not. And if not, then some coverage results are missing.

So, as discussed, I need to put together some more targeted tests to confirm what is or is not working with regard to cross-crate inline function instantiations. Then, if anything is missing, I'll work on implementing a resolution.

/usr/local/google/home/richkadel/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/bitflags-1.2.1/src/lib.rs:
.
.
.
  670|       |            __fn_bitflags! {
  671|       |                /// Returns `true` all of the flags in `other` are contained within `self`.
  672|       |                #[inline]
  673|     94|                pub const fn contains(&self, other: $BitFlags) -> bool {
  674|     94|                    (self.bits & other.bits) == other.bits
  675|     94|                }
  ------------------
  | <clap[521a2cb83d8b579f]::app::settings::Flags>::contains:
  |  673|     34|                pub const fn contains(&self, other: $BitFlags) -> bool {
  |  674|     34|                    (self.bits & other.bits) == other.bits
  |  675|     34|                }
  ------------------
  | <clap[521a2cb83d8b579f]::args::settings::Flags>::contains:
  |  673|     60|                pub const fn contains(&self, other: $BitFlags) -> bool {
  |  674|     60|                    (self.bits & other.bits) == other.bits
  |  675|     60|                }
  ------------------
  676|       |            }
  677|       |
  678|       |            /// Inserts the specified flags in-place.
  679|       |            #[inline]
  680|      7|            pub fn insert(&mut self, other: $BitFlags) {
  681|      7|                self.bits |= other.bits;
  682|      7|            }
  ------------------
  | <clap[521a2cb83d8b579f]::app::settings::Flags>::insert:
  |  680|      3|            pub fn insert(&mut self, other: $BitFlags) {
  |  681|      3|                self.bits |= other.bits;
  |  682|      3|            }
  ------------------
  | <clap[521a2cb83d8b579f]::args::settings::Flags>::insert:
  |  680|      4|            pub fn insert(&mut self, other: $BitFlags) {
  |  681|      4|                self.bits |= other.bits;
  |  682|      4|            }
  ------------------
  683|       |
  684|       |            /// Removes the specified flags in-place.
  685|       |            #[inline]
  686|     11|            pub fn remove(&mut self, other: $BitFlags) {
  687|     11|                self.bits &= !other.bits;
  688|     11|            }
  ------------------
  | <clap[521a2cb83d8b579f]::app::settings::Flags>::remove:
  |  686|      1|            pub fn remove(&mut self, other: $BitFlags) {
  |  687|      1|                self.bits &= !other.bits;
  |  688|      1|            }
  ------------------
  | <clap[521a2cb83d8b579f]::args::settings::Flags>::remove:
  |  686|     10|            pub fn remove(&mut self, other: $BitFlags) {
  |  687|     10|                self.bits &= !other.bits;
  |  688|     10|            }
  ------------------
  689|       |
  690|       |            /// Toggles the specified flags in-place.
  691|       |            #[inline]
  692|      0|            pub fn toggle(&mut self, other: $BitFlags) {
  693|      0|                self.bits ^= other.bits;
  694|      0|            }
  ------------------

[tmandry]

@bors r+ rollup

[bors]

📌 Commit 34b26d6 has been approved by tmandry

[bors]

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

[richkadel]

to be honest, I'm not entirely sure how counters called from monomorphized generics with types in an importing crate would correctly map the counter data to the right generic function in the dependent crate's coverage map.

It might not. And if not, then some coverage results are missing.

FYI, I scanned my results for specific examples of external crate generic functions showing results with different type parameters, and I wasn't able to find any good, clear examples.

The results are too big, using my example off-the-shell crate, so it's hard to be sure of anything, but I will prioritize putting some more targeted tests together for this situation.

In any case, the fix in this PR is still a win, since it allows running coverage instrumentation on an entire crate with dependencies, and produces clean results, for what we're able to instrument and map today.

[richkadel]

@tmandry @wesleywiser - Building on what I've learned here, I think I have a more complete solution.

I currently take only the BytePos start and end positions from the Span during coverage injection, and I don't convert them to actual filename and file-specific location until codegen (requiring the SourceMap, which I only have for the local crate).

I can change the Rust intrinsic parameter arguments, from just the start and end byte (relative to the current SourceMap) to the actual filename, start line and col, and end line and col, and inject those values into the Call terminator, which will survive MIR export/import. I'll be able to include those locations in the coverage map after all, rather than skipping them.

[bors]

⌛ Testing commit 34b26d6 with merge ac91673...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: tmandry
Pushing ac91673 to master...