First draft of a sound ShallowCopy

[jonhoo]

This is trying to address the unsoundness that arises from the current version of ShallowCopy (see #74). In the process, it also deals with the fact that casting between Inner<.., T, ..> and Inner<.., ManuallyDrop<T>, ..> is likely not sound (rust-lang/unsafe-code-guidelines#35 (comment)).

It does not yet work. More notes to come.

---

This change is 

[jonhoo]

Okay, so, there are a couple of concerns here. I'll try to write a little bit about each one in the hopes that it sparks ideas in others.

At the core of ShallowCopy is the desire to alias a value among two distinct storage locations. Think: one heap allocation, two pointers. Let's assume that that fundamental premise is a good idea for now, an immediate concern immediately arises: how do we preserve memory safety? Well, at the very least we need to guarantee:

• The aliased memory is never mutated while it is being read (no data races).
• The aliased memory is only dropped once (no double-free).
• The aliased memory is not read after it is dropped (no use-after-free).

The way this works in ShallowCopy today is as follows:

1. Take in an "aliaseable type", say Box<T> (bear with me).
2. Call something like Box::from_raw(std::ptr::read(&a_box)) to get an alias of the type.
3. Wrap the aliased value in ManuallyDrop to ensure it doesn't get dropped accidentally.
4. Be really careful about only ever giving out &T into the aliased type.
5. Inside evmap, cast between Inner<.., V> and Inner<.., ManuallyDrop<V>> to control whether values should or should not be dropped during a write operation. Specifically, only use an "uncovered" V if any removed V must be the second (and thus last) alias of that V. Essentially, this means use ManuallyDrop<V> in apply_first/absorb_first and V in apply_second/absorb_second.
6. Similarly, when the evmap is dropped, first drop the first map copy with ManuallyDrop<V>, and then drop the second with an uncovered V (so the values actually get dropped).

Now, there are at least two problems with this approach:

1. For many common types (Box, Vec, and String among them) internally the compiler expects the pointer to not be aliased anywhere, and it takes advantage of this in optimizations (Aliasing rules for Box<T> rust-lang/unsafe-code-guidelines#258). The fact that we wrap the type in a ManuallyDrop does not do us any good — we are still violating the compiler's assumptions.
2. It is not generally sound to transmute between Wrapper<T> and Wrapper<ManuallyDrop<T>> (Deterministic (but undefined) layout rust-lang/unsafe-code-guidelines#35).

Unfortunately, this has pretty far-reaching implications for ShallowCopy, and for evmap's technique for deduplication. This PR tries to address them all at once by making a couple of changes:

First, it changes the definition of ShallowCopy to

pub trait ShallowCopy {
    type SplitType;
    type Target: ?Sized;

    fn split(self) -> (Self::SplitType, Self::SplitType);
    unsafe fn deref(split: &Self::SplitType) -> &Self::Target;
    unsafe fn drop(split: &mut Self::SplitType);

    fn deref_self(&self) -> &Self::Target;
}

This is a major change, but it's the cleanest I could come up with. For a type to be shallow-copiable, it must be possible to turn it into "raw" (and crucially, aliasable) parts — that's split. Then, given such a raw descriptor, it must be possible both to both dereference the type (deref) and drop the type (drop). You'll also notice that the trait has an associated type for the type that the implementor dereferences into. This is because given the raw components of a box, we're not allowed to construct a &Box<T> (at least I don't think so), as it would violate the aliasing rules again. Instead, we can only construct a &T. Same for Vec, String, etc. We'll get back to deref_self.

Since split has to consume self (we can't keep a Box around after aliasing it), this means all the values in the operational log have to change to become "maybe split" values. That is:

enum MaybeShallowCopied<T>
where
    T: ShallowCopy,
{
    Owned(T),
    Copied(T::SplitType),
}

Given something of that type from the oplog, we now need a way to insert the aliased T into the actual data structure. But, we can't just stick the T::SplitType in there directly, since it won't actually implement any of the traits the data structure might need (like Eq or Hash). Instead, we need a wrapper type that will automatically dereference into &T as needed (which is safe as long as we haven't called ShallowCopy::drop on the value). Enter ForwardThroughShallowCopy:

struct ForwardThroughShallowCopy<T>
where
    T: ShallowCopy,
{
    split: T::SplitType,
}

Now we just have to implement all the necessary traits on ForwardThroughShallowCopy by forwarding through the &T we get by calling T::deref(&self.split). Great, that seems good. And then we add some helper methods to MaybeShallowCopied:

impl<T> MaybeShallowCopied<T> where T: ShallowCopy {
    unsafe fn shallow_copy_first(&mut self) -> ForwardThroughShallowCopy<T> { ... }
    unsafe fn shallow_copy_second(self) -> ForwardThroughShallowCopy<T> { ... }
}

They must be unsafe, since ForwardThroughShallowCopy is going to assume that dereferencing through its split is fine. Internally, shallow_copy_first turns a MaybeShallowCopied::Owned into a MaybeShallowCopied::Copied using ShallowCopy::split, and gives out one of the two aliases it returns wrapped in a ForwardThroughShallowCopy. shallow_copy_first then gives out the second alias wrapped similarly when the oplog entry is processed the second time around.

For the final trick, we then impl Drop for ForwardThroughShallowCopy and use thread-local storage to decide whether we should re-assume ownership of the aliased value and drop it properly, or whether we should just let the SplitType disappear silently. By default, it does the latter. Only in the second map copy drop do we set the thread-local to true around the actual drop of the map, which in turn will cause all the ForwardThroughShallowCopy to drop their values using ShallowCopy::drop. This is safe since at the time of the second drop, the values are no longer aliased, and so it's safe to construct something like a Box again.

So far so good. Unfortunately, wrapper types get painful pretty quickly, because we cannot quite make them transparent. In particular, we run into problems when the user provides a value of type T (or &T), and we have a data structure that stores ForwardThroughShallowCopy<T>. For example, if you store a HashSet<ForwardThroughShallowCopy<T>>, you can't do set.remove(t) where t: &T. Same thing with HashSet::contains, HashMap::get, or even just trying to use PartialEq to see if a T is present in a Vec<ForwardThroughShallowCopy<T>>.

This gets worse when we take into account the fact that ShallowCopy derefs the T. If the user provides a &str, but we store ForwardThroughShallowCopy<String>, how do we make that work?

The first problem is fixable by making ForwardThroughShallowCopy instead be an enum that can hold short-lived references as well:

enum ForwardThroughShallowCopy<T>
where
    T: ShallowCopy,
{
    Split(T::SplitType),
    Ref(*const T::Target),
}

The Ref variant is unsafe to construct, and is never used for storage (yet we always have to store the discriminant...). It is only used to do lookups into data structures that hold ForwardThroughShallowCopy<T> by providing a &T::Target. Since it's pretty unergonomic to require that the user give us a &T::Target directly, we take advantage of ShallowCopy::deref_self to go from a T/&T the user has given us to a &T::Target.

Okay, where does that leave us, beyond making a lot of types much uglier? Well, there are three major downsides to this design. First, there is a decent amount more unsafe in the code now, and more tricky cases to have to keep track of. Second, this version of ShallowCopy cannot support compound types (like tuples, structs, Option<T>, etc.) — that would require generic associated types (more on that in a second). And third, ForwardThroughShallowCopy means that we can no longer retain the "nice" K: Borrow<Q> API for ReadHandle::contains_value and Values::contains — we must instead always accept exactly a &T::Target. This arises because we cannot implement Borrow for ForwardThroughShallowCopy due to the blanket implementation for T in the standard library. The first and third of these, I can live with. The second one is a hard pill to swallow, so let's dive into it a bit more.

Imagine that you wanted to implement ShallowCopy for Option<T> where T: ShallowCopy. How would you go about it? Let's give it a try:

impl<T> ShallowCopy for Option<T> where T: ShallowCopy {
    type SplitType = Option<T::SplitType>;
    type Target = Option<T::Target>;

    fn split(self) -> (Self::SplitType, Self::SplitType) {
         if let Some(this) = self {
             let (a, b) = this.split();
             (Some(a), Some(b))
         } else {
             (None, None)
         }
    }

    unsafe fn drop(split: &mut Self::SplitType) {
        if let Some(split) = split {
            T::drop(split);
        }
    }

    // So far so good — now we just need deref...
    unsafe fn deref(split: &Self::SplitType) -> &Self::Target {
        // We need to return an `&Option<T>` given an `&Option<T::SplitType>`. Uh oh...
        // We can call `T::deref` on the `&T::SplitType` inside the `Option`, but that gives
        // us an `Option<&T>`, not a `&Option<T>`.
        panic!();
    }
}

A similar issue arises for any compound type. The basic problem is that the ShallowCopy::deref signature requires that we return a &T::Target; we cannot return a structure that internally holds a number of references (e.g., Option<&T>). At first glance it might seem like we could just change deref to return Self::Target and set type Target = Option<&T>, but that doesn't work either — what is the lifetime of that &T? It needs to be tied to the lifetime of the &self that is given to deref! What we need here is generic associated lifetimes so that we can write:

impl<T> ShallowCopy for Option<T> where T: ShallowCopy {
    type Target<'a> = Option<&'a T::Target>;
    fn deref<'a>(&'a Self::SplitType) -> Self::Target<'a> { ... }
}

But, that doesn't exist at the moment. So, what do we do?

I honestly don't know. This is the way I found to make ShallowCopy sound again. But, it also removes support for complex ShallowCopy types, which is a huge pain. Tuples and Option<T> in particular are widely used. Sure, we could require that users box them, but that seems wasteful too.

Thoughts welcome.

[jonhoo]

Here's another fun observation: since this version of ShallowCopy effectively "strips" the pointer type, it also strips things like &'static. So, if you have a map whose values are &'static str, the values you get out will now all be &str tied to the lifetime of your ReadGuard, rather than &&'static str like you used to get. Whether that is better or worse, I'm not sure..

[digama0]

This is because given the raw components of a box, we're not allowed to construct a &Box<T> (at least I don't think so), as it would violate the aliasing rules again.

I think this is not true. A &Box<T> doesn't make any aliasing promises, as you can copy it freely. It's just a pointer to a pointer. It's the same deal with &&mut T, being roughly equivalent to &T. That said it is still unsafe in this setup since it gives you read access to the T which could be concurrently written via the other T::SplitType.

For example, if you store a HashSet<ForwardThroughShallowCopy>, you can't do set.remove(t) where t: &T. Same thing with HashSet::contains, HashMap::get, or even just trying to use PartialEq to see if a T is present in a Vec<ForwardThroughShallowCopy>.

Isn't this what the Borrow trait bound on HashMap methods is supposed to solve? You don't need to provide a &K, you provide a &Q where K can deref as a Q. In your case that should be fine, where Q is T and K is ForwardThroughShallowCopy<T>.

[jonhoo]

@digama0 Ah, I think maybe I phrased myself poorly. The reason &Box<T> won't work is that the only way to construct one is with Box::from_raw, but that means we're constructing a Box when its referent might be aliased (even if we only then expose it through &Box<T>). The other reason why it'll be painful is that you can't write a function that takes in *mut T and return &Box<T> 😅

Isn't this what the Borrow trait bound on HashMap methods is supposed to solve?

Well, so, sort of. Borrow is fairly limited in scope since you cannot implement it for your own types (it has a blanket impl in the standard library). It only allows you to a) allow callers to pass both T and &T as an argument; and b) allow callers to pass specific deref-ed types in place of their owned variants (e.g., &str for &String, &[T] for &Vec<T>), etc. All of these are implemented in the standard library, and the blanket implementation prevents you from opting into this behavior for your own type.

[digama0]

Oh, I just got to the part about the blanket implementation. But that sounds surprising to me. I just tried the following and it worked:

use std::borrow::Borrow;

struct MyWrapper<T>(T);

impl<T> Borrow<T> for MyWrapper<T> {
    fn borrow(&self) -> &T {
        &self.0
    }
}

[jonhoo]

@digama0 Yes, you're right, as of Rust 1.41.0 it's possible to write a "covered" implementation of Borrow without conflicting with the blanket implementation. Unfortunately, that doesn't extend to associated generic types. What we need is:

impl<T> Borrow<T::Target> for ForwardThroughShallowCopy<T>
where
    T: ShallowCopy,
{
    fn borrow(&self) -> &T {
        self.as_ref()
    }
}

which gives

error[E0119]: conflicting implementations of trait `std::borrow::Borrow<shallow_copy::ForwardThroughShallowCopy<_>>` for type `shallow_copy::ForwardThroughShallowCopy<_>`:
   --> evmap/src/shallow_copy.rs:395:1
    |
395 | / impl<T> Borrow<T::Target> for ForwardThroughShallowCopy<T>
396 | | where
397 | |     T: ShallowCopy,
398 | | {
...   |
401 | |     }
402 | | }
    | |_^
    |
    = note: conflicting implementation in crate `core`:
            - impl<T> Borrow<T> for T
              where T: ?Sized;

[digama0]

Ah right, because it could be that an implementation of ShallowCopy lets T::Target = ForwardThroughShallowCopy<T>, which would cause this impl to actually overlap the blanket impl. If you can get rid of Target though that might solve that problem.

Regarding the problem with deref for Option<T>, I think you can solve it by requiring that Self::SplitType be layout-compatible with Self, so that deref can just be a transmute. That's going to be the case as long as the only change in SplitType is the replacement of Box<T> with *mut T in subparts. Unfortunately that may require that you have some knowledge into the internals, specifically for Vec<T> and other types with non-public layout, because you need to replace them with a "shallow version" like (*mut [T], usize) and it's not clear to me that there is a way to express this without opening up Vec to look at how it works.

[digama0]

Given something of that type from the oplog, we now need a way to insert the aliased T into the actual data structure. But, we can't just stick the T::SplitType in there directly, since it won't actually implement any of the traits the data structure might need (like Eq or Hash).

Thinking about this some more, I think this isn't necessary. I'm working from the model of evmap as of your latest stream, where the concurrency stuff is in a separate LeftRight data structure. In that case you only need the hashmap or what have you itself implement ShallowCopy; the underlying value type doesn't need to (or at least it may not, depending on how construction of new values works).

Suppose you have a LeftRight<HashMap<K, V>> which I think is approximately what evmap itself is. LeftRight will require that HashMap<K, V>: ShallowCopy, and we let HashMap<K, Box<V>>::SplitType = HashMap<K, *mut V>, adding an extra indirection so that we don't have to inspect the guts of HashMap too much. (This can be eliminated but for now let's consider this simple version.)

If we assume that SplitType is transmutable with Self, then we don't need the Target associated type, the two deref methods become transmutes, and we can assume that drop is just SplitType::drop, so ShallowCopy simplifies to just

pub unsafe trait ShallowCopy {
    type SplitType;

    fn split(self) -> (Self::SplitType, Self::SplitType);
}

which we can implement as:

unsafe impl<K, V> ShallowCopy for HashMap<K, Box<V>> {
    type SplitType = HashMap<K, *mut V>;

    fn split(self) -> (Self::SplitType, Self::SplitType) {
        assert!(self.is_empty());
        (HashMap::new(), HashMap::new())
    }
}

We could support nonempty maps in split at the cost of a clone bound on K (or a ShallowCopy bound although this complicates the key type and you probably need something like ForwardThroughShallowCopy for that).

For a few more example implementations:

unsafe impl<T> ShallowCopy for Box<T> {
    type SplitType = *mut T;

    fn split(self) -> (Self::SplitType, Self::SplitType) {
        let p = Box::into_raw(self);
        (p, p)
    }
}
unsafe impl<T: ShallowCopy> ShallowCopy for Option<T> {
    type SplitType = Option<T::SplitType>;

    fn split(self) -> (Self::SplitType, Self::SplitType) {
        match self {
            None => (None, None),
            Some(val) => {
                let (left, right) = val.split();
                (Some(left), Some(right))
            }
        }
    }
}

