Generating the coverage map

[richkadel]

@tmandry @wesleywiser

rustc now generates the coverage map and can support (limited)
coverage report generation, at the function level.

Example commands to generate a coverage report:

$ BUILD=$HOME/rust/build/x86_64-unknown-linux-gnu
$ $BUILD/stage1/bin/rustc -Zinstrument-coverage \
$HOME/rust/src/test/run-make-fulldeps/instrument-coverage/main.rs
$ LLVM_PROFILE_FILE="main.profraw" ./main
called
$ $BUILD/llvm/bin/llvm-profdata merge -sparse main.profraw -o main.profdata
$ $BUILD/llvm/bin/llvm-cov show --instr-profile=main.profdata main

r? @wesleywiser

Rust compiler MCP rust-lang/compiler-team#278
Relevant issue: #34701 - Implement support for LLVMs code coverage instrumentation

[richkadel]

@tmandry @wesleywiser - I did my own detailed review of the output and tests and made a few important, small improvements to this PR.

I don't expect to make any more changes until I get feedback. Thanks!

[richkadel]

I noticed a mistake in the virtual_file_mapping creation and fixed it.

FYI, I also made a temporary change to src/ci/azure-pipelines/try.yml to enable testing on macos and windows in bors try tests. I'd like to try this PR to make sure there aren't any platform-specific issues before someone tries it in a rollup.

Thanks!

[wesleywiser]

Sorry for the delay, it's been a busy few days for me. I'm starting the review so let's go ahead and do

@bors try

[bors]

⌛ Trying commit 08b006f81ef8377ae1b121f3dfbef22e141f5201 with merge 31e6847482c04e9b90e2b2a8abf75c59af671f5f...

[richkadel]

@wesleywiser @tmandry - FYI, until now, I have only tested on very small, simple programs.

Just today I started trying -Zinstrument-coverage on larger crates, and there are some problems. Not really surprising.

I propose landing the current version--with review comments resolved, but otherwise, as-is--since we already recognize this is not "complete" yet.

But (your call), if you prefer, I can make more changes to this PR to get the function-level coverage working on more complex crates.

Just so you know, here are two problems I've found so far, when attempting to build the json5format crate:

