Make too-large byte sizes a validity error in Layout

[scottmcm]

Now that #95295 made sizes over isize::MAX disallowed in Layout, here's a PR to make it a validity error, and thus something that CTFE/MIRI can catch, like was done for alignment in #95361.

r? @Mark-Simulacrum
cc @CAD97

[CAD97]

I worry that this might have negative compiletime impact (c.f. #99117 and just how touchy this code is to insignificant changes), but there's no way to tell without trying it.

[scottmcm]

This doesn't touch monomorphized methods, and all of it should be inlined while compiling core by LLVM (or maybe even MIR, now!) since it's all runtime nothing, so hopefully it'll not have perf impact. But let's check:

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit b34f5b9 with merge 31f194589df939278844135001202a5fb3a984dd...

[bors]

☀️ Try build successful - checks-actions
Build commit: 31f194589df939278844135001202a5fb3a984dd (31f194589df939278844135001202a5fb3a984dd)

[rust-timer]

Queued 31f194589df939278844135001202a5fb3a984dd with parent c396bb3, future comparison URL.

[rust-timer]

Finished benchmarking commit (31f194589df939278844135001202a5fb3a984dd): comparison url.

Instruction count

• Primary benchmarks: mixed results
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	0.8%	0.8%	2
Regressions 😿 (secondary)	0.4%	0.4%	2
Improvements 🎉 (primary)	-0.6%	-0.8%	6
Improvements 🎉 (secondary)	-2.6%	-5.6%	5
All 😿🎉 (primary)	-0.3%	-0.8%	8

Max RSS (memory usage)

Results

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-4.8%	-4.8%	1
Improvements 🎉 (secondary)	-2.4%	-3.5%	4
All 😿🎉 (primary)	-4.8%	-4.8%	1

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	4.0%	4.0%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.8%	-3.8%	1
Improvements 🎉 (secondary)	-6.5%	-9.5%	7
All 😿🎉 (primary)	0.1%	4.0%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[scottmcm]

Perf here looks to me like it's a wash -- about as much improvements as regressions -- so that's good!

And one huge, supposedly significant improvement that I don't trust:

So I think this is good for review.

[CAD97]

(it took me a while to write this so I didn't notice @scottmcm had already posted a minor analysis)

Secondary: the externs stress test saw both the two regressions and the hugely significant boost, to incremental compilation. Odd that this would be the case. Both cases are an estimated cycles benefit, though, so /shrug

Primary: mixed, but more green than red. All primary changes are opt builds; debug is unchanged.

RSS: hey look at that, an unambiguous win, even if somewhat isolated.

I think these results at a minimum support that this change isn't a negative. Changes in this file just tend to be noisier than in others.

[Mark-Simulacrum]

I'm worried that we're increasingly making things like this language-UB (at least at a current-implementation level), rather than "just" adding asserts that CTFE/MIRI understand. It seems like catching someone transmuting from/to Layout is probably not the intent here, so adding an ub_checking_assert or something like it feels like a better path here. (I think we could define that in std/core today? We support panic! in CTFE and we have the closure-based dispatch intrinsic to detect CTFE, which I think includes miri evaluation?)

My main concern comes from this adding more noise to our IR and in general being harder to follow for folks working with these types, rather than just asserting in constructors. It also seems unfortunate to send a message that in order to get a little more checking for safety preconditions you have to make the leap to make violating them a full-blown validity error, not just around actual usage, which might escalate what would've been an assert of some kind into just UB. In general, it feels like just because we can define some range on a type doesn't mean that we should make it UB to be outside that range -- it's the equivalent of doing unchecked_add and such in cold code.

To be clear, I'm all for improving the ability of MIRI and other tooling to catch these bugs, but I'm not convinced that this is the right approach. In the long run we're definitely going to want a sanitizer_assert! or similar, I expect, so I think we should invest in that now, rather than running with the (poor, annoying to use, very sharp tool) rustc attr-based hack around its lack.

[CAD97]

That might be a good argument to revert the ValidAlign (is-power-of-two int via enum) change and add ValidSize instead. The support code to define ValidAlign is nontrivial, but ValidSize is a much simpler standard application of #[rustc_valid_scalar_range] and provides essentially the same amount of niche space.

Along those lines, perhaps we could add a #[cfg(miri)] arm to assert_unsafe_precondition!? (cargo miri goes through the runtime codepath. I don't know which -Zunleash-the-miri-inside-of-you takes.)

[scottmcm]

I would much rather keep ValidAlign than ValidSize, if I had to choose. Because CAD's change in https://github.com/rust-lang/rust/pull/99136/files#diff-787ccc657785af6ea82114db5d168f11efb1caf9adc8eeeafb39323a2163e2d8R77 depends on having the type boundary for it to be able to make that function safe.

I think looking at just the MIRI detection misses a big part of the value, here. I'd really like to make https://doc.rust-lang.org/nightly/std/alloc/struct.Layout.html#method.align_to take a public ValidAlign of some sort, because the current semantics of "you just get garbage if you pass something that's not a power of two" is distasteful to me.

Admittedly, that doesn't need a validity error for it, specifically, just restricted safe constructors. But honestly once the type exists the work to have the validity attributes is negligible, so I feel like they might as well exist to give LLVM nice !range metadata and such.

I could close this PR and file an ACP for making ValidAlign into a "real" type before revisiting more like this, if you'd like.

[Mark-Simulacrum]

I don't have any particular objections to the types existing (ValidSize and ValidAlign, etc.), but more so that it seems quite unfortunate if we need to escalate to validity to get this kind of checking. It seems to me that LLVM isn't really going to cooperate here for now anyway (based on #49572), so it's not clear that we gain much from validity.

Ultimately, I suppose it doesn't matter that much - it's unlikely that users will practically encounter problems from the validity invariant here (vs. a safety invariant), but it just feels icky to me. I think it comes down to the fact that if we have merely a safety invariant, it's much easier for us to catch that at runtime (without miri) via asserts, but with a validity invariant it becomes increasingly likely that it just ends up UB and LLVM optimizes it out entirely. (Sort of like how C & C++ code, and probably Rust too, have problems with assert!(ptr != nullptr) getting deleted on them by optimizers).

It might be that concern isn't really applicable to these types though, which are more constrained in use and less likely to get accidentally dereferenced or otherwise assert validity before the check.

Based solely on the diff, r=me; I'll leave it up to you whether the argument I'm making feels convincing enough that it merits further discussion (probably in a libs meeting?). I'm a little iffy on it myself, only around 60-70% convinced this is going to be painful later. But we can also revert; we're not making stable guarantees here, so maybe that's OK.

[scottmcm]

I'll close this for now, and decide whether to come back to it later after maybe proposing libs-api changes.

[scottmcm]

I opened an ACP to expose the Alignment newtype to core: rust-lang/libs-team#108