Skip to content

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 3, 2024

By landing #116675 we decided that objects larger than isize::MAX cannot exist in the address space of a Rust program, which lets us simplify these rules.

For offset_from, we can even state that the absolute distance fits into an isize, and therefore exclude isize::MIN. This PR also changes Miri to treat an isize::MIN difference like the other isize-overflowing cases.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 3, 2024

Cc @rust-lang/opsem

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
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
Copy link
Collaborator

rustbot commented Jul 3, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 3, 2024
@RalfJung RalfJung force-pushed the offset-from-isize-min branch from ebabe18 to 1ec2432 Compare July 3, 2024 14:42
@RalfJung RalfJung changed the title offset_from: "the difference must fit in an isize" is a corollary offset_from, offset: clearly separate safety requirements the user needs to prove from corollaries that automatically follow Jul 3, 2024
/// Allocated objects can never be larger than `isize::MAX` and they never "wrap around" the
/// edge of the address space, so as a consequence of this, `count * size_of::<T>()` must
/// fit in an `isize` and adding the computed offset to `self` produces a resulting address
/// that fits into a `usize`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now also done the same kind of editing to offset: clearly state what the user needs to prove, and what follows automatically due to the invariants we have about the address space.

These docs are copied a couple of times, so I will wait for PR feedback before updating all the copies.

///
/// * The computed offset, **in bytes**, cannot overflow an `isize`.
/// * If the computed offset is non-zero, then the entire memory range between `self` and the
/// result must be in bounds of the [allocated object] `self` is derived from.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence sounds like it refers to the allocated object of the pointer itself (self), rather than what it is pointing to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In offset_from we talk about self being derived from a pointer to a particular allocated object... but adding that here makes the sentence too nested, I think. I should probably give a name to the allocated object.

/// If any of the following conditions are violated, the result is Undefined
/// Behavior:
/// The resulting pointer's address is computed as the address of the original pointer plus the
/// computed offset, `count * size_of::<T>()`, with infinite precision.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// computed offset, `count * size_of::<T>()`, with infinite precision.
/// computed offset, `count * size_of::<T>()` bytes.

I think talking about infinite precision here is more confusing than it helps. It's clearly to just say the calculation is not allowed to overflow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'd have to spell out how exactly it is not allowed to overflow -- i.e., what the bounds are. The nice thing about doing the arithmetic in infinite precision is that we can avoid that, since it falls out of the in-bounds requirement. But this spec is too clever for its own good...

@RalfJung RalfJung force-pushed the offset-from-isize-min branch 2 times, most recently from d5505a7 to f4f6724 Compare July 3, 2024 18:45
@RalfJung
Copy link
Member Author

RalfJung commented Jul 3, 2024

@Amanieu I have pushed a new revision, attempting to resolve your comments.

@Amanieu
Copy link
Member

Amanieu commented Jul 3, 2024

LGTM r=me

@RalfJung RalfJung force-pushed the offset-from-isize-min branch from f4f6724 to 80d0f14 Compare July 4, 2024 07:06
@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2024

I have updated the other copies of the offset docs. I hope I got them all!

@RalfJung RalfJung force-pushed the offset-from-isize-min branch from 80d0f14 to 9ba492f Compare July 4, 2024 12:14
@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2024

I found more copies of these docs on NonNull, and updated those as well.

@Amanieu
Copy link
Member

Amanieu commented Jul 6, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 6, 2024

📌 Commit 9ba492f has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2024
@bors bors merged commit 2137d19 into rust-lang:master Jul 6, 2024
@rustbot rustbot added this to the 1.81.0 milestone Jul 6, 2024
@RalfJung RalfJung deleted the offset-from-isize-min branch July 6, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants