Open RalfJung opened 11 months ago
Ah, that's related to this skip in the layout sanity check:
In Miri it is the visitor creating these downcasts. Maye it just shouldn't downcast in some cases? However, while I can see that it makes sense to never downcast with variant.fields.count() == 0
(this is always valid) or variant.abi.is_uninhabited()
(this is always invalid), I am not sure what to do with neither of these cases apply but we have variant.size == Size::ZERO
...
This is a bug, right? If the enum has
Scalar
layout, then all its variants should also have that layout (and same forScalarPair
)?
It's... more complicated than that. The enum
behaves like union { tag, Variant1, Variant2, ... }
.
What you have there for the variant is a fieldless Aggregate
which should maybe be called a ZFT ("zero-field type"), though even if it had fields it might still not be an issue as long as there's no data (i.e. it's a tree of ZSTs).
You can see some special-casing for this in codegen, I believe, where ZST Aggregate
s are the "simplest" Immediate
s, before Scalar
s and Vector
s (but enum
s or their variants tend not to be treated as immediates in codegen, sadly, even if they theoretically could, if discriminants were read/written in a more flexible way).
What I'm referring to is a bit more obvious when you have a ScalarPair
enum
: if one of its two Scalar
s is a tag, then some variants may be (and some Scalar
Aggregate
if they have no data), but no variants will be ScalarPair
.
Sorry, while writing that, I realized that I'm not 100% sure that a variant can be Scalar
, like in Option<u8>
, does the variant having an u8
at offset of 1
rule it out from being Scalar
? It probably should/would but I'm not sure.
Either way, my point is that the variant doesn't include the tag, so it can't have an abi
that does (arguably a variant shouldn't even have an abi
field of its own, but that's harder to encode in the Layout
types than the status quo).
What I'm referring to is a bit more obvious when you have a ScalarPair enum: if one of its two Scalars is a tag, then some variants may be Scalar (and some Aggregate if they have no data), but no variants will be ScalarPair.
That's not what happens though. Variants do include the tag. Looking at this type:
#[rustc_layout(debug)]
type T = std::result::Result<usize, ()>;
then both variants have
abi: ScalarPair(
Initialized {
value: Int(
I64,
false,
),
valid_range: 0..=1,
},
Union {
value: Int(
I64,
false,
),
},
),
And that is quite required. Downcast leaves the offset the same, and so if downcast to the first variant had Scalar layout that would clearly be invalid (the data starts at offset 8 but Scalar cannot have leading padding).
However, with this type
#[rustc_layout(debug)]
type T = std::result::Result<&'static [u8], ()>;
we have a similar first variant
abi: ScalarPair(
Initialized {
value: Pointer(
AddressSpace(
0,
),
),
valid_range: 1..=18446744073709551615,
},
Initialized {
value: Int(
I64,
false,
),
valid_range: 0..=18446744073709551615,
},
),
but the 2nd variant now is
abi: Aggregate {
sized: true,
},
This is a problem since it means a downcast to the 2nd variant creates invalid data (an immediate with Aggregate
ABI), and there is no simple check for the visitor to avoid that downcast. (We can't just skip everything zero-sized since !
is zero-sized and clearly we must complain if that were present.)
Hah, we already do "variants don't have their own abi
" but by writing to it (instead of disallowing access).
Just remove the variant.fields.count() > 0
condition and the related comment, looks like a microopt?
Sadly that does not seem to help. After all, this type does have fields in each variant
#[rustc_layout(debug)]
type T = std::result::Result<&'static [u8], ()>;
and yet its 2nd variant gets Aggregate
layout. Removing the variant.fields.count() > 0 &&
doesn't change that.
Sadly that does not seem to help. After all, this type does have fields in each variant
#[rustc_layout(debug)] type T = std::result::Result<&'static [u8], ()>;
and yet its 2nd variant gets
Aggregate
layout. Removing thevariant.fields.count() > 0 &&
doesn't change that.
That's a niche enum
, not a tag one, whereas the code I linked is tag-specific. It's possible niches also need to be changed, but it would likely look a bit different anyway (and I don't know that there is anything like that snippet I linked already).
Honestly, reviewing the definition of Layout
, for variant Layout
s:
fields: FieldsShape
: definitely wanted/neededvariants
: always equal to Variants::Single { index }
(of its own VariantIdx
in the enum
)abi
, size
, align
: almost always reset to the whole enum
's
largest_niche
, max_repr_align
, unadjusted_abi_align
: never used, AFAICT
So a potentially clean solution might be to change Variants::Multiple
itself
variants: IndexVec<VariantIdx, LayoutS>
variant_fields: IndexVec<VariantIdx, FieldsShape>
The only complication is the overreliance on TyAndLayout::for_variant
, which makes a whole TyAndLayout
, but doing so with the new scheme would require separating FieldsShape
and Variants
from the other LayoutS
fields, or some more cursed contrivance (or just rewriting all for_variant
callers...).
Turns out that the cases variant.size == Size::ZERO
and variant.abi.is_uninhabited()
are already handled correctly. So I just had to add a check in the visitor to not do place projection when variant.fields.count() == 0
.
@eddyb do you want to keep this open to track the refactoring you suggested?
do you want to keep this open to track the refactoring you suggested?
I think so, at least until someone tries it and realizes it's too disruptive (but I think long-term removing redundancy like this should be an improvement - if truly redundant, that is).
See here for an elaboration of the redundancy. Below follows the original issue that lead to us looking into this.
I just discovered something rather strange in our enum layouts:
std::sync::atomic::Ordering
hasbut its variants look like
Note the
abi: Aggregate
!I am surprised that this has not already lead to ICEs in CTFE/Miri -- via downcasting, one can end up with
Immediate::Scalar
that haveAggregate
ABI, which can quickly panic.This is a bug, right? If the enum has
Scalar
layout, then all its variants should also have that layout? For ScalarPair enums, this seems to already be the case. Cc @oli-obk @eddyb