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

Do not const prop unions #121628

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Do not const prop unions #121628

merged 1 commit into from
Feb 26, 2024

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Feb 26, 2024

Unions can produce values whose types don't match their underlying layout types which can lead to ICEs on eval.

Fixes #121534

@rustbot
Copy link
Collaborator

rustbot commented Feb 26, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
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 Feb 26, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 26, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

as they can made to produce values whose types don't
match their underlying layout types which can lead to
ICEs on eval
@oli-obk oli-obk changed the title Do not const pop unions Do not const prop unions Feb 26, 2024
})
.collect(),
variant: match **kind {
AggregateKind::Adt(_, variant, _, _, _) => variant,
Copy link
Contributor

Choose a reason for hiding this comment

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

could add the bail out logic here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my first approach, but it looked ugly especially with that longish comment

@oli-obk
Copy link
Contributor

oli-obk commented Feb 26, 2024

r? oli-obk

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 26, 2024

📌 Commit 633c92c 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 Feb 26, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#121389 (llvm-wrapper: fix few warnings)
 - rust-lang#121493 (By changing some attributes to only_local, reducing encoding attributes in the crate metadate.)
 - rust-lang#121615 (Move `emit_stashed_diagnostic` call in rustfmt.)
 - rust-lang#121617 (Actually use the right closure kind when checking async Fn goals)
 - rust-lang#121628 (Do not const prop unions)
 - rust-lang#121629 (fix some references to no-longer-existing ReprOptions.layout_seed)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 218be27 into rust-lang:master Feb 26, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
Rollup merge of rust-lang#121628 - gurry:121534-ice-asymm-binop, r=oli-obk

Do not const prop unions

Unions can produce values whose types don't match their underlying layout types which can lead to ICEs on eval.

Fixes rust-lang#121534
@gurry gurry deleted the 121534-ice-asymm-binop branch February 27, 2024 02:45
@cjgillot
Copy link
Contributor

I suspect that this PR does not fully fix #121534. From a glance, that ICE is caused by projecting a union field. This PR prohibits creating constants of union type in const-prop. We could still have constants of union type due to inline consts for instance.

@gurry
Copy link
Contributor Author

gurry commented Mar 24, 2024

I suspect that this PR does not fully fix #121534. From a glance, that ICE is caused by projecting a union field. This PR prohibits creating constants of union type in const-prop. We could still have constants of union type due to inline consts for instance.

That makes sense, but I tested it and the eval of inline consts does not ICE 😑.

This snippet, which is effectively the one reported in #121534, does ICE:

union Union {
    u32_field: u32,
    i32_field: i32,
}

pub fn main() {
    let u32_variant = Union { u32_field: 2 };
    let i32_variant = Union { i32_field: 10 };
    let a = unsafe { u32_variant.u32_field };
    let b = unsafe { i32_variant.u32_field };

    let _diff = b - a;
}

(Plaground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9aa52ce49ce5c1a37bde946ab6e74631)

but this one, which evaluates an inline const with the exact same code in the body, does not:

union Union {
    u32_field: u32,
    i32_field: i32,
}

const C: u32 = {
    let u32_variant = Union { u32_field: 2 };
    let i32_variant = Union { i32_field: 10 };
    let a = unsafe { u32_variant.u32_field };
    let b = unsafe { i32_variant.u32_field };

    b - a
};

pub fn main() {
    let a = [3; C as usize];
    println!("{}", a.len())
}

(Playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4bcf0f59f14964a9c090aa8bdc2d043e)

In fact, not only does it not ICE, it prints the correct value of 8.

I've looked into the location where the ICE originates in bin_int_op():

if left_layout.ty != right_layout.ty {

and found that in the first case the layout tys of the operands passed to this method are different (left_layout.ty is i32 and right_layout.ty is u32), but in the second case they are the same (both u32). I have not investigated further as to why this is. Perhaps some accidental twist of type inference.

@cjgillot If you can share a different reproducer, I can try again.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Apr 29, 2024
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.
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.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request May 3, 2024
Mark unions non-const-propagatable in `KnownPanicsLint` without calling layout

Fixes #123710

The ICE occurs during the layout calculation of the union `InvalidTag` in #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/rust#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.
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#121389 (llvm-wrapper: fix few warnings)
 - rust-lang#121493 (By changing some attributes to only_local, reducing encoding attributes in the crate metadate.)
 - rust-lang#121615 (Move `emit_stashed_diagnostic` call in rustfmt.)
 - rust-lang#121617 (Actually use the right closure kind when checking async Fn goals)
 - rust-lang#121628 (Do not const prop unions)
 - rust-lang#121629 (fix some references to no-longer-existing ReprOptions.layout_seed)

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
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.

ICE: invalid asymmetric binary op
6 participants