Put `noundef` on all scalars that don't allow uninit by Noratrieb · Pull Request #106294 · rust-lang/rust

Noratrieb · 2022-12-30T15:22:14Z

Previously, it was only put on scalars with range validity invariants like bool, was uninit was obviously invalid for those.

Since then, we have normatively declared all uninit primitives to be undefined behavior and can therefore put noundef on them.

The remaining concern was the mem::uninitialized function, which cause quite a lot of UB in the older parts of the ecosystem. After #99182, this function now doesn't return uninit values anymore, making users of it safe from this change.

The only real sources of UB where people could encounter uninit primitives are MaybeUninit::uninit().assume_init(), which has always be clear in the docs about being UB and from heap allocations (like reading from the spare capacity of a vec). This is hopefully rare enough to not break anything.

cc @nagisa @scottmcm @nikic

rustbot · 2022-12-30T15:22:19Z

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

Noratrieb · 2022-12-30T16:28:11Z

@bors try @rust-timer queue

bors · 2022-12-30T16:28:20Z

⌛ Trying commit 50d325328bce3d3489a447c988558c9f88c80caf with merge 71266de2dfc6e16a09e705633d9ff32e86d0ab3c...

bors · 2022-12-30T18:57:59Z

☀️ Try build successful - checks-actions
Build commit: 71266de2dfc6e16a09e705633d9ff32e86d0ab3c (71266de2dfc6e16a09e705633d9ff32e86d0ab3c)

rust-timer · 2022-12-30T20:17:13Z

Finished benchmarking commit (71266de2dfc6e16a09e705633d9ff32e86d0ab3c): 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)	0.5%	[0.4%, 0.6%]	4
Regressions ❌ (secondary)	0.9%	[0.7%, 1.1%]	2
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	21
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.2%]	14
All ❌✅ (primary)	-0.1%	[-0.6%, 0.6%]	25

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)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	0.7%	[0.7%, 0.7%]	1

Cycles

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

CAD97 · 2022-12-31T04:45:05Z

Drive-by perf analysis:

Ignoring opt builds, the change is strictly positive ([ -0.56%, -0.22%] -0.28% 27 (13)).
opt/unchanged is also strictly positive ([ -0.33%, -0.20%] -0.24% 7 (7)).
opt/incr is mixed ([ -0.23%, +0.71%] +0.41% 6 (6)) and there's one meaningful opt/full change at +1.10%.

Any time we give LLVM more information, LLVM's going to take more time to process that information.

Given the improvements on non-opt builds, it seems likely this does result in better codegen. To my eye, the regressions to opt time seem reasonably justified by the improvements gotten from the new optimization potential.

Noratrieb · 2022-12-31T13:34:23Z

Let's see what happens if we don't apply to on loads as suggested in #106294 (comment). I haven't adjusted the codegen tests but try doesn't care
@bors try @rust-timer queue

bors · 2022-12-31T13:34:32Z

⌛ Trying commit a0b1c2866f4b30bde9fefb09aa97b861d5f775fc with merge 27e50c7f29cb52f18a29d138ed6d4d6a99689998...

bors · 2022-12-31T16:25:36Z

☀️ Try build successful - checks-actions
Build commit: 27e50c7f29cb52f18a29d138ed6d4d6a99689998 (27e50c7f29cb52f18a29d138ed6d4d6a99689998)

rust-timer · 2022-12-31T17:44:31Z

`noundef` on scalars but not on loads:

Finished benchmarking commit (27e50c7f29cb52f18a29d138ed6d4d6a99689998): comparison URL.

Overall result: ❌ regressions - no 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.

@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)	0.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

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)	2.6%	[2.0%, 3.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.0%, 3.0%]	3

Cycles

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

Noratrieb · 2023-01-17T13:11:43Z

@bors try pleeeaaase

bors · 2023-01-17T13:11:52Z

⌛ Trying commit 82386840d1674b438f5f35efc68511ad21e982fa with merge 73f9d3a67b5db8f28fd07717aa81f78031a6f838...

bors · 2023-01-17T14:27:38Z

☀️ Try build successful - checks-actions
Build commit: 73f9d3a67b5db8f28fd07717aa81f78031a6f838 (73f9d3a67b5db8f28fd07717aa81f78031a6f838)

Noratrieb · 2023-01-17T14:29:37Z

well, that builder is green as well now. onto the next one!
@bors r=nikic

bors · 2023-01-17T14:29:39Z

📌 Commit 82386840d1674b438f5f35efc68511ad21e982fa has been approved by nikic

