Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

Zalathar
Copy link
Contributor

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
Copy link
Collaborator

rustbot commented Mar 15, 2024

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 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
Copy link
Collaborator

rustbot commented Mar 15, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Mar 15, 2024
@Zalathar Zalathar force-pushed the cleanup branch 2 times, most recently from d137432 to 49029e3 Compare March 15, 2024 12:32
@Zalathar
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Zalathar commented Mar 22, 2024

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.

Comment on lines +34 to +36
// These kinds of coverage statements are markers inserted during
// MIR building, and are not needed after InstrumentCoverage.
kind: CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an appropriate check to CfgChecker (diff).

@rustbot
Copy link
Collaborator

rustbot commented Mar 22, 2024

This PR changes MIR

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

@Zalathar
Copy link
Contributor Author

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

@Zalathar
Copy link
Contributor Author

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
Copy link
Contributor

oli-obk commented Mar 22, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 22, 2024

📌 Commit 91aae58 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 added a commit to matthiaskrgr/rust that referenced this pull request Mar 22, 2024
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 22, 2024
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
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2024
…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
@bors bors merged commit e13c40c into rust-lang:master Mar 22, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2024
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
@Zalathar Zalathar deleted the cleanup branch March 22, 2024 21:57
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 25, 2024
…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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 25, 2024
…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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 25, 2024
…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.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2024
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.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 4, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants