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]

This does look slightly better, but not by much. The extra check we add is utterly trivial now when the type is a wide pointer, not sure why that would show up at all -- maybe LLVM is just having a harder time optimizing this code now.

[JonathanBrouwer]

Code looks good to me with one nit/question.
We need to wait for crater & lang team

View changes since this review

[craterbot]

🚧 Experiment pr-148967 is now running

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

[RalfJung]

I just realized this has the interesting consequence of making the following code UB according to Miri:

#![feature(core_intrinsics)]
#![feature(custom_mir)]

use std::intrinsics::mir::*;

#[custom_mir(dialect = "runtime", phase = "optimized")]
fn test(x: (i64, i8)) {
    mir! {
        {
            x = x;
            Return()
        }
    }
}

fn main() {
    test((0, 1));
}

I think that's fine, many such assignments where LHS and RHS overlap are already UB -- but ScalarPair types have been exempt so far. However this means we have to update the docs for which MIR assignments allow the LHS and RHS to overlap, and which do not.

(Note that this only affects MIR-level assignments. In the surface language obviously arbitrary overlap is allowed; MIR building introduces copies to avoid UB.)

[craterbot]

🎉 Experiment pr-148967 is completed!
📊 6 regressed and 2 fixed (737827 total)
📊 1953 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-148967/retry-regressed-list.txt ↩

[RalfJung]

2k spurious results seems like a lot
@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-148967/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-148967-1 created and queued.
🤖 Automatically detected try build 1a91d48
⚠️ Try build based on commit 4ba01da, but latest commit is 4a3e937. 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

[RalfJung]

In terms of those 6 regressions

• There's an ICE that seems unrelated ("no resolution for an import")
• "unable to start container"
• "unable to get packages from source"
• A build failure in a C/C++ dependency

So, those all seem spurious too.

[theemathas]

The "no resolution for an import" one is #147966

[craterbot]

🚧 Experiment pr-148967-1 is now running

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

[craterbot]

🎉 Experiment pr-148967-1 is completed!
📊 0 regressed and 0 fixed (1905 total)
📊 119 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-148967-1/retry-regressed-list.txt ↩

[scottmcm]

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

Just checking here: is this a typed copy or an untyped copy?

My instinct here is that on a typed copy we need to uninit-out the padding because otherwise we're just changing to a different "rustc implementation choices" that leaks.

Basically, it's my understanding that this is UB:

use std::mem::MaybeUninit;
#[derive(Debug, Copy, Clone)]
struct Demo(u8, u16);

let first = MaybeUninit::<Demo>::zeroed();
let mut second = MaybeUninit::<Demo>::uninit();
*second.as_mut_ptr() = *first.as_ptr(); // <-- HERE
let demo: u32 = std::mem::transmute(second);
dbg!(demo);

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=58a11499139a5d55224d7833bdbda562

But is this changing the HERE copy to copy the uninit bytes too? Or am I misunderstanding which copies we're talking about?

---

Oh, maybe a better example because it's a hard error from CTFE right now:

let x = const { unsafe {
    use std::mem::MaybeUninit;
    #[derive(Debug, Copy, Clone)]
    struct Demo(u8, u16);
    
    let first = MaybeUninit::<Demo>::zeroed();
    let mut second = MaybeUninit::<Demo>::uninit();
    if true {
        // Typed copy causes hard error
        *second.as_mut_ptr() = *first.as_ptr(); 
    } else {
        // Untyped copy is fine
        second.as_mut_ptr().copy_from_nonoverlapping(first.as_ptr(), 1);
    }
    let demo: u32 = std::mem::transmute(second);
    demo
} };
dbg!(x);

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0d3a17f4ec8d8f31f7c7788ea7c76063

So if that typed copy in CTFE changed to a mem-to-mem copy (which I think is essentially an untyped copy?) that'd no longer be a hard error, which seems like potentially not what we'd want?

I might be completely misunderstanding this issue, though 😬

[RalfJung]

Just checking here: is this a typed copy or an untyped copy?

We are talking about what happens inside the interpreter on a typed copy. For performance reason, those are not implemented exactly -- they are implemented as in a way that is equivalent for non-UB code, but fails to detect some UB. (Specifically, for many but not all types, they are implemented as just an untyped copy.) That was the idea, anyway. It turns out the difference is also observable without UB because, due to LLVM and ultimately system linker limitations, pointer fragments may not occur in the final value of a const.

We could fix this by doing proper, full typed copies all the time, but that would be expensive. So what this PR proposed is to instead make behavior consistent in a different way that still prevents leaking details of the layout computation code.

Oh, maybe a better example because it's a hard error from CTFE right now:

Indeed, under this PR, we no longer detect the UB in that code. We can't have everything... and which const UB we detect is not guaranteed to be stable.

[RalfJung]

I guess here is another way to frame it:

Generally, it is correct for a Rust implementation to implement a typed copy via an untyped copy. That just reduces how much UB there is. The const-eval interpreter, at the moment, performs something very close to proper typed copies for types that happen to get Scalar or ScalarPair layout -- but that's not a stable guarantee, so this does not leak the arbitrary layout choices in a meaningful way.

However, for types with padding, due to the "no pointer fragments in final value" check, whether we do a typed copy or not becomes actually observable in UB-free code. This can go both ways: there are cases where doing the typed copy makes more code compile (because we avoid copying pointer fragments in padding that later cause problems), and there are cases where doing the typed copy makes less code compile (because the pointer fragments in padding that we copy can combine with other pointer fragments to form a full pointer which satisfies the "no pointer fragments" check). So to make things behave consistently, I propose we have const-eval always fall back to an untyped copy when there is padding. This means we miss some UB, but it ensures behavior is independent of rustc layout choices.

[RalfJung]

After some more discussion with @Nadrieril, let me try to phrase this in terms of what we might say in the reference (instead of phrasing this in terms of breaking changes, which is what I did so far). I will assume that we do want to keep supporting pointer fragments (since they make low-level unsafe code during const-eval behave more like at runtime). First, a recap of some relevant facts:

• We rely on linkers to deal with pointers in the final values of consts, and linkers don't support pointer fragments.
• The result of a single const-eval query is more than just one value. It can be multiple allocations, and we don't have type information for all of them. Today we could probably get type information for all of them if we tried hard enough (multiple allocations can only arise due to temporary lifetime extension so we could use the type of that temporary). In the future however, we want to allow heap allocations during const-eval and turning those allocations into global memory that can be accessed at runtime, and there's no natural way to get type information for these heap allocations. We could make the "carry alloc to runtime" operation typed, but that would feel quite strange.

As a consequence, we can't know which bytes in the result are padding, and if we see a pointer fragment, we have to hard error. For the Language Reference, we seem to have two choices:

• We fully nail down when which byte of a const-eval result is a pointer fragment. That would mean specifying and then never changing the exact implementation strategy of typed copies: which exact bytes get copied from where to where, even when that cannot be observed in the AM (since it would be UB for the program to check, since those bytes are nominally uninitialized).
• 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.

[Nadrieril]

At the last t-lang meeting I expressed confusion as to what exactly was being asked of t-lang here. Thanks to @RalfJung answering my questions I now have the following understanding, if you'll allow me to be pedantic about what is and isn't specified:

Constraints

Constraint 1: The memory we get from const-eval is untyped.

For root allocations (e.g. a const FOO item), we could know the type, but non-root allocations exist today with lifetime extension and crucially we want to leave the door open to more of them, notably via a const-eval allocator.

Constraint 2: The memory we get from const-eval is not fully defined by the Rust Abstract Machine (i.e. the Rust runtime specification).

The Rust Abstract Machine specifies how a Rust program behaves on execution, but doesn't specify it completely: for example, on a typed copy for a type with padding, the AM does not specify what happens to the padding bytes. A concrete implementation like const-eval can and must make choices for such cases.

Constraint 3: Codegen can't give a sensible byte value to AM-defined ptr fragments found in const memory.

If the AM says that a byte in memory is a pointer fragment, the only legal value it can take would be the corresponding byte of the final codegenned address. We can't zero it or uninitialize it or anything like that. However rustc never learns the final address (it is handled by linker shenanigans that don't work on pointer fragments), so if we encounter such a byte we must raise a compile error.

Lang consequences

Putting all of that together, since we can't know which byte is AM-defined vs implementation-dependent, we must error on any ptr fragment found in const-eval memory. This includes pointer fragments that happen to be there because of implementation choices. Despite the byte "not mattering" (reading it would be UB, so we could replace it with any value), we can't know that it doesn't matter. Hence within-spec implementation choices of the const-eval interpreter necessarily affect which programs compile.

Therefore, the least that the language could commit to would be:

Memory obtained from const-eval that isn't fully defined by the Rust AM (e.g. because it resulted from a typed copy for a type with padding) may cause the program not to compile.

I shall take this to be the de-facto current state of the language specification on this topic, even though this isn't in the Reference.

T-lang asks

This is where most of my confusion resided: there are two intertwined questions that sound very similar.

Question 1: Assuming that t-lang is ok with the Reference only saying "undefined const-eval bytes may cause compilation to fail", the question this PR asks of t-lang is a vibe-check about an implementation detail that affects which programs compile. This may change again in the future if the breakage stays low.

Question 2: T-lang may also choose to make the new behavior part of the Reference/Rust spec. In other words, the Reference would say something like "const-eval executes the program according to the given modified Abstract Machine/specification: ..." and the bytes in const-eval memory would then be fully pinned-down by the spec.

And to make question 2 more tricky, if we're strengthening the const-eval AM one could argue that the more natural choice is "typed copies uninitialize padding", which I understand to have negative perf consequences.

EDIT: Ah I hadn't seen Ralf's comment that already summarized our discussion. We're saying the same thing.