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

Inlining -Cinstrument-coverage code into a uninstrumented crate causes problems #132436

Open
Zalathar opened this issue Nov 1, 2024 · 0 comments
Labels
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.

Comments

@Zalathar
Copy link
Contributor

Zalathar commented Nov 1, 2024

-Cinstrument-coverage is a per-crate setting, which makes it possible for instrumented and uninstrumented crates to be mixed during compilation.

This must be supported to some extent, because the standard libraries are built without coverage instrumentation, and instrumenting them would be unwanted in most cases. And we would also like to support instrumenting a project without instrumenting its dependencies, though cargo currently doesn't have a good way to actually set different rustflags for dependencies. Fortunately, inlining uninstrumented code into an instrumented crate mostly works, though in some cases it might end up being unexpectedly instrumented.

The more difficult case is when instrumented code gets inlined into an uninstrumented crate. This is an unusual configuration, but it can occur in practice, e.g. when building doctests as in #132395 (comment). When this happens, two problems can arise:

  • Inlined MIR might contain StatementKind::Coverage statements, despite the current crate being built without coverage instrumentation. When codegen sees coverage statements, it cannot assume that coverage instrumentation is enabled.
  • If the inlined code isn't instrumented, then executing it won't increase coverage counts, despite its original crate having been built with instrumentation.

For now, the workaround is to quietly discard any coverage statements that make their way into an uninstrumented crate. This avoids the ICE seen in #132395, but doesn't solve the problem of that code not being instrumented as a result.

@Zalathar Zalathar added 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 Nov 1, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 1, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
jieyouxu added a commit to jieyouxu/rust that referenced this issue Nov 1, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 2, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 2, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 2, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 3, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 3, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 3, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 3, 2024
…r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 3, 2024
Rollup merge of rust-lang#132437 - Zalathar:inline-mixed-regression, r=jieyouxu

coverage: Regression test for inlining into an uninstrumented crate

Regression test for rust-lang#132395, after I was able to figure out a simple way to reproduce it. See also rust-lang#132436.

In addition to confirming that there is no ICE, this test also demonstrates that the affected code is undercounted, because executing the inlined copy doesn't increment coverage counters.
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 5, 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) 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

3 participants