first stage of implementing LLVM code coverage #73011

richkadel · 2020-06-05T02:31:55Z

This PR replaces #70680 (WIP toward LLVM Code Coverage for Rust) since I am re-implementing the Rust LLVM code coverage feature in a different part of the compiler (in MIR pass(es) vs AST).

This PR updates rustc with -Zinstrument-coverage option that injects the llvm intrinsic instrprof.increment() for code generation.

This initial version only injects counters at the top of each function, and does not yet implement the required coverage map.

Upcoming PRs will add the coverage map, and add more counters and/or counter expressions for each conditional code branch.

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

I put together some development notes here, under a separate branch.

update from upstream

rust-highfive · 2020-06-05T02:31:59Z

r? @davidtwco

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

richkadel · 2020-06-05T02:33:07Z

r? @davidtwco

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

r? @tmandry @wesleywiser

richkadel · 2020-06-05T02:34:22Z

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

r? @tmandry @wesleywiser

richkadel · 2020-06-05T02:54:54Z

@wesleywiser - PTAL.

Note that Tyler and I also discussed the possibility of implementing the code generation with a custom Statement (rather than injecting a Call Terminator, as I did in this PR).

The Call Terminator allowed me to model the injected intrinsic the way that other intrinsics are handled (with a few important differences).

An alternative, suggested by Tyler, would be to define a new, custom StatementKind. Knowing that the llvm.instrprof.increment intrinsic will never panic, its BasicBlock does not branch, so a Statement would potentially avoid the need to inject a new BB.

My only hesitation is that a new StatementKind for this intrinsic becomes a very special case, different from all other intrinsics, and there may be hidden expectations and assumptions in the compiler that could add unexpected complexity.

But, I'm open to this alternative approach if there's a good reason to make that change.

Let us know your thoughts.

@tmandry - Please feel free to expand on or correct my explanation here. Thank you!

src/libcore/intrinsics.rs

src/librustc_mir/transform/mod.rs

src/librustc_mir/transform/instrument_coverage.rs

src/librustc_codegen_llvm/intrinsic.rs

tmandry

Looks great! Left a couple comments but otherwise I think this is ready to land.

src/librustc_mir/interpret/intrinsics.rs

src/librustc_mir/transform/instrument_coverage.rs

src/librustc_mir/transform/mod.rs

src/librustc_codegen_llvm/intrinsic.rs

tmandry · 2020-06-08T21:38:42Z

@bors d+

r=me after comments

src/librustc_mir/transform/instrument_coverage.rs

tmandry · 2020-06-08T23:41:49Z

@bors r+

bors · 2020-06-08T23:41:50Z

📌 Commit 84d4424ff88db00736e940da9443c29ef5911101 has been approved by tmandry

richkadel · 2020-06-09T02:15:29Z

Did not mean to close the PR. The GitHub app on Android has a Pull Request status window with a "Close" button. It looks like that button is how you close the window, but it closes the PR... Ach!

oli-obk · 2020-06-10T11:43:54Z

Please add tests in src/tests/mir-opt that show various things that get annotated by the annotation pass.

You'll need to add // compile-flags: -Zinstrument-coverage and // EMIT_MIR rustc.some_function_you_want_to_see_the_mir_of.InstrumentCoverage.diff to the test file in order to get dumps.

src/librustc_mir/transform/instrument_coverage.rs

richkadel · 2020-06-10T17:50:05Z

@oli-obk

Please add tests in src/tests/mir-opt that show various things that get annotated by the annotation pass.

Done

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

tmandry · 2020-06-17T17:30:33Z

@bors r+

bors · 2020-06-17T17:30:34Z

📌 Commit c338729 has been approved by tmandry

richkadel · 2020-06-17T17:35:37Z

@Dylan-DPC - Would you be willing to include this in the next rollup? Thanks!

richkadel · 2020-06-17T18:27:30Z

@Dylan-DPC - Would you be willing to include this in the next rollup? Thanks!

Please hold off. We're going to do another bors try with macos and windows tests enabled, to make sure there are no more surprises.

testing platform-specific changes

tmandry · 2020-06-17T18:41:34Z

@bors try

bors · 2020-06-17T18:41:50Z

⌛ Trying commit b9f0304 with merge 7e5ef77edd62e7082b227400416e103834f1ef1a...

mati865 · 2020-06-17T18:56:46Z

~~This PR is next in bors queue. It'd be safer for rollups to not include it.~~

Dylan-DPC-zz · 2020-06-17T18:57:57Z

it's not next on the queue 🤔 it's on "try" which is separate so it won't be merged

mati865 · 2020-06-17T19:01:51Z

@Dylan-DPC right, I mistook 73011 for 70311 😮

bors · 2020-06-17T21:48:00Z

☀️ Try build successful - checks-azure
Build commit: 7e5ef77edd62e7082b227400416e103834f1ef1a (7e5ef77edd62e7082b227400416e103834f1ef1a)

https://dev.azure.com/rust-lang/rust/_build/results?buildId=32224&view=results

richkadel · 2020-06-17T21:54:54Z

I removed the "try" config changes (PR passes on all platforms now!).

