coverage: Simplify the heuristic for ignoring async fn return spans
#118666
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
|
r? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
the
A-code-coverage
Zalathar
commented
Dec 6, 2023
Swatinem
approved these changes
Dec 6, 2023
Zalathar
force-pushed
the
body-closure
branch
4 times, most recently
from
a27e878
to
5d00fe9
Compare
|
I was able to find a simple repro for the I manually verified that it passes before #118525, then fails, and now passes again. |
This should make it easier to investigate unwrap failures in bug reports.
Zalathar
force-pushed
the
body-closure
branch
from
5d00fe9
to
e01338a
Compare
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
|
It's out of scope for this PR, but another thing that might be worth trying is to have the heuristic try to accurately detect whether the current function really is We can probably get that information by looking backwards at HIR, or by inspecting span contexts. |
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Dec 9, 2023
coverage: Simplify the heuristic for ignoring `async fn` return spans The code for extracting coverage spans from MIR has a special heuristic for dealing with `async fn`, so that the function's closing brace does not have a confusing double count. The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand. We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop. The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for `async fn` have not changed. --- This PR also has the side-effect of fixing the `None some_prev` panic that started appearing after rust-lang#118525. The old code assumed that `prev` would always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to rust-lang#118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics. The new code uses an `if let` to inspect `prev`, which correctly does nothing if there is no span present.
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Dec 9, 2023
coverage: Simplify the heuristic for ignoring `async fn` return spans The code for extracting coverage spans from MIR has a special heuristic for dealing with `async fn`, so that the function's closing brace does not have a confusing double count. The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand. We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop. The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for `async fn` have not changed. --- This PR also has the side-effect of fixing the `None some_prev` panic that started appearing after rust-lang#118525. The old code assumed that `prev` would always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to rust-lang#118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics. The new code uses an `if let` to inspect `prev`, which correctly does nothing if there is no span present.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 9, 2023
…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#118198 (coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions) - rust-lang#118512 (Add tests related to normalization in implied bounds) - rust-lang#118610 (update target feature following LLVM API change) - rust-lang#118666 (coverage: Simplify the heuristic for ignoring `async fn` return spans) - rust-lang#118737 (Extend tidy alphabetical checking to `tests/`.) - rust-lang#118756 (use bold magenta instead of bold white for highlighting) - rust-lang#118762 (Some more minor `async gen`-related nits) - rust-lang#118764 (Make async generators fused by default) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 9, 2023
…kingjubilee Rollup of 7 pull requests Successful merges: - rust-lang#118198 (coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions) - rust-lang#118512 (Add tests related to normalization in implied bounds) - rust-lang#118610 (update target feature following LLVM API change) - rust-lang#118666 (coverage: Simplify the heuristic for ignoring `async fn` return spans) - rust-lang#118737 (Extend tidy alphabetical checking to `tests/`.) - rust-lang#118762 (Some more minor `async gen`-related nits) - rust-lang#118764 (Make async generators fused by default) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 9, 2023
Rollup merge of rust-lang#118666 - Zalathar:body-closure, r=cjgillot coverage: Simplify the heuristic for ignoring `async fn` return spans The code for extracting coverage spans from MIR has a special heuristic for dealing with `async fn`, so that the function's closing brace does not have a confusing double count. The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand. We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop. The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for `async fn` have not changed. --- This PR also has the side-effect of fixing the `None some_prev` panic that started appearing after rust-lang#118525. The old code assumed that `prev` would always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to rust-lang#118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics. The new code uses an `if let` to inspect `prev`, which correctly does nothing if there is no span present.
This was referenced Dec 12, 2023
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Dec 18, 2023
coverage: Skip instrumenting a function if no spans were extracted from MIR The immediate symptoms of rust-lang#118643 were fixed by rust-lang#118666, but some users reported that their builds now encounter another coverage-related ICE: ``` error: internal compiler error: compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs:98:17: A used function should have had coverage mapping data but did not: (...) ``` I was able to reproduce at least one cause of this error: if no relevant spans could be extracted from a function, but the function contains `CoverageKind::SpanMarker` statements, then codegen still thinks the function is instrumented and complains about the fact that it has no coverage spans. This PR prevents that from happening in two ways: - If we didn't extract any relevant spans from MIR, skip instrumenting the entire function and don't create a `FunctionCoverateInfo` for it. - If coverage codegen sees a `CoverageKind::SpanMarker` statement, skip it early and avoid creating `func_coverage`. --- Fixes rust-lang#118850.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 18, 2023
Rollup merge of rust-lang#118852 - Zalathar:no-spans, r=cjgillot coverage: Skip instrumenting a function if no spans were extracted from MIR The immediate symptoms of rust-lang#118643 were fixed by rust-lang#118666, but some users reported that their builds now encounter another coverage-related ICE: ``` error: internal compiler error: compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs:98:17: A used function should have had coverage mapping data but did not: (...) ``` I was able to reproduce at least one cause of this error: if no relevant spans could be extracted from a function, but the function contains `CoverageKind::SpanMarker` statements, then codegen still thinks the function is instrumented and complains about the fact that it has no coverage spans. This PR prevents that from happening in two ways: - If we didn't extract any relevant spans from MIR, skip instrumenting the entire function and don't create a `FunctionCoverateInfo` for it. - If coverage codegen sees a `CoverageKind::SpanMarker` statement, skip it early and avoid creating `func_coverage`. --- Fixes rust-lang#118850.
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.
The code for extracting coverage spans from MIR has a special heuristic for dealing with
async fn, so that the function's closing brace does not have a confusing double count.The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand.
We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop.
The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for
async fnhave not changed.This PR also has the side-effect of fixing the
None some_prevpanic that started appearing after #118525.The old code assumed that
prevwould always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to #118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics.The new code uses an
if letto inspectprev, which correctly does nothing if there is no span present.