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

Native cargo coverage support #13040

Open
nagisa opened this issue Nov 23, 2023 · 3 comments
Open

Native cargo coverage support #13040

nagisa opened this issue Nov 23, 2023 · 3 comments
Labels
A-profiles Area: profiles C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-test S-needs-rfc Status: Needs an RFC to make progress.

Comments

@nagisa
Copy link
Member

nagisa commented Nov 23, 2023

Problem

I was looking at a performance issue caused by env RUSTFLAGS="-Cinstrument-coverage" in our codebase and have determined that the major part of the degradation (~10x slowdown) was because dependencies were instrumented. In one particular case, a hot & tight AVX-intrinsic heavy code has been instrumented as well. I suspect that this was leading to AVX registers being spilled and reloaded between each intrinsic to invoke a call to the coverage runtime library(-ies.)

The best fix for me would have been to somehow exclude the dependencies external to the cargo workspace from being passed this flag (collecting coverage for external dependencies isn’t particularly useful in the first place), but I was not able to find any way to do so, before Jakub Beránek suggested to write a wrapper script that would be applied to my cargo invocations with RUSTC_WORKSPACE_WRAPPER.

The solution works, of course, but it isn’t particularly discoverable. The obvious way to achieve -Cinstrument-coverage continues to be via RUSTFLAGS and in most cases it will do something a little different from what a good default would be (to only instrument your own code.)

Proposed Solution

I believe adding a profile setting to enable or disable addition of -Cinstrument-coverage flag would be a pretty good option here. It would allow to specifically instrument the local crates only (via profile.*.packages."*".instrument-coverage=false) while also retaining a full ability to selectively or fully instrument all the other crates as well using the same mechanism.

The one negative to this proposed solution is that the obvious setting of

[profile.dev]
instrument-coverage = true

is still going to instrument all the crates, including the dependencies.

[profile.dev]
instrument-coverage = true
[profile.dev.packages."*"]
instrument-coverage = false

Notes

Relevant discussion on Zulip

cc @Ekleog
cc @taiki-e (as a developer of llvm-cov)

@nagisa nagisa added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Nov 23, 2023
@epage
Copy link
Contributor

epage commented Nov 24, 2023

See also rust-lang/rfcs#3287

@epage epage added the A-profiles Area: profiles label Nov 24, 2023
@epage epage changed the title Add a profile option to specify -Cinstrument-coverage Native cargo coverage support Dec 5, 2023
@epage
Copy link
Contributor

epage commented Dec 5, 2023

I've generalized the title so we can focus the conversation on generally solving coverage, rather than having piece-meal conversations in different issues for different solutions.

@Zalathar
Copy link

On a few occasions I've been asked to track down coverage bugs that cause either an ICE in the compiler or a fatal error in llvm-cov, both of which make coverage reports unusable until a fix or workaround is found.

In almost all cases, the code that triggered the bug was in some third-party dependency, and not in the code that the user actually cared about getting coverage reports for.

Coverage instrumentation also causes general artifact bloat, in terms of both instructions (counter increments and inhibited optimizations) and metadata (coverage mappings embedded in binaries), such that it's preferable to instrument only those crates that the user cares about.

So I would be happy to see some mechanism for having Cargo set -Cinstrument-coverage on a per-crate basis, either via a more general per-crate rustflags feature, or by a specific instrument-coverage setting within profiles.

(The main advantage of a specific setting would be that it's much easier to specify/implement/review/stabilize, compared to the more general rustflags capability.)

@epage epage added S-needs-rfc Status: Needs an RFC to make progress. Command-test and removed S-triage Status: This issue is waiting on initial triage. labels Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-profiles Area: profiles C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-test S-needs-rfc Status: Needs an RFC to make progress.
Projects
Status: No status
Development

No branches or pull requests

3 participants