This version should be good to merge, assuming no new merge conflicts.

Thanks!

tmandry · 2020-06-17T22:13:13Z

@bors r+

bors · 2020-06-17T22:13:15Z

📌 Commit 36c9014 has been approved by tmandry

@ghost

Rollup of 10 pull requests Successful merges: - rust-lang#72280 (Fix up autoderef when reborrowing) - rust-lang#72785 (linker: MSVC supports linking static libraries as a whole archive) - rust-lang#73011 (first stage of implementing LLVM code coverage) - rust-lang#73044 (compiletest: Add directives to detect sanitizer support) - rust-lang#73054 (memory access sanity checks: abort instead of panic) - rust-lang#73136 (Change how compiler-builtins gets many CGUs) - rust-lang#73280 (Add E0763) - rust-lang#73317 (bootstrap: read config from $RUST_BOOTSTRAP_CONFIG) - rust-lang#73350 (bootstrap/install.rs: support a nonexistent `prefix` in `x.py install`) - rust-lang#73352 (Speed up bootstrap a little.) Failed merges: r? @ghost

@wesleywiser

…tmandry code coverage foundation for hash and num_counters This PR is the next iteration after PR rust-lang#73011 (which is still waiting on bors to merge). @wesleywiser - PTAL r? @tmandry (FYI, I'm also working on injecting the coverage maps, in another branch, while waiting for these to merge.) Thanks!

@wesleywiser

…tmandry code coverage foundation for hash and num_counters This PR is the next iteration after PR rust-lang#73011 (which is still waiting on bors to merge). @wesleywiser - PTAL r? @tmandry (FYI, I'm also working on injecting the coverage maps, in another branch, while waiting for these to merge.) Thanks!

RalfJung · 2020-06-27T08:49:55Z

src/librustc_mir/interpret/intrinsics.rs

@@ -389,6 +389,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                );
                self.copy_op(self.operand_index(args[0], index)?, dest)?;
            }
+            // FIXME(#73156): Handle source code coverage in const eval
+            sym::count_code_region => (),


Please always ping @rust-lang/wg-const-eval for new intrinsics in CTFE.
It is not great when I realize by mere accident that we have a new intrinsic here. It's just a stub this time, so no big deal, but who knows what it might be next time. ;)

Also, note that as per this comment, T-lang should have been involved as well:

rust/src/libcore/intrinsics.rs

Lines 8 to 9 in 394e1b4

//! Note: any changes to the constness of intrinsics should be discussed with the language team.

//! This includes changes in the stability of the constness.

amusingly this intrinsic is not marked as const at all. The static checks preventing non-const intrinsics from being invoked happen before mir optimizations. Since instrumentation is a MIR transformation... this isn't caught. Maybe instrumentation should happen before const checking and such?

Oh, so this intrinsic is not called by users, but the call is inserted by an instrumentation pass later? Interesting. That reduces my T-lang concern.

Maybe instrumentation should happen before const checking and such?

Maybe -- I am not sure to what extend it would be useful to check the instrumented code as if it was user-written.

If I write

const fn a() -> u8 { 42 } const A: u8 = a(); fn main() { println!("{}", A); }

Then currently a will contribute towards coverage, yet it is not marked as executed. I think the count_code_region intrinsic should also mark code as executed during CTFE.

@bjorn3 that is a separate issue though from the other discussion here? Namely, it is #73156.

Didn't see that issue, thanks.

Sorry about that @RalfJung! That was my fault; I didn't realize we should be pinging that group. I will try to keep that in mind in the future.

Do you think we still need to get the lang team involved? This intrinsic is used by an unstable -Z compiler flag which generates code coverage data using LLVM features. The compiler team already signed off on this approach via an MCP.

I doubt they will have any objections, but sending T-lang an "FYI this is what we did here" would still be a good idea I think.

Thanks for flagging the issue @RalfJung ...

Note also, the next PR increment is under review, and adds some additional Rust intrinsics that I believe we need for coverage map generation.

See this related note:

#73684 (comment)

Since the MIR analysis is minimal at the moment (injecting at the function level only) I'm not generating calls to these intrinsics yet, but when I do, I suspect I may need to add these new intrinsics to src/librustc_mir/interpret/intrinsics.rs as well, similarly stubbed out.

Merge pull request #1 from rust-lang/master

a6127e3

update from upstream

rust-highfive assigned davidtwco Jun 5, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2020

rust-highfive assigned tmandry and unassigned davidtwco Jun 5, 2020

richkadel mentioned this pull request Jun 5, 2020

WIP toward LLVM Code Coverage for Rust #70680

Closed

Aaron1011 reviewed Jun 5, 2020

View reviewed changes

src/libcore/intrinsics.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 5, 2020

View reviewed changes

src/libcore/intrinsics.rs Outdated Show resolved Hide resolved

wesleywiser reviewed Jun 5, 2020

View reviewed changes

src/librustc_mir/transform/mod.rs Outdated Show resolved Hide resolved

src/librustc_mir/transform/instrument_coverage.rs Outdated Show resolved Hide resolved

Aaron1011 reviewed Jun 8, 2020

View reviewed changes