1. When I try to instrument a complete package, including dependencies (via RUSTFLAGS=-Zinstrument-coverage cargo build), and need to call the query tcx.coverageinfo(def_id) from codegen_intrinsic_call(), injecting the LLVM instrprof.increment intrinsic fails for dependent crates because tcx.hir().get_if_local(def_id) returns None (i.e., the def_id is not "local" in the current type context (not found in it's hir::map::Map). I'm not sure how I should resolve this yet. (I'm also not sure, yet, if this is failing on all dependent crate functions, or possibly for functions pulled in from one crate to another -- wild guess based on the first crate that failed like this.)

   FYI, I worked around this problem (so far, at least) by limiting the instrumentation to the main crate (not dependencies) only, using cargo rust instead.

2. I'm still not computing the virtual_file_mapping array correctly when building coverage data for a module with functions in more than one file. This shouldn't be hard to resolve. I just misinterpreted the format spec (which is a little vague on this point, and the examples only have one file).

And just so you are aware, I know the CounterExpression indexes are wrong, but this is by design. The indexes for CounterExpressions need to be recomputed (which also affects the expression operands themselves). This should be a very simple translation, and I know how I plan to do this, but I want to validate the solution before checking it in, but I don't (yet) produce any actual Counter Expressions expressions to validate against.

[richkadel]

I propose landing the current version--with review comments resolved, but otherwise, as-is--since we already recognize this is not "complete" yet.

But (your call), if you prefer, I can make more changes to this PR to get the function-level coverage working on more complex crates.

Oh, and if I do land this PR "as-is", my next PR would resolve the known issues with larger crates.

[tmandry]

🎉

cc @petrhosek who I think would be interested in taking a look.

[tmandry]

1. When I try to instrument a complete package, including dependencies (via RUSTFLAGS=-Zinstrument-coverage cargo build), and need to call the query tcx.coverageinfo(def_id) from codegen_intrinsic_call(), injecting the LLVM instrprof.increment intrinsic fails for dependent crates because tcx.hir().get_if_local(def_id) returns None (i.e., the def_id is not "local" in the current type context (not found in it's hir::map::Map). I'm not sure how I should resolve this yet. (I'm also not sure, yet, if this is failing on all dependent crate functions, or possibly for functions pulled in from one crate to another -- wild guess based on the first crate that failed like this.)

Yeah, computing the hash on HIR will fail when codegenning functions from another crate (either #[inline] or generic functions being monomorphized by the current crate). All that's available for those functions is their signature and MIR, roughly.

We'll have to find another way to compute a somewhat-stable hash, or include it in crate metadata.

In terms of whether to do it now, it seems like an issue from a past PR and this is an experimental feature, so I'd say it's okay to leave as-is and fix in a follow up PR.

[bors]

💔 Test failed - checks-azure

[richkadel]

💔 Test failed - checks-azure

Good thing we "try"d !

MacOS and Windows both failed, but earlier than I expected. I'll need to resolve these issues and we'll need to "try" again I think.

[richkadel]

Yeah, computing the hash on HIR will fail when codegenning functions from another crate (either #[inline] or generic functions being monomorphized by the current crate). All that's available for those functions is their signature and MIR, roughly.

In that case, I should be able to work around that limitation by adding the hash to the Rust coverage intrinsics injected into the MIR.

In terms of whether to do it now, it seems like an issue from a past PR and this is an experimental feature, so I'd say it's okay to leave as-is and fix in a follow up PR.

SGTM, if ok with Wesley too. Thanks!

[wesleywiser]

Noting this so we don't forget it.

[wesleywiser]

Maybe assert!() or debug_assert!() that these files are the same?

[richkadel]

@wesleywiser Yes, thanks! So I added this check, and surprisingly saw some cases (during ad hoc testing) where the assert failed. I was getting different filenames for the start and end of the byte_pos range!

But I've also seen some coverage results that show source ranges inside comments (not rustdoc code comments), so something is off sometimes.

It's almost surprising that a lot of things work correctly. So I still need to do more investigation.

For now, to avoid crashing, I changed this function to return an Option. Coverage mappings that have this problem get None from coverage_loc(), and I just skip them.

[wesleywiser]

@richkadel I wonder if any of those are related to macros? If you look at the MIR for a simple hello world (playground), you can see that some of the spans come from the playground file (src/lib.rs) while many of the others come from the expansion of the println!() macro and point into the std library (/rustc/{git commit}/src/libstd/macros.rs).

[wesleywiser]

In terms of whether to do it now, it seems like an issue from a past PR and this is an experimental feature, so I'd say it's okay to leave as-is and fix in a follow up PR.

SGTM, if ok with Wesley too. Thanks!

That seems fine to me as well. There's no expectation that unstable features are fully implemented or bug-free during development.

[richkadel]

@wesleywiser @tmandry - Assuming the PR build succeeds, can we start another @bors try?

I think the error on Windows is resolved. I'm confused by the MacOS error, but with other changes made, I'd like to test it again.

Thanks!

[richkadel]

FYI, the normal PR build is failing with:

Command failed. Attempt 5/5:
error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, use the --offline flag.
failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed

I did add a new tool, rust-demangler, to the main Cargo.toml, which caused Cargo.lock to be updated. If that's the cause, what do I need to do?

[richkadel]

FYI, the normal PR build is failing with:

Command failed. Attempt 5/5:
error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this

Never mind! This is resolved.

[richkadel]

@wesleywiser @tmandry - I think I've addressed all of your comments with the latest changes. I'm going to go back through them though and reply or resolve.

Note that when I started working on your feedback last week, I already anticipated some restructuring. Instead of submitting changes just to resolve the feedback alone, I incorporated your feedback into the restructured code.

So there are some more differences from what you reviewed last time, but all feedback was taken into account.

I was able to generate coverage on the entire json5format crate, which was awesome. There are some SourceMap glitches I think (user-error I'm sure) but a lot of it works well, showing coverage counts across functions, closures, and multiple instances of generic functions (woohoo!).

I also implemented support for name demangling, so llvm-cov show results are quite readable!

Here's an example from the revised test:

https://github.com/rust-lang/rust/pull/74091/files#diff-b91fcde238799d1eb0c57d2c4651ccc0

[richkadel]

1. When I try to instrument a complete package, including dependencies (via RUSTFLAGS=-Zinstrument-coverage cargo build), and need to call the query tcx.coverageinfo(def_id) from codegen_intrinsic_call(), injecting the LLVM instrprof.increment intrinsic fails for dependent crates because tcx.hir().get_if_local(def_id) returns None (i.e., the def_id is not "local" in the current type context (not found in it's hir::map::Map). I'm not sure how I should resolve this yet. (I'm also not sure, yet, if this is failing on all dependent crate functions, or possibly for functions pulled in from one crate to another -- wild guess based on the first crate that failed like this.)

Yeah, computing the hash on HIR will fail when codegenning functions from another crate (either #[inline] or generic functions being monomorphized by the current crate). All that's available for those functions is their signature and MIR, roughly.

As previously discussed, I resolved this by adding the function source hash to the count_code_region() intrinsic calls. (There will always be at least 1 of these.) That allows me to push the hash value into the coverage map in codegen, and I have it when generating the LLVM-IR. This "works", but there may be other issues with mapping coverage to source references from external crates (as I noted in another comment earlier today). So I may have to revisit this if it's only the tip of the iceberg.

[mati865]

Looks spurious:

Details

failures:

---- [ui] rustdoc-ui\failed-doctest-output.rs stdout ----
\rustdoc-ui\failed-doctest-output.rs

-	---- $DIR/failed-doctest-output.rs - OtherStruct (line 21) stdout ----
-	error[E0425]: cannot find value `no` in this scope
-	  --> $DIR/failed-doctest-output.rs:22:1
-	   |
-	LL | no
-	   | ^^ not found in this scope
-	
-	error: aborting due to previous error
-	
-	For more information about this error, try `rustc --explain E0425`.
-	Couldn't compile the test.
19	---- $DIR/failed-doctest-output.rs - SomeStruct (line 11) stdout ----
20	Test executable failed (exit code 101).
21	

30	note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
31	
32	
+	---- $DIR/failed-doctest-output.rs - OtherStruct (line 21) stdout ----
+	error[E0425]: cannot find value `no` in this scope
+	  --> $DIR/failed-doctest-output.rs:22:1
+	   |
+	LL | no
+	   | ^^ not found in this scope
+	
+	error: aborting due to previous error
+	
+	For more information about this error, try `rustc --explain E0425`.
+	Couldn't compile the test.
33	
34	failures:
35	    $DIR/failed-doctest-output.rs - OtherStruct (line 21)

[wesleywiser]

@bors retry

[bors]

⌛ Testing commit a6f8b8a with merge 28487bc5bc0433159b337420f8f7f1a7b3571703...

[bors]

💥 Test timed out

[richkadel]

Ugh. I checked the Pipeline UI just a couple of minutes before this timeout, and it was still going strong, just slow to complete the remaining tests on Windows.

It was chugging along on test/ui at the time.

Timing out like this after 4 hours without an actual problem was a waste of resources.

Any way to extend the timeout?

[richkadel]

Actually, it looks like the build was proactively cancelled by someone.

I agree the job was taking exceptionally long, but I assume it is due to an infrastructure issue, not the PR. Sad to see it get that far without getting the chance to complete.

@wesleywiser @tmandry - Any reason to think that the PR has a problem? (It did complete the bors try (3 days ago), in a normal amount of time.)

If not, can we queue it up again?

Thanks

[wesleywiser]

Intermittent timeouts are relatively common so it's probably just that. I'm not at a computer to verify but we can give it another try on the assumption that's the issue.

@bors rety

[wesleywiser]

@bors retry

[richkadel]

Thanks @wesleywiser !

[bors]

⌛ Testing commit a6f8b8a with merge 47ea6d9...

[bors]

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

[eggyal]

This is absolutely fantastic—huge thank you @richkadel, and everyone who's worked on this so far.

One thing that I've noticed on a large library crate, when building the test harness with -Zinstrument-coverage -Clink-dead-code, is that there are a number of functions for which there is no coverage data at all—presumably because, despite the provided codegen option, they are dead code that's being stripped somewhere along the way. Could the presently known limitations with -Zinstrument-coverage lead to such behaviour, or is this likely a bug (either here or in -Clink-dead-code)—in which case I'll try to produce an MCVE?

[mati865]

@eggyal could you make sure LTO is not enabled?

[eggyal]

@mati865 It's not. Have been investigating since my previous comment and it appears to be due to generics/monomorphisation. For example:

pub struct Foo<T>(T);

impl<T> Foo<T> {
    pub fn foo(&self) {
        // coverage exists (although regions look a bit buggy to me?)
    }

    pub fn bar(&self) {
        // no coverage
    }
}

#[cfg(test)]
mod tests {
    #[test]
    fn test() {
        let foo = super::Foo(());
        foo.foo();
    }
}

I guess Foo::<()>::bar is not actually generated unless it's called?

[wesleywiser]

@eggyal Thanks for the repro! This feature is still very much "in progress" so lots of stuff doesn't work correctly. Even still, I think it would be helpful to open bug reports as you find things so that we can track what issues have been resolved and which are still open.

[richkadel]

@mati865

I guess Foo::<()>::bar is not actually generated unless it's called?

Yes, I believe that's true. Generics are still just source templates, and don't actually become executable code until instantiated, when used. So the impl block is not instantiated as a whole. The parameterized impl block essentially represents a collection of template functions, inheriting the impls type parameters. Each generic function can be instantiated independently (or not at all, in the case of bar() in your example).

[mati865]

You probably meant to cc @eggyal