code coverage foundation for hash and num_counters #73488

richkadel · 2020-06-18T23:48:25Z

This PR is the next iteration after PR #73011 (which is still waiting on bors to merge).

(FYI, I'm also working on injecting the coverage maps, in another branch, while waiting for these to merge.)

Thanks!

update from origin 2020-06-18

richkadel · 2020-06-19T00:15:28Z

FYI, this PR (currently) represents only 1 commit since #73011 but until that one lands, the changes in both PRs show up in "Files changed". To see only the diffs in this PR, use this link:

4caf882

src/librustc_middle/mir/mod.rs

src/librustc_codegen_llvm/intrinsic.rs

bors · 2020-06-19T16:23:52Z

☔ The latest upstream changes (presumably #73504) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2020-06-19T16:26:33Z

☔ The latest upstream changes (presumably #73504) made this pull request unmergeable. Please resolve the merge conflicts.

update from origin 2020-06-19

Replaced dummy values for hash and num_counters with computed values, and refactored InstrumentCoverage pass to simplify injecting more counters per function in upcoming versions. Improved usage documentation and error messaging.

This commit adds a query that allows the CoverageData to be pulled from a call on tcx, avoiding the need to change the `codegen_intrinsic_call()` signature (no need to pass in the FunctionCx or any additional arguments. The commit does not change where/when the CoverageData is computed. It's still done in the `pass`, and saved in the MIR `Body`. See discussion (in progress) here: rust-lang#73488 (comment)

src/librustc_mir/transform/instrument_coverage.rs

src/librustc_metadata/locator.rs

src/librustc_middle/query/mod.rs

src/librustc_mir/transform/instrument_coverage.rs

src/librustc_middle/query/mod.rs

The mod uses both MIR bodies and HIR bodies, so I'm trying to maintain consistency with these names.

tmandry · 2020-06-23T04:58:04Z

src/librustc_mir/transform/instrument_coverage.rs

+                {
+                    if let FnDef(called_fn_def_id, _) = func.literal.ty.kind {
+                        if called_fn_def_id == count_code_region_fn {
+                            num_counters += 1;


It occurs to me that a MIR optimization could (a) duplicate a counter, or (b) inline counters from other functions. How should we deal with cases like that?

For (a) we could keep a set of counter id's we've already seen to deduplicate.

For handling (b), I'm assuming we'll want the name of the function where the counter originated in source? We could pass a def_id of the source function as part of the "intrinsic". But then how would we keep track of monomorphizations? We can always address this point in a follow-up PR, but it's worth thinking about.

For (a), I'll change the query implementation to return the max(counters) + 1. This fits well with the LLVM spec for instrprof.increment.

But (b) raises some interesting questions. Assuming it is possible for the MIR to include BasicBlocks from multiple "functions" in the source (which sounds logical), then I'm thinking the same thing you suggested; I'll probably need to add the DefId of the function in the injected Call terminator, rather than assuming the code region to be counted is part of the function represented by the MIR's DefId.

I addressed (a) in the latest commit.

For (b) I added FIXME comments for now. I'd like to address this in a separate PR, but will start looking into it.

Oh, I just remembered, we did talk about this (effect of inlining, for example) and my recommendation at the time is still something I'd like to propose: Disable inlining, and any other optimizations that might "break" code coverage, if the instrument_coverage option is turned on.

I'm not sure if we can address all of the edge cases like (b) with this, but I think it's plausible.

WDYT?

We can disable inlining in that case, sure (it's still not enabled by default). It seems like we might want a solution for this long-term, but it doesn't seem like a super high priority.

Also added FIXME comments to note the possible need to accommodate counter increment calls in source-based functions that differ from the function context of the caller instance (e.g., inline functions).

tmandry · 2020-06-23T18:25:39Z

@bors r+

bors · 2020-06-23T18:25:40Z

📌 Commit 977ce57 has been approved by tmandry

@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!

@ghost

…arth Rollup of 11 pull requests Successful merges: - rust-lang#72780 (Enforce doc alias check) - rust-lang#72876 (Mention that BTreeMap::new() doesn't allocate) - rust-lang#73244 (Check for assignments between non-conflicting generator saved locals) - rust-lang#73488 (code coverage foundation for hash and num_counters) - rust-lang#73523 (Fix -Z unpretty=everybody_loops) - rust-lang#73587 (Move remaining `NodeId` APIs from `Definitions` to `Resolver`) - rust-lang#73601 (Point at the call span when overflow occurs during monomorphization) - rust-lang#73613 (The const propagator cannot trace references.) - rust-lang#73614 (fix `intrinsics::needs_drop` docs) - rust-lang#73630 (Provide context on E0308 involving fn items) - rust-lang#73665 (rustc: Modernize wasm checks for atomics) Failed merges: r? @ghost

@tmandry

…r=wesleywiser add spans to injected coverage counters, extract with CoverageData query This is the next iteration on the Rust Coverage implementation, and follows PR rust-lang#73488 @tmandry @wesleywiser I came up with an approach for coverage spans, pushing them through the Call terminator as additional args so they can be extracted by the CoverageData query. I'm using an IndexVec to store them in CoverageData such that there can be only one per index (even if parts of the MIR get duplicated during optimization). If this approach works for you, I can quickly expand on this to build a separate IndexVec for counter expressions, using a separate call that will be ignored during code generation, but from which I can extract the counter expression values. Let me know your thoughts. Thanks! r? @tmandry Rust compiler MCP rust-lang/compiler-team#278 Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage instrumentation

@tmandry

…r=wesleywiser add spans to injected coverage counters, extract with CoverageData query This is the next iteration on the Rust Coverage implementation, and follows PR rust-lang#73488 @tmandry @wesleywiser I came up with an approach for coverage spans, pushing them through the Call terminator as additional args so they can be extracted by the CoverageData query. I'm using an IndexVec to store them in CoverageData such that there can be only one per index (even if parts of the MIR get duplicated during optimization). If this approach works for you, I can quickly expand on this to build a separate IndexVec for counter expressions, using a separate call that will be ignored during code generation, but from which I can extract the counter expression values. Let me know your thoughts. Thanks! r? @tmandry Rust compiler MCP rust-lang/compiler-team#278 Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage instrumentation

@tmandry

…r=wesleywiser add spans to injected coverage counters, extract with CoverageData query This is the next iteration on the Rust Coverage implementation, and follows PR rust-lang#73488 @tmandry @wesleywiser I came up with an approach for coverage spans, pushing them through the Call terminator as additional args so they can be extracted by the CoverageData query. I'm using an IndexVec to store them in CoverageData such that there can be only one per index (even if parts of the MIR get duplicated during optimization). If this approach works for you, I can quickly expand on this to build a separate IndexVec for counter expressions, using a separate call that will be ignored during code generation, but from which I can extract the counter expression values. Let me know your thoughts. Thanks! r? @tmandry Rust compiler MCP rust-lang/compiler-team#278 Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage instrumentation

Merge pull request #4 from rust-lang/master

7ef9eb3

update from origin 2020-06-18

rust-highfive assigned tmandry Jun 18, 2020

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

richkadel changed the title ~~Llvm coverage map gen~~ code coverage foundation for hash and num_counters Jun 19, 2020

wesleywiser reviewed Jun 19, 2020

View reviewed changes

src/librustc_middle/mir/mod.rs Outdated Show resolved Hide resolved

src/librustc_codegen_llvm/intrinsic.rs Outdated Show resolved Hide resolved

richkadel added 3 commits June 19, 2020 09:50

Merge pull request #5 from rust-lang/master

791cccc

update from origin 2020-06-19

code coverage foundation for hash and num_counters

8c7c84b

Replaced dummy values for hash and num_counters with computed values, and refactored InstrumentCoverage pass to simplify injecting more counters per function in upcoming versions. Improved usage documentation and error messaging.

num_counters to u32, after implementing TypeFoldable

933fe80

richkadel force-pushed the llvm-coverage-map-gen branch from 4caf882 to 933fe80 Compare June 22, 2020 06:55

richkadel commented Jun 22, 2020

View reviewed changes

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

tmandry reviewed Jun 22, 2020

View reviewed changes

src/librustc_metadata/locator.rs Outdated Show resolved Hide resolved

src/librustc_middle/query/mod.rs Outdated Show resolved Hide resolved

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

Address remaining feedback items

994d9d0

wesleywiser reviewed Jun 23, 2020

View reviewed changes

src/librustc_middle/query/mod.rs Outdated Show resolved Hide resolved

richkadel added 3 commits June 22, 2020 19:21

moves coverage data computation from pass to query

08ec4cb

PR no longer requires u32 impl TypeFoldable

3d0192e

using "mir_body" (vs "body") in InstrumentCoverage

a045140

The mod uses both MIR bodies and HIR bodies, so I'm trying to maintain consistency with these names.

tmandry reviewed Jun 23, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-map-gen branch from 8b6978a to af71595 Compare June 23, 2020 06:43

Updated query for num_counters to compute from max index

977ce57

Also added FIXME comments to note the possible need to accommodate counter increment calls in source-based functions that differ from the function context of the caller instance (e.g., inline functions).

richkadel force-pushed the llvm-coverage-map-gen branch from af71595 to 977ce57 Compare June 23, 2020 06:50

wesleywiser approved these changes Jun 23, 2020

View reviewed changes

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 23, 2020

This was referenced Jun 23, 2020

Rollup of 10 pull requests #73668

Closed

Rollup of 11 pull requests #73669

Merged

bors merged commit f5e46fe into rust-lang:master Jun 24, 2020

richkadel mentioned this pull request Jun 24, 2020

add spans to injected coverage counters, extract with CoverageData query #73684

Merged

cuviper added this to the 1.46 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code coverage foundation for hash and num_counters #73488

code coverage foundation for hash and num_counters #73488

richkadel commented Jun 18, 2020

richkadel commented Jun 19, 2020

bors commented Jun 19, 2020

bors commented Jun 19, 2020

tmandry Jun 23, 2020

richkadel Jun 23, 2020

richkadel Jun 23, 2020

richkadel Jun 23, 2020

tmandry Jun 23, 2020

tmandry commented Jun 23, 2020

bors commented Jun 23, 2020

code coverage foundation for hash and num_counters #73488

code coverage foundation for hash and num_counters #73488

Conversation

richkadel commented Jun 18, 2020

richkadel commented Jun 19, 2020

bors commented Jun 19, 2020

bors commented Jun 19, 2020

tmandry Jun 23, 2020

Choose a reason for hiding this comment

richkadel Jun 23, 2020

Choose a reason for hiding this comment

richkadel Jun 23, 2020

Choose a reason for hiding this comment

richkadel Jun 23, 2020

Choose a reason for hiding this comment

tmandry Jun 23, 2020

Choose a reason for hiding this comment

tmandry commented Jun 23, 2020

bors commented Jun 23, 2020