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

Make coverage robust against MIR optimizations removing all counter-increment statements #116171

Closed
Zalathar opened this issue Sep 26, 2023 · 5 comments · Fixed by #122860
Closed
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-mir-opt Area: MIR optimizations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Zalathar
Copy link
Contributor

When coverage instrumentation and MIR opts are both enabled, coverage relies on two assumptions:

  • MIR opts that would delete StatementKind::Coverage statements instead move them into bb0 and change them to CoverageKind::Unreachable.

  • MIR opts won't delete all CoverageKind::Counter statements from an instrumented function.

The first assumption will be lifted by #116046, which stores coverage mappings in a separate per-function struct, so the original mappings will be available during codegen even if MIR opts delete arbitrary coverage statements.


The second assumption is trickier. If a function is instrumented for coverage, but MIR opts remove all of its counter-increment statements (e.g. because bb0's ends with TerminatorKind::Unreachable), then we will codegen a function with no llvm.instrprof.increment intrinsics. This causes LLVM to assume that the function wasn't instrumented, so it will disappear from the final coverage mappings and won't participate in coverage reports.

Coverage has special code for handling functions that are deemed “unused” at the MIR level, but if a function is used (e.g. as a function pointer) but its body is unreachable, then that special code isn't applied.


Ideally what we need is some way for coverage codegen to detect that an instrumented function has no remaining counter-increment statements, and take special action to either re-insert at least one counter-increment (so LLVM treats it as instrumented), or treat it as though it were unused (so that it shows up in reports as never-executed).


@rustbot label +A-code-coverage +T-compiler

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 26, 2023
@Zalathar
Copy link
Contributor Author

Creation of this issue was requested in #116166 (comment), so that it can be linked to in a comment explaining why we turn off the UnreachablePropagation pass when coverage is enabled.

Once coverage instrumentation is robust against MIR opts that delete all coverage statements, we can remove that condition and enable UnreachablePropagation in optimized coverage builds.

@Zalathar
Copy link
Contributor Author

Currently it's difficult to detect this case because coverage codegen normally only knows about a function if it contains one or more StatementKind::Coverage statements, or if it was not codegenned at all. There is no way to detect a function that was instrumented and then codegenned without any coverage statements.

Once we have per-function coverage info from #116046, we'll be in a position to detect instrumented functions that have no remaining coverage statements, because they'll still have the coverage info that was attached during instrumentation.

@Zalathar

This comment was marked as outdated.

@Zalathar
Copy link
Contributor Author

II have a work-in-progress patch that fixes this via a new MIR pass that runs after optimizations, detects functions that were instrumented but no longer have any counter increments, and re-adds a counter increment to bb0. That seems to do the trick.

I’ll polish it up and add it to my patch queue.

@saethlin saethlin added A-mir-opt Area: MIR optimizations and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 27, 2023
@Zalathar
Copy link
Contributor Author

I also found a way to test this that doesn't rely on #113970: write a function that uses std::intrinsics::unreachable(). That triggers the existing code in UnreachablePropagation that can remove all statements from a block.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 1, 2023
coverage: Regression test for functions with unreachable bodies

This is a regression test for the coverage issue that was addressed temporarily by rust-lang#116166, and is tracked by rust-lang#116171.

---

If we instrument a function for coverage, but all of its counter-increment statements are removed by MIR optimizations, LLVM will think it isn't instrumented and it will disappear from coverage maps and coverage reports.

Most MIR opts won't cause this because they tend not to remove statements from bb0, but `UnreachablePropagation` can do so if it sees that bb0 ends with `TerminatorKind::Unreachable`.

Currently we have worked around this by turning off `UnreachablePropagation` when coverage instrumentation is enabled, which is why this test is able to pass.

---

`@rustbot` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 26, 2024
coverage: Re-enable `UnreachablePropagation` for coverage builds

This is a sequence of 3 related changes:
- Clean up the existing code that scans for unused functions
- Detect functions that were instrumented for coverage, but have had all their coverage statements removed by later MIR transforms (e.g. `UnreachablePropagation`)
- Re-enable `UnreachablePropagation` in coverage builds

Because we now detect functions that have lost their coverage statements, and treat them as unused, we don't need to worry about `UnreachablePropagation` removing all of those statements. This is demonstrated by `tests/coverage/unreachable.rs`.

Fixes rust-lang#116171.
@bors bors closed this as completed in 8a7f285 Mar 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 27, 2024
Rollup merge of rust-lang#122860 - Zalathar:unused, r=cjgillot

coverage: Re-enable `UnreachablePropagation` for coverage builds

This is a sequence of 3 related changes:
- Clean up the existing code that scans for unused functions
- Detect functions that were instrumented for coverage, but have had all their coverage statements removed by later MIR transforms (e.g. `UnreachablePropagation`)
- Re-enable `UnreachablePropagation` in coverage builds

Because we now detect functions that have lost their coverage statements, and treat them as unused, we don't need to worry about `UnreachablePropagation` removing all of those statements. This is demonstrated by `tests/coverage/unreachable.rs`.

Fixes rust-lang#116171.
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-opt Area: MIR optimizations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
3 participants