Skip to content

Send/Sync audit for libcollections #22715

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
Merged
bors merged 2 commits into from
Feb 26, 2015
Merged

Send/Sync audit for libcollections #22715

bors merged 2 commits into from
Feb 26, 2015

Conversation

@edwardw
Copy link
Contributor

@edwardw edwardw commented Feb 23, 2015

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

cc #22709

@rust-highfive
Copy link
Contributor

rust-highfive commented Feb 23, 2015

r? @pcwalton

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

@edwardw
Copy link
Contributor Author

edwardw commented Feb 23, 2015

r? @huonw

@rust-highfive rust-highfive assigned huonw and unassigned pcwalton Feb 23, 2015
Gankra
Gankra reviewed Feb 23, 2015
View reviewed changes
src/libcollections/btree/node.rs Outdated
Copy link
Contributor

@Gankra Gankra Feb 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@edwardw edwardw Feb 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@huonw huonw mentioned this pull request Feb 23, 2015
Closed
30 tasks
alexcrichton
alexcrichton reviewed Feb 24, 2015
View reviewed changes
src/libcollections/vec_deque.rs Outdated
Copy link
Member

@alexcrichton alexcrichton Feb 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this could store Unique<T> instead?

Copy link
Contributor Author

@edwardw edwardw Feb 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@alexcrichton alexcrichton Feb 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These unsafe impl blocks can be removed now, right?

Copy link
Contributor Author

@edwardw edwardw Feb 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 24, 2015

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned huonw Feb 24, 2015
alexcrichton
alexcrichton reviewed Feb 24, 2015
View reviewed changes
src/libcollections/vec_deque.rs Outdated
Copy link
Member

@alexcrichton alexcrichton Feb 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@edwardw edwardw Feb 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alexcrichton
Copy link
Member

alexcrichton commented Feb 24, 2015

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

@edwardw
Copy link
Contributor Author

edwardw commented Feb 25, 2015

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

@edwardw
Copy link
Contributor Author

edwardw commented Feb 25, 2015

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

alexcrichton
alexcrichton reviewed Feb 25, 2015
View reviewed changes
src/libcollections/binary_heap.rs Outdated
Copy link
Member

@alexcrichton alexcrichton Feb 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@edwardw edwardw Feb 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alexcrichton
Copy link
Member

alexcrichton commented Feb 25, 2015

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:

  1. Are there any interior unsafe pointers or UnsafeCell types? If not, then the problem lies in the constituent types, not at the top-level.
  2. 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
Copy link
Member

alexcrichton commented Feb 25, 2015

@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
Copy link
Contributor Author

edwardw commented Feb 25, 2015

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

@edwardw
Tweak VecDeque's IterMut implementation
101498c 
So it is symmetric to its `Iter` implementation. Also kills an FIXME.
alexcrichton
alexcrichton reviewed Feb 26, 2015
View reviewed changes
src/libcore/slice.rs
Copy link
Member

@alexcrichton alexcrichton Feb 26, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T: Sync => T: Send

Copy link
Contributor

@huonw huonw Feb 26, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@alexcrichton alexcrichton Feb 26, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@edwardw
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
@alexcrichton
Copy link
Member

alexcrichton commented Feb 26, 2015

@bors: r+ 6849006

@bors
Copy link
Collaborator

bors commented Feb 26, 2015

⌛ Testing commit 6849006 with merge a5214e4...

bors added a commit that referenced this pull request Feb 26, 2015
@bors
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
Copy link
Collaborator

bors commented Feb 26, 2015

☀️ 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

@bors bors merged commit 6849006 into rust-lang:master Feb 26, 2015
@bors bors mentioned this pull request Feb 26, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@alexcrichton alexcrichton

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@edwardw @rust-highfive @alexcrichton @bors @Gankra @huonw @pcwalton