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

exclude mutable references to !Unpin types from uniqueness guarantees #1952

Merged
merged 1 commit into from
Jan 9, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 1, 2022

This basically works around rust-lang/unsafe-code-guidelines#148 by not requiring uniqueness any more for mutable references to self-referential generators. That corresponds to the same work-around that was applied in rustc itself.

I am not entirely sure if this is a good idea since it might hide too many errors in case types are "accidentally" !Unpin. OTOH, our test suite still passes, and to my knowledge the vast majority of types is Unpin. (place.layout.ty is monomorphic, we should always exactly know which type this is.)

@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2022

It looks like indeed almost all types are Unpin... so far I have not found an example of a stdlib type that is not Unpin.^^

fn check_unpin<T: Unpin>() {}

fn main() {
    check_unpin::<Vec<i32>>();
    check_unpin::<std::collections::HashMap<i32, i32>>();
    check_unpin::<std::collections::BTreeMap<i32, i32>>();
    check_unpin::<std::cell::Cell<i32>>();
    check_unpin::<std::cell::RefCell<i32>>();
    check_unpin::<std::rc::Rc<i32>>();
    check_unpin::<std::sync::Mutex<i32>>();
    check_unpin::<std::sync::Arc<i32>>();
    check_unpin::<std::sync::atomic::AtomicI32>();
}

@oli-obk
Copy link
Contributor

oli-obk commented Jan 2, 2022

I don't think libstd contains self referential types ^^

@Nemo157
Copy link
Member

Nemo157 commented Jan 2, 2022

There is one (but it's likely not useful to test any behaviour with): PhantomPinned.

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Jan 2, 2022

So does that mean we can finally have Aliasable<T> as:

#[repr(transparent)]
struct Aliasable<T : ?Sized>(T);

impl<T : ?Sized> !Unpin for AliasedCell<T> {}

as done in https://docs.rs/pinned-aliasable 's inline implementation?

@Darksonn
Copy link

Darksonn commented Jan 2, 2022

But can an &Aliasable<T> or &mut Aliasable<T> coexist with an &mut T (or to a field of T)?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2022

So does that mean we can finally have Aliasable as:

Well, this might silence Miri, but note that making Unpin significant like this is just a temporary work-around -- proper language-level support for this will have to be added eventually, and no guarantees are made until then. Sorry.

But can an &Aliasable or &mut Aliasable coexist with an &mut T (or to a field of T)?

&mut T (with this patch: for T: Unpin) cannot coexist with anything (assuming both pointers are being used). Weakening that guarantee would make it entirely useless for optimizations.

But &mut T can be reborrowed from &mut Aliasable<T>, as long as for the time that the &mut T is active, no other reference/pointer to that memory is used.

@Darksonn
Copy link

Darksonn commented Jan 2, 2022

&mut T (with this patch: for T: Unpin) cannot coexist with anything (assuming both pointers are being used). Weakening that guarantee would make it entirely useless for optimizations.

Well, a &mut T can coexist with an &UnsafeCell<T>, no?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 3, 2022

Well, a &mut T can coexist with an &UnsafeCell, no?

No. (Assuming by "coexist" you mean both pointers are actually being used.) &mut T requires uniqueness, which means it cannot coexist with anything. No exceptions. That is the only way it can be useful for optimizations.

If any exception was permitted, then the compiler (seeing some &mut T being used) would have to assume that some other code exploits that exception to violate uniqueness, thus entirely undermining the uniqueness property.

The special property of &UnsafeCell<T> is that it can coexist with mutation performed through raw pointers. Only the guarantees of the shared reference itself are affected, there is no way that this can weaken the guarantees of other pointers elsewhere in the program.

@danielhenrymantilla
Copy link
Contributor

To add to @RalfJung's last point: IIUC, whilst a &UnsafeCell<T> can be reborrowed as a &mut T, it can't be used (as in: dereferenced or upgraded) concurrently with an outstanding &mut Ts.


That being said, let's consider:

let rw = &UnsafeCell::new(42);
let uniq = unsafe { &mut *rw.get() };
let r2 = rw; // or some other dummy use of `&UnsafeCell<T>`
*uniq = 0; // use &mut
unsafe { *r2.get() = 42; }

I'd say that snippet showcases well-defined behavior.
So I guess it's a matter of what "using a &UnsafeCell<T> pointer" means: as long as it never gets T-dereferenced (or upgraded into a &T / &mut T), a "&UnsafeCell<T> pointer is not really used", and thus can "coexist" with a non-directly-reborrowed &mut T.


All that to say that "coexisting" is quite a fuzzy word: maybe we should be showcasing actual snippets with Aliasable<T> to assess whether they'd be featuring well-defined behavior?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 3, 2022

I'd say that snippet showcases well-defined behavior.

Yes, though this is subtle. let r2 = rw is a reborrow (not a read/write) of a shared mutable pointer (due to UnsafeCell), and that does not invalidate other pointers.

If this were a read or write though, it would invalidate uniq.

All that to say that "coexisting" is quite a fuzzy word

Indeed. I tried to clarify but I guess using the verb "used" is still quite fuzzy.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 9, 2022

Given the feedback above I'll go ahead and land this. The fact that the compile-fail tests still work make me reasonable confident that this will not lead to huge amounts of false negatives, and without something like this Stacked Borrows is entirely unusable for self-referential generators.

This is not a commitment that Unpin disables aliasing guarantees, it is a temporary compromise that we can and likely will tweak as time goes on.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2022

📌 Commit 77cec81 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jan 9, 2022

⌛ Testing commit 77cec81 with merge deb9bfd...

@bors
Copy link
Contributor

bors commented Jan 9, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing deb9bfd to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants