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

BTreeMap: tougher checking on most uses of copy_nonoverlapping #80391

Merged
merged 1 commit into from
Jan 10, 2021

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Dec 26, 2020

Miri checks pointer provenance and destination, but we can check it in debug builds already.
Also, we can let Miri confirm we don't mistake imprints of moved keys and values as genuine.
r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 26, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Dec 26, 2020

Includes #80390. I only just noticed that many slice functions like copy_from_slice are defined as assignment: destination before source. slice_move is like ptr::copy_nonoverlapping: source then destination. Which one is best?

@Mark-Simulacrum
Copy link
Member

I don't think order matters too much, but we can switch to using copy_nonoverlapping_to which is defined directly on raw pointers, which seems much better to me than thinking about order of arguments :)

Ideally I think we'd seek to do the same for the slice functions, and we might look at putting them on MaybeUninit itself.

ptr::copy_nonoverlapping(source.as_ptr(), dest.as_mut_ptr(), source.len());
}
if cfg!(debug_assertions) {
// Tell Miri that the copied slice became uninitialized, and zero it in debug builds.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by zero it in debug builds? This isn't zeroing anything...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not contractually, but in practice it zeroes, both in builds of alloc and in this playground example.

@Mark-Simulacrum
Copy link
Member

@RalfJung I am interested in hearing your thoughts on a move and move_nonoverlapping function on ptr or MaybeUninit - it seems kind of odd to have it hand-rolled in terms of the de-initializing and such in btree code (indeed I would rather reject this PR as-is; I think we should not hand roll primitives in the btree code - it's sufficiently complicated without needing to know new functions behavior).

@ssomers
Copy link
Contributor Author

ssomers commented Dec 27, 2020

I think we should not hand roll primitives in the btree code - it's sufficiently complicated without needing to know new functions behavior).

This baffles me. Do you wish slice_insert and slice_remove (which I just happened to have enhanced similarly) would not exist and presumably be written out? How about all other non-public functions in the btree realm, or the primitive data structure offered by the node module?

Naturally I wish there were standard ways for working with variable length arrays of MaybeUninit containers.

@Mark-Simulacrum
Copy link
Member

I mean moving them to standard library APIs, they are of course useful as methods and I wouldn't just inline them.

@@ -1739,9 +1725,35 @@ unsafe fn slice_remove<T>(slice: &mut [MaybeUninit<T>], idx: usize) -> T {
let slice_ptr = slice.as_mut_ptr();
let ret = (*slice_ptr.add(idx)).assume_init_read();
ptr::copy(slice_ptr.add(idx + 1), slice_ptr.add(idx), len - idx - 1);
if cfg!(debug_assertions) {
// Tell Miri about the trailing element, and zero it in debug builds.
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean to "tell Miri" about an element?

Copy link
Contributor Author

@ssomers ssomers Dec 27, 2020

Choose a reason for hiding this comment

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

As in the description, that it became uninitialized.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see... that wasn't clear (also because it's in the hidden part of the diff).

@RalfJung
Copy link
Member

I am interested in hearing your thoughts on a move and move_nonoverlapping function on ptr or MaybeUninit

This is a weird operation... it's not really useful aside from making Miri complain about future uses of this memory (assuming those uses actually do things that are illegal for uninit memory, such as arithmetic/logic operations). Or am I missing something?

@ssomers
Copy link
Contributor Author

ssomers commented Dec 27, 2020

it's not really useful aside from making Miri complain

I'm not sure you're talking about slice_move, but I call Miri's complaints really useful regardless. Aside from that, slice_move checks sizes and conveys to the reader that it transfers ownership, whereas copy_nonoverlapping suggests it copies.

@RalfJung
Copy link
Member

whereas copy_nonoverlapping suggests it copies.

I guess I considered it to work on a level below the one where "copy" vs "move" is a meaningful distinction. I view this as being mostly a type-system-level fiction; on the level of unsafe code messing around with the underlying bytes, they are both the same.

In particular, note that the exact behavior of "MIR-level move" is not determined yet, and whether it replaces the source by undef is unclear. Miri currently treats copy and move exactly identical. Also see rust-lang/unsafe-code-guidelines#188.

I call Miri's complaints really useful regardless

Fair. I guess what I meant to say is -- there is no precedent for APIs that express such "Miri-only effects".

@ssomers
Copy link
Contributor Author

ssomers commented Dec 28, 2020

whereas copy_nonoverlapping suggests it copies.

I guess I considered it to work on a level below the one where "copy" vs "move" is a meaningful distinction.

Yes, that's another way of putting it: ptr::copy_nonoverlapping is too low level for arrays of MaybeUninit<T>, that really model an Option<T> hiding whether it contains a value (and thus doesn't drop the value). Or at least, that's how the btree code treats them, and that's what Miri appears to track meticulously (when asked to).

Once you have that, there is some meaningful distinction between "move" and "copy". But what slice_move does is incomplete because it can also overwrite an initialized MaybeUninit<T> and you loose an owned value. I'm not going to try to complete that ownership idea, so let's say we rename slice_move to slice_copy_nonoverlapping and forget the marking of uninitialized positions. Actually, that should be slice_copy because if you have an exclusive &mut [MaybeUninit<T>] destination, the source slice cannot overlap.

What's left is the debug_assert on slice lengths, the most important bit that Miri's pointer provenance checking detected late in various places. One might say, that's what copy_from_slice already does, so let's try to mimic that, without the Copy bound.

Fair. I guess what I meant to say is -- there is no precedent for APIs that express such "Miri-only effects".

Isn't MaybeUninit::uninit a "debug build and Miri-only effect"?

@RalfJung
Copy link
Member

Isn't MaybeUninit::uninit a "debug build and Miri-only effect"?

No, that function is called in many situations where people don't care about Miri at all.

@ssomers ssomers force-pushed the btree_cleanup_slices_3 branch 2 times, most recently from 5ac3db4 to 77eb289 Compare December 29, 2020 16:58
@Mark-Simulacrum
Copy link
Member

modulo rename r=me

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2021

📌 Commit 26b9462 has been approved by Mark-Simulacrum

@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 Jan 9, 2021
@bors
Copy link
Contributor

bors commented Jan 10, 2021

⌛ Testing commit 26b9462 with merge fd34606...

@bors
Copy link
Contributor

bors commented Jan 10, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing fd34606 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 10, 2021
@bors bors merged commit fd34606 into rust-lang:master Jan 10, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 10, 2021
@ssomers ssomers deleted the btree_cleanup_slices_3 branch January 10, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

6 participants