coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions #118198

Zalathar · 2023-11-23T02:30:51Z

Coverage instrumentation works by extracting source code spans from MIR. However, some kinds of syntax are effectively erased during MIR building, so their spans don't necessarily exist anywhere in MIR, making them invisible to the coverage instrumentor (unless we resort to various heuristics and hacks to recover them).

This PR introduces CoverageKind::SpanMarker, which is a new variant of StatementKind::Coverage. Its sole purpose is to represent spans that would otherwise not appear in MIR, so that the coverage instrumentor can extract them.

When coverage is enabled, the MIR builder can insert these dummy statements as needed, to improve the accuracy of spans used by coverage mappings.

Fixes #115468.

@rustbot label +A-code-coverage

rustbot · 2023-11-23T02:30:58Z

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-11-23T02:31:00Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-11-23T05:59:24Z

It occurs to me that another approach would have been to make the UnOp::Not match arm conditional on !this.tcx.sess.instrument_coverage().

That would avoid the need to inject a span marker in this case, though I still think continue justifies having span markers on its own.

(And the downside would be that coverage builds wouldn't get to omit the temporary boolean value.)

Zalathar · 2023-11-23T22:47:11Z

It occurs to me that another approach would have been to make the UnOp::Not match arm conditional on !this.tcx.sess.instrument_coverage().

That would avoid the need to inject a span marker in this case, though I still think continue justifies having span markers on its own.

(And the downside would be that coverage builds wouldn't get to omit the temporary boolean value.)

Looking again at the thread for #111752, I realise that some of those match arms are now load-bearing for language semantics. I think that mostly applies to the &&/|| arms, but overall it means I'd have to be pretty careful about that sort of thing.

Swatinem

you mentioned that you plan to eventually move capturing this span information to HIR, so I think injecting these coverage statements here is just temporary?

Zalathar · 2023-11-26T00:43:10Z

I actually think of these span markers as mostly separate from the ones that I intend to introduce for capturing branch spans.

This PR was the result of a few different ideas coming together in my head:

I had been experimenting with marker statements for branch coverage, so the idea of injecting marker statements was on my mind.
I remembered that there is pre-existing code in MIR building that injects dummy assignment statements for continue expressions, so that they can be picked up properly by the coverage instrumentor.
I realised that introducing SpanMarker would let me clean up the continue handling a bit (by not having to inject “real” dummy statements), while also letting me solve Coverage instrumentation sometimes thinks the negation operator isn't executable #115468.

So I think these particular marker statements will hang around even after we also have proper branch detection in MIR building. These span markers are mainly about giving more accurate hints to the existing span-extraction code in the instrumentor, whereas the branch markers will be separate from that span-extraction stuff.

Zalathar · 2023-11-26T00:45:58Z

One distinction to note is that these span markers get injected into the basic block that represents the condition, whereas branch markers will get injected into the blocks representing the then/else arms.

cjgillot

r=me with 2 nits.

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

cjgillot · 2023-12-03T18:00:46Z

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -90,6 +90,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                let local_scope = this.local_scope();
                let (success_block, failure_block) =
                    this.in_if_then_scope(local_scope, expr_span, |this| {
+                        // Help out coverage instrumentation by injecting a dummy statement with
+                        // the original condition's span (including `!`). This fixes #115468.
+                        if this.tcx.sess.instrument_coverage() {


Should this test happen inside push_coverage_span_marker?

push_coverage_span_marker is a method on CFG, which doesn't have its own copy of tcx or sess.

So the only way to perform the test would be to also pass tcx/sess as an additional argument, which seems backwards to me. Better to perform the test in the outer code that already has access to the necessary information.

Having the test outside also makes it easier to see from outside that this code has no effect on the CFG in non-coverage builds.

cjgillot · 2023-12-03T18:02:37Z

compiler/rustc_mir_build/src/build/scope.rs

-        let temp_place = Place::from(self.local_decls.push(local_decl));
-        self.cfg.push_assign_unit(block, source_info, temp_place, self.tcx);
-    }
-


… MIR There are cases where coverage instrumentation wants to show a span for some syntax element, but there is no MIR node that naturally carries that span, so the instrumentor can't see it. MIR building can now use this new kind of coverage statement to deliberately include those spans in MIR, attached to a dummy statement that has no other effect.

This replaces the previous workaround, which was to inject a dummy `Assign` statement.

When MIR is built for an if-not expression, the `!` part of the condition doesn't correspond to any MIR statement, so coverage instrumentation normally can't see it. We can fix that by deliberately injecting a dummy statement whose sole purpose is to associate that span with its enclosing block.

cjgillot · 2023-12-09T00:07:56Z

@bors r+ rollup

bors · 2023-12-09T00:07:59Z

📌 Commit d90fd02 has been approved by cjgillot

It is now in the queue for this repository.

coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions Coverage instrumentation works by extracting source code spans from MIR. However, some kinds of syntax are effectively erased during MIR building, so their spans don't necessarily exist anywhere in MIR, making them invisible to the coverage instrumentor (unless we resort to various heuristics and hacks to recover them). This PR introduces `CoverageKind::SpanMarker`, which is a new variant of `StatementKind::Coverage`. Its sole purpose is to represent spans that would otherwise not appear in MIR, so that the coverage instrumentor can extract them. When coverage is enabled, the MIR builder can insert these dummy statements as needed, to improve the accuracy of spans used by coverage mappings. Fixes rust-lang#115468. --- `@rustbot` label +A-code-coverage

coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions Coverage instrumentation works by extracting source code spans from MIR. However, some kinds of syntax are effectively erased during MIR building, so their spans don't necessarily exist anywhere in MIR, making them invisible to the coverage instrumentor (unless we resort to various heuristics and hacks to recover them). This PR introduces `CoverageKind::SpanMarker`, which is a new variant of `StatementKind::Coverage`. Its sole purpose is to represent spans that would otherwise not appear in MIR, so that the coverage instrumentor can extract them. When coverage is enabled, the MIR builder can insert these dummy statements as needed, to improve the accuracy of spans used by coverage mappings. Fixes rust-lang#115468. --- ``@rustbot`` label +A-code-coverage

…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

…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

Rollup merge of rust-lang#118198 - Zalathar:if-not, r=cjgillot coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions Coverage instrumentation works by extracting source code spans from MIR. However, some kinds of syntax are effectively erased during MIR building, so their spans don't necessarily exist anywhere in MIR, making them invisible to the coverage instrumentor (unless we resort to various heuristics and hacks to recover them). This PR introduces `CoverageKind::SpanMarker`, which is a new variant of `StatementKind::Coverage`. Its sole purpose is to represent spans that would otherwise not appear in MIR, so that the coverage instrumentor can extract them. When coverage is enabled, the MIR builder can insert these dummy statements as needed, to improve the accuracy of spans used by coverage mappings. Fixes rust-lang#115468. --- ```@rustbot``` label +A-code-coverage

Zalathar · 2023-12-12T03:21:32Z

This turns out to have contributed to the emergence of #118850.

coverage: Regression test for `assert!(!false)` This verifies that rust-lang#118904 has already been fixed by rust-lang#118198. --- `@rustbot` label +A-code-coverage

rustbot assigned compiler-errors Nov 23, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 23, 2023

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Nov 23, 2023

Zalathar force-pushed the if-not branch 2 times, most recently from d1f2f5c to 913183e Compare November 23, 2023 02:58

Zalathar force-pushed the if-not branch from 913183e to b1d9598 Compare November 25, 2023 01:29

Swatinem approved these changes Nov 25, 2023

View reviewed changes

Zalathar force-pushed the if-not branch 3 times, most recently from 49100ec to 5f0d15f Compare November 29, 2023 01:16

cjgillot reviewed Dec 3, 2023

View reviewed changes

Zalathar force-pushed the if-not branch 2 times, most recently from 389d682 to 9b0d342 Compare December 4, 2023 00:30

compiler-errors assigned cjgillot and unassigned compiler-errors Dec 5, 2023

Zalathar added 4 commits December 8, 2023 22:40

coverage: Add a dedicated test for coverage of if !

d2d742c

coverage: Use SpanMarker to mark continue expressions.

9816635

This replaces the previous workaround, which was to inject a dummy `Assign` statement.

Zalathar force-pushed the if-not branch from 9b0d342 to d90fd02 Compare December 8, 2023 11:45

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 9, 2023

workingjubilee mentioned this pull request Dec 9, 2023

Rollup of 9 pull requests #118767

Closed

workingjubilee mentioned this pull request Dec 9, 2023

Rollup of 8 pull requests #118768

Closed

workingjubilee mentioned this pull request Dec 9, 2023

Rollup of 7 pull requests #118771

Merged

bors merged commit a71ab45 into rust-lang:master Dec 9, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Dec 9, 2023

Zalathar deleted the if-not branch December 9, 2023 12:44

This was referenced Dec 16, 2023

Missing coverage on assert!(!...) statements #118904

Closed

coverage: Regression test for assert!(!false) #119011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions #118198

coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions #118198

Zalathar commented Nov 23, 2023

rustbot commented Nov 23, 2023

rustbot commented Nov 23, 2023

Zalathar commented Nov 23, 2023

Zalathar commented Nov 23, 2023

Swatinem left a comment

Zalathar commented Nov 26, 2023

Zalathar commented Nov 26, 2023

cjgillot left a comment

cjgillot Dec 3, 2023

Zalathar Dec 4, 2023

cjgillot Dec 3, 2023

cjgillot commented Dec 9, 2023

bors commented Dec 9, 2023

Zalathar commented Dec 12, 2023

coverage: Use SpanMarker to improve coverage spans for if ! expressions #118198

coverage: Use SpanMarker to improve coverage spans for if ! expressions #118198

Conversation

Zalathar commented Nov 23, 2023

rustbot commented Nov 23, 2023

rustbot commented Nov 23, 2023

Zalathar commented Nov 23, 2023

Zalathar commented Nov 23, 2023

Swatinem left a comment

Choose a reason for hiding this comment

Zalathar commented Nov 26, 2023

Zalathar commented Nov 26, 2023

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot Dec 3, 2023

Choose a reason for hiding this comment

Zalathar Dec 4, 2023

Choose a reason for hiding this comment

cjgillot Dec 3, 2023

Choose a reason for hiding this comment

cjgillot commented Dec 9, 2023

bors commented Dec 9, 2023

Zalathar commented Dec 12, 2023

coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions #118198

coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions #118198