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 depended 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

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

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

[RalfJung]

Based on earlier review:
@bors r=JonathanBrouwer

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: S-waiting-on-t-lang.

[RalfJung]

Ah, I missed the waiting-on-docs part... here we go: rust-lang/reference#2138.

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