Mark unions non-const-propagatable in KnownPanicsLint without calling layout
#124504
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
as they have a potential to ICE during layout calculation
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
gurry
changed the title
KnownPanicsLintKnownPanicsLint without calling layout
oli-obk
reviewed
Apr 29, 2024
|
Though the reason it ICEs is that unions are incompatible with const propagation, as Rust has no concept of "active variant" of unions. So either we permit using unions for transmutes in const prop, or we can't support them at all. The reason we don't allow unions transmutes in const prop is that the current design of const prop just isn't compatible with it @bors r+ rollup |
bors
added
S-waiting-on-bors
Oh I see. Thanks for the context. I'll try to add it in the comments in a subsequent PR. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 29, 2024
Rollup of 7 pull requests Successful merges: - rust-lang#124269 (Pretty-print parenthesis around binary in postfix match) - rust-lang#124415 (Use probes more aggressively in new solver) - rust-lang#124475 (Remove direct dependencies on lazy_static, once_cell and byteorder) - rust-lang#124484 (Fix rust-lang#124478 - offset_of! returns a temporary) - rust-lang#124504 (Mark unions non-const-propagatable in `KnownPanicsLint` without calling layout) - rust-lang#124508 (coverage: Avoid hard-coded values when visiting logical ops) - rust-lang#124522 ([Refactor] Rename `Lint` and `LintGroup`'s `is_loaded` to `is_externally_loaded` ) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 29, 2024
Rollup merge of rust-lang#124504 - gurry:123710-union-ICE, r=oli-obk Mark unions non-const-propagatable in `KnownPanicsLint` without calling layout Fixes rust-lang#123710 The ICE occurs during the layout calculation of the union `InvalidTag` in rust-lang#123710 because the following assert fails:https://github.com/rust-lang/rust/blob/5fe8b697e729b6eb64841a3905e57da1b47f4ca3/compiler/rustc_abi/src/layout.rs#L289-L292 The layout calculation is invoked by `KnownPanicsLint` when it is trying to figure out which locals it can const prop. Since `KnownPanicsLint` is never actually going to const props unions thanks to PR rust-lang#121628 there's no point calling layout to check if it can. So in this fix I skip the call to layout and just mark the local non-const propagatable if it is a union.
Updated the comment in #124550 |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Apr 30, 2024
…-obk Remove redundant union check in `KnownPanicsLint` const prop Removes the below check which prevents unions from being const propagated:https://github.com/rust-lang/rust/blob/f9dca46218d4b8efa062aec4fd0820cbb4942aa2/compiler/rustc_mir_transform/src/known_panics_lint.rs#L587-L594 It is not needed because after PR rust-lang#124504 we mark unions as `NoPropagation` over here: https://github.com/rust-lang/rust/blob/f9dca46218d4b8efa062aec4fd0820cbb4942aa2/compiler/rustc_mir_transform/src/known_panics_lint.rs#L899-L902 which is enough to prevent them from being const propagated.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 30, 2024
Rollup merge of rust-lang#124550 - gurry:remove-redundant-code, r=oli-obk Remove redundant union check in `KnownPanicsLint` const prop Removes the below check which prevents unions from being const propagated:https://github.com/rust-lang/rust/blob/f9dca46218d4b8efa062aec4fd0820cbb4942aa2/compiler/rustc_mir_transform/src/known_panics_lint.rs#L587-L594 It is not needed because after PR rust-lang#124504 we mark unions as `NoPropagation` over here: https://github.com/rust-lang/rust/blob/f9dca46218d4b8efa062aec4fd0820cbb4942aa2/compiler/rustc_mir_transform/src/known_panics_lint.rs#L899-L902 which is enough to prevent them from being const propagated.
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.
Fixes #123710
The ICE occurs during the layout calculation of the union
InvalidTagin #123710 because the following assert fails:rust/compiler/rustc_abi/src/layout.rs
Lines 289 to 292 in 5fe8b69
The layout calculation is invoked by
KnownPanicsLintwhen it is trying to figure out which locals it can const prop. SinceKnownPanicsLintis never actually going to const props unions thanks to PR #121628 there's no point calling layout to check if it can. So in this fix I skip the call to layout and just mark the local non-const propagatable if it is a union.