-
Notifications
You must be signed in to change notification settings - Fork 13.4k
UnsafePinned: also include the effects of UnsafeCell #140638
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
Conversation
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting rid of those Copy/Clone impls is nice
b08491e
to
66ca017
Compare
This comment has been minimized.
This comment has been minimized.
66ca017
to
bc8609c
Compare
bc8609c
to
d87cf5a
Compare
This comment has been minimized.
This comment has been minimized.
d87cf5a
to
405c954
Compare
@rustbot labels -I-lang-nominated +I-lang-radar We talked about this in the lang call today with 2/5 present. We felt it indeed made sense, given what we've found, for |
@WaffleLapkin @tgross35 could one of you review this? |
After 3 weeks without a review, I'll reassign. |
405c954
to
259d6f4
Compare
Soundness fix. Well, pre-fix. Code looks fine to me. Approving. @bors r+ rollup |
…sed, r=workingjubilee UnsafePinned: also include the effects of UnsafeCell This tackles rust-lang#137750 by including an `UnsafeCell` in `UnsafePinned`, thus imbuing it with all the usual properties of interior mutability (no `noalias` nor `dereferenceable` on shared refs, special treatment by Miri's aliasing model). The soundness issue is not fixed yet because coroutine lowering does not use `UnsafePinned`. The RFC said that `UnsafePinned` would not permit mutability on shared references, but since then, rust-lang#137750 has demonstrated that this is not tenable. In the face of those examples, I propose that we do the "obvious" thing and permit shared mutable state inside `UnsafePinned`. This seems loosely consistent with the fact that we allow going from `Pin<&mut T>` to `&T` (where the former can be aliased with other pointers that perform mutation, and hence the same goes for the latter) -- but the `as_ref` example shows that we in fact would need to add this `UnsafeCell` even if we didn't have a safe conversion to `&T`, since for the compiler and Miri, `&T` and `Pin<&T>` are basically the same type. To make this possible, I had to remove the `Copy` and `Clone` impls for `UnsafePinned`. Tracking issue: rust-lang#125735 Cc `@rust-lang/lang` `@rust-lang/opsem` `@Sky9x` I don't think this needs FCP since the type is still unstable -- we'll finally decide whether we like this approach when `UnsafePinned` is moved towards stabilization (IOW, this PR is reversible). However, I'd still like to make sure that the lang team is okay with the direction I am proposing here.
Rollup of 8 pull requests Successful merges: - #140638 (UnsafePinned: also include the effects of UnsafeCell) - #141272 (modularize the config module bootstrap) - #141777 (Do not run PGO/BOLT in x64 Linux alt builds) - #141870 (Fix broken link to rustc_type_ir module in serialization docs) - #141938 (update rust offload bootstrap) - #141962 (rustc-dev-guide subtree update) - #141965 (`tests/ui`: A New Order [3/N]) - #141970 (implement new `x` flag: `--skip-std-check-if-no-download-rustc`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 6 pull requests Successful merges: - #140638 (UnsafePinned: also include the effects of UnsafeCell) - #141777 (Do not run PGO/BOLT in x64 Linux alt builds) - #141938 (update rust offload bootstrap) - #141962 (rustc-dev-guide subtree update) - #141965 (`tests/ui`: A New Order [3/N]) - #141970 (implement new `x` flag: `--skip-std-check-if-no-download-rustc`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #140638 - RalfJung:unsafe-pinned-shared-aliased, r=workingjubilee UnsafePinned: also include the effects of UnsafeCell This tackles #137750 by including an `UnsafeCell` in `UnsafePinned`, thus imbuing it with all the usual properties of interior mutability (no `noalias` nor `dereferenceable` on shared refs, special treatment by Miri's aliasing model). The soundness issue is not fixed yet because coroutine lowering does not use `UnsafePinned`. The RFC said that `UnsafePinned` would not permit mutability on shared references, but since then, #137750 has demonstrated that this is not tenable. In the face of those examples, I propose that we do the "obvious" thing and permit shared mutable state inside `UnsafePinned`. This seems loosely consistent with the fact that we allow going from `Pin<&mut T>` to `&T` (where the former can be aliased with other pointers that perform mutation, and hence the same goes for the latter) -- but the `as_ref` example shows that we in fact would need to add this `UnsafeCell` even if we didn't have a safe conversion to `&T`, since for the compiler and Miri, `&T` and `Pin<&T>` are basically the same type. To make this possible, I had to remove the `Copy` and `Clone` impls for `UnsafePinned`. Tracking issue: #125735 Cc ``@rust-lang/lang`` ``@rust-lang/opsem`` ``@Sky9x`` I don't think this needs FCP since the type is still unstable -- we'll finally decide whether we like this approach when `UnsafePinned` is moved towards stabilization (IOW, this PR is reversible). However, I'd still like to make sure that the lang team is okay with the direction I am proposing here.
Rollup of 6 pull requests Successful merges: - rust-lang/rust#140638 (UnsafePinned: also include the effects of UnsafeCell) - rust-lang/rust#141777 (Do not run PGO/BOLT in x64 Linux alt builds) - rust-lang/rust#141938 (update rust offload bootstrap) - rust-lang/rust#141962 (rustc-dev-guide subtree update) - rust-lang/rust#141965 (`tests/ui`: A New Order [3/N]) - rust-lang/rust#141970 (implement new `x` flag: `--skip-std-check-if-no-download-rustc`) r? `@ghost` `@rustbot` modify labels: rollup
@RalfJung This PR missed changing the comment and doctest on rust/library/core/src/pin/unsafe_pinned.rs Lines 91 to 107 in 321dde1
|
Good point, I filed a follow-up at #142162. |
This tackles #137750 by including an
UnsafeCell
inUnsafePinned
, thus imbuing it with all the usual properties of interior mutability (nonoalias
nordereferenceable
on shared refs, special treatment by Miri's aliasing model). The soundness issue is not fixed yet because coroutine lowering does not useUnsafePinned
.The RFC said that
UnsafePinned
would not permit mutability on shared references, but since then, #137750 has demonstrated that this is not tenable. In the face of those examples, I propose that we do the "obvious" thing and permit shared mutable state insideUnsafePinned
. This seems loosely consistent with the fact that we allow going fromPin<&mut T>
to&T
(where the former can be aliased with other pointers that perform mutation, and hence the same goes for the latter) -- but theas_ref
example shows that we in fact would need to add thisUnsafeCell
even if we didn't have a safe conversion to&T
, since for the compiler and Miri,&T
andPin<&T>
are basically the same type.To make this possible, I had to remove the
Copy
andClone
impls forUnsafePinned
.Tracking issue: #125735
Cc @rust-lang/lang @rust-lang/opsem @Sky9x
I don't think this needs FCP since the type is still unstable -- we'll finally decide whether we like this approach when
UnsafePinned
is moved towards stabilization (IOW, this PR is reversible). However, I'd still like to make sure that the lang team is okay with the direction I am proposing here.