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

[Zalathar]

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

[Swatinem]

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.

[Swatinem]

❤️

[Zalathar]

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]

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

[Zalathar]

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]

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]

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]

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

[bors]

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

[bors]

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

[Zalathar]

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

[eirnym]

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