Miri read_discriminant: return a scalar instead of raw underlying bytes #72419
Conversation
| } | ||
| }; | ||
| // Compute the size of the scalar we need to return. | ||
| // FIXME: Why do we not need to do a cast here like we do above? |
There was a problem hiding this comment.
This one particularly confused me -- we should resolve that question before landing.
There was a problem hiding this comment.
For the niche layout, the variant indices have to not overflow host u32 nor the target (type-checking) discriminant type (isize by default but I guess it could be u8/i8).
So you can add an assert that it fits as a positive number in the discriminant type, but no cast should be necessary.
There was a problem hiding this comment.
Scalar::from_u32 already has such an assert (at least on debug builds).
3d65f0c
to
daceb18
Compare
This comment has been minimized.
This comment has been minimized.
daceb18
to
824fa23
Compare
|
This makes the generator-related Miri tests fail: I'll need to investigate. |
| // - The field storing the discriminant has a layout, which my be a pointer. | ||
| // This is `discr_val.layout`; we just use it for sanity checks. |
There was a problem hiding this comment.
"storing the discriminant" could be something more like "encoding the discriminant as a tag/niche".
That might make it clearer why the type differs: the only reason to have a simple encoding is cheap decoding, it could be any arbitrary mapping as long as you have enough values for every possible discriminant to be represented.
There was a problem hiding this comment.
"encoding the discriminant as a tag/niche".
But the layout of the tag/niche is discr_layout, right? The second item in my list? And then discr_ty is the high-level thing?
There was a problem hiding this comment.
But the layout of the tag/niche is discr_layout, right?
If this is correct I'll rename the variables to tag_layout or so.
There was a problem hiding this comment.
Did that, and also opened #72497 for the general terminology cleanup.
| // - The discriminant has a layout for tag storing purposes, which is always an integer. | ||
| // This is `discr_layout` and is used to interpret the value we read from the | ||
| // discriminant field. |
There was a problem hiding this comment.
Not sure what this refers to. Is this a tag/niche-sized integer? So for e.g. a pointer niche, it would be a pointer-sized integer?
There was a problem hiding this comment.
This is the layout stored in Variants::Multiple::discr. So I think the answer to your questions is "yes".
20f6874
to
0af3ac0
Compare
This comment has been minimized.
This comment has been minimized.
0af3ac0
to
7220175
Compare
This comment has been minimized.
This comment has been minimized.
|
This PR now also includes a |
27bb80b
to
9fbdad0
Compare
|
Rebased as my cast-cleanup-PR also included a commit from this one. |
9fbdad0
to
95b853c
Compare
|
@bors r=oli-obk,eddyb |
|
📌 Commit 95b853c has been approved by |
|
Failed in #72737 (comment) |
95b853c
to
c4b6224
Compare
|
Fixes type sanity check assertions. |
|
📌 Commit c4b6224 has been approved by |
Rollup of 10 pull requests Successful merges: - rust-lang#72033 (Update RELEASES.md for 1.44.0) - rust-lang#72162 (Add Extend::{extend_one,extend_reserve}) - rust-lang#72419 (Miri read_discriminant: return a scalar instead of raw underlying bytes) - rust-lang#72621 (Don't bail out of trait selection when predicate references an error) - rust-lang#72677 (Fix diagnostics for `@ ..` binding pattern in tuples and tuple structs) - rust-lang#72710 (Add test to make sure -Wunused-crate-dependencies works with tests) - rust-lang#72724 (Revert recursive `TokenKind::Interpolated` expansion for now) - rust-lang#72741 (Remove unused mut from long-linker-command-lines test) - rust-lang#72750 (Remove remaining calls to `as_local_node_id`) - rust-lang#72752 (remove mk_bool) Failed merges: r? @ghost
tag/niche terminology cleanup The term "discriminant" was used in two ways throughout the compiler: * every enum variant has a corresponding discriminant, that can be given explicitly with `Variant = N`. * that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling). After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either `TagEncoding::Direct` (formerly `DiscriminantKind::Tag`) or `TagEncoding::Niche` (formerly `DiscrimianntKind::Niche`). This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in rust-lang#72419. (There is also a `DiscriminantKind` type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.) r? @eddyb
tag/niche terminology cleanup The term "discriminant" was used in two ways throughout the compiler: * every enum variant has a corresponding discriminant, that can be given explicitly with `Variant = N`. * that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling). After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either `TagEncoding::Direct` (formerly `DiscriminantKind::Tag`) or `TagEncoding::Niche` (formerly `DiscrimianntKind::Niche`). This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in rust-lang#72419. (There is also a `DiscriminantKind` type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.) r? @eddyb
tag/niche terminology cleanup The term "discriminant" was used in two ways throughout the compiler: * every enum variant has a corresponding discriminant, that can be given explicitly with `Variant = N`. * that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling). After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either `TagEncoding::Direct` (formerly `DiscriminantKind::Tag`) or `TagEncoding::Niche` (formerly `DiscrimianntKind::Niche`). This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in rust-lang#72419. (There is also a `DiscriminantKind` type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.) r? @eddyb
tag/niche terminology cleanup The term "discriminant" was used in two ways throughout the compiler: * every enum variant has a corresponding discriminant, that can be given explicitly with `Variant = N`. * that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling). After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either `TagEncoding::Direct` (formerly `DiscriminantKind::Tag`) or `TagEncoding::Niche` (formerly `DiscrimianntKind::Niche`). This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in rust-lang#72419. (There is also a `DiscriminantKind` type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.) r? @eddyb
r? @oli-obk @eddyb