coverage: Clean up marker statements that aren't needed later #122542

Zalathar · 2024-03-15T12:11:55Z

Some of the marker statements used by coverage are added during MIR building for use by the InstrumentCoverage pass (during analysis), and are not needed afterwards.

@rustbot label +A-code-coverage

rustbot · 2024-03-15T12:12:03Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-15T12:12:05Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2024-03-15T13:47:18Z

I’ve had the idea of doing this for a while, but I knew it couldn’t be done directly in InstrumentCoverage (because you can’t have nop statements that early), so I figured it would require its own separate clean pass.

But then it occurred to me to look for existing cleanup passes to piggy-back on, and this seems like a perfect fit.

Zalathar · 2024-03-16T02:29:28Z

Based on #114917 (comment) I suspect the //@ ignore-windows isn't actually needed for coverage mir-opt tests.

But I think I'll leave things as-is in this PR, and then change the ignores in a follow-up PR, so that if anything goes wrong the changes won't be mixed together.

Zalathar · 2024-03-22T06:43:09Z

But I think I'll leave things as-is in this PR, and then change the ignores in a follow-up PR, so that if anything goes wrong the changes won't be mixed together.

I ended up changing this after all. The new test now runs on any target, and uses -Zno-profiler-runtime instead of //@ needs-profiler-support.

Zalathar · 2024-03-22T06:47:58Z

This is now a prerequisite of #122860.

~~It can still be reviewed independently in its own right, or it can be incorporated into the review of that PR.~~

oli-obk · 2024-03-22T08:26:39Z

compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs

+                        // These kinds of coverage statements are markers inserted during
+                        // MIR building, and are not needed after InstrumentCoverage.
+                        kind: CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. },


Please also add this to validation to ensure they never get reintroduced

I added an appropriate check to CfgChecker (diff).

rustbot · 2024-03-22T09:11:37Z

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Zalathar · 2024-03-22T09:14:26Z

The only change to syntax.rs is describing the new validation rule in the docs for AnalysisPhase::PostCleanup.

Zalathar · 2024-03-22T09:18:22Z

Oh, I guess mir-opt already sets -Zvalidate-mir, so I don't need to set it explicitly.

Some of the marker statements used by coverage are added during MIR building for use by the InstrumentCoverage pass (during analysis), and are not needed afterwards.

oli-obk · 2024-03-22T09:22:34Z

@bors r+ rollup

bors · 2024-03-22T09:22:37Z

📌 Commit 91aae58 has been approved by oli-obk

It is now in the queue for this repository.

coverage: Clean up marker statements that aren't needed later Some of the marker statements used by coverage are added during MIR building for use by the InstrumentCoverage pass (during analysis), and are not needed afterwards. `@rustbot` label +A-code-coverage

coverage: Clean up marker statements that aren't needed later Some of the marker statements used by coverage are added during MIR building for use by the InstrumentCoverage pass (during analysis), and are not needed afterwards. ``@rustbot`` label +A-code-coverage

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#121619 (Experimental feature postfix match) - rust-lang#122370 (Gracefully handle `AnonConst` in `diagnostic_hir_wf_check()`) - rust-lang#122537 (interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit) - rust-lang#122542 (coverage: Clean up marker statements that aren't needed later) - rust-lang#122800 (Add `NonNull::<[T]>::is_empty`.) - rust-lang#122820 (Stop using `<DefId as Ord>` in various diagnostic situations) - rust-lang#122847 (Suggest `RUST_MIN_STACK` workaround on overflow) - rust-lang#122855 (Fix Itanium mangling usizes) - rust-lang#122863 (add more ice tests ) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122542 - Zalathar:cleanup, r=oli-obk coverage: Clean up marker statements that aren't needed later Some of the marker statements used by coverage are added during MIR building for use by the InstrumentCoverage pass (during analysis), and are not needed afterwards. ```@rustbot``` label +A-code-coverage

…ulacrum Clean up unnecessary headers/flags in coverage mir-opt tests During rust-lang#122542, I noticed that some of the headers and flags I had copied over from `tests/mir-opt/instrument_coverage.rs` were unnecessary. And while working to remove those, I noticed even more that could be removed or replaced.

Rollup merge of rust-lang#122995 - Zalathar:flags-mir-opt, r=Mark-Simulacrum Clean up unnecessary headers/flags in coverage mir-opt tests During rust-lang#122542, I noticed that some of the headers and flags I had copied over from `tests/mir-opt/instrument_coverage.rs` were unnecessary. And while working to remove those, I noticed even more that could be removed or replaced.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#121619 (Experimental feature postfix match) - rust-lang#122370 (Gracefully handle `AnonConst` in `diagnostic_hir_wf_check()`) - rust-lang#122537 (interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit) - rust-lang#122542 (coverage: Clean up marker statements that aren't needed later) - rust-lang#122800 (Add `NonNull::<[T]>::is_empty`.) - rust-lang#122820 (Stop using `<DefId as Ord>` in various diagnostic situations) - rust-lang#122847 (Suggest `RUST_MIN_STACK` workaround on overflow) - rust-lang#122855 (Fix Itanium mangling usizes) - rust-lang#122863 (add more ice tests ) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned fmease Mar 15, 2024

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 Mar 15, 2024

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Mar 15, 2024

Zalathar force-pushed the cleanup branch 2 times, most recently from d137432 to 49029e3 Compare March 15, 2024 12:32

Swatinem approved these changes Mar 15, 2024

View reviewed changes

Zalathar force-pushed the cleanup branch from 49029e3 to c04a152 Compare March 18, 2024 02:33

Zalathar mentioned this pull request Mar 22, 2024

coverage: Re-enable UnreachablePropagation for coverage builds #122860

Merged

Zalathar force-pushed the cleanup branch 2 times, most recently from 8aa9f01 to 313b6a0 Compare March 22, 2024 06:37

oli-obk reviewed Mar 22, 2024

View reviewed changes

Zalathar force-pushed the cleanup branch from 313b6a0 to 42d7797 Compare March 22, 2024 09:11

coverage: Clean up marker statements that aren't needed later

91aae58

Some of the marker statements used by coverage are added during MIR building for use by the InstrumentCoverage pass (during analysis), and are not needed afterwards.

Zalathar force-pushed the cleanup branch from 42d7797 to 91aae58 Compare March 22, 2024 09:20

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2024

matthiaskrgr mentioned this pull request Mar 22, 2024

Rollup of 8 pull requests #122866

Closed

This was referenced Mar 22, 2024

Rollup of 7 pull requests #122867

Closed

Rollup of 9 pull requests #122869

Merged

bors merged commit e13c40c into rust-lang:master Mar 22, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Mar 22, 2024

Zalathar deleted the cleanup branch March 22, 2024 21:57

Zalathar mentioned this pull request Mar 24, 2024

Clean up unnecessary headers/flags in coverage mir-opt tests #122995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Clean up marker statements that aren't needed later #122542

coverage: Clean up marker statements that aren't needed later #122542

Zalathar commented Mar 15, 2024

rustbot commented Mar 15, 2024

rustbot commented Mar 15, 2024

Zalathar commented Mar 15, 2024

Zalathar commented Mar 16, 2024

Zalathar commented Mar 22, 2024

Zalathar commented Mar 22, 2024 •

edited

oli-obk Mar 22, 2024

Zalathar Mar 22, 2024

rustbot commented Mar 22, 2024

Zalathar commented Mar 22, 2024

Zalathar commented Mar 22, 2024

oli-obk commented Mar 22, 2024

bors commented Mar 22, 2024

coverage: Clean up marker statements that aren't needed later #122542

coverage: Clean up marker statements that aren't needed later #122542

Conversation

Zalathar commented Mar 15, 2024

rustbot commented Mar 15, 2024

rustbot commented Mar 15, 2024

Zalathar commented Mar 15, 2024

Zalathar commented Mar 16, 2024

Zalathar commented Mar 22, 2024

Zalathar commented Mar 22, 2024 • edited

oli-obk Mar 22, 2024

Choose a reason for hiding this comment

Zalathar Mar 22, 2024

Choose a reason for hiding this comment

rustbot commented Mar 22, 2024

Zalathar commented Mar 22, 2024

Zalathar commented Mar 22, 2024

oli-obk commented Mar 22, 2024

bors commented Mar 22, 2024

Zalathar commented Mar 22, 2024 •

edited