Insert checks for enum discriminants when debug assertions are enabled #141759

1c3t3a · 2025-05-30T09:44:26Z

Similar to the existing null-pointer and alignment checks, this checks for valid enum discriminants on creation of enums through unsafe transmutes. Essentially this sanitizes patterns like the following:

let val: MyEnum = unsafe { std::mem::transmute<u32, MyEnum>(42) };

An extension of this check will be done in a follow-up that explicitly sanitizes for extern enum values that come into Rust from e.g. C/C++.

This check is similar to Miri's capabilities of checking for valid construction of enum values.

This PR is inspired by saethlin@'s PR
#104862. Thank you so much for keeping this code up and the detailed comments!

I also pair-programmed large parts of this together with vabr-g@.

r? @saethlin

rustbot · 2025-05-30T09:44:32Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

src/tools/rust-analyzer/crates/intern/src/symbol/symbols.rs

Similar to the existing nullpointer and alignment checks, this checks for valid enum discriminants on creation of enums through unsafe transmutes. Essentially this sanitizes patterns like the following: ```rust let val: MyEnum = unsafe { std::mem::transmute<u32, MyEnum>(42) }; ``` An extension of this check will be done in a follow-up that explicitly sanitizes for extern enum values that come into Rust from e.g. C/C++. This check is similar to Miri's capabilities of checking for valid construction of enum values. This PR is inspired by saethlin@'s PR rust-lang#104862. Thank you so much for keeping this code up and the detailed comments! I also pair-programmed large parts of this together with vabr-g@.

Kobzol · 2025-06-10T19:33:47Z

@bors2 try @rust-timer queue

rust-bors · 2025-06-10T19:33:50Z

⌛ Trying commit 5587fd7 with merge 7488b2b…

To cancel the try build, run the command @bors2 try cancel.

Insert checks for enum discriminants when debug assertions are enabled Similar to the existing null-pointer and alignment checks, this checks for valid enum discriminants on creation of enums through unsafe transmutes. Essentially this sanitizes patterns like the following: ```rust let val: MyEnum = unsafe { std::mem::transmute<u32, MyEnum>(42) }; ``` An extension of this check will be done in a follow-up that explicitly sanitizes for extern enum values that come into Rust from e.g. C/C++. This check is similar to Miri's capabilities of checking for valid construction of enum values. This PR is inspired by saethlin@'s PR #104862. Thank you so much for keeping this code up and the detailed comments! I also pair-programmed large parts of this together with vabr-g@. r? `@saethlin`

1c3t3a · 2025-06-10T19:54:07Z

This patch is finally ready for review! Let's see what the perf-impact of this is, but I wouldn't assume it is much, as this only emits checks for transmutes to enums.

rust-bors · 2025-06-10T22:01:57Z

☀️ Try build successful (CI)
Build commit: 7488b2b (7488b2b1de7ccc9272a1b2f6015d82794d20c065)

rust-timer · 2025-06-10T23:53:37Z

Finished benchmarking commit (7488b2b): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 3.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[1.2%, 8.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 754.321s -> 757.215s (0.38%)
Artifact size: 372.15 MiB -> 372.16 MiB (0.00%)

compiler/rustc_mir_transform/src/check_enums.rs

scottmcm · 2025-06-12T06:51:25Z

compiler/rustc_mir_transform/src/check_enums.rs

+        ))),
+    });
+
+    // Loop over the list of the discriminants and insert checks for equality.


Hmm, this can be a very large amount of additional MIR. I worry about things like the transmute from usize to ptr::Alignment, for example -- adding another, what, at least 128 MIR statements every time that happens?

Yeah I was already thinking about this... my perception was that such enums are not super prevalent, but ptr::Alignment does look relevant.

One idea I've had was that most enums have contiguous range (ptr::Alignment is a bad example :/) and we could transform this list to a list of WrappingRanges and then compare them. As I said that wouldn't work for ptr::Alignment though...

We could also think about excluding everything with more than e.g. 10 variants and have an option to opt-in for all checks?

every time that happens?

I'm not sure it happens very often. Such transmutes are usually done in a helper function, so they should not be inserted very many times in a debug build.

And the perf results suggest that it does not happen very often. At least currently.

Maybe to give some background: In the whole standard library compiled for MacOS I saw one (!) such check being emitted. For Linux it's two, so all-in-all I wouldn't believe that this is very prevalent.

This may change once we extend the check to e.g. reads through a union or pointer, but it still shouldn't be super often I assume.

compiler/rustc_mir_transform/src/check_enums.rs

saethlin · 2025-06-13T21:39:54Z

compiler/rustc_mir_transform/src/check_enums.rs

+                // An empty enum that tries to be constructed from an inhabited value, this
+                // is never correct.
+                Variants::Empty => {
+                    // The enum layout is uninhabited but we construct it from sth inhabited.


Isn't this case already detected statically?

Yes it is. I was wondering whether I should put it here as it already creates a warning. But I thought that it would be good to have it for the sake of completeness. I am happy to remove it if you think that's better.

tests/mir-opt/enum_check_niche.rs

tests/mir-opt/enum_check_no_niche.main.CheckEnums.diff

…TyAndSize

rustbot assigned saethlin May 30, 2025

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 30, 2025