Rollup of 3 pull requests - #162353
Merged
Merged
Conversation
…un-make` without failing if no check is present
This variant will be used for marker statements that are injected when a HIR expression is about to be lowered to MIR, and at other points relevant to coverage instrumentation. Adding this variant in its own commit keeps the mundane plumbing steps separate from later changes that will perform injection and analysis.
In order to make coverage instrumentation HIR-aware, we need to have MIR building inject marker statements at key locations, so that instrumentation can accurately reconstruct the relationship between HIR constructs and MIR-level control flow. The most important of these is `PointKind::Expr`, which associates a HIR expression with the start of the MIR that will evaluate that expression. There isn't a single convenient place to inject these, but we can get close by observing that THIR building wraps every expression in a `thir::ExprKind::Scope` node, which also contains the `HirId` of that expression. If we inject a marker whenever `thir::ExprKind::Scope` is lowered, that results in an accurate marker for every expression. Other markers are also injected at key points that don't correspond directly to a HIR node, to avoid major regressions in coverage-map quality.
Instead of trying to heuristically recover source-code spans from MIR soup, we can now take advantage of the `CoverageKind::Point` statements injected for specific HIR expressions. This is a transitional implementation that lets us remove the old MIR-soup code, but still relies heavily on the existing span-refinement heuristics, and does not take advantage of the full possibilities of HIR-aware coverage instrumentation. In order to avoid annoying never-executed spans in code that contains assertions, a heuristic has been added that ignores any expression within the arguments of a macro-expanded function call that returns `!`. Some coverage tests now require `//@ min-llvm-version: 23`, due to changes in how LLVM sorts the coverage mapping entries.
Now that we have a span for every HIR expression, we don't need these markers.
coverage: Switch over to hybrid HIR-aware coverage spans --- One of the deep flaws in coverage instrumentation at the moment is that the instrumentor doesn't have actual knowledge of source-level code structure. Instead, it resorts to guessing source spans based on scanning through MIR statements and terminators, and applying a few crude heuristics that can never give truly accurate results. (The decision to use MIR-based spans was made during the original implementation of `-Zinstrument-coverage`. I was not able to find any discussion of this design decision or its consequences in the stabilization thread at rust-lang#90132.) This PR is a partial step towards a better way of doing things. During THIR-to-MIR lowering, we now inject special `CoverageKind::Point` marker statements that represent a specific connection between some part of the source code (in HIR form) and a specific point in MIR control-flow. Instead of trying to recover coverage spans from arbitrary MIR statements/terminators, we now only extract spans from those dedicated marker statements. Having accurate HIR-to-MIR correlations unlocks a lot of exciting possibilities for better coverage instrumentation. This PR mostly sets the stage for potential future improvements, but does demonstrate the ability to exclude specific HIR expressions from the set of coverage spans (to make assertions less noisy), which was previously unthinkable. --- The key to making this all work is being able to accurately inject marker statements during MIR building. There is no single point in MIR building that all THIR expressions pass through, and most THIR expression nodes do not keep track of their corresponding HirId. However, it turns out that THIR building already wraps every expression node in a `thir::ExprKind::Scope` node that *does* store the original HirId, and the number of places in MIR building that handle those scopes is relatively small. So if we simply inject an appropriate marker statement at those places, with the HirId that is already available, that takes care of marking every HIR-level expression node in the built MIR. To avoid major regressions in coverage-span quality, we also need to inject markers in a few extra places that don't correspond to a HIR node, because they represent implied code such as the `else {}` of a one-sided `if`, or the automatic `return` at the end of a function body. --- There are necessarily some changes to coverage output as a result of the new approach. I have tried to keep these minimal, but I don't think it's worth trying to chase 1:1 compatibility with the old spans, which were very much a product of several layers of implementation details and heuristics. The differences seem reasonable overall, and in some cases are a clear improvement.
…-html, r=fmease Prevent `--test` to be used in `rustdoc-html` testsuite As [promised](rust-lang#162014 (comment)). CI will fail until rust-lang#162014 is merged though. r? @fmease
…t-extra-borrow, r=estebank Suggest extra borrows for nested reference arguments Fixes rust-lang#78613 r? @estebank
rustbot
added
A-compiletest
Member
Author
|
@bors r+ p=5 |
Contributor
rust-bors
Bot
added
the
S-waiting-on-bors
rust-bors
Bot
removed
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
rust-bors
Bot
added
merged-by-bors
Contributor
This was referenced Sep 6, 2026
Contributor
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing dbad1ba (parent) -> 46a266b (this PR) Test differencesShow 253 test diffsStage 1
Stage 2
(and 27 additional test diffs) Additionally, 126 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 46a266b9fb0fb75e05fa4c9132f7355fcb62d4a4 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Contributor
|
📌 Perf builds for each rolled up PR:
parent commit: dbad1bab60 In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
Collaborator
|
Finished benchmarking commit (46a266b): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary 1.6%, secondary 1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.1%, secondary -3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 478.423s -> 478.283s (-0.03%) |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Successful merges:
--testto be used inrustdoc-htmltestsuite #162030 (Prevent--testto be used inrustdoc-htmltestsuite)r? @ghost
Create a similar rollup