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

Vec drop and truncate: drop using raw slice *mut [T] #71148

Merged
merged 2 commits into from
May 1, 2020

Conversation

bluss
Copy link
Member

@bluss bluss commented Apr 14, 2020

By creating a *mut [T] directly (without going through &mut [T]), avoid
questions of validity of the contents of the slice.

Consider the following risky code:

unsafe {
    let mut v = Vec::<bool>::with_capacity(16);
    v.set_len(16);
}

The intention is that with this change, we avoid one of the soundness
questions about the above snippet, because Vec::drop no longer
produces a mutable slice of the vector's contents.

r? @RalfJung

By creating a *mut [T] directly (without going through &mut [T]), avoid
questions of validity of the contents of the slice.

Consider the following risky code:

```rust
unsafe {
    let mut v = Vec::<bool>::with_capacity(16);
    v.set_len(16);
}
```

The intention is that with this change, the above snippet will be
sound because Vec::drop does no longer produces a mutable slice of
the vector's contents.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2020
@RalfJung
Copy link
Member

The intention is that with this change, the above snippet will be
sound because Vec::drop no longer produces a mutable slice of
the vector's contents.

I am not sure if this change actually achieves that -- that depends on whether drop_in_place may be called on uninitialized data for types that have no drop glue.

@bluss bluss added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2020
@bluss
Copy link
Member Author

bluss commented Apr 14, 2020

I am not sure if this change actually achieves that -- that depends on whether drop_in_place may be called on uninitialized data for types that have no drop glue.

This just seems like the best choice; it should be a no-op if the type does not have a drop glue.
But it does conflict(?) with the current documentation for the drop_in_place intrinsic - but there is no reason the pointer should actually be read from, for a Copy type.

@RalfJung
Copy link
Member

"Valid for reads" just means that you may read the memory (the pointer doesn't dangle and is still Stacked-Borrows-valid), it doesn't say anything about the data stored there.

I am okay landing this just based on the principle of using the weakest sufficient type. I am just saying to achieve what you stated in the OP, you should get official blessing that drop_in_place works like that (and I cannot grant that blessing, that's lang team territory).

@@ -2379,7 +2379,7 @@ unsafe impl<#[may_dangle] T> Drop for Vec<T> {
fn drop(&mut self) {
unsafe {
// use drop for [T]
ptr::drop_in_place(&mut self[..]);
ptr::drop_in_place(ptr::slice_from_raw_parts_mut(self.as_mut_ptr(), self.len))
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding a brief comment for why we use this complicated operation here instead of the simple one.

Really, already for #70558, what we should really have is a way to get a raw slice for the entire Vec, and then subslicing on raw slices. Then we would not have to call slice_from_raw_parts for any of the methods changed in that PR or here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment added, though it's very vague since I don't dare say so much about what it actually allows

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2020
@RalfJung
Copy link
Member

@bluss this has some minor outstanding review comments.

Update Vec drop with a comment to explain why we want to use a raw
slice, and extend this pattern to also include the Vec's IntoIter.
@bluss
Copy link
Member Author

bluss commented Apr 28, 2020

Updated. PR description updated to weaken the promises made. I found a workaround for my original problem in ndarray; just making drop conditional for when we have needs_drop::<T>().

That could be a way to sidestep the question even inside Vec::drop too - (the question being, are we allowed to drop_in_place(*mut [T]) when the values are uninit, but without drop glue?). In the past it has just been decided that using needs_drop here is just noise - calling drop_in_place would have been equivalent in effect.

@bluss bluss added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 29, 2020
@RalfJung
Copy link
Member

Looking good, thanks!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 30, 2020

📌 Commit f654daf has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#71148 (Vec drop and truncate: drop using raw slice *mut [T])
 - rust-lang#71465 (Add a convenience method on `TyCtxt` for checking for thread locals)
 - rust-lang#71567 (Handle build completion message from Cargo)
 - rust-lang#71590 (MIR dump: print pointers consistently with Miri output)
 - rust-lang#71682 (Bump pulldown-cmark)
 - rust-lang#71688 (Allow `Downcast` projections unconditionally in const-checking)
 - rust-lang#71691 (Allow `Unreachable` terminators unconditionally in const-checking)
 - rust-lang#71719 (Update backtrace-sys)

Failed merges:

r? @ghost
@bors bors merged commit 4adebb9 into rust-lang:master May 1, 2020
@bluss bluss deleted the vec-drop-raw-slice branch May 1, 2020 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants