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

Add documentation about the memory layout of UnsafeCell<T> #101717

Merged
merged 4 commits into from
Oct 16, 2022

Conversation

Pointerbender
Copy link
Contributor

@Pointerbender Pointerbender commented Sep 12, 2022

The documentation for UnsafeCell<T> currently does not make any promises about its memory layout. This PR adds this documentation, namely that the memory layout of UnsafeCell<T> is the same as the memory layout of its inner T.

Use case

Without this layout promise, the following cast would not be legally possible:

fn example<T>(ptr: *mut T) -> *const UnsafeCell<T> {
  ptr as *const UnsafeCell<T>
}

A use case where this can come up involves FFI. If Rust receives a pointer over a FFI boundary which provides shared read-write access (with some form of custom synchronization), and this pointer is managed by some Rust struct with lifetime 'a, then it would greatly simplify its (internal) API and safety contract if a &'a UnsafeCell<T> can be created from a raw FFI pointer *mut T. A lot of safety checks can be done when receiving the pointer for the first time through FFI (non-nullness, alignment, initialize uninit bytes, etc.) and these properties can then be encoded into the &UnsafeCell<T> type. Without this documentation guarantee, this is not legal today outside of the standard library.

Caveats

Casting in the opposite direction is still not valid, even with this documentation change:

fn example2<T>(ptr: &UnsafeCell<T>) -> &mut T {
  let t = ptr as *const UnsafeCell<T> as *mut T;
  unsafe { &mut *t }
}

This is because the only legal way to obtain a mutable pointer to the contents of the shared reference is through UnsafeCell::get and UnsafeCell::raw_get. Although there might be a desire to also make this legal at some point in the future, that part is outside the scope of this PR. Also see this relevant Zulip thread.

Alternatives

Instead of adding a new documentation promise, it's also possible to add a new method to UnsafeCell<T> with signature pub fn from_ptr_bikeshed(ptr: *mut T) -> *const UnsafeCell<T> which indirectly only allows one-way casting to *const UnsafeCell<T>.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 12, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 12, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2022
@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 12, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Sep 13, 2022

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Sep 13, 2022

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 13, 2022
@m-ou-se m-ou-se added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2022
@Pointerbender
Copy link
Contributor Author

Just a gentle little nudge: this (small) documentation PR needs only 1 more approval from either @Amanieu, @joshtriplett or @yaahc to start the FCP :) If there is any further feedback I could address in the mean time, please let me know! ❤️

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 4, 2022
@rfcbot
Copy link

rfcbot commented Oct 4, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@Amanieu
Copy link
Member

Amanieu commented Oct 4, 2022

LGTM.

On a somewhat unrelated note, should the documentation also mention the fact that UnsafeCell disables enum niche optimizations? For example Option<UnsafeCell<NonNull<u8>> is 16 bytes instead of 8. This is intentional for safety reasons (#68491, #87341).

library/core/src/cell.rs Outdated Show resolved Hide resolved
@Pointerbender
Copy link
Contributor Author

@Amanieu @RalfJung Thanks for the feedback! I will update the PR by tomorrow to incorporate both your suggestions 👍

library/core/src/cell.rs Outdated Show resolved Hide resolved
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 14, 2022
@rfcbot
Copy link

rfcbot commented Oct 14, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@Amanieu
Copy link
Member

Amanieu commented Oct 16, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 16, 2022

📌 Commit ddd119b has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 16, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#101717 (Add documentation about the memory layout of `UnsafeCell<T>`)
 - rust-lang#102023 (Add MaybeUninit array transpose From impls)
 - rust-lang#103033 (Update pkg-config)
 - rust-lang#103080 (pretty: fix to print some lifetimes on HIR pretty-print)
 - rust-lang#103082 (Surround type with backticks)
 - rust-lang#103088 (Fix settings page)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
/// thus this can cause distortions in the type size in these cases. Furthermore, it is only valid
/// to obtain a `*mut T` pointer to the contents of a _shared_ `UnsafeCell<T>` through [`.get()`]
/// or [`.raw_get()`]. A `&mut T` reference can be obtained by either dereferencing this pointer or
/// by calling [`.get_mut()`] on an _exclusive_ `UnsafeCell<T>`, e.g.:
Copy link
Member

@RalfJung RalfJung Oct 16, 2022

Choose a reason for hiding this comment

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

I still find the last two sentences very confusing. They have nothing to do with the rest of the paragraph, do they? And the examples have nothing to do with the 90% of the paragraph that talk about the by-value representation of UnsafeCell<T>.

There are two rather unrelated things mixed up between this paragraph and the examples:

  • the by-value layout of UnsafeCell<T>
  • some aspect of the interior mutability rules and how to get a mutable ptr to a shared reference to an UnsafeCell

We should have a paragraph and examples only about the first point.

And then maybe another paragraph and examples only about the 2nd point (which is not a new point, it just was not fully explicit so far).

@RalfJung
Copy link
Member

RalfJung commented Oct 16, 2022

@bors r-
the writing is still confusing and needs improving IMO (as I had said 10 days ago already)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 16, 2022
@RalfJung
Copy link
Member

Ah this will probably land because it is rolled up.

@Pointerbender please make a follow-up PR that fixes the writing issues here.

@bors bors merged commit cbc0a73 into rust-lang:master Oct 16, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 16, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 27, 2022
…manieu

Clarify documentation about the memory layout of `UnsafeCell`

This PR addresses a [comment](rust-lang#101717 (comment)) by `@RalfJung` in PR rust-lang#101717 to further clarify the documentation of `UnsafeCell<T>`. The previous PR was merged already before we had a chance to correct this, hence this second PR :)

To goal of this PR is:

1. Split the paragraph about the memory layout of `UnsafeCell<T>` and the usage of `UnsafeCell::(raw_)get()` into two paragraphs, so that it is easier to digest for the reader.
2. Slightly simplify the previously added examples in order to reduce redundancy between the new examples and the examples that already [existed](https://github.com/rust-lang/rust/blob/ddd119b2fed57eb6b19c44c18108de95c564a48d/library/core/src/cell.rs#L1858-L1908) before these 2 PRs (which remained untouched by both PRs).
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 5, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
Clarify documentation about the memory layout of `UnsafeCell`

This PR addresses a [comment](rust-lang/rust#101717 (comment)) by `@RalfJung` in PR #101717 to further clarify the documentation of `UnsafeCell<T>`. The previous PR was merged already before we had a chance to correct this, hence this second PR :)

To goal of this PR is:

1. Split the paragraph about the memory layout of `UnsafeCell<T>` and the usage of `UnsafeCell::(raw_)get()` into two paragraphs, so that it is easier to digest for the reader.
2. Slightly simplify the previously added examples in order to reduce redundancy between the new examples and the examples that already [existed](https://github.com/rust-lang/rust/blob/ddd119b2fed57eb6b19c44c18108de95c564a48d/library/core/src/cell.rs#L1858-L1908) before these 2 PRs (which remained untouched by both PRs).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2023
Add documentation about the memory layout of `Cell`

rust-lang#101717 guaranteed the memory layout of `UnsafeCell<T>`.

This property (a guaranteed memory layout) can be useful to have on `Cell<T>` as well.

(Note that `Cell<u8>` [already doesn't trigger the `improper_ctypes` lint](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=34af59ef60b96d8a8bdaec1d52cb5420) since it is `#[repr(transparent)]`).

The concrete use-case is for the crate [`objc2`](https://github.com/madsmtm/objc2) to specify that `Cell<T>` is safe to use as an instance variable when `T` is.

Fixes rust-lang#79303.

---

I'm unsure if we should specify less, for example say that the `Cell` may have extra restrictions on when it may be accessed, or if that's implicit in the (deliberately minimal) way I've worded it here?
saethlin pushed a commit to saethlin/miri that referenced this pull request Mar 15, 2023
Add documentation about the memory layout of `Cell`

rust-lang/rust#101717 guaranteed the memory layout of `UnsafeCell<T>`.

This property (a guaranteed memory layout) can be useful to have on `Cell<T>` as well.

(Note that `Cell<u8>` [already doesn't trigger the `improper_ctypes` lint](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=34af59ef60b96d8a8bdaec1d52cb5420) since it is `#[repr(transparent)]`).

The concrete use-case is for the crate [`objc2`](https://github.com/madsmtm/objc2) to specify that `Cell<T>` is safe to use as an instance variable when `T` is.

Fixes rust-lang/rust#79303.

---

I'm unsure if we should specify less, for example say that the `Cell` may have extra restrictions on when it may be accessed, or if that's implicit in the (deliberately minimal) way I've worded it here?
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Jun 1, 2023
Add documentation about the memory layout of `Cell`

rust-lang/rust#101717 guaranteed the memory layout of `UnsafeCell<T>`.

This property (a guaranteed memory layout) can be useful to have on `Cell<T>` as well.

(Note that `Cell<u8>` [already doesn't trigger the `improper_ctypes` lint](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=34af59ef60b96d8a8bdaec1d52cb5420) since it is `#[repr(transparent)]`).

The concrete use-case is for the crate [`objc2`](https://github.com/madsmtm/objc2) to specify that `Cell<T>` is safe to use as an instance variable when `T` is.

Fixes rust-lang/rust#79303.

---

I'm unsure if we should specify less, for example say that the `Cell` may have extra restrictions on when it may be accessed, or if that's implicit in the (deliberately minimal) way I've worded it here?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.