interpret: make typed copies lossy wrt provenance and padding

[RalfJung]

A "typed copy" in Rust can be a lossy process: when copying at type usize (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang/miri#845
Fixes rust-lang/miri#2182

[rustbot]

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
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

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

[michaelwoerister]

r? compiler

[saethlin]

r? saethlin

[saethlin]

This seems like the sort of thing that would normally be a query. Is there a reason it isn't?

[RalfJung]

I'm not sure we even want this cached during a normal compilation (but it probably won't hurt much either), and the RangeSet type is currently only declared in the interpreter so can't be used as a query. Also I am largely unfamiliar with the query system so adding a query is scary. ;)

[saethlin]

Ah, because we won't encounter unions repeatedly in const eval. That makes sense.

[RalfJung]

If we have many constants of the same (union) type, then we would encounter it repeatedly and caching might be worth it. But we don't do validation during const-eval by default so the cache is not nearly as necessary as for Miri.

I can see how making this a query makes sense. But it would probably be a query we wouldn't want to serialize for incremental (I assume there's a way to do that), and as I said I am not sure where we would then put the RangeSet. Probably rustc_data_structures, but that seems odd for such a specific data structure. I'm open for suggestions though. :)

[saethlin]

You've made a good case that we shouldn't make this into a query in this PR, I'm filing away this conversation for the next time I'm squinting at interpreter profiles :)

[saethlin]

Why does this single caller not want provenance and padding reset? I looked at the docs for this function for context and I think they refer to a function might_permit_raw_init that doesn't exist?

[RalfJung]

This is used for assert_zero_valid / assert_mem_uninitialized_valid. The CompileTimeMachine that we operate in here is discarded at the end of this function, alongside the data we are validating here. So there's no reason to reset its padding and provenance, nobody will ever see those writes anyway.

[saethlin]

So this function parameter and all the branching on it is supposed to be a performance optimization?

[saethlin]

FYI I have taken to always using the hyperlink rustdoc syntax when mentioning other functions so that the link checker ensures that I can't mention a function that doesn't exist. The habit has already prevented a few of my doc comments from rotting. It's extra work, up to you if you want to adopt it.

[RalfJung]

So this function parameter and all the branching on it is supposed to be a performance optimization?

That's one part.

Also this is not the only caller that doesn't to the reset -- const_validate_operand also doesn't reset. Basically I wanted this change to be Miri-only.

And validate_operand in the recursive case, when it validates behind references, also doesn't reset padding and provenance -- and that is crucial, it would be wrong to reset this (and indeed the Miri test suite fails when you try, since if you ever have a reference to read-only memory it then blows up when trying to reset padding and provenance there). So branching in the validator on whether we reset provenance and padding is definitely required.

[saethlin]

I'm very happy to see less interior mutability. The compiler uses more casual Cell and RefCell than any other codebase I've worked in which seems ironic.

@rustbot author

[rust-timer]

Finished benchmarking commit (304b7f8): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	-	-	0
Regressions ❌ (secondary)	2.1%	[0.2%, 4.7%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -2.8%)

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-3.7%, -1.4%]	3
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -2.3%)

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

Binary size

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

Bootstrap: 757.734s -> 756.628s (-0.15%)
Artifact size: 341.11 MiB -> 341.23 MiB (0.03%)

[RalfJung]

Oh wow, just having these branches sit there, even if they are never taken, shows up so strongly in the benchmarks?

I could add a Machine associated const that should make the branches trivially optimized away... but that will also mean one cannot enable resetting padding with -Zextra-const-ub-checks any more.

[Kobzol]

It only got a significant regression on CTFE stress tests. If you don't think that this should affect real programs, I think it would be fine to just take the hit (as it sounds that the proposed optimization would introduce a lot of complexity into the code).

[RalfJung]

It wouldn't be a lot of complexity, but it would make the (nightly-only) const-eval UB check less effective. But those checks already miss a bunch of UB anyway so maybe that's okay.

[djkoloski]

Is it intended that after this change MaybeUninit::zeroed() no longer returns a completely-zeroed value? Because it's being returned from a function, the padding bytes of the interior T are being uninitialized.

I ran into this just now in rkyv, as I was using MaybeUninit::zeroed() to make a zeroed struct that eventually gets cast to bytes and written to a vector.

[saethlin]

Function returns are a typed copy. This PR only changed the behavior of the const eval interpreter, codegen has always been capable of breaking such code.

[RalfJung]

Yes, this is intended -- padding bytes are always uninitialized. And for very unfortunate reasons related to repr(transparent) and how the C ABI works on some targets, MaybeUninit<T> inherits the padding of T.

Happy to hear that is already found a bug in the wild. :D Is there something we can link to for this bug? If you wouldn't mind I'd like to add this to the "bugs Miri found" list.

[djkoloski]

Function returns are a typed copy.

Of course. I think the more salient question here is "what does a typed copy mean for unions?". Is there any documentation about what bytes are considered padding in unions? Is just MaybeUninit special? I think there's an argument to be made that unions having padding doesn't make sense, as what's stored in a union is not generally known to the compiler. I personally think "unions have padding where every field has padding" could make sense too, but I haven't seen any documentation about this as far as I can remember.

Is there something we can link to for this bug?

I think the best I have is this failed CI run. I haven't checked for other potentially-affected sources yet. Feel free to add whatever you'd like! 🙂

[RalfJung]

I think the more salient question here is "what does a typed copy mean for unions?".

Yes, that is indeed non-trivial.

I would love it to be a fully copy of all bytes, but sadly, that does not work. So currently we use "a union has padding where all fields have padding". I'd love to change this (see rust-lang/unsafe-code-guidelines#518), but I am not sure if that is possible.