Skip to content

improve TagEncoding::Niche docs, sanity check, and UB checks #133681

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 30, 2024

Turns out the niche_variants range can actually contain the untagged_variant. We should report this as UB in Miri, so this PR implements that.

Also rename partially_check_layout to layout_sanity_check for better consistency with how similar functions are called in other parts of the compiler.

Turns out my adjustments to the transmutation logic also fix #126267.

@rustbot
Copy link
Collaborator

rustbot commented Nov 30, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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 Nov 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 30, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

The transmute logic seems to be calling tag_for_variant on uninhabited variants. Those variants do not have a tag, so... it is not clear to me what the logic should be in that case. Cc @jswrenn , I hope what I did in the last commit makes sense.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

r=me with or without the nit 🙂

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 3, 2024

📌 Commit 611a991 has been approved by wesleywiser

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 Dec 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 3, 2024
improve TagEncoding::Niche docs, sanity check, and UB checks

Turns out the `niche_variants` range can actually contain the `untagged_variant`. We should report this as UB in Miri, so this PR implements that.

Also rename `partially_check_layout` to `layout_sanity_check` for better consistency with how similar functions are called in other parts of the compiler.

Turns out my adjustments to the transmutation logic also fix rust-lang#126267.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#132937 (a release operation synchronizes with an acquire operation)
 - rust-lang#133681 (improve TagEncoding::Niche docs, sanity check, and UB checks)
 - rust-lang#133726 (Add `core::arch::breakpoint` and test)
 - rust-lang#133768 (Remove `generic_associated_types_extended` feature gate)
 - rust-lang#133811 ([AIX] change AIX default codemodel=large)
 - rust-lang#133812 (Update wasm-component-ld to 0.5.11)
 - rust-lang#133813 (compiletest: explain that UI tests are expected not to compile by default)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6e87eb5 into rust-lang:master Dec 4, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
Rollup merge of rust-lang#133681 - RalfJung:niches, r=wesleywiser

improve TagEncoding::Niche docs, sanity check, and UB checks

Turns out the `niche_variants` range can actually contain the `untagged_variant`. We should report this as UB in Miri, so this PR implements that.

Also rename `partially_check_layout` to `layout_sanity_check` for better consistency with how similar functions are called in other parts of the compiler.

Turns out my adjustments to the transmutation logic also fix rust-lang#126267.
@RalfJung RalfJung deleted the niches branch December 4, 2024 06:32
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 19, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2024
Rollup merge of rust-lang#133702 - RalfJung:single-variant, r=oli-obk

Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 20, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang/rust#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Dec 23, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang/rust#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Dec 28, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang/rust#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request Jan 13, 2025
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang/rust#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#132937 (a release operation synchronizes with an acquire operation)
 - rust-lang#133681 (improve TagEncoding::Niche docs, sanity check, and UB checks)
 - rust-lang#133726 (Add `core::arch::breakpoint` and test)
 - rust-lang#133768 (Remove `generic_associated_types_extended` feature gate)
 - rust-lang#133811 ([AIX] change AIX default codemodel=large)
 - rust-lang#133812 (Update wasm-component-ld to 0.5.11)
 - rust-lang#133813 (compiletest: explain that UI tests are expected not to compile by default)

r? `@ghost`
`@rustbot` modify labels: rollup
# for free to join this conversation on GitHub. Already have an account? # 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: overflow computing relative variant idx in rustc_const_eval/src/interpret/discriminant.rs
6 participants