const-eval: always do mem-to-mem copies if there might be padding involved

[RalfJung]

This is the final piece of the puzzle for #148470: when copying data of a type that has padding, always do a mem-to-mem copy, so that we always preserve the source padding exactly. That prevents rustc implementation choices from leaking into user-visible behavior.

This is technically a breaking change: the example at the top of #148470 no longer compiles with this. However, it seems very unlikely that anyone would have dependent on this. My main concern is not backwards compatibility, it is performance.

Fixes #148470

---

Actually that seems to be entirely fine, it even helps with some benchmarks! I guess the mem-to-mem codepath is actually faster than the scalar pair codepath for the copy itself. It can slow things down later since now we have to do everything bytewise, but that doesn't show up in our benchmarks and might not be very relevant after all (in particular, it only affects types with padding, so the rather common wide pointers still always use the efficient scalar representation).

So that would be my proposal to for resolving this issue then: to make const-eval behavior consistent, we always copy the padding from the source to the target. IOW, potentially pre-existing provenance in the target always gets overwritten (that part is already in #148259), and potentially existing provenance in padding in the source always gets carried over (that's #148967). If there's provenance elsewhere in the source our existing handling is fine:

• If it's in an integer, that's UB during const-eval so we can do whatever.
• If it's in a pointer, the the fragments must combine back together to a pointer or else we have UB.
• If it's in a union we just carry it over unchanged.

@traviscross we should check that this special const-eval-only UB is properly reflected in the reference. Currently we have this but that only talks about int2ptr, not about invalid pointer fragments at pointer type. I also wonder if this shouldn't rather be part of "invalid values" to make it clear that this applies recursively inside fields as well.
EDIT: Reference PR is up at rust-lang/reference#2091.

Originally posted by @RalfJung in #148470

Worth noting that this does not resolve the concerns @theemathas had about -Zextra-const-ub-checks sometimes causing more code to compile. Specifically, with that flag, the behavior changes to "potentially existing provenance in padding in the source never gets carried over". However, it's a nightly-only flag (used by Miri) so while the behavior is odd, I don't think this is a problem.

Originally posted by @RalfJung in #148470

---

Related:

• Partial pointers in padding can make const-eval fail #148470
• UB: update the extra clause for provenance UB during const evaluation reference#2091

[rustbot]

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[rustbot]

r? @JonathanBrouwer

rustbot has assigned @JonathanBrouwer.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[RalfJung]

@bors try
@rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 78c81ee (78c81ee3917a99dcff6e2e6822800f0492c415c3, parent: 733108b6d4acaa93fe26ae281ea305aacd6aac4e)

[rust-timer]

Finished benchmarking commit (78c81ee): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.0%, 0.3%]	7
Improvements ✅ (primary)	-2.8%	[-2.8%, -2.8%]	1
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.2%]	12
All ❌✅ (primary)	-2.8%	[-2.8%, -2.8%]	1

Max RSS (memory usage)

Results (primary -3.2%)

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

Cycles

Results (primary -2.7%, secondary -9.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)	-	-	0
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-9.4%	[-16.0%, -2.8%]	2
All ❌✅ (primary)	-2.7%	[-2.7%, -2.7%]	1

Binary size

Results (primary -1.1%, secondary 0.0%)

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

Bootstrap: 472.272s -> 472.014s (-0.05%)
Artifact size: 388.64 MiB -> 388.68 MiB (0.01%)

[RalfJung]

Uh okay I guess this is actually good for perf.^^ At least for the benchmarks we have. The copy apparently gets a little cheaper, but we force more things to use the less efficient in-memory representation. The latter just does not seem to matter in our benchmarks. Just to be safe: @craterbot check

[craterbot]

👌 Experiment pr-148967 created and queued.
🤖 Automatically detected try build 78c81ee
⚠️ Try build based on commit c4acb77, but latest commit is 01194d7. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[theemathas]

Most of the performance regressions are from the coercions benchmark. All it does is create an array of a large number of string literals in const. Why did this benchmark's performance regress? There is no padding involved in any of the types.

[RalfJung]

That's fair -- I should have gotten up-to-date perf numbers. We did try enabling full validation in const-eval before and it was never close to acceptable, but there's no harm in getting new numbers.

I made a separate PR for that: #149901.

[RalfJung]

