coverage: Split out counter increment sites from BCB node/edge counters #120564
Conversation
This makes it possible for two nodes/edges in the coverage graph to share the same counter, without causing the instrumentor to inject unwanted duplicate counter-increment statements.
|
r? @estebank (rustbot has picked a reviewer for you, use r? to override) |
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
For historical context, the original implementation of coverage didn't really distinguish between referring to a counter and incrementing that counter. The work done for rust-lang/compiler-team#645 ended up creating a clean separation between those two concepts in most of the coverage code, but the one remaining holdout was the code that is updated by this PR. This is a necessary step towards allowing the instrumentor to be a bit smarter about how it assigns counters to nodes/edges in the coverage graph. For example, future PRs might allow it to observe that the coverage expression |
|
The change lgtm. Is it possible to design a test that is changed by this PR? It seems like an important enough edge case to warrant ensuring the generated coverage statements don't change across future changes. |
|
By itself, this PR still inserts exactly the same counters in exactly the same places. It's now legal for multiple nodes or edges to share the same counter, but the code that actually sets up the counters was historically written under the assumption that sharing was not allowed (and isn't substantially changed by this PR), so currently that sharing never actually happens. (I've been experimenting with some follow-up changes that do take advantage of this PR, but they are more involved and need some more polish, so I haven't included them here.) If a future change somehow causes counters to be inappropriately duplicated, or inserted in the wrong places, that should typically show up as incorrect line coverage counts in the |
|
@bors r+ rollup |
coverage: Split out counter increment sites from BCB node/edge counters This makes it possible for two nodes/edges in the coverage graph to share the same counter, without causing the instrumentor to inject unwanted duplicate counter-increment statements. --- `@rustbot` label +A-code-coverage
coverage: Split out counter increment sites from BCB node/edge counters This makes it possible for two nodes/edges in the coverage graph to share the same counter, without causing the instrumentor to inject unwanted duplicate counter-increment statements. --- ``@rustbot`` label +A-code-coverage
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#120302 (various const interning cleanups) - rust-lang#120520 (Some cleanups around diagnostic levels.) - rust-lang#120521 (Make `NonZero` constructors generic.) - rust-lang#120527 (Switch OwnedStore handle count to AtomicU32) - rust-lang#120564 (coverage: Split out counter increment sites from BCB node/edge counters) - rust-lang#120575 (Simplify codegen diagnostic handling) - rust-lang#120597 (Suggest `[tail @ ..]` on `[..tail]` and `[...tail]` where `tail` is unresolved) - rust-lang#120609 (hir: Stop keeping prefixes for most of `use` list stems) - rust-lang#120633 (pattern_analysis: gather up place-relevant info) r? `@ghost` `@rustbot` modify labels: rollup
coverage: Split out counter increment sites from BCB node/edge counters This makes it possible for two nodes/edges in the coverage graph to share the same counter, without causing the instrumentor to inject unwanted duplicate counter-increment statements. --- ```@rustbot``` label +A-code-coverage
Rollup of 13 pull requests Successful merges: - rust-lang#110482 (Add armv8r-none-eabihf target for the Cortex-R52.) - rust-lang#119162 (Add unstable `-Z direct-access-external-data` cmdline flag for `rustc`) - rust-lang#120302 (various const interning cleanups) - rust-lang#120455 ( Add FileCheck annotations to MIR-opt SROA tests) - rust-lang#120470 (Mark "unused binding" suggestion as maybe incorrect) - rust-lang#120479 (Suggest turning `if let` into irrefutable `let` if appropriate) - rust-lang#120564 (coverage: Split out counter increment sites from BCB node/edge counters) - rust-lang#120633 (pattern_analysis: gather up place-relevant info) - rust-lang#120664 (Add parallel rustc ui tests) - rust-lang#120721 (fix `llvm_out` to use the correct LLVM root) - rust-lang#120726 (Don't use bashism in checktools.sh) - rust-lang#120733 (MirPass: make name more const) - rust-lang#120735 (Remove some `unchecked_claim_error_was_emitted` calls) Failed merges: - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered") r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 13 pull requests Successful merges: - rust-lang#110482 (Add armv8r-none-eabihf target for the Cortex-R52.) - rust-lang#119162 (Add unstable `-Z direct-access-external-data` cmdline flag for `rustc`) - rust-lang#120302 (various const interning cleanups) - rust-lang#120455 ( Add FileCheck annotations to MIR-opt SROA tests) - rust-lang#120470 (Mark "unused binding" suggestion as maybe incorrect) - rust-lang#120479 (Suggest turning `if let` into irrefutable `let` if appropriate) - rust-lang#120564 (coverage: Split out counter increment sites from BCB node/edge counters) - rust-lang#120633 (pattern_analysis: gather up place-relevant info) - rust-lang#120664 (Add parallel rustc ui tests) - rust-lang#120726 (Don't use bashism in checktools.sh) - rust-lang#120733 (MirPass: make name more const) - rust-lang#120735 (Remove some `unchecked_claim_error_was_emitted` calls) - rust-lang#120746 (Record coroutine kind in coroutine generics) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#120564 - Zalathar:increment-site, r=oli-obk coverage: Split out counter increment sites from BCB node/edge counters This makes it possible for two nodes/edges in the coverage graph to share the same counter, without causing the instrumentor to inject unwanted duplicate counter-increment statements. --- ````@rustbot```` label +A-code-coverage
This makes it possible for two nodes/edges in the coverage graph to share the same counter, without causing the instrumentor to inject unwanted duplicate counter-increment statements.
@rustbot label +A-code-coverage