Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Injects counters before every block (experimental only) #74856

Closed
wants to merge 3 commits into from

Conversation

richkadel
Copy link
Contributor

Uses the span from the start of the first statement to the end of the Terminator.

This simple approach does introduce some poor coverage results. For
example, a function that only calls macros (like println!, for
example) may only have counters injected at the macro site, and these
may never get counted, for some reason. (I'm seeing some functions with
coverage counts of zero, even though they clearly should be called.)

I'm developing a follow-up change to this one that injects counters more
precisely, by inspecting the Statement and Terminator types, and making
different decisions regarding what blocks should be counted, and what
spans to use to generate the code regions.

Found some problems with the coverage map encoding when testing with
more than one counter per function.

While debugging, I realized some better ways to structure the Rust
implementation of the coverage mapping generator. I refactored somewhat,
resulting in less code overall, expanded coverage of LLVM Coverage Map
capabilities, and much closer alignment with LLVM data structures, APIs,
and naming.

This should be easier to follow and easier to maintain.
Lays a better foundation for injecting more counters in each function.
Uses the span from the start of the first statement to the end of the
Terminator.

Includes "cleanup" subgraphs.

This simple approach does introduce some poor coverage results. For
example, a function that only calls macros (like `println!`, for
example) may only have counters injected at the macro site, and these
may never get counted, for some reason. (I'm seeing some functions with
coverage counts of zero, even though they clearly should be called.)

I'm developing a follow-up change to this one that injects counters more
precisely, by inspecting the Statement and Terminator types, and making
different decisions regarding what blocks should be counted, and what
spans to use to generate the code regions.
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2020
@richkadel
Copy link
Contributor Author

r? @tmandry

@richkadel
Copy link
Contributor Author

@tmandry - I tried to push a specific commit on the branch, but got other commits, including future changes.

Ignore this PR for now (it's draft anyway), and I'll add a note when it's ready to take a look at.

Thanks!

@richkadel
Copy link
Contributor Author

@tmandry - Fixed. You can see the changes specific to this PR in the last commit. (The first 2 commits are from the PR currently in review.)

ec050f2

This is just what you looked at yesterday during our meeting, simply adding counters to each BasicBlock.

As discussed, I'll do some more investigating of SourceScope and Span values in the contributing Statement and Terminator components each BasicBlock.

And as noted, SourceScope may or may not need to be set on the injected calls based on its surrounding context, depending on how it's used. (I have a feeling it's not, but need to confirm. If it's really only relevant to variables and debugging, as we speculated yesterday, the injected BB's do not introduce user-relevant variables; the temporary variable to capture the () return result of the call is eventually thrown away, in codegen.) If not, I can probably just use a default value, like OUTERMOST_SOURCE_SCOPE.

I may also push a parallel PR with the work I did to be more selective about which BasicBlocks to use, based on CFG branching, because I thought that had promise. But even with that approach, extracting reasonable Spans (and maybe SourceScope, if required) also needs work. So the investigation will help in both approaches.

@bors
Copy link
Contributor

bors commented Jul 29, 2020

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

@Muirrum Muirrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020
@Dylan-DPC-zz
Copy link

@richkadel if you rebase and resolve the latest conflicts, we can get this reviewed if possible

@richkadel
Copy link
Contributor Author

The gist of this PR is incorporated in #75828 instead.

@richkadel richkadel closed this Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants