Skip to content

Initial UnsafePinned implementation [Part 1: Libs] #137043

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

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

Sky9x
Copy link
Contributor

@Sky9x Sky9x commented Feb 14, 2025

Initial libs changes necessary to unblock further work on RFC 3467.
Tracking issue: #125735

This PR is split off from #136964, and includes just the libs changes:

  • UnsafePinned struct
  • private UnsafeUnpin structural auto trait
  • Lang items for both
  • Compiler changes necessary to block niches on UnsafePinned

This PR does not change codegen, miri, the existing !Unpin hack, or anything else. That work is to be split into later PRs.


cc @RalfJung @Noratrieb

@rustbot label F-unsafe_pinned T-libs-api

@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2025

r? @tgross35

rustbot has assigned @tgross35.
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 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. F-unsafe_pinned `#![feature(unsafe_pinned)]` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 14, 2025
@Sky9x Sky9x changed the title Initial UnsafePinned impl [Part 1: Libs] Initial UnsafePinned implementation [Part 1: Libs] Feb 14, 2025
@Sky9x Sky9x force-pushed the unsafe-pinned-pt1-libs branch from e823a89 to 97308a2 Compare February 16, 2025 05:43
@tgross35
Copy link
Contributor

Cc @rust-lang/libs-api, this unstably adds the interfaces from the accepted RFC https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html.

@Sky9x Sky9x force-pushed the unsafe-pinned-pt1-libs branch 2 times, most recently from 24af599 to 9ec74c7 Compare February 24, 2025 16:44
@rust-log-analyzer

This comment has been minimized.

@Sky9x Sky9x force-pushed the unsafe-pinned-pt1-libs branch from 9ec74c7 to 74efc47 Compare February 24, 2025 16:51
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Mar 12, 2025

☔ The latest upstream changes (presumably #138366) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

This API appears to match RFC 3467 so I think it is good to go. The docs could probably use elaboration at some point, but that should be fine to defer until the feature is fully available.

r=me with one doc request and conflicts resolved.

Cc @WaffleLapkin @RalfJung if you have anything else.

Copy link
Member

@WaffleLapkin WaffleLapkin 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 to me 👍🏻

@RalfJung
Copy link
Member

compiler changes LGTM.

@tgross35
Copy link
Contributor

tgross35 commented Apr 4, 2025

@rustbot author for #137043 (review)

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 4, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 4, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2025
Rollup of 11 pull requests

Successful merges:

 - rust-lang#137043 (Initial `UnsafePinned` implementation [Part 1: Libs])
 - rust-lang#138962 (Expect an array when expected and acutal types are both arrays during cast)
 - rust-lang#139001 (add `naked_functions_rustic_abi` feature gate)
 - rust-lang#139379 (Use delayed bug for normalization errors in drop elaboration)
 - rust-lang#139582 (Various coercion cleanups)
 - rust-lang#139628 (Suggest remove redundant `$()?` around `vis`)
 - rust-lang#139644 (Micro-optimize `InstSimplify`'s `simplify_primitive_clone`)
 - rust-lang#139671 (Proc macro span API redesign: Replace proc_macro::SourceFile by Span::{file, local_file})
 - rust-lang#139674 (In `rustc_mir_transform`, iterate over index newtypes instead of ints)
 - rust-lang#139740 (Convert `tests/ui/lint/dead-code/self-assign.rs` to a known-bug test)
 - rust-lang#139741 (fix smir's run! doc and import)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2025
Rollup of 10 pull requests

Successful merges:

 - rust-lang#137043 (Initial `UnsafePinned` implementation [Part 1: Libs])
 - rust-lang#138962 (Expect an array when expected and acutal types are both arrays during cast)
 - rust-lang#139001 (add `naked_functions_rustic_abi` feature gate)
 - rust-lang#139379 (Use delayed bug for normalization errors in drop elaboration)
 - rust-lang#139582 (Various coercion cleanups)
 - rust-lang#139628 (Suggest remove redundant `$()?` around `vis`)
 - rust-lang#139644 (Micro-optimize `InstSimplify`'s `simplify_primitive_clone`)
 - rust-lang#139674 (In `rustc_mir_transform`, iterate over index newtypes instead of ints)
 - rust-lang#139740 (Convert `tests/ui/lint/dead-code/self-assign.rs` to a known-bug test)
 - rust-lang#139741 (fix smir's run! doc and import)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d04df1c into rust-lang:master Apr 14, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 14, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2025
Rollup merge of rust-lang#137043 - Sky9x:unsafe-pinned-pt1-libs, r=tgross35,RalfJung,WaffleLapkin

Initial `UnsafePinned` implementation [Part 1: Libs]

Initial libs changes necessary to unblock further work on [RFC 3467](https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html).
Tracking issue: rust-lang#125735

This PR is split off from rust-lang#136964, and includes just the libs changes:
- `UnsafePinned` struct
- private `UnsafeUnpin` structural auto trait
- Lang items for both
- Compiler changes necessary to block niches on `UnsafePinned`

This PR does not change codegen, miri, the existing `!Unpin` hack, or anything else. That work is to be split into later PRs.

---

cc ``@RalfJung`` ``@Noratrieb``

``@rustbot`` label F-unsafe_pinned T-libs-api
@Sky9x Sky9x deleted the unsafe-pinned-pt1-libs branch April 14, 2025 13:44
@onestacked
Copy link
Contributor

onestacked commented Apr 18, 2025

@Sky9x I've sent a RFC version for including part of this API in the linux kernel, please let me know if you want me to include a Co-Developed-By tag (or something else) to be included?

Also are you alright with relicensing as GPL v2 or should the Apache & MIT license be kept?

Link: https://lore.kernel.org/rust-for-linux/[email protected]/

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Apr 18, 2025
`UnsafePinned<T>` is useful for cases where a value might be shared with C
code but not directly used by it. In particular this is added for
storing additional data in the `MiscDeviceRegistration` which will be
shared between `fops->open` and the containing struct.

Similar to `Opaque` but guarantees that the value is always initialized
and that the inner value is dropped when `UnsafePinned` is dropped.

This was originally proposed for the IRQ abstractions [0] and is also
useful for other where the inner data may be aliased, but is always valid
and automatic `Drop` is desired.

Since then the `UnsafePinned` type was added to upstream Rust [1] as a
unstable feature, therefore this patch implements the subset required
for additional data in `MiscDeviceRegistration` on older rust versions
and using the upstream type on new rust versions which include this
feature.

Some differences to the upstream type definition are required in the
kernel implementation, because upstream type uses some compiler changes
to opt out of certain optimizations, this is documented in a comment on
the `UnsafePinned` type.

The documentation on is based on the upstream rust documentation with
minor modifications.

Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
Link: rust-lang/rust#137043 [1]
Suggested-by: Alice Ryhl <[email protected]>
Signed-off-by: Christian Schrefl <[email protected]>
onestacked added a commit to onestacked/linux that referenced this pull request Apr 18, 2025
`UnsafePinned<T>` is useful for cases where a value might be shared with C
code but not directly used by it. In particular this is added for
storing additional data in the `MiscDeviceRegistration` which will be
shared between `fops->open` and the containing struct.

Similar to `Opaque` but guarantees that the value is always initialized
and that the inner value is dropped when `UnsafePinned` is dropped.

This was originally proposed for the IRQ abstractions [0] and is also
useful for other where the inner data may be aliased, but is always valid
and automatic `Drop` is desired.

Since then the `UnsafePinned` type was added to upstream Rust [1] as a
unstable feature, therefore this patch implements the subset required
for additional data in `MiscDeviceRegistration` on older rust versions
and using the upstream type on new rust versions which include this
feature.

Some differences to the upstream type definition are required in the
kernel implementation, because upstream type uses some compiler changes
to opt out of certain optimizations, this is documented in a comment on
the `UnsafePinned` type.

The documentation on is based on the upstream rust documentation with
minor modifications.

Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
Link: rust-lang/rust#137043 [1]
Suggested-by: Alice Ryhl <[email protected]>
Signed-off-by: Christian Schrefl <[email protected]>
@WaffleLapkin
Copy link
Member

@onestacked please note that this type doesn't do anything yet. the language part that allows aliasing of mutable references isn't implemented yet.

@onestacked
Copy link
Contributor

The plan is to only use the Rust UnsafePinned once it is stabilized so thats no real problem (since by then this will be implemented).
Until then using the "reimplemented" UnsafePinned is the best we can do from what I understand.

@ojeda
Copy link
Contributor

ojeda commented Apr 18, 2025

It is always nice to test the new feature works before it gets stabilized, so the patch is useful :)

@Sky9x
Copy link
Contributor Author

Sky9x commented Apr 18, 2025

@Sky9x I've sent a RFC version for including part of this API in the linux kernel, please let me know if you want me to include a Co-Developed-By tag (or something else) to be included?

Sure? My git email is Sky <[email protected]>

Also are you alright with relicensing as GPL v2 or should the Apache & MIT license be kept?

OK with relicensing as GPLv2.

Link: https://lore.kernel.org/rust-for-linux/[email protected]/

Your polyfill with PhantomPinned + UnsafeCell is sufficient for matching the eventual behavior of our implementation.

@onestacked please note that this type doesn't do anything yet. the language part that allows aliasing of mutable references isn't implemented yet.

The existing !Unpin hacks in place should prevent miscompliations. Miri currently disables protectors when retagging such types, so it won't be able to catch any violations. That being said, you can use the wrapper in its current state for writing code (and I have), it just may not be optimized as well or checked by miri.

@traviscross
Copy link
Contributor

Also are you alright with relicensing as GPL v2 or should the Apache & MIT license be kept?

OK with relicensing as GPLv2.

Keep in mind, if the code maintained on the kernel side does not preserve the MIT/Apache 2.0 license, then any changes or improvements made to that code (by others) cannot be propagated back to Rust (as those changes will only be GPLv2 licensed).

@Sky9x
Copy link
Contributor Author

Sky9x commented Apr 18, 2025

Also are you alright with relicensing as GPL v2 or should the Apache & MIT license be kept?

OK with relicensing as GPLv2.

Keep in mind, if the code maintained on the kernel side does not preserve the MIT/Apache 2.0 license, then any changes or improvements made to that code (by others) cannot be propagated back to Rust (as those changes will only be GPLv2 licensed).

I'm ok licensing it however, but yeah maybe MIT/Apache is better

@onestacked
Copy link
Contributor

Alright I'll keep it then.

@ojeda
Copy link
Contributor

ojeda commented Apr 18, 2025

Sure?

Thanks!

If @onestacked is adding you as a Co-developed-by/Signed-off-by, then please make sure you are OK with the DCO: https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

@Sky9x
Copy link
Contributor Author

Sky9x commented Apr 19, 2025

Sure?

Thanks!

If @onestacked is adding you as a Co-developed-by/Signed-off-by, than please make sure you are OK with the DCO: https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

Signed-off-by: Sky <[email protected]>


Also probably best to keep it licensed as MIT OR Apache-2.0. Some of the code (comments really) is from the RFC which is not written by me and is licensed under MIT/Apache.

jhpratt added a commit to jhpratt/rust that referenced this pull request Apr 19, 2025
…ompiler-errors

Re-remove `AdtFlags::IS_ANONYMOUS`

Removed in rust-lang#138296.
I accidentally re-added it in rust-lang#137043 while resolving merge conflicts. This PR re-removes it.

r? `@compiler-errors` (sorry)
@RalfJung
Copy link
Member

RalfJung commented Apr 19, 2025 via email

ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 19, 2025
…ompiler-errors

Re-remove `AdtFlags::IS_ANONYMOUS`

Removed in rust-lang#138296.
I accidentally re-added it in rust-lang#137043 while resolving merge conflicts. This PR re-removes it.

r? ``@compiler-errors`` (sorry)
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 19, 2025
…ross35,RalfJung,WaffleLapkin

Initial `UnsafePinned` implementation [Part 1: Libs]

Initial libs changes necessary to unblock further work on [RFC 3467](https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html).
Tracking issue: rust-lang#125735

This PR is split off from rust-lang#136964, and includes just the libs changes:
- `UnsafePinned` struct
- private `UnsafeUnpin` structural auto trait
- Lang items for both
- Compiler changes necessary to block niches on `UnsafePinned`

This PR does not change codegen, miri, the existing `!Unpin` hack, or anything else. That work is to be split into later PRs.

---

cc ``@RalfJung`` ``@Noratrieb``

``@rustbot`` label F-unsafe_pinned T-libs-api
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2025
Rollup merge of rust-lang#140025 - Sky9x:re-remove-adtflags-anon, r=compiler-errors

Re-remove `AdtFlags::IS_ANONYMOUS`

Removed in rust-lang#138296.
I accidentally re-added it in rust-lang#137043 while resolving merge conflicts. This PR re-removes it.

r? ``@compiler-errors`` (sorry)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-unsafe_pinned `#![feature(unsafe_pinned)]` 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. 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.

10 participants