safe transmute: support `Single` enums by jswrenn · Pull Request #126358 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits1 Checks6 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

Previously, the implementation of Tree::from_enum incorrectly treated enums with Variants::Single and Variants::Multiple identically. This is incorrect for Variants::Single enums, which delegate their layout to that of a variant with a particular index (or no variant at all if the enum is empty).

This flaw manifested first as an ICE. Tree::from_enum attempted to compute the tag of variants other than the one at Variants::Single's index, and fell afoul of a sanity-checking assertion in compiler/rustc_const_eval/src/interpret/discriminant.rs. This assertion is non-load-bearing, and can be removed; the routine its in is well-behaved even without it.

With the assertion removed, the proximate issue becomes apparent: calling Tree::from_variant on a variant that does not exist is ill-defined. A sanity check the given variant has FieldShapes::Arbitrary fails, and the analysis is (correctly) aborted with Err::NotYetSupported.

This commit corrects this chain of failures by ensuring that Tree::from_variant is not called on variants that are, as far as layout is concerned, nonexistent. Specifically, the implementation of Tree::from_enum is now partitioned into three cases:

enums that are uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

Tree::from_variant is now only invoked in the third case. In the first case, Tree::uninhabited() is produced. In the second case, the layout is delegated to Variants::Single's index.

Fixes #125811

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
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 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 12, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@@ -241,10 +241,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
variant_index: VariantIdx,
) -> InterpResult<'tcx, Option<(ScalarInt, usize)>> {
match self.layout_of(ty)?.variants {
abi::Variants::Single { index } => {
assert_eq!(index, variant_index);

Choose a reason for hiding this comment

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

Removing this assertion was recommended by @RalfJung in #125811 (comment), and I agree with his assessment that it isn't a load-bearing assertion. An index mismatch here doesn't suggest a bug in computing the tag.

That said, removing this assertion is irrelevant to fixing the bug — the salient changes are all in compiler/rustc_transmute/src/layout/tree.rs. Those changes fix the ICE even with this assertion intact.

Choose a reason for hiding this comment

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

I would suggest to replace the assertion by a comment, like

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

This comment has been minimized.

@rustbot author

can take a look once this is fixed

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jun 12, 2024

Also, can you explain somewhere what this PR actually does? This PR has like no description -- typically a fix should justify what it's fixing unless the code speaks for itself, and in this case it doesn't really.

enums that are uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

Tree::from_variant is now only invoked in the third case. In the first case, Tree::uninhabited() is produced. In the second case, the layout is delegated to Variants::Single's index.

Fixes rust-lang#125811

Choose a reason for hiding this comment

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

Thanks!

📌 Commit fb662f2 has been approved by compiler-errors

It is now in the queue for this repository.

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

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jun 13, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jun 13, 2024

…kingjubilee

Rollup of 10 pull requests

Successful merges:

rust-lang#125674 (Rewrite symlinked-extern, symlinked-rlib and symlinked-libraries run-make tests in rmake.rs format)
rust-lang#125688 (Walk into alias-eq nested goals even if normalization fails)
rust-lang#126142 (Harmonize using root or leaf obligation in trait error reporting)
rust-lang#126303 (Urls to docs in rust_hir)
rust-lang#126328 (Add Option::is_none_or)
rust-lang#126337 (Add test for walking order dependent opaque type behaviour)
rust-lang#126353 (Move MatchAgainstFreshVars to old solver)
rust-lang#126356 (docs(rustc): Improve discoverable of Cargo docs)
rust-lang#126358 (safe transmute: support Single enums)
rust-lang#126362 (Make try_from_target_usize method public)

r? @ghost @rustbot modify labels: rollup

PR already got rolled up so the comment should probably be a follow-up.

// the (unlikely?) event that an uninhabited enum is
// non-zero-sized, this assert will trigger an ICE, and this
// code should be modified such that a `layout.size` amount
// of uninhabited bytes is returned instead.

Choose a reason for hiding this comment

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

Have you tried an enum like enum E { Variant(!, i32) }? That may give rise to a non-zero-sized uninhabited enum.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Jun 13, 2024

Rollup merge of rust-lang#126358 - jswrenn:fix-125811, r=compiler-errors

safe transmute: support Single enums

enums that are uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

Tree::from_variant is now only invoked in the third case. In the first case, Tree::uninhabited() is produced. In the second case, the layout is delegated to Variants::Single's index.

Fixes rust-lang#125811

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
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 narrower category of "enums that are variantless". These enums, whose layouts are described with Variants::Single { index }, are special in that they do not have a variant at index.

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 } but that do have a variant at index.

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

enum Uninhabited { A(!, u128) }

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

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

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(!) }

enum Uninhabited { A(!, u128) }

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

enum Uninhabited { A(!, u128) }

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

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

enum Uninhabited { A(!, u128) }

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

enum Uninhabited { A(!, u128) }

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

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

enum Uninhabited { A(!, u128) }

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

enum Uninhabited { A(!, u128) }

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

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

enum Uninhabited { A(!, u128) }

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) }

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

enum Uninhabited { A(!, u128) }

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

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

enum Uninhabited { A(!, u128) }

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) }

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

enum Uninhabited { A(!, u128) }

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

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

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

jswrenn added a commit to jswrenn/rust that referenced this pull request

Jun 14, 2024

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

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple inhabited variants

enum Uninhabited { A(!, u128) }

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

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

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

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:

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple variants

enum Uninhabited { A(!, u128) }

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

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

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:

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple variants

enum Uninhabited { A(!, u128) }

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

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

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

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:

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple variants

enum Uninhabited { A(!, u128) }

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

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

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

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:

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple variants

enum Uninhabited { A(!, u128) }

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

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

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

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:

enums that uninhabited
enums for which all but one variant is uninhabited
enums with multiple variants

enum Uninhabited { A(!, u128) }

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

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

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

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.

safe transmute: support Single enums by jswrenn · Pull Request #126358 · rust-lang/rust (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safe transmute: support `Single` enums by jswrenn · Pull Request #126358 · rust-lang/rust (original) (raw)