Send/Sync audit for libcollections #22715

edwardw · 2015-02-23T14:44:34Z

In the process, also replaces two raw mutable pointers with Unique to
spell out the ownership semantics.

cc #22709

rust-highfive · 2015-02-23T14:44:40Z

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

edwardw · 2015-02-23T14:46:17Z

r? @huonw

Gankra · 2015-02-23T16:43:17Z

src/libcollections/btree/node.rs

@@ -200,6 +200,9 @@ struct RawItems<T> {
    tail: *const T,
 }

+unsafe impl<T: Send> Send for RawItems<T> { }


IIIII'm not sure this is sound. RawItems is basically for draining values out of an allocation, but doesn't actually own its allocation. Sending this seems unsound?

Also RawItems is 100% internal, so I don't think it needs Send/Sync (only exported inside of MoveTraversalImpl, but you impl Send/Sync on that already).

Good catch. I assumed RawItems is safe since it has Iterator and DoubleEndedIterator implementations which take &mut self. But by closer inspection, the real mutations are all in unsafe block. MoveTraversalImpl is also a very good point.

Comment addressed.

alexcrichton · 2015-02-24T00:55:53Z

src/libcollections/vec_deque.rs

@@ -1529,6 +1529,11 @@ pub struct IterMut<'a, T:'a> {
 }

 #[stable(feature = "rust1", since = "1.0.0")]
+unsafe impl<'a, T: Send+'a> Send for IterMut<'a, T> { }
+#[stable(feature = "rust1", since = "1.0.0")]
+unsafe impl<'a, T: Sync+'a> Sync for IterMut<'a, T> { }


Perhaps this could store Unique<T> instead?

Done and then I tweaked the implementation to match Iter, which kills pointer all together.

These unsafe impl blocks can be removed now, right?

edwardw · 2015-02-24T17:51:29Z

r? @alexcrichton

alexcrichton · 2015-02-24T18:42:45Z

src/libcollections/vec_deque.rs


        unsafe {
-            Some(&mut *self.ptr.offset(tail as isize))
+            let elem = self.ring.get_unchecked_mut(tail);
+            Some(mem::transmute(elem))


I think we tend to prefer avoiding transmute wherever possible, could this be written as Some(&mut *(elem as *mut _))?

alexcrichton · 2015-02-24T18:44:35Z

Can you also add some tests to ensure that we don't regress in these aspects?

edwardw · 2015-02-25T05:41:08Z

Hmm, seems I've tackled problem the wrong way. Test-driven would be better here. Will rework the patch.

edwardw · 2015-02-25T15:33:50Z

An extensive test was added. r? @alexcrichton @huonw

alexcrichton · 2015-02-25T17:05:37Z

src/libcollections/binary_heap.rs

@@ -568,6 +568,9 @@ pub struct Iter <'a, T: 'a> {
    iter: slice::Iter<'a, T>,
 }

+unsafe impl<'a, T: Sync> Sync for Iter<'a, T> {}
+unsafe impl<'a, T: Sync> Send for Iter<'a, T> {}


Shouldn't these be unnecessary? I would figure that slice::Iter would implement these as appropriate

Currently it isn't. This can be removed, however, after libcore is properly Sync and Send audited. A FIXME maybe?

alexcrichton · 2015-02-25T17:12:34Z

Nice test! Sorry for being so pedantic about these, but this feels like an extra level of unsafety I want to be sure we tread carefully with. To me there are a bunch more unsafe impl blocks than I would have expected originally. I would consider something like the following steps for considering whether to mark something with unsafe impl:

Are there any interior unsafe pointers or UnsafeCell types? If not, then the problem lies in the constituent types, not at the top-level.
Can any unsafe pointers be represented by other abstractions? For example Unique is one but there may be other helper structs as well.

Along those lines, some of the structures marked with unsafe impl fall into (1) where they don't contain unsafe things and the problem seems to lie in the underlying abstractions. I think we already got all the instances of (2).

Does that make sense?

alexcrichton · 2015-02-25T17:14:12Z

@edwardw ah it seems you responded as I was responding! Would it be possible to go ahead and "audit" the relevant portions of libcore ahead of time? It should just involve dealing with slice::Iter for now (you don't have to do the whole audit of course). It's generally preferable to not have an intermediate state where lots of stuff is "slated for deletion"

edwardw · 2015-02-25T17:15:58Z

Certainly. That leaves only RawItems business. I'll wait Mr. @gankro to confirm that one.

So it is symmetric to its `Iter` implementation. Also kills an FIXME.

alexcrichton · 2015-02-26T06:18:10Z

src/libcore/slice.rs

@@ -740,6 +740,9 @@ pub struct Iter<'a, T: 'a> {
    _marker: marker::PhantomData<&'a T>,
 }

+unsafe impl<'a, T: Sync> Sync for Iter<'a, T> {}
+unsafe impl<'a, T: Sync> Send for Iter<'a, T> {}


T: Sync => T: Send

Sync is correct; Iter<T> is essentially the same as &T.

Oh wow that is subtle, looks like I should read this again.

In the process, also replaces a raw mutable pointers with Unique to spell out the ownership semantics. cc rust-lang#22709

alexcrichton · 2015-02-26T07:45:56Z

@bors: r+ 6849006

bors · 2015-02-26T15:43:39Z

⌛ Testing commit 6849006 with merge a5214e4...

In the process, also replaces two raw mutable pointers with `Unique` to spell out the ownership semantics. cc #22709

bors · 2015-02-26T18:44:39Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

rust-highfive assigned pcwalton Feb 23, 2015

rust-highfive assigned huonw and unassigned pcwalton Feb 23, 2015

Gankra reviewed Feb 23, 2015
View reviewed changes

edwardw force-pushed the send-sync-audit branch from 6ed9786 to 41315be Compare February 23, 2015 17:33

huonw mentioned this pull request Feb 23, 2015

Many std iterators do not implement Send/Sync #22709

Closed

30 tasks

alexcrichton reviewed Feb 24, 2015
View reviewed changes

edwardw force-pushed the send-sync-audit branch from 41315be to 38b22dc Compare February 24, 2015 14:17

rust-highfive assigned alexcrichton and unassigned huonw Feb 24, 2015

alexcrichton reviewed Feb 24, 2015
View reviewed changes

edwardw force-pushed the send-sync-audit branch from 38b22dc to 428e795 Compare February 25, 2015 15:28

alexcrichton reviewed Feb 25, 2015
View reviewed changes

edwardw force-pushed the send-sync-audit branch from 428e795 to 8665138 Compare February 25, 2015 18:55

Tweak VecDeque's IterMut implementation

101498c

So it is symmetric to its `Iter` implementation. Also kills an FIXME.

alexcrichton reviewed Feb 26, 2015
View reviewed changes

edwardw force-pushed the send-sync-audit branch from 8665138 to 8119e78 Compare February 26, 2015 07:08

edwardw force-pushed the send-sync-audit branch from 8119e78 to 296714c Compare February 26, 2015 07:39

Send/Sync audit for libcollections

6849006

In the process, also replaces a raw mutable pointers with Unique to spell out the ownership semantics. cc rust-lang#22709

edwardw force-pushed the send-sync-audit branch from 296714c to 6849006 Compare February 26, 2015 07:44

bors added a commit that referenced this pull request Feb 26, 2015

Auto merge of #22715 - edwardw:send-sync-audit, r=alexcrichton

a5214e4

In the process, also replaces two raw mutable pointers with `Unique` to spell out the ownership semantics. cc #22709

bors merged commit 6849006 into rust-lang:master Feb 26, 2015

bors mentioned this pull request Feb 26, 2015

Implement arithmetic overflow changes #22532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send/Sync audit for libcollections #22715

Send/Sync audit for libcollections #22715

edwardw commented Feb 23, 2015

rust-highfive commented Feb 23, 2015

edwardw commented Feb 23, 2015

Gankra Feb 23, 2015

edwardw Feb 23, 2015

alexcrichton Feb 24, 2015

edwardw Feb 24, 2015

alexcrichton Feb 24, 2015

edwardw Feb 25, 2015

edwardw commented Feb 24, 2015

alexcrichton Feb 24, 2015

edwardw Feb 25, 2015

alexcrichton commented Feb 24, 2015

edwardw commented Feb 25, 2015

edwardw commented Feb 25, 2015

alexcrichton Feb 25, 2015

edwardw Feb 25, 2015

alexcrichton commented Feb 25, 2015

alexcrichton commented Feb 25, 2015

edwardw commented Feb 25, 2015

alexcrichton Feb 26, 2015

huonw Feb 26, 2015

alexcrichton Feb 26, 2015

alexcrichton commented Feb 26, 2015

bors commented Feb 26, 2015

bors commented Feb 26, 2015

Send/Sync audit for libcollections #22715

Send/Sync audit for libcollections #22715

Conversation

edwardw commented Feb 23, 2015

rust-highfive commented Feb 23, 2015

edwardw commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardw commented Feb 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 24, 2015

edwardw commented Feb 25, 2015

edwardw commented Feb 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 25, 2015

alexcrichton commented Feb 25, 2015

edwardw commented Feb 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 26, 2015

bors commented Feb 26, 2015

bors commented Feb 26, 2015