coverage: Avoid splitting spans during span extraction/refinement #139102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zalathar
added
the
A-code-coverage
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
I don't feel like I have enough context to review this, sorry.. r? compiler |
|
r? compiler |
oli-obk
reviewed
Mar 31, 2025
Comment on lines
-135
to
137
| if let mir::Operand::Constant(box constant) = func { | ||
| if constant.span.lo() > span.lo() { | ||
| span = span.with_lo(constant.span.lo()); | ||
| } | ||
| if let mir::Operand::Constant(constant) = func | ||
| && span.contains(constant.span) | ||
| { | ||
| span = constant.span; | ||
| } |
There was a problem hiding this comment.
What does this to method calls?
And wouldn't you also make this work in case of let x = func; x(foo); to point at the x part of the function call?
There was a problem hiding this comment.
The original version of this heuristic predates me, but I think the intention was to deal with chained calls like this:
data.foo().bar().baz(args);
// ^^^^^^^^^^^^^^^^^^^^^^^^^^ (original span of the MIR call to baz)
// ^^^^^^^^^ (modified call span before this PR)
// ^^^ (modified call span after this PR)In practice, a lot of these small differences are hard to observe in the output, because there's a subsequent step that merges nearby spans that are considered to have the same control-flow for coverage purposes.
(This merging even happens for non-adjacent/overlapping spans in some cases, which is a behaviour I would eventually like to get rid of.)
There was a problem hiding this comment.
Similarly, I don't know the original reason for restricting this heuristic to “constant” function/method names, but I suspect it comes down to the fact that evaluating a non-constant callee can potentially have its own internal control flow, which can potentially result in confusing spans.
That said, such code is (a) relatively uncommon in practice, and (b) will probably cause the current span-refinement code to just discard the whole call span.
So I guess my reason for not changing it in this PR is to mostly just to avoid changes beyond my intended scope.
This test is intended to demonstrate that a particular macro-argument span doesn't get lost during span-refinement, but it turns out that span-extraction currently doesn't yield any MIR spans for this position. This patch therefore tweaks the test to add a function call in that position, so that it still remains relevant to span refinement.
This is a way to shrink call spans that doesn't involve mixing different spans, and avoids overlap with argument spans. This patch also removes some low-value comments that were causing rustfmt to ignore the match arms.
|
(Rebased to resolve snapshot conflicts in |
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Apr 1, 2025
coverage: Avoid splitting spans during span extraction/refinement This PR removes or simplifies some of the steps involved in extracting coverage-relevant spans from MIR, and preparing them for use in coverage instrumentation metadata. A common theme is that we now try harder to avoid modifying or combining spans in non-trivial ways, because those modifications present the most risk for weird behaviour or ICEs. The main changes are: - When extracting spans from MIR call terminators, try to restrict them to just the function name. - Instead of splitting spans around “holes”, just discard any span that overlaps with a hole. - Instead of splitting macro-invocation spans into two parts, truncate them to just the macro name and subsequent `!`. --- This results in a lot of tiny changes to the spans that end up in coverage metadata, and a few changes to coverage reports. Judging by test snapshots, these changes appear to be quite minor in practice.
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Apr 1, 2025
coverage: Avoid splitting spans during span extraction/refinement This PR removes or simplifies some of the steps involved in extracting coverage-relevant spans from MIR, and preparing them for use in coverage instrumentation metadata. A common theme is that we now try harder to avoid modifying or combining spans in non-trivial ways, because those modifications present the most risk for weird behaviour or ICEs. The main changes are: - When extracting spans from MIR call terminators, try to restrict them to just the function name. - Instead of splitting spans around “holes”, just discard any span that overlaps with a hole. - Instead of splitting macro-invocation spans into two parts, truncate them to just the macro name and subsequent `!`. --- This results in a lot of tiny changes to the spans that end up in coverage metadata, and a few changes to coverage reports. Judging by test snapshots, these changes appear to be quite minor in practice.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 1, 2025
Rollup of 12 pull requests Successful merges: - rust-lang#110406 (rustdoc-json: Add test for #[automatically_derived] attribute) - rust-lang#137738 (Make slice iterator constructors unstably const) - rust-lang#138492 (remove `feature(inline_const_pat)`) - rust-lang#138928 (Fix UWP reparse point check) - rust-lang#138950 (replace extra_filename with strict version hash in metrics file names) - rust-lang#139002 (Add release notes for 1.86.0) - rust-lang#139022 (increment depth of nested obligations) - rust-lang#139060 (replace commit placeholder in vendor status with actual commit) - rust-lang#139102 (coverage: Avoid splitting spans during span extraction/refinement) - rust-lang#139129 (Add tests for slice bounds check optimization) - rust-lang#139188 (PassWrapper: adapt for llvm/llvm-project@94122d58fc77079a291a3d008914…) - rust-lang#139193 (Feed HIR for by-move coroutine body def, since the inliner tries to read its attrs) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 2, 2025
Rollup of 14 pull requests Successful merges: - rust-lang#135295 (Check empty SIMD vector in inline asm) - rust-lang#138003 (Add the new `amx` target features and the `movrs` target feature) - rust-lang#138823 (rustc_target: RISC-V: add base `I`-related important extensions) - rust-lang#138913 (Remove even more instances of `@ts-expect-error` from search.js) - rust-lang#138941 (Do not mix normalized and unnormalized caller bounds when constructing param-env for `receiver_is_dispatchable`) - rust-lang#139060 (replace commit placeholder in vendor status with actual commit) - rust-lang#139102 (coverage: Avoid splitting spans during span extraction/refinement) - rust-lang#139191 (small opaque type/borrowck cleanup) - rust-lang#139200 (Skip suggest impl or dyn when poly trait is not a real trait) - rust-lang#139208 (fix dead link netbsd.md) - rust-lang#139210 (chore: remove redundant backtick) - rust-lang#139212 (Update mdbook to 0.4.48) - rust-lang#139214 (Tell rustfmt to use the 2024 edition in ./x.py fmt) - rust-lang#139225 (move autodiff from EnzymeAD/Enzyme to our rust-lang/Enzyme soft-fork) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 2, 2025
Rollup merge of rust-lang#139102 - Zalathar:no-split, r=oli-obk coverage: Avoid splitting spans during span extraction/refinement This PR removes or simplifies some of the steps involved in extracting coverage-relevant spans from MIR, and preparing them for use in coverage instrumentation metadata. A common theme is that we now try harder to avoid modifying or combining spans in non-trivial ways, because those modifications present the most risk for weird behaviour or ICEs. The main changes are: - When extracting spans from MIR call terminators, try to restrict them to just the function name. - Instead of splitting spans around “holes”, just discard any span that overlaps with a hole. - Instead of splitting macro-invocation spans into two parts, truncate them to just the macro name and subsequent `!`. --- This results in a lot of tiny changes to the spans that end up in coverage metadata, and a few changes to coverage reports. Judging by test snapshots, these changes appear to be quite minor in practice.
github-merge-queue bot
pushed a commit
to model-checking/kani
that referenced
this pull request
Apr 4, 2025
Culprit PR: rust-lang/rust#139102 Resolves #3986 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes or simplifies some of the steps involved in extracting coverage-relevant spans from MIR, and preparing them for use in coverage instrumentation metadata.
A common theme is that we now try harder to avoid modifying or combining spans in non-trivial ways, because those modifications present the most risk for weird behaviour or ICEs.
The main changes are:
!.This results in a lot of tiny changes to the spans that end up in coverage metadata, and a few changes to coverage reports. Judging by test snapshots, these changes appear to be quite minor in practice.