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

MIR InstrumentCoverage: Improve coverage of #[should_panic] tests and catch_unwind() handlers #78544

Open
richkadel opened this issue Oct 29, 2020 · 0 comments
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@richkadel
Copy link
Contributor

richkadel commented Oct 29, 2020

At present, the MIR InstrumentCoverage pass omits unwind paths when computing coverage spans and injecting counters because (1) statements executed during an unwind are generally compiler-generated, so they don't have source code to "cover"; and (2) in normal runtime execution, unwind paths are generally called when an unexpected and/or unhandled error is encountered.

Unexpected errors can (and probably should) be corrected before addressing coverage errors.

Some exceptions to this are: (1) tests that include testing for known panic situations, for instance, unit tests annotated with the #[should_panic] attribute; and (2) code that leverages a catch_unwind() handler.

Since coverage analysis is commonly used to validate that test cases execute all source code, and #[should_panic] tests might be included among the test cases, InstrumentCoverage could be improved to provide a more accurate representation of coverage when a Call unwinds.

Specifically, non-branching Statements and Terminators that follow a TerminatorKind::Call (if not other TerminatorKinds) are counted as having been "executed" by the same counter that counts the execution of the Call, as well as the non-branching statements that preceded the Call.

But if the Call panics, non-branching statements leading up to the Call will have been executed, and statements following the Call will not be executed. With only one counter, both sets of statements (before and after, and the Call itself will have the same execution count (either 1 or 0, depending on the type of counter, and where the counter is incremented).

To accurately count these sequences with the potential for panics, an actual code counter needs to be incremented after each successful call.

Adding these counters will increase the physical size of the binaries and slow down execution, so this is a feature that should only be enabled as an option.

One question is, how can this option be enabled granularly?

For #[should_panic] tests, it makes sense to enable the option for the test function, but this would not correct the counters for panicking calls in functions called by the test function. It may be possible to trace the calls to enable the option on the functions called by #[should_panic] test functions, and the functions executed by those functions, but this won't work for functions from external crates.

Similar logic could be used for functions called via catch_unwind(), with similar limitations.

So a worst-case approach would be to enable this feature when compiling the tested crate and all of its dependencies, using a rustc compiler flag.

@richkadel richkadel changed the title MIR InstrumentCoverage: Improve coverage of #[should_panic] tests MIR InstrumentCoverage: Improve coverage of #[should_panic] tests and catch_unwind() handlers Oct 29, 2020
@camelid camelid added the A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Oct 29, 2020
@wesleywiser wesleywiser added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Oct 22, 2021
@Nilstrieb Nilstrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
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) A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants