Decide on when MIR Discriminant() operation is UB

@wesleywiser

We do not currently have a clear description of what the semantics of the Discriminant() MIR operation, and the corresponding intrinsic (exposed via mem::discriminant()), are -- specifically, what are the safety preconditions of this operation, and when is it UB?

Note that this operation works on all types, not just enums. For valid values of non-enum types it returns some valid integer value (currently, 0).

The implementation in Miri (to be restored with #91088) does the minimum amount of work necessary to determine the discriminant: if the type has no discriminant (since there are not at least 2 variants), the operation is always defined; otherwise it reads the tag (which encodes the discriminant) and causes UB if that is uninitialized or does not encode a valid discriminant. (There are some thorny question here around what happens if the discriminant has provenance; I would like to keep that out of scope for this issue -- it should likely be treated like a ptr-to-int transmute, whatever we end up doing with that: rust-lang/unsafe-code-guidelines#286.)

The codegen backend adds some extra UB for the case where the type is uninhabited:

rust/compiler/rustc_codegen_ssa/src/mir/place.rs

Lines 206 to 215 in 81117ff

    
               /// Obtain the actual discriminant of a value. 
        
               pub fn codegen_get_discr<Bx: BuilderMethods<'a, 'tcx, Value = V>>( 
        
                   self, 
        
                   bx: &mut Bx, 
        
                   cast_to: Ty<'tcx>, 
        
               ) -> V { 
        
                   let cast_to = bx.cx().immediate_backend_type(bx.cx().layout_of(cast_to)); 
        
                   if self.layout.abi.is_uninhabited() { 
        
                       return bx.cx().const_undef(cast_to); 
        
                   }

We also have a related MIR optimization in https://github.com/rust-lang/rust/blob/93542a8240c5f926ac5f3f99cef99366082f9c2b/compiler/rustc_mir_transform/src/uninhabited_enum_branching.rs. I am not quite sure what this does though, it seems to be more about assuming that if a particular enum variant is uninhabited then we will never see the discriminant for that variant, and can hence remove it from the SwitchInt?

An 'obvious' choice is to say that the value passed to the Discriminant operator must fully satisfy its validity invariant -- that would certainly justify both the MIR optimization and what the codegen backend does. However, this also has problems:

Drop elaboration generates Discriminant operations on partially moved-out-of enums (Safe function MIR reads discriminant of moved-out local #91029). Depending on the semantics of 'move' and whether validity invariants might take into account what a pointer points to (such as requiring that a Box be initialized), this might lead to calling Discriminant on invalid values.
To even check the validity invariant we need to read the full value, but there are legitimate situations where parts of that value are pointed to by an active mutable reference that makes uniqueness assumptions (Fix variant index / discriminant confusion in uninhabited enum branching #89764 (comment)). Doing a read violates those assumptions.

These observations make me doubtful that requiring full validity is the right thing. Making the fewest assumptions is appealing IMO, but not compatible with our codegen backend nor with the MIR optimizations -- the optimization seems to kick in even for operations of the form Discriminant(*ptr), so the validity invariant of ptr itself does not help either. It could be possible to strike some middle ground, but that feels like a rather ad-hoc adjustment to the current set of optimizations.

To summarize:

Miri implements "minimal UB", requiring just enough to actually be able to compute the discriminant. This is incompatible with what MIR optimizations and the codegen backend do.
A principled alternative would be requiring full validity of the value, but that is incompatible with code emitted for dropping, and with aliasing assumptions.
Some middle ground is probably possible but seems entirely ad-hoc.

Cc @wesleywiser @tmandry @rust-lang/wg-mir-opt

The implementation in Miri ... reads the tag (which encodes the discriminant) and causes UB if that is uninitialized or does not encode a valid discriminant.

There seems to be an additional mismatch in terms of which discriminants are valid. The code generation considers discriminants corresponding to uninhabited variants to be invalid, but they do not seem to cause any errors in Miri. Does Miri validate a scalar range?

The code generation considers discriminants corresponding to uninhabited variants to be invalid,

How concretely does that assumption look like -- can you link to the code that does this?

Does Miri validate a scalar range?

Miri checks that a variant with the given tag exists. But Miri does not do further special casing of uninhabited variants.

For Niche-encoded tags, AFAIK uninhabited variants are not even necessarily assigned a tag, so Miri can never return such a variant. Similarly, if all but 1 variant are uninhabited, rustc will not use a multi-variant layout for this type -- so the other variants cannot possibly be returned either. (This cannot give rise to UB though, it simply means that there exists no sequence of bits that would be interpreted as encoding that variant.)

The code generation considers discriminants corresponding to uninhabited variants to be invalid,

How concretely does that assumption look like -- can you link to the code that does this?

The layout for tag describes a valid range, which is used to emit LLVM range metadata when loading the tag. For example, for a direct encoding:

rust/compiler/rustc_middle/src/ty/layout.rs

Lines 1146 to 1160 in 81117ff

    
           for (i, discr) in def.discriminants(tcx) { 
        
               if variants[i].iter().any(|f| f.abi.is_uninhabited()) { 
        
                   continue; 
        
               } 
        
               let mut x = discr.val as i128; 
        
               if discr_type.is_signed() { 
        
                   // sign extend the raw representation to be an i128 
        
                   x = (x << (128 - bits)) >> (128 - bits); 
        
               } 
        
               if x < min { 
        
                   min = x; 
        
               } 
        
               if x > max { 
        
                   max = x; 
        
               }

rust/compiler/rustc_codegen_llvm/src/builder.rs

Lines 483 to 485 in 81117ff

    
           if !scalar.is_always_valid(bx) { 
        
               bx.range_metadata(load, scalar.valid_range); 
        
           }

which is used to emit LLVM range metadata when loading the tag

Ah, and then that load_operand function is called at

rust/compiler/rustc_codegen_ssa/src/mir/place.rs

Line 232 in 5b2f757

let tag = bx.load_operand(tag);

Yeah that seems like there are a lot of special cases for uninhabited variants / enums built into the codegen backend that do not seem to fall out of any more general underlying principle -- except for "the entire value must be valid", but that is incompatible with things we do elsewhere.

I don't really see how there's more than one option here. For Option<NonZeroU8> and any other enum for which we may want to do layout optimizations in the future (ie all of them), we have to ban calling discriminant on Some(uninit), since that's just uninit. That means that at least for the stable mem::discriminant of user code, I think the reasonable rule to choose is this:

With the exception of any fields, or parts of fields, that are behind an UnsafeCell, all validity invariants for the value and its fields must be upheld prior to calling this function.

For SB this can then be a read of those places. This lets this example continue to be DB.

Despite requiring near full validity, I do think this is effectively encoding the idea of "doing minimal work" - just in a way that is cognizant of enum layout optimizations. Now of course we could instead try and define the safety requirements of mem::discriminant in terms of the layout that is chosen, ie something like "all those fields for which at least a part of their niche is occupied must be valid" but I don't see how this benefits either us or users.

For the associated MIR rvalue, I suppose we could choose a weaker requirement (since layout information is actually available there), but I'm also not sure what the benefit of that is.

Note that this doesn't necessarily have to make the elaborate drops code wrong, if we decide that moving out leaves the value initialized (not that I'm advocating for that)

Now of course we could instead try and define the safety requirements of mem::discriminant in terms of the layout that is chosen, ie something like "all those fields for which at least a part of their niche is occupied must be valid" but I don't see how this benefits either us or users.

That's not actually so bad -- it is, I think, basically what Miri currently does. One "just" implements the logic of determining the discriminant in the obvious way, and if any of the values considered in that process are uninit, we raise UB.

Now of course we could instead try and define the safety requirements of mem::discriminant in terms of the layout that is chosen, ie something like "all those fields for which at least a part of their niche is occupied must be valid" but I don't see how this benefits either us or users.

That's not actually so bad -- it is, I think, basically what Miri currently does. One "just" implements the logic of determining the discriminant in the obvious way, and if any of the values considered in that process are uninit, we raise UB.

What are the benefits of this though? Users can't stably rely on this anyway, so the only way that they can make use of this is to do some cursed thing where they try and analyze the chosen layout - I'm not even convinced though that it's actually possible to do this for enums the same way it is for structs. Do we expect MIR optimization passes to do this sort of analysis? Do we even want them to? This is one of those instances where I expect that adding more UB to the language is going to decrease the amount of UB in the ecosystem, because it will reduce the chance that people do dangerous things for which there are probably better solutions.

I do recognize that this might be useful if we allow #[repr(u16)] or whatever on non-fieldless enums in the future (and make some layout guarantees along with that). In that case though, I think if/when T-Lang decides to do that, we should revisit these rules and loosen them for those cases

@RalfJung

oh, another relevant point here @RalfJung : Types don't have to have niches in order for us to want to read some of their bytes in relation to a discriminant call. Specifically, imagine that I have an enum like the following:

enum E {
    A(NonZeroU8, u8),
    B,
    C,
}

rustc could choose a layout like this:

A(x, y) => [x, y],
B => [0, 1],
C => [0, 0],

Now we could imagine a future in which we get enough features (eg a byte type) to implement mem::discriminant in MIR. In that case, we might want to optimize

x == E::B

into

(x as [byte; 2]) == [0, 1]

But if we guarantee that u8 in A is not read by the mem::discriminant call, then this optimization is incorrect. LLVM also won't be able to do it either, because it won't know that the read of the second byte can be hoisted out of first_byte == 0 condition. This, by the way, is why the UnsafeCell distinction is important. Because if we replace u8 with UnsafeCell<u8> we can no longer do this optimization

What all this means is that "the niche is partially occupied" would be an insufficient description of the requirements (assuming we want to enable this optimization). What we'd instead need is some condition that talks about the use of a field's bytes in conjunction with the niche in another field

What are the benefits of this though?

It's the least amount of UB we can have. That's IMO the default state and any extra UB needs justification. :)

I am not convinced that special-casing UnsafeCell in the safety contract of this operation is a good idea -- it is quite complicated, and it is still not enough ti justify what current codegen does. (UnsafeCell<!> is still uninhabited, but if I understand your proposal then it would be allowed to ask for its discriminant.)

What are the benefits of this though?

It's the least amount of UB we can have. That's IMO the default state and any extra UB needs justification. :)

I am not convinced that special-casing UnsafeCell in the safety contract of this operation is a good idea -- it is quite complicated, and it is still not enough ti justify what current codegen does. (UnsafeCell<!> is still uninhabited, but if I understand your proposal then it would be allowed to ask for its discriminant.)

This last part is a good point, and I'm not sure what the best strategy for proceeding with it is. But my point above still stands. There are real (and probably non-trivial) optimizations that require us to read possibly anything that is not behind an UnsafeCell. What would the alternative rule that allows the above optimization even be?

LLVM also won't be able to do it either, because it won't know that the read of the second byte can be hoisted out of first_byte == 0 condition.

Actually it should be able to do that, since all our references are dereferencable.

LLVM also won't be able to do it either, because it won't know that the read of the second byte can be hoisted out of first_byte == 0 condition.

Actually it should be able to do that, since all our references are dereferencable.

dereferenceable is not strong enough, since it's not enough to guarantee that the read doesn't race. See this example, where LLVM has to keep the select because the second load might have yielded poison (llc opts don't figure this out).

Edit: After more experimentation, this example is inconclusive because LLVM doesn't figure it out even with more metadata. Still though, I do think that dereferenceable is not strong enough, since it does not guarantee that the read isn't racey

Races are not a concern, in LLVM a read-write race just means the read returns undef.
And yes LLVM might have to add a freeze to get around poison, but that's still very possible. (But I have no idea if it will actually do that. All I am saying is that it legally could.)

Races are not a concern, in LLVM a read-write race just means the read returns undef. And yes LLVM might have to add a freeze to get around poison, but that's still very possible. (But I have no idea if it will actually do that. All I am saying is that it legally could.)

Oh this is a good point about the races, I thought they returned poison instead of undef. Still though, ideally LLVM would be able to combine the two loads into one, and it can't do that by freezing the result of the places. It might be able to do it by freezing the load itself (or something equivalent)

Yeah, it can do the 2-byte load, freeze that, and then compare with [0, 1].

499 enum support in mir-semantics (#521)

This came up again in a recent discussion, and it seems I somewhat changed my mind compared to what I wrote above. My current thinking is: this operation reads a field, the field holding the tag. That field is special (it's not exposed in the language), but conceptually it is a field in the compiler. We don't have to require the entire enum to be initialized, but since we read the field we should check everything we can that's encoded in the field. This means:

If the discriminant encodes an uninhabited or non-existent variant, that's UB.
For an enum like Option<bool>, any value outside 0..=2 is UB since that's the validity range encoded on the tag field.
For an enum where the niched (untagged) variant is in the middle of the discriminant range, we currently waste one value in the bitspace by including it in the niche variants. That "tagged encoding of the untagged variant" is never generated by SetDiscriminant, and therefore should be considered UB by GetDiscriminant. (This is the case that covers the example in the Zulip discussion mentioned above.)

and therefore should be considered UB by GetDiscriminant

Note that since https://github.com/rust-lang/rust/pull/143784/changes#diff-013fea668c72159276fa60d94808141397bb0ec0f53f93c79c91335cbcda135eR610-R622 this is already insta-UB in cg_ssa-emitted codegen, so I'm certainly in favour 🙃

Oh, I had no idea. Yeah then codegen doesn't currently leave us any wiggle-room here.

(Would be good to involve Miri or opsem folks for PRs like that which add new UB to the LLVM IR we generate. :) Whatever PR added the range metadata on discriminant loads added new UB which Miri doesn't check until today; that's a situation we should do our best to avoid.

You're absolutely right; ideally I'd have put a reference section note next to the UB.

At the time I hadn't even considered looking for a UCG-like conversation about it, though -- just thought from the descriptions in https://doc.rust-lang.org/nightly/nightly-rustc/rustc_public/abi/enum.TagEncoding.html#variant.Niche and related that "of course" it was already UB 😬

(We could certainly stop emitting this if needed, but hopefully instead it gives a direct justification for why making this language UB is useful: GetDiscriminant(_1) == CONST is something we want to optimize to just the one variant, never to two variants, which requires making any other values that could return that constant UB instead.)

Navigation Menu

Repository navigation

Decide on when MIR Discriminant() operation is UB #91095

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	/// Obtain the actual discriminant of a value.
	pub fn codegen_get_discr<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
	self,
	bx: &mut Bx,
	cast_to: Ty<'tcx>,
	) -> V {
	let cast_to = bx.cx().immediate_backend_type(bx.cx().layout_of(cast_to));
	if self.layout.abi.is_uninhabited() {
	return bx.cx().const_undef(cast_to);
	}

	for (i, discr) in def.discriminants(tcx) {
	if variants[i].iter().any(\|f\| f.abi.is_uninhabited()) {
	continue;
	}
	let mut x = discr.val as i128;
	if discr_type.is_signed() {
	// sign extend the raw representation to be an i128
	x = (x << (128 - bits)) >> (128 - bits);
	}
	if x < min {
	min = x;
	}
	if x > max {
	max = x;
	}

	if !scalar.is_always_valid(bx) {
	bx.range_metadata(load, scalar.valid_range);
	}

Decide on when MIR Discriminant() operation is UB #91095

Description

Activity

tmiasko commented on Nov 21, 2021

RalfJung commented on Nov 21, 2021

tmiasko commented on Nov 21, 2021

RalfJung commented on Nov 23, 2021

JakobDegen commented on Mar 3, 2022

JakobDegen commented on Mar 3, 2022

RalfJung commented on Mar 3, 2022

JakobDegen commented on Mar 3, 2022

JakobDegen commented on Mar 3, 2022

RalfJung commented on Mar 4, 2022

JakobDegen commented on Mar 4, 2022

RalfJung commented on Mar 4, 2022

JakobDegen commented on Mar 4, 2022

RalfJung commented on Mar 4, 2022

JakobDegen commented on Mar 4, 2022

RalfJung commented on Mar 4, 2022

RalfJung commented on Mar 4, 2026

scottmcm commented on Mar 4, 2026

RalfJung commented on Mar 4, 2026

scottmcm commented on Mar 4, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions