Check for occupied niches #104862

saethlin · 2022-11-25T04:51:12Z

Implementation of rust-lang/compiler-team#624

Crater run has 62 crates that hit the check, 43 of those are published to crates.io. I see a lot of null function pointers and use of mem::uninitialized where the 0x1-filling collides with an enum niche.

But that is with full niche checks; checking transmute, plus any place where that we Copy, Move, or Inspect. Such checking is definitely too thorough to be on by default because it is 2x compile time overhead.

During implementation, this ran into llvm/llvm-project#68381

r? @ghost

rustbot · 2023-04-03T02:04:00Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

saethlin · 2023-04-03T02:50:17Z

This is not done yet. I wish generalized niche checks were as simple as what I whipped up here, and also CI doesn't pass for... reasons.

bors · 2023-05-02T17:45:33Z

☔ The latest upstream changes (presumably #111082) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-11-10T23:12:47Z

☀️ Try build successful - checks-actions
Build commit: eaa4669 (eaa46697f8834416cadb541c3d8b28c9d7da8e68)

rust-timer · 2023-11-11T01:10:38Z

Finished benchmarking commit (eaa4669): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.1%, 3.5%]	17
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-1.2%, -0.2%]	3
All ❌✅ (primary)	1.1%	[0.1%, 3.5%]	17

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.8%	[0.2%, 7.9%]	17
Regressions ❌ (secondary)	3.9%	[2.6%, 5.2%]	3
Improvements ✅ (primary)	-2.1%	[-4.6%, -0.7%]	5
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	0.9%	[-4.6%, 7.9%]	22

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[0.4%, 3.3%]	8
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[-0.4%, 3.3%]	9

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 1.1%]	45
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[-0.2%, 1.1%]	48

Bootstrap: 674.395s -> 673.749s (-0.10%)
Artifact size: 311.13 MiB -> 311.07 MiB (-0.02%)

Dylan-DPC · 2024-02-05T15:18:51Z

@saethlin any updates on this?

saethlin · 2024-02-05T16:14:43Z

Yes; I'm working on an alternative implementation. It's almost ready for a draft PR.

saethlin · 2024-02-16T04:46:22Z

Replaced by #121174

Check for occupied niches This is a replacement for rust-lang#104862 r? `@ghost`

@saethlin

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) }; ``` This is the only way to create an invalid enum in pure Rust. 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.

@saethlin

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) }; ``` This is the only way to create an invalid enum in pure Rust. 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.

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) }; ``` This is the only way to create an invalid enum in pure Rust. 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@.

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@.

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 25, 2022

saethlin mentioned this pull request Nov 25, 2022

Insert alignment checks for pointer dereferences when debug assertions are enabled #98112

Merged

3 tasks