struct SplitRawVec<T> {
    ptr: NonNull<T>,
    cap: usize,
}

impl<T> Clone for SplitRawVec<T> {
    fn clone(&self) -> Self {
        Self { ptr: self.ptr, cap: self.cap }
    }
}
impl<T> Copy for SplitRawVec<T> {}

pub struct SplitVec<T> {
    buf: SplitRawVec<T>,
    len: usize,
}

impl<T> Clone for SplitVec<T> {
    fn clone(&self) -> Self {
        Self { buf: self.buf, len: self.len }
    }
}
impl<T> Copy for SplitVec<T> {}

unsafe impl<T: ShallowCopy> ShallowCopy for Vec<T> {
    type SplitType = SplitVec<T>;

    fn split(self) -> (Self::SplitType, Self::SplitType) {
        let split: SplitVec<T> = unsafe { std::mem::transmute(self) };
        (split, split)
    }
}

For Vec<T>, we have to peek into the implementation details in order to get something layout compatible, and even then this is not guaranteed to work (rust makes extremely few layout compatibility guarantees and this is ongoing work). Note that the trait is unsafe because the impl itself is not all there is to it, we also have to promise that we can transmute to and from the SplitType. It is a matter of taste whether to leave that implicit and keep the ShallowCopy trait unsafe, or to add unsafe trait methods for performing the transmutations, essentially your deref() method, which more or less have to be implemented by transmute by implementors; in that case the trait itself can be safe but the deref method has to be unsafe.

The oplog doesn't need to store an enum, although it logically is so; instead you just always use the SplitType and convert it to a T just before second drop to make it drop "for real".

Now in the functions like get where you need a real hashmap, you can filter through the functions

unsafe fn deref<T: ShallowCopy>(t: &T::SplitType) -> &T {
    std::mem::transmute(t)
}
unsafe fn deref_mut<T: ShallowCopy>(t: &mut T::SplitType) -> &mut T {
    std::mem::transmute(t)
}

which are safe by the contract of ShallowCopy, except that they don't check uniqueness; these get called in the core of evmap when you know that you have the appropriate level of access, for example when a ReadGuard is around. Since this is happening at the level of the whole HashMap you don't need to worry about a Borrow impl; after the deref call you just have a &HashMap<K, Box<V>> and can call regular methods on it.

Finally, to avoid the extra Box<V> indirection, we can use a recursive ShallowCopy instance, which recovers this implementation in the simple case because Box's ShallowCopy impl does not require that the contained type also be ShallowCopy.

unsafe impl<K: Clone + Hash + Eq, V: ShallowCopy> ShallowCopy for HashMap<K, V> {
    type SplitType = HashMap<K, V::SplitType>;

    fn split(self) -> (Self::SplitType, Self::SplitType) {
        let mut left = HashMap::new();
        let mut right = HashMap::new();
        for (k, v) in self {
            let (v1, v2) = v.split();
            left.insert(k.clone(), v1);
            right.insert(k, v2);
        }
        (left, right)
    }
}

It's debatable whether K ought to be shallow copied as well; cloning the keys seems like a more reasonable thing to me since the hashmap wants some control over when to drop them, but I think something like ForwardThroughShallowCopy can be used here if it's important.

I think that covers all the use cases in your post, but let me know if I missed something since I'm mostly just projecting what the code change would look like.

[codecov]

Codecov Report

Merging #81 (c14ccce) into master (a898eda) will decrease coverage by 0.22%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   79.39%   79.16%   -0.23%     
==========================================
  Files          16       16              
  Lines        1286     1296      +10     
==========================================
+ Hits         1021     1026       +5     
- Misses        265      270       +5     

Impacted Files	Coverage Δ
evmap/src/lib.rs	34.00% <ø> (ø)
evmap/src/read.rs	82.00% <ø> (ø)
evmap/src/read/read_ref.rs	65.00% <ø> (ø)
evmap/tests/quick.rs	93.65% <ø> (ø)
evmap/src/aliasing.rs	47.05% <47.05%> (ø)
evmap/src/write.rs	64.48% <80.48%> (-0.26%)	⬇️
evmap/src/values.rs	89.67% <100.00%> (+2.26%)	⬆️
evmap/tests/lib.rs	98.16% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c295f19...c14ccce. Read the comment docs.

[RustyYato]

@digama0 A simple layout assertion doesn't work, because HashMap<_, Foo> is not layout compatible with HashMap<_, Bar> even if Foo is layout compatible with Bar. This is especially true when you turn on profile guided optimizations.

@jonhoo One approach you could use without changing the interface, and possibly removing the ShallowCopy trait entirely,
you could wrap each value in MaybeUninit<T> (to change all validity invariants to safety invariants). Then you can freely copy them around using ptr::read. When you need to drop the two maps, you can ignoring them the first time, and actually drop them the second time. It doesn't matter what the validity invariants are, so it should be fine to use any type inside a MaybeUninit in this way.

[jonhoo]

@RustyYato Hehe, that's exactly the same thought I had last night. Take a look at the latest commits, and in particular the new aliasing.rs file: https://github.com/jonhoo/rust-evmap/blob/787ffb755b8b76536b1a9650f5cc0d21b8abb5b3/evmap/src/aliasing.rs

[jonhoo]

I'm going to close this in favor of #83.