Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify safety documentation of ptr::swap and ptr::copy #114794

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

RalfJung
Copy link
Member

Closes #81005

@rustbot
Copy link
Collaborator

rustbot commented Aug 14, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 14, 2023
Comment on lines 2712 to 2714
/// * `src` must remain valid for reads even after `dst` is written, and vice versa.
/// (In other words, there cannot be aliasing restrictions on the use of these pointers.)
///
Copy link
Member

@m-ou-se m-ou-se Aug 15, 2023

Choose a reason for hiding this comment

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

I didn't quite understand what this meant in practice until I checked out #81005.

Maybe there's a different way of wording this?

Would it work to not add this bullet point, but instead add "during the entire call to swap()" to the first two bullet points? (Maybe with a clarifying note saying that this means that the src and dst regions may not overlap.)

Copy link
Member Author

Choose a reason for hiding this comment

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

They may overlap though, the pointers just must allow overlapping access. So for instance passing ptr and ptr.add(N) is fine even if things overlap. However passing &mut *ptr twice generates two fresh mutable references and now if things overlap, that lead to UB due to aliasing violations.

It's hard to precisely say this while the aliasing model is still being determined.

add "during the entire call to swap()" to the first two bullet points?

If we do that I think we have to say what happens during the call. So we could say something like

  • src must be [valid] for reads of count * size_of::<T>() bytes, and must remain valid even if dst is written to for count * size_of::<T>() bytes.

But that seems very similar to the current wording, so I am not sure if it is clear enough.

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2023
@RalfJung
Copy link
Member Author

Not sure if this wording is any better, but it's the best I could come up with so far.
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2023

@m-ou-se I think I replied to all your comments -- what's the next step for this PR?

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Small nit: Would it be better to say even when or even while instead of even if? (The if makes it sound a bit like those things only happen conditionally.)

r=me either way.

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2023

Small nit: Would it be better to say even when or even while instead of even if? (The if makes it sound a bit like those things only happen conditionally.)

My mother tongue uses the same word for "if" and "when" so this is probably just me missing a connotation that will be obvious to native speakers. I changed the wording.

@bors r=m-ou-se

@bors
Copy link
Contributor

bors commented Sep 5, 2023

📌 Commit 4684ffa has been approved by m-ou-se

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 Sep 5, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2023

@bors rollup

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 5, 2023
clarify safety documentation of ptr::swap and ptr::copy

Closes rust-lang#81005
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2023
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#114794 (clarify safety documentation of ptr::swap and ptr::copy)
 - rust-lang#115397 (Add support to return value in StableMIR interface and not crash due to compilation error)
 - rust-lang#115559 (implied bounds: do not ICE on unconstrained region vars)

Failed merges:

 - rust-lang#115532 (Implement SMIR generic parameter instantiation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 14c57f1 into rust-lang:master Sep 5, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 5, 2023
@RalfJung RalfJung deleted the swap-safety branch September 8, 2023 13:45
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-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.

Safety of ptr::swap is unclear
4 participants