code coverage foundation for hash and num_counters #73488
Conversation
update from origin 2020-06-18
rust-highfive
added
the
S-waiting-on-review
richkadel
changed the title
|
☔ The latest upstream changes (presumably #73504) made this pull request unmergeable. Please resolve the merge conflicts. |
1 similar comment
|
☔ 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.
richkadel
force-pushed
the
llvm-coverage-map-gen
branch
from
4caf882 to
933fe80
Compare
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)
The mod uses both MIR bodies and HIR bodies, so I'm trying to maintain consistency with these names.
| { | ||
| if let FnDef(called_fn_def_id, _) = func.literal.ty.kind { | ||
| if called_fn_def_id == count_code_region_fn { | ||
| num_counters += 1; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
richkadel
force-pushed
the
llvm-coverage-map-gen
branch
from
8b6978a to
af71595
Compare
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
|
@bors r+ |
|
📌 Commit 977ce57 has been approved by |
bors
added
S-waiting-on-bors
6 participants
This PR is the next iteration after PR #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!