Allow unsafe through inline const #105147

nbdd0121 · 2022-12-01T19:25:47Z

Handle similar to closures.

Note that this PR does not fix the issue for unsafe { [0; function_requiring_unsafe()] }. This is fundamentally unfixable for MIR unsafeck IMO.

This PR also does not fix unsafety checking for inline const in pattern position. It actually breaks it, allowing unsafe functions to be used in inline const in pattern position without unsafe blocks. Inline const in pattern position is not visible in MIR so ignored by MIR unsafety checking (currently it is also not checked by borrow checker, which is the reason why it's considered an incomplete feature).

@rustbot label: +T-lang +F-inline_const

rustbot · 2022-12-01T19:25:53Z

r? @nagisa

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

rustbot · 2022-12-01T19:25:55Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2022-12-03T10:51:48Z

Note that this PR does not fix the issue for unsafe { [0; function_requiring_unsafe()] }. This is fundamentally unfixable for MIR unsafeck IMO.

Why is that any harder than having unsafe propagate into closures? Both are completely separate MIR bodies after all. So somehow the MIR body for the closure is informed that is "lives" in an unsafe block, and the same could happen with array lengths?

EDIT: Either way that is separate from inline consts, so I opened a new issue at #105204.

compiler/rustc_mir_build/src/check_unsafety.rs

nbdd0121 · 2022-12-04T11:11:22Z

Why is that any harder than having unsafe propagate into closures? Both are completely separate MIR bodies after all. So somehow the MIR body for the closure is informed that is "lives" in an unsafe block, and the same could happen with array lengths?

The difficulty arises precisely from the fact that they are separate MIR bodies. For closures (and after this PR, inline consts), their use is visible in the MIR of the containing body -- closures appear as a Rvalue::Aggregate and inline consts appear as Operand::Constant. However the same thing cannot be said for array length. Because array length feeds into the type system, it was (and needs to be) simplified and evaluated before the MIR is built, and the MIR built will reference these constants anymore.

So the issue for anon consts cannot easily be resolved with MIR unsafeck. It should be solvable, though, once we move to THIR unsafeck.

compiler/rustc_mir_build/src/check_unsafety.rs

nagisa · 2022-12-11T15:33:03Z

r? @oli-obk

I can’t even begin to guess whether we actually want to do this…

This is handled similar to closures

The closure handling code is changed slightly to avoid allocation when THIR building failed.

MIR unsafety checking requires this to be valid

All bodies are unsafe checked anyway. Current MIR unsafeck also just returns for closures.

oli-obk · 2022-12-13T09:29:07Z

@bors try @rust-timer queue

bors · 2022-12-13T09:29:16Z

⌛ Trying commit d6dc912 with merge 26e606eb2c7253f83a3db16651f9ed5443c7d61d...

bors · 2022-12-13T11:38:55Z

☀️ Try build successful - checks-actions
Build commit: 26e606eb2c7253f83a3db16651f9ed5443c7d61d (26e606eb2c7253f83a3db16651f9ed5443c7d61d)

rust-timer · 2022-12-13T13:08:14Z

Finished benchmarking commit (26e606eb2c7253f83a3db16651f9ed5443c7d61d): comparison URL.

Overall result: ❌ regressions - 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%	[1.0%, 1.2%]	2
Regressions ❌ (secondary)	2.3%	[2.1%, 2.7%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.0%, 1.2%]	2

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)	3.4%	[2.9%, 3.8%]	2
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	3.4%	[2.9%, 3.8%]	2

Cycles

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

oli-obk · 2022-12-13T15:45:26Z

These regressions are just noise, the exact same regression is happening on unrelated other PRs.

@bors r+ rollup

bors · 2022-12-13T15:45:29Z

📌 Commit d6dc912 has been approved by oli-obk

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#105147 (Allow unsafe through inline const) - rust-lang#105438 (Move some codegen-y methods from `rustc_hir_analysis::collect` -> `rustc_codegen_ssa`) - rust-lang#105464 (Support #[track_caller] on async closures) - rust-lang#105476 (Change pattern borrowing suggestions to be verbose and remove invalid suggestion) - rust-lang#105500 (Make some diagnostics not depend on the source of what they reference being available) - rust-lang#105628 (Small doc fixes) - rust-lang#105659 (Don't require owned data in `MaybeStorageLive`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned nagisa Dec 1, 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 1, 2022

rustbot added F-inline_const Inline constants (aka: const blocks, const expressions, anonymous constants) T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 1, 2022

This comment has been minimized.

Sign in to view

RalfJung reviewed Dec 3, 2022

View reviewed changes

compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved

oli-obk reviewed Dec 5, 2022

View reviewed changes

compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved

rustbot assigned oli-obk and unassigned nagisa Dec 11, 2022

nbdd0121 added 7 commits December 13, 2022 01:38

Allow unsafe through inline const

9342d1e

This is handled similar to closures

Allow unsafe through inline const for THIR unsafety checker

aa5af2a

The closure handling code is changed slightly to avoid allocation when THIR building failed.

Add tests

f03c2ae

Ensure valid local_data is set for custom mir building

adf1717

MIR unsafety checking requires this to be valid

Remove unnecessary recursive call to parent unsafeck

5c58a1b

All bodies are unsafe checked anyway. Current MIR unsafeck also just returns for closures.

Add tests (currently broken) for unsafe + inline const pat

d2830cb

Extract shared logic into a new function

d6dc912

nbdd0121 force-pushed the inline_const_unsafe branch from 14a90a8 to d6dc912 Compare December 13, 2022 02:35

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

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

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

matthiaskrgr mentioned this pull request Dec 13, 2022

Rollup of 7 pull requests #105667

Merged

bors merged commit 7357cfb into rust-lang:master Dec 14, 2022

rustbot added this to the 1.68.0 milestone Dec 14, 2022

nbdd0121 deleted the inline_const_unsafe branch December 14, 2022 01:41

nbdd0121 mentioned this pull request Dec 14, 2022

Stabilise inline_const #104087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow unsafe through inline const #105147

Allow unsafe through inline const #105147

nbdd0121 commented Dec 1, 2022 •

edited

Loading

rustbot commented Dec 1, 2022

rustbot commented Dec 1, 2022

This comment has been minimized.

RalfJung commented Dec 3, 2022 •

edited

Loading

nbdd0121 commented Dec 4, 2022

nagisa commented Dec 11, 2022

oli-obk commented Dec 13, 2022

This comment has been minimized.

bors commented Dec 13, 2022

bors commented Dec 13, 2022

This comment has been minimized.

rust-timer commented Dec 13, 2022

oli-obk commented Dec 13, 2022

bors commented Dec 13, 2022

Allow unsafe through inline const #105147

Allow unsafe through inline const #105147

Conversation

nbdd0121 commented Dec 1, 2022 • edited Loading

rustbot commented Dec 1, 2022

rustbot commented Dec 1, 2022

This comment has been minimized.

RalfJung commented Dec 3, 2022 • edited Loading

nbdd0121 commented Dec 4, 2022

nagisa commented Dec 11, 2022

oli-obk commented Dec 13, 2022

This comment has been minimized.

bors commented Dec 13, 2022

bors commented Dec 13, 2022

This comment has been minimized.

rust-timer commented Dec 13, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

oli-obk commented Dec 13, 2022

bors commented Dec 13, 2022

nbdd0121 commented Dec 1, 2022 •

edited

Loading

RalfJung commented Dec 3, 2022 •

edited

Loading