rustc_mir_transform: Enforce rustc::potential_query_instability lint
#119252
Conversation
This file contains 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
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
cjgillot
reviewed
Dec 25, 2023
|
r? @cjgillot since they already started review |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Dec 29, 2023
coverage: Avoid a possible query stability hazard in `CoverageCounters` rust-lang#119252 revealed a possible query stability hazard in `CoverageCounters`: we iterate over the entries of an `FxHashMap` in a way that allows the iteration order to potentially affect the relative creation order of MIR blocks. I'm not sure whether there's an actual stability problem or not in practice, but it's certainly a hazard, and I don't see any reason not to switch over to `FxIndexMap` to avoid potential issues. --- This can either be merged on its own, or incorporated into rust-lang#119252. cc `@Enselic` r? `@cjgillot`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 29, 2023
Rollup merge of rust-lang#119401 - Zalathar:query-stability, r=Nilstrieb coverage: Avoid a possible query stability hazard in `CoverageCounters` rust-lang#119252 revealed a possible query stability hazard in `CoverageCounters`: we iterate over the entries of an `FxHashMap` in a way that allows the iteration order to potentially affect the relative creation order of MIR blocks. I'm not sure whether there's an actual stability problem or not in practice, but it's certainly a hazard, and I don't see any reason not to switch over to `FxIndexMap` to avoid potential issues. --- This can either be merged on its own, or incorporated into rust-lang#119252. cc `@Enselic` r? `@cjgillot`
cjgillot
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #116012) made this pull request unmergeable. Please resolve the merge conflicts. |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 5, 2024
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 5, 2024
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? ```@cjgillot``` who is reviewer for the highly related PR rust-lang#119252.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 6, 2024
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 6, 2024
Rollup merge of rust-lang#119591 - Enselic:DestinationPropagation-stable, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
Stop allowing `rustc::potential_query_instability` on all of rustc_mir_transform and instead allow it on a case-by-case basis if it is safe to do so. In this particular crate, all instances were safe to allow.
Enselic
force-pushed
the
rustc_mir_transform-query-stability
branch
from
1a0f173
to
6d8fb57
Compare
Enselic
added
S-waiting-on-review
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 7, 2024
…mpiler-errors Rollup of 6 pull requests Successful merges: - rust-lang#119252 (rustc_mir_transform: Enforce `rustc::potential_query_instability` lint) - rust-lang#119548 (Use the current target instead of iterating over all targets) - rust-lang#119656 (document rounding behavior of rint/nearbyint for ties) - rust-lang#119657 (Fix typo in docs for slice::split_once, slice::rsplit_once) - rust-lang#119666 (Populate `yield` and `resume` types in MIR body while body is being initialized) - rust-lang#119679 (Ask for rustc version in diagnostic reports, remind users to update their toolchain) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 7, 2024
…mpiler-errors Rollup of 6 pull requests Successful merges: - rust-lang#119252 (rustc_mir_transform: Enforce `rustc::potential_query_instability` lint) - rust-lang#119548 (Use the current target instead of iterating over all targets) - rust-lang#119656 (document rounding behavior of rint/nearbyint for ties) - rust-lang#119657 (Fix typo in docs for slice::split_once, slice::rsplit_once) - rust-lang#119666 (Populate `yield` and `resume` types in MIR body while body is being initialized) - rust-lang#119679 (Ask for rustc version in diagnostic reports, remind users to update their toolchain) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 7, 2024
Rollup merge of rust-lang#119252 - Enselic:rustc_mir_transform-query-stability, r=cjgillot rustc_mir_transform: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` on all of rustc_mir_transform and instead allow it on a case-by-case basis if it is safe to do so. In this particular crate, all instances were safe to allow. Part of rust-lang#84447 which is E-help-wanted.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Stop allowing
rustc::potential_query_instabilityon all of rustc_mir_transform and instead allow it on a case-by-case basis if it is safe to do so. In this particular crate, all instances were safe to allow.Part of #84447 which is E-help-wanted.