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

safe transmute: support non-ZST, variantful, uninhabited enums #126493

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented Jun 14, 2024

Previously, Tree::from_enum's implementation branched into three disjoint cases:

  1. enums that uninhabited
  2. enums for which all but one variant is uninhabited
  3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

enum Uninhabited { A(!, u128) }

...which, currently, has the same size as u128. This faulty assumption manifested as the ICE reported in #126460.

In this PR, we revise the first case of Tree::from_enum to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with Variants::Single { index }, are special in their layouts otherwise resemble the ! type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with Variants::Single { index } and that do have a variant at index. This second case captures uninhabited enums that are not ZSTs, like:

enum Uninhabited { A(!, u128) }

...which represent their variants with Variants::Single.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with Variants::Multiple.

This PR also adds a comment requested by @RalfJung in his review of #126358 to compiler/rustc_const_eval/src/interpret/discriminant.rs.

Fixes #126460

r? @compiler-errors

@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 Jun 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Comment on lines +244 to +253
abi::Variants::Single { .. } => {
// The tag of a `Single` enum is like the tag of the niched
// variant: there's no tag as the discriminant is encoded
// entirely implicitly. If `write_discriminant` ever hits this
// case, we do a "validation read" to ensure the the right
// discriminant is encoded implicitly, so any attempt to write
// the wrong discriminant for a `Single` enum will reliably
// result in UB.
Ok(None)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Requested by @RalfJung here: #126358 (comment)

@RalfJung
Copy link
Member

RalfJung commented Jun 14, 2024

What about enums like enum E { A(!, u64), B(!, u32) }? These use Multiple variants despite all variants being uninhabited. See here for the layout debug info.

@jswrenn
Copy link
Member Author

jswrenn commented Jun 14, 2024

I've amended this PR to add a test case for such enums.

Those enums are supported, as before; they're covered by the Variants::Multiple case here:

Variants::Multiple { tag_field, .. } => {
// `Variants::Multiple` denotes an enum with multiple
// inhabited variants. The layout of such an enum is the
// disjunction of the layouts of its tagged variants.
// For enums (but not coroutines), the tag field is
// currently always the first field of the layout.
assert_eq!(*tag_field, 0);
let variants = def.discriminants(cx.tcx()).try_fold(
Self::uninhabited(),
|variants, (idx, ref discriminant)| {
let variant = layout_of_variant(idx)?;
Result::<Self, Err>::Ok(variants.or(variant))
},
)?;
return Ok(Self::def(Def::Adt(def)).then(variants));

They don't require any special handling, because they don't get the !-like representation implemented here:

None if is_enum => {
return Some(self.layout_of_never_type());
}

@RalfJung
Copy link
Member

So in which case do they fall from the PR description?

  1. enums for which all but one variant is uninhabited
  2. enums with multiple inhabited variants

@jswrenn
Copy link
Member Author

jswrenn commented Jun 14, 2024

Whoops, that needs to be updated.

@RalfJung
Copy link
Member

Btw I am getting a notification from github for each time you push a commit that has @RalfJung in the commit message... I'd appreciate if you could remove the ping from the commit message. :)

@jswrenn jswrenn force-pushed the fix-126460 branch 2 times, most recently from 34f8d2f to 18f9d5e Compare June 14, 2024 20:31
@jswrenn
Copy link
Member Author

jswrenn commented Jun 14, 2024

Whoops, that needs to be updated.

Done

Btw I am getting a notification from github for each time you push a commit that has @RalfJung in the commit message... I'd appreciate if you could remove the ping from the commit message. :)

Yikes! Sorry. Done!

@RalfJung
Copy link
Member

Whoops, that needs to be updated.

Ah okay. :)
If that exactly corresponds to Variants::Single and Variants::Multiple, then that is worth mentioning.

@jswrenn
Copy link
Member Author

jswrenn commented Jun 14, 2024

Agreed. I've updated the PR description and commit message to note the correspondence to Variants::Single and Variants::Multiple.

Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with
multiple variants", which captures uninhabited, non-ZST enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 18, 2024

📌 Commit df1d616 has been approved by compiler-errors

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 Jun 18, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by `@RalfJung` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? `@compiler-errors`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by ``@RalfJung`` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? ``@compiler-errors``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 18, 2024
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#123782 (Test that opaque types can't have themselves as a hidden type with incompatible lifetimes)
 - rust-lang#124580 (Suggest removing unused tuple fields if they are the last fields)
 - rust-lang#125852 (improve tip for inaccessible traits)
 - rust-lang#126422 (Suggest using a standalone doctest for non-local impl defs)
 - rust-lang#126427 (Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs)
 - rust-lang#126493 (safe transmute: support non-ZST, variantful, uninhabited enums)
 - rust-lang#126572 (override user defined channel when using precompiled rustc)
 - rust-lang#126615 (Add `rustc-ice*` to `.gitignore`)
 - rust-lang#126632 (Replace `move||` with `move ||`)

Failed merges:

 - rust-lang#126558 (hir_typeck: be more conservative in making "note caller chooses ty param" note)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 19, 2024
safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by ```@RalfJung``` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? ```@compiler-errors```
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup of 10 pull requests

Successful merges:

 - rust-lang#124135 (delegation: Implement glob delegation)
 - rust-lang#125078 (fix: break inside async closure has incorrect span for enclosing closure)
 - rust-lang#125293 (Place tail expression behind terminating scope)
 - rust-lang#126422 (Suggest using a standalone doctest for non-local impl defs)
 - rust-lang#126493 (safe transmute: support non-ZST, variantful, uninhabited enums)
 - rust-lang#126504 (Sync fuchsia test runner with clang test runner)
 - rust-lang#126558 (hir_typeck: be more conservative in making "note caller chooses ty param" note)
 - rust-lang#126586 (Add `@badboy` and `@BlackHoleFox` as Mac Catalyst maintainers)
 - rust-lang#126615 (Add `rustc-ice*` to `.gitignore`)
 - rust-lang#126632 (Replace `move||` with `move ||`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0e46111 into rust-lang:master Jun 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup merge of rust-lang#126493 - jswrenn:fix-126460, r=compiler-errors

safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by ````@RalfJung```` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? ````@compiler-errors````
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: layout: 🌲 : Size != Size
5 participants