@traviscross here are the numbers for the "naive" implementation: perf. It's pretty red. ;)

Now, there may be ways to optimize this if we only have to reset padding and provenance, but not do full validation... and there could be a fast-path for scalar types... like, one can pour basically an unbounded amount of time into making this faster. It's unclear how far this would actually get us. Also if we do that guided by our benchmark suite, we'd be overfitting for that very quickly I think. I'm afraid I don't think I have the time, patience, and motivation for that. I'd be happy to mentor/review patches for someone who does want to work on it, though.

But meanwhile, this PR doesn't really make things any worse than they are already, and it avoids accidentally changing behavior when the layout algorithm changes.

[traviscross]

Thanks for running those numbers. Does caching/memoizing come into play for optimizing this (and are we doing any now)? (Is that hard here?) As described ("doing a full traversal"), it seems the sort of thing that would be at risk of traversing the same path repeatedly, e.g. as a final value is built up from parts. Without memoizing, it's easy to see how this would go O(N^2).

[RalfJung]

There's currently no caching of any sort. Also, adding caching is not trivial... the naive option of "here's some pairs of types and raw byte lists for which we can skip validation in the future" would quickly consume huge amounts of RAM caching values we'll never need again. Representing raw byte lists (including uninit and provenance) is itself non-trivial, and even just checking the cache would be expensive.

Or alternatively we build a system where we remember "the bytes at offset O in allocation A have been validated for type T" -- but we'd have to be really sure we catch all codepaths where memory gets changed so that we can invalidate the cache. Also, Rust moves values around a lot, so this alone wouldn't suffice; we'd also want this information to be preserved when copying things from one place to another. This could be an interesting student project to speed up Miri, but it's not going to happen short-term.

[traviscross]

But meanwhile, this PR doesn't really make things any worse than they are already, and it avoids accidentally changing behavior when the layout algorithm changes.

For concreteness, the proposal I'd put before us, then, would be to accept this PR and to document in the Reference:

• We say that basically any uninit byte in the result may actually end up being a pointer fragment. This means the compiler could legally fail to compile any time there's an uninit byte in the result -- which of course it won't, but we make no hard stable guarantees for when exactly this will or will not fail. It's kind of a cop-out but also avoids us having to nail down details of the const-eval interpreter that cannot be observed in the AM.

We would be explicit that this remains part of our RFC 1122 underspecified language semantics and that we remain within our rights to change the behavior of typed copies within consteval.

@rfcbot fcp merge lang
@rustbot label +S-waiting-on-documentation

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[Nadrieril]

I find these perf results rather encouraging actually, given that this was a naive implementation. Noticeable regressions are on a limited set of tests, all of them stress-tests. I don't mind the currently-proposed lang decision so that we can get this merged, but if anyone with enough const-eval knowledge could try a non-naive version I'd be keen to revisit this.

[RalfJung]

Noticeable regressions are on a limited set of tests, all of them stress-tests.

I think this mostly shows that we have very few interesting consts in our benchmarks.

[tmandry]

@rfcbot reviewed

Ok, the proposal on the table sounds reasonable. Options I can see for having a more satisfying story in the future:

1. Uninitialize padding on typed copy (if we can improve the performance enough).
2. Tracking type information + Ralf's suggestion in const-eval: always do mem-to-mem copies if there might be padding involved #148967 (comment) for a typed "carry alloc to runtime" operation – that sounds like it would be very visible to end-users. I could imagine making optional to users for more precise tracking if this error becomes a problem for them.

The second brings to mind a more general typed allocator API, which also would have to be optional. Then we could have typed/untyped allocations just as we have typed/untyped copies. That could be useful e.g. for heap profiling applications, among other things.

I'm certain I'm not the first to think of this though. I guess the reasons not to go down this path are some combination of the following; are there others?

1. Not wanting added monomorphization; needing a TypeId mechanism that works for non-'static types to avoid it
2. Not being usable in 100% of applications
3. Prior art
4. Lack of need

[theemathas]

Turns out that there is already some code in the wild that depends on how exactly we do typed copies in consteval, according to crater on the PR implementing an alternative to this PR.

Edit: It is indeed just previously-uncaught consteval UB.

[RalfJung]

I think that's just code with const-UB that we currently do not detect. We are at liberty to break such code.

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.