It is now in the queue for this repository.

bors · 2023-01-17T14:29:47Z

⌛ Testing commit 82386840d1674b438f5f35efc68511ad21e982fa with merge 5916677c0e7704ae21521aa4ce69e115df8323aa...

bors · 2023-01-17T15:09:08Z

💔 Test failed - checks-actions

Noratrieb · 2023-01-17T15:41:22Z

time for the next one! maybe we'll get through this time (also almost committed the changed try files, oops)
@bors r=nikic

bors · 2023-01-17T15:41:24Z

📌 Commit f125538 has been approved by nikic

It is now in the queue for this repository.

bors · 2023-01-17T17:39:51Z

⌛ Testing commit f125538 with merge 3984bc5...

bors · 2023-01-17T20:52:57Z

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 3984bc5 to master...

rust-timer · 2023-01-18T00:47:28Z

Finished benchmarking commit (3984bc5): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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)	0.6%	[0.5%, 0.6%]	2
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	52
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	24
All ❌✅ (primary)	-0.2%	[-0.3%, 0.6%]	54

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	-	-	0

Cycles

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

pnkfelix · 2023-01-18T17:48:19Z

awesome look at all that green in the perf results!

RalfJung · 2023-01-27T15:52:43Z

Cc @rust-lang/wg-unsafe-code-guidelines
No objections here but seems like something for us to be aware of. :)

rustbot assigned oli-obk Dec 30, 2022

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 Dec 30, 2022

Noratrieb force-pushed the noundef-everything branch from 0162428 to 50d3253 Compare December 30, 2022 15:49

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 30, 2022

WaffleLapkin reviewed Dec 30, 2022

View reviewed changes

Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated

erikdesjardins reviewed Dec 30, 2022

View reviewed changes

Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated

Comment thread src/test/codegen/function-arguments.rs Outdated

Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated

Comment thread src/test/codegen/loads.rs Outdated

erikdesjardins reviewed Dec 30, 2022

View reviewed changes

Comment thread compiler/rustc_codegen_llvm/src/builder.rs Outdated

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 30, 2022

Noratrieb force-pushed the noundef-everything branch from 0ce2102 to b774934 Compare December 30, 2022 20:42

This comment has been minimized.

Sign in to view

Noratrieb force-pushed the noundef-everything branch from b774934 to 25c3235 Compare December 31, 2022 13:32

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 31, 2022

This comment has been minimized.

Sign in to view

Noratrieb force-pushed the noundef-everything branch from ed43b59 to 8238684 Compare January 17, 2023 13:11

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 17, 2023

Noratrieb removed A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 17, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2023

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 17, 2023

This comment has been minimized.

Sign in to view

Add more codegen tests

f125538

saethlin mentioned this pull request Jan 18, 2023

[crater experiment] Enable validate-mir by default #107051

Closed

tmiasko mentioned this pull request Jan 21, 2023

Memory sanitizer : false positive regression in nightly-2023-01-18 #107149

Closed

saethlin mentioned this pull request Aug 22, 2023

Add bounds checked get_unchecked, use it everywhere. simd-lite/simd-json#231

Merged

scottmcm mentioned this pull request Mar 29, 2024

Array-as-Immediate array ABI is missing noundef parameter metadata #123183

Closed

Uh oh!

Conversation

Noratrieb commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 30, 2022

Uh oh!

Noratrieb commented Dec 30, 2022

Uh oh!

This comment has been minimized.

bors commented Dec 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Dec 30, 2022

Uh oh!

This comment has been minimized.

rust-timer commented Dec 30, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

This comment has been minimized.

CAD97 commented Dec 31, 2022

Uh oh!

Noratrieb commented Dec 31, 2022

Uh oh!

This comment has been minimized.

bors commented Dec 31, 2022

Uh oh!

bors commented Dec 31, 2022

Uh oh!

This comment has been minimized.

rust-timer commented Dec 31, 2022 • edited by Noratrieb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

noundef on scalars but not on loads:

Overall result: ❌ regressions - no action needed

Uh oh!

Noratrieb commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

Noratrieb commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

This comment has been minimized.

Noratrieb commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

bors commented Jan 17, 2023

Uh oh!

rust-timer commented Jan 18, 2023

Overall result: ✅ improvements - no action needed

Uh oh!

pnkfelix commented Jan 18, 2023

Uh oh!

RalfJung commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Noratrieb commented Dec 30, 2022 •

edited

Loading

rust-timer commented Dec 31, 2022 •

edited by Noratrieb

Loading

`noundef` on scalars but not on loads: