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

[v0.5] New design with two wrapper types: VolatilePtr and VolatileRef #29

Merged
merged 71 commits into from
Jun 24, 2023

Conversation

phil-opp
Copy link
Member

@phil-opp phil-opp commented Jan 17, 2023

This draft PR is a follow-up to #22. There was a lot of good discussion on that PR, mostly about the question whether the volatile wrapper type should implement Copy or Send. Implementing both of these traits would not be safe because it would allow unsynchronized concurrent writes from different threads.

I think that both approaches are valid, each with its advantages and drawbacks. For this reason, I tried to implement them both in this PR, as a base for further discussion. The following table summarizes the fundamental differences between the two pointer types:

VolatilePtr VolatileRef
Behavior Behaves like Rust's raw pointer types. Implement Copy, but are not Send. Behaves like Rust's reference types. Implement Send if T: Sync and implement Copy only if no mutation is allowed.
Usable with Mutex types ✖️
Mutex types require Send, but VolatilePtrCopy is !Send because writing the pointer from different threads would be UB.
Workaround: Create a custom wrapper type that unsafely implements Send
✔️
Type can safely implement Send since writes require a &mut reference and type is not clonable.
read and write methods are safe to call (after unsafe construction) ( ✔️ ?)
Implements Copy, so multiple instances can point to the same address. However, the instances are bound to a single thread, so concurrent writes should not be possible. We never construct &mut references, so we should not have mutable aliasing.
✔️
Writes require an &mut self and the type is not clonable, so concurrent writes should not be possible even when used concurrently.
Ease of use 👍
No borrow conflicts since all methods simply copy the pointer
👎
Methods borrow self, which requires manual reborrows and makes splitting struct pointers into field pointers difficult.
Workaround: We could create an unsafe duplicate function that could be used for splitting a struct pointer into multiple field pointers. Maybe it's also possible to create a safe macro for this use case.

To simplify this PR and focus the discussion, I removed the unsafe access types for now. We can add them later once we settled on a fundamental API design. I also moved the unstable and very_unstable functions to separate submodules, so we can ignore them too for now.

Fixes #39

phil-opp and others added 30 commits May 18, 2021 14:18
The lifetime parameter is required to make `map_mut` sound (otherwise we might get mutable aliasing).

Also:
- add new `from_ref` and `from_mut_ref` contructor methods again.
- add a `new_generic` constructor method to construct a `VolatilePtr` with arbitrary lifetime and access parameters.
The methods are still unsable through `as_ptr`/`as_mut_ptr`.
- Remove potentially unsafe constructor methods from `&T`/`&mut T`. They're still available on `VolatileRef` if needed.
- Refactor: Move most method implementations to new submodule.
- Update docs and examples
- Rename `as_ptr` method to `as_raw_ptr` to reduce confusion
@phil-opp
Copy link
Member Author

Update:

How about the following design:

* We keep `VolatilePtrCopy` as-is, we just try to find a better name later.

* We also keep `VolatilePtrSend`, but remove most of its methods.

* Instead, we provide two new conversion methods:
  
  * `fn as_ref<'b>(&'b self) -> VolatilePtrCopy<'b, T, A::RestrictShared>;`
  * `fn as_mut<'b>(&'b mut self) -> VolatilePtrCopy<'b, T, A>;`

So the VolatilePtrSend type becomes a wrapper type that prevents shared mutable aliasing and can thus safely provide a Send implementation.

I think this is the most promising solution. We need nicer names though, so I renamed the Copy type to VolatilePtr and the non-Copy type to VolatileRef. I also removed all access methods from VolatileRef and documented that the new as_ptr()/as_mut_ptr() methods should be used instead, which create temporary VolatilePtr instances. This way, we avoid most of the code duplication while keeping the crate flexible.

I also drafted up a short introduction of the two types for our crate docs:

This crate provides two different wrapper types: [VolatilePtr] and [VolatileRef]. The
difference between the two types is that the former behaves like a raw pointer, while the
latter behaves like a Rust reference type. For example, VolatilePtr can be freely copied,
but not sent across threads because this could introduce mutable aliasing. The VolatileRef
type, on the other hand, requires exclusive access for mutation, so that sharing it across
thread boundaries is safe.

Given that the API of the current release is not sound, I'm favor of merging this PR soon. We can continue improving the API and implementation in future PRs. The only thing that I want to do before merging is to write proper docs for all methods.

@phil-opp phil-opp marked this pull request as ready for review April 28, 2023 20:17
@phil-opp phil-opp changed the title [v0.5] VolatilePtrCopy or VolatilePtrSend? [v0.5] New design with two wrapper types: VolatilePtr and VolatileRef Apr 28, 2023
@Lokathor
Copy link

I'm trying to page things back into my memory, but this sounds like basically a "more restrictive than theoretically necessary, but at least we know it is sound" solution. Is that right? If so, I would agree to merge it now and if possible improve it later (again, if possible).

@mkroening
Copy link
Member

There is a relevant discussion on having a VolatileCell as a language feature: rust-lang/unsafe-code-guidelines#411

@phil-opp phil-opp merged commit 6d7b516 into master Jun 24, 2023
10 checks passed
@phil-opp phil-opp deleted the next branch June 24, 2023 12:32
@phil-opp
Copy link
Member Author

Published as v0.5.0

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.

This crate is currently unsound due to spurious reads
5 participants