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

coverage: Don't show coverage for code paths that must panic/diverge #120013

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Zalathar
Copy link
Contributor

One of the limitations of coverage instrumentation at the moment is that there's no good way to exclude “unreachable” code paths from coverage reports (e.g. see #80549).

However, as I noted in #80549 (comment), there is potentially a way around this in some cases. Coverage instrumentation already assumes that panics don't occur, so if we can detect code paths that must panic/diverge (e.g. because they call panic! or unreachable!), we can potentially exclude their spans from the coverage mappings.

Doing so seems to give quite pleasing results in some previously-troublesome cases.


Marking this as draft for now because it's a big user-visible change, and while I like it overall, I'm not fully convinced that it's the right thing to do.

r? @ghost
@rustbot label +A-code-coverage

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Jan 16, 2024
Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. we also recently had an internal discussion about this thing at sentry/codecov.

IMO, it would be okay to make this the default. However it would also be good to give people more freedom here. I could think about making the -C instrument-coverage flag take a list of options (instead of being a mutually exclusive enum like now) which can control some of these details.

@@ -20,7 +20,6 @@
LL| 1|fn eq_good_message() {
LL| 1| println!("b");
LL| 1| assert_eq!(Foo(1), Foo(1), "message b");
^0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@Zalathar
Copy link
Contributor Author

Zalathar commented Jan 21, 2024

Yeah, the design I’ve had floating around in my head for a while (from here) is to have some kind of separate -Zcoverage-options=a,b,c flag.

So -Cinstrument-coverage itself would give you sensible defaults, and then you can toggle the more fine-grained flags to get non-default behaviour.

@Zalathar
Copy link
Contributor Author

I thought of a case where this approach might give unexpected results: std::process:exit.

@Zalathar
Copy link
Contributor Author

I thought of a case where this approach might give unexpected results: std::process:exit.

Thinking about this some more, I realised that from the instrumentor's perspective, there is no reliable difference between a peaceful process exit and a panic/unwind/abort.

For example, suppose we have a function like this:

fn maybe_exit(cond: bool) {
    if cond {
        std::process::exit(0);
    }
}

When instrumenting callers of this function, the instrumentor is already committed to the assumption that it will return normally and not exit. If it does exit, any subsequent code in the caller is liable to give incorrect counts anyway.

So by assuming the absence of panics, we have already assumed the absence of peaceful termination as well. Therefore, the fact that this PR interacts poorly with std::process::exit should not be considered a showstopper, because we have already committed to not properly instrumenting such code.

@Swatinem
Copy link
Contributor

If it does exit, any subsequent code in the caller is liable to give incorrect counts anyway.

Is this related to continuous counter updates (%c) in any way?

As documented here (https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program):

This means that if the instrumented program crashes, or is killed by a signal, perfect coverage information can still be recovered.

This might be a very niche use-case, but in the sentry-native crash reported, we were using the %c flag to specifically get coverage about the inner working of the crash reported at the time of crash / panic, etc.

@Zalathar
Copy link
Contributor Author

Is this related to continuous counter updates (%c) in any way?

I don't think there's any relation. What I'm thinking of is the fact that (e.g.) code after an if/else often won't have its own counter, and will instead have a counter expression that ends up being equivalent to the counter of the code before the if/else.

So if one of the if/else arms panics/crashes/terminates, then even if the counters are properly synced to disk, the coverage value displayed for the code after the if/else will still be “wrong” in the sense that it includes the execution that terminated (and therefore never reached the post-if/else code).

@bors
Copy link
Contributor

bors commented Feb 4, 2024

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

@Dylan-DPC Dylan-DPC added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2024
@bors
Copy link
Contributor

bors commented Feb 22, 2024

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 22, 2024
This makes it easier to see that we're manipulating the instance that is being
constructed, and is a lot less verbose than `basic_coverage_blocks`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 23, 2024
coverage: Use variable name `this` in `CoverageGraph::from_mir`

A tiny little improvement, extracted from rust-lang#120013.

This makes it easier to see that we're manipulating the instance that is being constructed, and is a lot less verbose than `basic_coverage_blocks`.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
Rollup merge of rust-lang#121484 - Zalathar:this, r=oli-obk

coverage: Use variable name `this` in `CoverageGraph::from_mir`

A tiny little improvement, extracted from rust-lang#120013.

This makes it easier to see that we're manipulating the instance that is being constructed, and is a lot less verbose than `basic_coverage_blocks`.
@bors
Copy link
Contributor

bors commented Feb 23, 2024

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

@Zalathar
Copy link
Contributor Author

This might be a nice use-case for #122226, if we're uncertain about just enabling it by default.

@eirnym
Copy link

eirnym commented Apr 18, 2024

I'd love to see this feature in stable, as currently I exclude panic! and unreachable! from coverage by manually parsing source files and explicitly excluding these regions (just add them into final report files :D

It's not a native solution, but it works good enough for me

@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants