Keep valid scalar ranges attached to newtypes when dealing with their inner field.

[luqmana]

Fixes #97161 by keeping an "effective" scalar validity range on PlaceRef and propagating it for newtypes of primitives.

[rust-highfive]

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

[luqmana]

cc @oli-obk @drmeepster

[michaelwoerister]

Thanks for the PR, @luqmana! This looks good to me (great tests!) but I'm not deeply familiar with the layout code and have run into surprises there recently. So, it would be good to get a second set of eyes on it:

r? rust-lang/compiler (if @eddyb has time to take a look at this, that would be ideal)

[eddyb]

I've left some comments but before we proceed in either direction I want to check what the perf numbers are like.

[eddyb]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 7a2ea71d46532bf1c916462deb5d264865ef94ba with merge 078c002eef9695d4b5ad23da2d16ce3096b06451...

[bors]

☀️ Try build successful - checks-actions
Build commit: 078c002eef9695d4b5ad23da2d16ce3096b06451 (078c002eef9695d4b5ad23da2d16ce3096b06451)

[rust-timer]

Queued 078c002eef9695d4b5ad23da2d16ce3096b06451 with parent 56fd680, future comparison URL.

[rust-timer]

Finished benchmarking commit (078c002eef9695d4b5ad23da2d16ce3096b06451): comparison url.

Instruction count

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	0.2%	0.2%	1
Regressions 😿 (secondary)	0.5%	0.5%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.1%	-1.1%	3
All 😿🎉 (primary)	0.2%	0.2%	1

Max RSS (memory usage)

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: 😿 relevant regressions found

	mean1	max	count2
Regressions 😿 (primary)	3.0%	3.0%	1
Regressions 😿 (secondary)	2.3%	2.7%	2
Improvements 🎉 (primary)	-0.8%	-0.8%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	1.1%	3.0%	2

Cycles

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

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.

@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 ↩ ↩²

[luqmana]

I've left some comments but before we proceed in either direction I want to check what the perf numbers are like.

Could I get a new perf run?

[tmiasko]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

☀️ Try build successful - checks-actions
Build commit: 3fa09c607228910837729e1e850ae97490c9d916 (3fa09c607228910837729e1e850ae97490c9d916)

[rust-timer]

Queued 3fa09c607228910837729e1e850ae97490c9d916 with parent 0a43923, future comparison URL.

[rust-timer]

Finished benchmarking commit (3fa09c607228910837729e1e850ae97490c9d916): comparison url.

Instruction count

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.5%	0.5%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.8%	-1.6%	7
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

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

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.4%	-2.6%	3
Improvements 🎉 (secondary)	-2.9%	-2.9%	1
All 😿🎉 (primary)	-2.4%	-2.6%	3

Cycles

Results

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

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.9%	-2.9%	1
Improvements 🎉 (secondary)	-6.5%	-6.5%	1
All 😿🎉 (primary)	-2.9%	-2.9%	1

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.

@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 ↩ ↩² ↩³

[JakobDegen]

So, based on MIR semantics as currently documented, I do not believe this is sound (and would lead to miscompilations at least in conjunction with dest prop as specced above). We currently specify that the validity of a load depends on the type at which the load operation happens. This would mean that for

#[repr(transparent)]
#[rustc_layout_scalar_valid_range_start(1)]
pub struct NonZeroUsize(usize);

MIR like this:

// _2: NonZeroUsize, _3: usize
_2.0 = 0; 
_3 = _2.0;

is not UB because the load in the second assignment is at type usize. Of course, this:

// _2: NonZeroUsize, _3: NonZeroUsize
_2.0 = 0;
_3 = _2

is UB because the load is at type NonZeroUsize.

Of course, that is not to say that this change is undesirable - it does seem nice to have this. But we should change the MIR documentation to reflect an alternative that supports this first. Essentially, the way that I think we would need to do this is to pass the validity range down through field projections the same way we do it for metadata. This doesn't make much sense for generic types though, although I suppose we can just ban the annotations on generic types? (Maybe we do already?) @RalfJung thoughts?

Edit: The suggestion above comes with trouble actually. Now optimizing

_10 = &mut _2.0;
*_10 = _3;

to _2.0 = _3 is unsound. Fixing that would require us to also propagate some kind of increased constraint to the provenance of _10 (or something like that). This is basically equivalent to some of the suggestions for rust-lang/unsafe-code-guidelines#204 though and we have no real model for that.

[RalfJung]

I think you meant to reference rust-lang/unsafe-code-guidelines#84.

Good catch!
Basically the optimization has to determine if the NonNull has possibly been changed sine it was last used as a NonNull. I don't think we want to make code like this UB (but this PR would do that):

r = &mut nonnull.0;
(r as *mut usize).write(0); // possibly in another function
v = nonnull.0;

[JakobDegen]

Ah, yeah, thanks for linking the right issue.

Fwiw, I'm not so convinced we don't want code like that UB - but if we make it UB, that should be done in conjunction with an appropriate resolution to the UCG issue, and not on its own

[luqmana]

Thanks @JakobDegen & @RalfJung for chiming in and the UCG link! Looks like there's a lot more subtleties to this so gonna take some time to look into that.

@rustbot label +S-blocked

[lcnr]

going through my assigned prs rn, not really something I can review even if it weren't blocked

r? @RalfJung for now

[RalfJung]

Sorry, I cannot review this -- I can talk without end about MIR semantics conceptually, but that is quite different from actually implementing them in the LLVM backend. ;)
r? rust-lang/compiler

However, this is also a language change I think, and would need T-lang signoff. This introduces UB Miri does not currently detect. My personal opinion is this should not be UB.

[apiraino]

Adding T-lang as per this comment, hopefully this will automatically reach someone.

@rustbot label T-lang

[RalfJung]

Cc @rust-lang/lang
The question is basically, should this code have UB? That is related to UCG discussion rust-lang/unsafe-code-guidelines#77: is it UB to have a reference that points to something that violates the validity invariant? The reference says yes, but that is just to keep our options open. I don't think this should be UB:

• Making validity purely a property of the value (not depending on the "current state of memory") is conceptually a lot simpler; it lets us define validity in a very elegant way. This is ultimately the spec we have to teach, so I think this matters.
• If validity depends on memory, that means a value that was once valid can become invalid any time without an action of the local code. That's too much "spooky action at a distance" for me. In particular it makes whether or not code has UB depend a lot on when exactly we do and do not check the validity invariant. I think this could actually make some optimizations harder: if we start with a valid reference (let's say it has interior mutability), then we might still not be able to hoist a "use" of that reference out of a loop unless we are sure no other code will in the mean time make the memory this reference points to invalid. (Arguably this example does not apply to noalias references -- &mut Unpin and &Freeze. The argument becomes a lot trickier than it would otherwise have been though, for cases where we do hand out the reference to other threads.) So we would probably want to be able to represent in the IR "assert deep validity here, assert shallow validity there", to still easy do these optimizations. There's a lot of design here that we should do before actually changing codegen, IMO -- if we want this.
• Also I am getting nightmares when I think of how slow Miri will become when we check validity recursively. ;) Though to be fair, I haven't actually tried this.

My personal opinion is hence that we should not merge this. I would even prefer us to commit to not making this code UB.

[JakobDegen]

@RalfJung I don't think the scope of this question needs to be this wide. If we are only concerned about this PR, there is no need to involve references. It suffices to ask whether this code is UB:

#![feature(rustc_attrs)]

#[rustc_layout_scalar_valid_range_start(1)]
struct NonZero(usize);

let x: NonZero = NonZero(5);
x.0 = 0;
dbg!(x.0);
x.0 = 5

This PR would make the access inside dbg! UB, despite there being no access at type NonZero.

I also think we should avoid making a decision on UCG#77 right now. I feel pretty strongly that we need to investigate further how it interacts with non-freeze enum retagging and that whole chain of complexity. (For anyone who wants details)

Separately of the concrete question though, I am not quite sure what is being asked of T-Lang here. Are we really asking for them to commit to [whichever code] not being UB? I would find that surprising, as it feels like there are much more obvious parts of the memory model that do not have T-Lang approval yet.

[RalfJung]

It suffices to ask whether this code is UB:

I don't think so. Your example is rust-lang/unsafe-code-guidelines#84, which is basically orthogonal to rust-lang/unsafe-code-guidelines#77. But the example at the top of #97161 is quite clearly about a reference. In particular, no matter how rust-lang/unsafe-code-guidelines#84 is resolved, the optimization requested in #97161 is not correct if validity is "shallow" for references. I assume, based on the description, that this PR implements that optimization. So this is definitely about rust-lang/unsafe-code-guidelines#77.

[RalfJung]

Separately of the concrete question though, I am not quite sure what is being asked of T-Lang here. Are we really asking for them to commit to [whichever code] not being UB? I would find that surprising, as it feels like there are much more obvious parts of the memory model that do not have T-Lang approval yet.

Well, I don't think we should land this PR without deciding that we actually want this to be UB. Which we only get by resolving UCG#77. Not landing this PR doesn't mean it's not UB. But landing this PR means it is UB. The optimizations we do and attributes we emit are a lower bound on the UB we have. This PR adds more attributes, so it increases that lower bound, and that is what I am objecting.

We're so close to finally catching up with the LLVM flags we emit in Miri, making sure we have our existing bases covered. This PR proposes to race ahead and emit more flags, make more promises to LLVM. This time I think we should design first, and then start adding attributes. I know that's a new strategy for us, so far we "have just always had" those LLVM attributes (noalias and dereferenceable and nonnull and whatnot) and then we designed the spec around it, but it doesn't have to be that way.

[JakobDegen]

I don't think so. Your example is rust-lang/unsafe-code-guidelines#84

I've updated the example to show that that's not the case (or at least to show that x never being used again is immaterial to my point).

the optimization requested in #97161 is not correct if validity is "shallow" for references

That's not necessarily true, but I don't want to get into that here. We can discuss more on Zulip.

Well, I don't think we should land this PR without deciding that we actually want #97161 (comment). Which we only get by resolving UCG#77. Not landing this PR doesn't mean it's not UB. But landing this PR means it is UB. The optimizations we do and attributes we emit are a lower bound on the UB we have. This PR adds more attributes, so it increases that lower bound, and that is what I am objecting.

Agreed. That makes me think that there isn't actually a T-Lang question here. I mean, I suppose we can ask, but I agree with you that I hope that the response is "that's a good question that we don't want to commit to right now"

[RalfJung]

This PR would make the access inside dbg! UB, despite there being no access at type NonZero.

Okay so your example shows that both UCG#77 and UCG#84 to be resolved to evaluate whether this PR is sound. Fair.

But UCG#84 alone is certainly not sufficient; the motivating example needs UCG#77.

Agreed. That makes me think that there isn't actually a T-Lang question here. I mean, I suppose we can ask, but I agree with you that I hope that the response is "that's a good question that we don't want to commit to right now"

Well, I would also be fine with closing this PR without going through T-lang. I just wanted to keep them in the loop to ensure that I am not going rogue here.

@luqmana sorry that this is so hard, but whether you realized it when writing the code or not, your proposed change is a Big Deal and basically changing the specification of Rust. So I think the chances of it landing at this time are slim.

[luqmana]

@luqmana sorry that this is so hard, but whether you realized it when writing the code or not, your proposed change is a Big Deal and basically changing the specification of Rust. So I think the chances of it landing at this time are slim.

Oh, no need to apologize! I like learning more about some of these subtle details. And don't worry, I figured this wasn't gonna be completely straight forward :P Alas, just been too busy to do anything more with this rn. Feel free to close this and the conversation can be moved to the issue instead.

[joshtriplett]

I agree with @RalfJung's proposal here: it should be fine for &T to point somewhere that doesn't meet the validity invariant of a T, as long as you don't dereference it.

@RalfJung, is that an accurate statement of your proposal?

If so, could we get a reference PR making this proposal, and we can FCP it for ratification?

[RalfJung]

it should be fine for &T to point somewhere that doesn't meet the validity invariant of a T, as long as you don't dereference it.

@RalfJung, is that an accurate statement of your proposal?

That depends on how exactly you define "dereference". :)

For x: &NonNull, the code in question does (*x).field. So, it does reference x in the sense of using the * operator on it. However, it then continues to project to a field before converting the place to a value, i.e., before any memory is actually being accessed. The only memory access here is at type *const ().

So memory is never accessed at the offending T (NonNull), but a dereference at that type does happen.