src/librustc_codegen_llvm/intrinsic.rs Show resolved Hide resolved

tmandry approved these changes Jun 8, 2020

View reviewed changes

src/librustc_mir/interpret/intrinsics.rs Show resolved Hide resolved

src/librustc_mir/transform/instrument_coverage.rs Outdated Show resolved Hide resolved

src/librustc_mir/transform/mod.rs Outdated Show resolved Hide resolved

src/librustc_codegen_llvm/intrinsic.rs Show resolved Hide resolved

tmandry reviewed Jun 8, 2020

View reviewed changes

src/librustc_mir/transform/instrument_coverage.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2020

richkadel closed this Jun 9, 2020

richkadel reopened this Jun 9, 2020

oli-obk reviewed Jun 10, 2020

View reviewed changes

src/librustc_mir/transform/instrument_coverage.rs Outdated Show resolved Hide resolved

oli-obk reviewed Jun 10, 2020

View reviewed changes

src/librustc_mir/transform/instrument_coverage.rs Outdated Show resolved Hide resolved

oli-obk reviewed Jun 10, 2020

View reviewed changes

src/librustc_mir/transform/instrument_coverage.rs Outdated Show resolved Hide resolved

Update src/libcore/intrinsics.rs

c338729

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2020

temporarily enable mac and windows tests on bors try

b9f0304

testing platform-specific changes

richkadel force-pushed the llvm-count-from-mir-pass branch from 2238705 to b9f0304 Compare June 17, 2020 18:40

removed try config to test mac & windows (passed)

36c9014

https://dev.azure.com/rust-lang/rust/_build/results?buildId=32224&view=results

richkadel mentioned this pull request Jun 18, 2020

code coverage foundation for hash and num_counters #73488

Merged

RalfJung mentioned this pull request Jun 19, 2020

Rollup of 10 pull requests #73504

Merged

bors merged commit 1dc6c3c into rust-lang:master Jun 19, 2020

djc mentioned this pull request Jun 23, 2020

Implement support for LLVMs code coverage instrumentation #34701

Closed

RalfJung reviewed Jun 27, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first stage of implementing LLVM code coverage #73011

first stage of implementing LLVM code coverage #73011

richkadel commented Jun 5, 2020

rust-highfive commented Jun 5, 2020

richkadel commented Jun 5, 2020

richkadel commented Jun 5, 2020

richkadel commented Jun 5, 2020

tmandry left a comment

tmandry commented Jun 8, 2020

tmandry commented Jun 8, 2020

bors commented Jun 8, 2020

richkadel commented Jun 9, 2020

oli-obk commented Jun 10, 2020

richkadel commented Jun 10, 2020

tmandry commented Jun 17, 2020

bors commented Jun 17, 2020

richkadel commented Jun 17, 2020

richkadel commented Jun 17, 2020

tmandry commented Jun 17, 2020

bors commented Jun 17, 2020

mati865 commented Jun 17, 2020 •

edited

Loading

Dylan-DPC-zz commented Jun 17, 2020

mati865 commented Jun 17, 2020

bors commented Jun 17, 2020

richkadel commented Jun 17, 2020

tmandry commented Jun 17, 2020

bors commented Jun 17, 2020

RalfJung Jun 27, 2020 •

edited

Loading

RalfJung Jun 27, 2020

oli-obk Jun 27, 2020

RalfJung Jun 27, 2020

bjorn3 Jun 27, 2020

RalfJung Jun 27, 2020

bjorn3 Jun 27, 2020

wesleywiser Jun 27, 2020 •

edited

Loading

RalfJung Jun 27, 2020

richkadel Jun 27, 2020

	//! Note: any changes to the constness of intrinsics should be discussed with the language team.
	//! This includes changes in the stability of the constness.

first stage of implementing LLVM code coverage #73011

first stage of implementing LLVM code coverage #73011

Conversation

richkadel commented Jun 5, 2020

rust-highfive commented Jun 5, 2020

richkadel commented Jun 5, 2020

richkadel commented Jun 5, 2020

richkadel commented Jun 5, 2020

tmandry left a comment

Choose a reason for hiding this comment

tmandry commented Jun 8, 2020

tmandry commented Jun 8, 2020

bors commented Jun 8, 2020

richkadel commented Jun 9, 2020

oli-obk commented Jun 10, 2020

richkadel commented Jun 10, 2020

tmandry commented Jun 17, 2020

bors commented Jun 17, 2020

richkadel commented Jun 17, 2020

richkadel commented Jun 17, 2020

tmandry commented Jun 17, 2020

bors commented Jun 17, 2020

mati865 commented Jun 17, 2020 • edited Loading

Dylan-DPC-zz commented Jun 17, 2020

mati865 commented Jun 17, 2020

bors commented Jun 17, 2020

richkadel commented Jun 17, 2020

tmandry commented Jun 17, 2020

bors commented Jun 17, 2020

RalfJung Jun 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleywiser Jun 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mati865 commented Jun 17, 2020 •

edited

Loading

RalfJung Jun 27, 2020 •

edited

Loading

wesleywiser Jun 27, 2020 •

edited

Loading