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.

Sign up for GitHub

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

Jump to bottom

Tracking Issue for RFC 3467: UnsafePinned #125735

Open
1 of 6 tasks
traviscross opened this issue May 29, 2024 · 2 comments
Open
1 of 6 tasks

Tracking Issue for RFC 3467: UnsafePinned #125735

traviscross opened this issue May 29, 2024 · 2 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-unsafe_pinned `#![feature(unsafe_pinned)]` T-lang Relevant to the language team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team

Comments

@traviscross
Copy link
Contributor

traviscross commented May 29, 2024 •
edited
Loading

This is a tracking issue for RFC 3467: UnsafePinned.

The feature gate for the issue is #![feature(unsafe_pinned)].

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • Do we want to bikeshed the name further before stabilization?

Related

Implementation history

TODO.
@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-opsem Relevant to the opsem team B-experimental Blocker: In-tree experiment; RFC pending or unneeded. labels May 29, 2024
@traviscross traviscross mentioned this issue May 29, 2024
Merged
@traviscross traviscross added the F-unsafe_pinned `#![feature(unsafe_pinned)]` label May 29, 2024
@traviscross traviscross added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. and removed B-experimental Blocker: In-tree experiment; RFC pending or unneeded. labels Jun 18, 2024
This was referenced Aug 2, 2024
Closed
Open
@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2024

Turns out the existing hack that attempts to make self-referential async lowering sound is incomplete, and we most likely need UnsafePinned to make it actually sound.

@RalfJung RalfJung mentioned this issue Aug 20, 2024
Merged
@RalfJung
Copy link
Member

In the RFC we had the question whether UnsafePinned should also act like an UnsafeCell. I am now fairly sure that the answer is "yes, it must". The reason is this: due to Pin::deref being safe, it is at any moment possible to create a shared reference to a future that is halted at a suspension point. If there is no UnsafeCell, this acts like a read of the entire future data, and that read conflicts with mutable references that the future may hold to its own data. This is not a necessary property of aliasing with mutable reference, but it is a necessary consequence of the decision that Pin::deref should be safe.

Cc @rust-lang/opsem

@RalfJung RalfJung mentioned this issue Aug 20, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 4, 2024
@bors
Auto merge of rust-lang#129313 - RalfJung:coroutine-niches, r=compile…
15287f5 
…r-errors

Supress niches in coroutines to avoid aliasing violations

As mentioned [here](rust-lang#63818 (comment)), using niches in fields of coroutines that are referenced by other fields is unsound: the discriminant accesses violate the aliasing requirements of the reference pointing to the relevant field. This issue causes [Miri errors in practice](rust-lang/miri#3780).

The "obvious" fix for this is to suppress niches in coroutines. That's what this PR does. However, we have several tests explicitly ensuring that we *do* use niches in coroutines. So I see two options:
- We guard this behavior behind a `-Z` flag (that Miri will set by default). There is no known case of these aliasing violations causing miscompilations. But absence of evidence is not evidence of absence...
- (What this PR does right now.) We temporarily adjust the coroutine layout logic and the associated tests until the proper fix lands. The "proper fix" here is to wrap fields that other fields can point to in [`UnsafePinned`](rust-lang#125735) and make `UnsafePinned` suppress niches; that would then still permit using niches of *other* fields (those that never get borrowed). However, I know that coroutine sizes are already a problem, so I am not sure if this temporary size regression is acceptable.

`@compiler-errors` any opinion? Also who else should be Cc'd here?
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 6, 2024
@bors
Auto merge of rust-lang#129313 - RalfJung:coroutine-niches, r=compile…
7e24610 
…r-errors

Supress niches in coroutines to avoid aliasing violations

As mentioned [here](rust-lang#63818 (comment)), using niches in fields of coroutines that are referenced by other fields is unsound: the discriminant accesses violate the aliasing requirements of the reference pointing to the relevant field. This issue causes [Miri errors in practice](rust-lang/miri#3780).

The "obvious" fix for this is to suppress niches in coroutines. That's what this PR does. However, we have several tests explicitly ensuring that we *do* use niches in coroutines. So I see two options:
- We guard this behavior behind a `-Z` flag (that Miri will set by default). There is no known case of these aliasing violations causing miscompilations. But absence of evidence is not evidence of absence...
- (What this PR does right now.) We temporarily adjust the coroutine layout logic and the associated tests until the proper fix lands. The "proper fix" here is to wrap fields that other fields can point to in [`UnsafePinned`](rust-lang#125735) and make `UnsafePinned` suppress niches; that would then still permit using niches of *other* fields (those that never get borrowed). However, I know that coroutine sizes are already a problem, so I am not sure if this temporary size regression is acceptable.

`@compiler-errors` any opinion? Also who else should be Cc'd here?
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 7, 2024
@bors
Auto merge of rust-lang#129313 - RalfJung:coroutine-niches, r=compile…
b0cfc07 
…r-errors

Supress niches in coroutines to avoid aliasing violations

As mentioned [here](rust-lang#63818 (comment)), using niches in fields of coroutines that are referenced by other fields is unsound: the discriminant accesses violate the aliasing requirements of the reference pointing to the relevant field. This issue causes [Miri errors in practice](rust-lang/miri#3780).

The "obvious" fix for this is to suppress niches in coroutines. That's what this PR does. However, we have several tests explicitly ensuring that we *do* use niches in coroutines. So I see two options:
- We guard this behavior behind a `-Z` flag (that Miri will set by default). There is no known case of these aliasing violations causing miscompilations. But absence of evidence is not evidence of absence...
- (What this PR does right now.) We temporarily adjust the coroutine layout logic and the associated tests until the proper fix lands. The "proper fix" here is to wrap fields that other fields can point to in [`UnsafePinned`](rust-lang#125735) and make `UnsafePinned` suppress niches; that would then still permit using niches of *other* fields (those that never get borrowed). However, I know that coroutine sizes are already a problem, so I am not sure if this temporary size regression is acceptable.

`@compiler-errors` any opinion? Also who else should be Cc'd here?
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2024
@bors
Auto merge of rust-lang#129313 - RalfJung:coroutine-niches, r=compile…
7f4b270 
…r-errors

Supress niches in coroutines to avoid aliasing violations

As mentioned [here](rust-lang#63818 (comment)), using niches in fields of coroutines that are referenced by other fields is unsound: the discriminant accesses violate the aliasing requirements of the reference pointing to the relevant field. This issue causes [Miri errors in practice](rust-lang/miri#3780).

The "obvious" fix for this is to suppress niches in coroutines. That's what this PR does. However, we have several tests explicitly ensuring that we *do* use niches in coroutines. So I see two options:
- We guard this behavior behind a `-Z` flag (that Miri will set by default). There is no known case of these aliasing violations causing miscompilations. But absence of evidence is not evidence of absence...
- (What this PR does right now.) We temporarily adjust the coroutine layout logic and the associated tests until the proper fix lands. The "proper fix" here is to wrap fields that other fields can point to in [`UnsafePinned`](rust-lang#125735) and make `UnsafePinned` suppress niches; that would then still permit using niches of *other* fields (those that never get borrowed). However, I know that coroutine sizes are already a problem, so I am not sure if this temporary size regression is acceptable.

`@compiler-errors` any opinion? Also who else should be Cc'd here?
RalfJung pushed a commit to RalfJung/miri that referenced this issue Sep 10, 2024
@bors
Auto merge of #129313 - RalfJung:coroutine-niches, r=compiler-errors
48a5233 
Supress niches in coroutines to avoid aliasing violations

As mentioned [here](rust-lang/rust#63818 (comment)), using niches in fields of coroutines that are referenced by other fields is unsound: the discriminant accesses violate the aliasing requirements of the reference pointing to the relevant field. This issue causes [Miri errors in practice](rust-lang#3780).

The "obvious" fix for this is to suppress niches in coroutines. That's what this PR does. However, we have several tests explicitly ensuring that we *do* use niches in coroutines. So I see two options:
- We guard this behavior behind a `-Z` flag (that Miri will set by default). There is no known case of these aliasing violations causing miscompilations. But absence of evidence is not evidence of absence...
- (What this PR does right now.) We temporarily adjust the coroutine layout logic and the associated tests until the proper fix lands. The "proper fix" here is to wrap fields that other fields can point to in [`UnsafePinned`](rust-lang/rust#125735) and make `UnsafePinned` suppress niches; that would then still permit using niches of *other* fields (those that never get borrowed). However, I know that coroutine sizes are already a problem, so I am not sure if this temporary size regression is acceptable.

`@compiler-errors` any opinion? Also who else should be Cc'd here?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-unsafe_pinned `#![feature(unsafe_pinned)]` T-lang Relevant to the language team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@RalfJung @traviscross