Tweak and extend internal BTreeMap documentation, including debug asserts. #67812

ssomers · 2020-01-02T16:40:12Z

Gathered from work on various other pull requests (e.g. #67725, #67686).

rust-highfive · 2020-01-02T16:40:16Z

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

ssomers · 2020-01-02T16:44:23Z

src/liballoc/collections/btree/map.rs

@@ -2605,7 +2605,7 @@ impl<'a, K: Ord, V> OccupiedEntry<'a, K, V> {

        // Handle underflow
        let mut cur_node = small_leaf.forget_type();
-        while cur_node.len() < node::CAPACITY / 2 {
+        while cur_node.len() < node::MIN_LEN {


Changing this constant really is documentation: no other code uses CAPACITY / 2, but CAPACITY equals 2 * B - 1, so CAPACITY / 2 equals B - 1 which is MIN_LEN.

src/liballoc/collections/btree/map.rs

hanna-kruppe · 2020-01-03T13:40:46Z

src/liballoc/collections/btree/node.rs

    /// Returns a raw ptr to avoid asserting exclusive access to the entire node.
    fn as_leaf_mut(&mut self) -> *mut LeafNode<K, V> {
-        // We are mutable, so we cannot be the shared root, so accessing this as a leaf is okay.
+        debug_assert!(!self.is_shared_root());


I believe this assertion/precondition is incorrect. Calling this method on a NodeRef to the shared root doesn't cause any immediate problem, right? No &LeafNode or &mut LeafNode is constructed or other UB is triggered. Granted, most uses of this function probably need to take some care re: the shared root in some way, but I believe into_key_slice_mut sometimes calls it on the shared root in a way that it correct (see my comment there).

So this should not be a precondition of this method, but rather the fact that the returned pointer may refer to the shared root is just something that the caller must be cognizant of when using the returned pointer. That's worth documenting.

Before I read your comment, I would write (and I probably have, in other PRs) that it does cause undefined behaviour if K is unfortunately aligned, but I had no idea there was a difference between &LeafNode and *LeafNode in that respect. I'll ask for Miri's opinion when I force feed it into_key_slice_mut.

As in the into_key_slice_mut comment and another experiment, I now believe both comments are overly optimistic.

Invoking as_leaf_mut on the shared root (which never happened) always is undefined behaviour, flagged by miri. With ordinary aligned keys, on Intel CPUs (Linux or Windows, 32 or 64 bit) there is no practical harm. Well, maybe there is if you manage to allocate the shared root near the end of a page or good old 16 bit segment or something...

Invoking as_leaf_mut on the shared root for exotically aligned key types (which into_key_slice_mut takes evasive action against), causes an 'attempt to create unaligned or null slice' panic.

I'm confused. Raw pointers have no inherent alignment requirement, casting them around has no impact on stacked borrows rules, and there's nothing here creating a slice of any kind in as_leaf_mut. It sounds to me like the miri errors you mention occurred in into_key_slice_mut or other consumers of as_leaf_mut, not in as_leaf_mut. To illustrated, I believe that adding statements of the form some_noderef.as_leaf_mut(); (i.e., calling the function but ignoring) anywhere in this module will never cause UB.

Sorry for confusing you (but now you know how I've been feeling for a month...). You're right, of course, miri does not complain about let leaf = self.as_leaf_mut(); but about &mut (*leaf).keys.
Now I might have really understood the other comments...

Even if my new attempt to document as_leaf_mut is correct, I still don't understand this: how does shared_empty_root get away with treating &EMPTY_ROOT_NODE as a LeafNode<K, V> pointer? Surely the alignment requirement of EMPTY_ROOT_NODE is much more relaxed than that of LeafNode<K, V>.

Raw pointers (including NonNull which is just a wrapper around a raw pointer for this purpose) do not have any alignment requirement "just by existing" the way references have. Loading and storing normally through them does require alignment, but if you don't do that or use methods like {read,write}_unaligned, alignment simply does not matter for them. See also the std::ptr documentation.

hanna-kruppe · 2020-01-03T13:44:18Z

src/liballoc/collections/btree/node.rs

@@ -570,6 +584,7 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Mut<'a>, K, V, Type> {
        unsafe { &mut *(self.root as *mut Root<K, V>) }
    }

+    /// The caller must ensure that this is a proper node, and not the shared root.


Is this really necessary? This method seems to handle the shared root correctly, just like into_key_slice. Although the shared root is immutable and this method generally constructs a mutable reference into the node, the slice constructed in the else branch has zero elements in this case, so no mutable aliasing occurs.

On further thought, the temporary &mut [MaybeUninit<K>; CAPACITY] needed to call first_ptr_mut might be a problem. But if that matters, it can be side-stepped easily by using &raw mut and pointer casts (or adding a raw pointer version of first_ptr_mut to MaybeUninit), no need to add extra preconditions.

I shouldn't have added the comment without the rest of #67725.
All I'm sure of at the moment is that the current code never feeds shared roots there.

When I make a mutable-all-the-way variant of get_mut, then Miri says on test_basic_small (so even without fancy alignment):

642 | MaybeUninit::first_ptr_mut(&mut (*self.as_leaf_mut()).keys), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: Memory access failed: pointer must be in- bounds at offset 104, but is outside bounds of allocation 22835 which has size 16 |

So as far as I understand, pointers too "must be dereferencable for the entire size of their pointee".

If I understand correctly the problem is just (as predicted above) that the shared root node has no keys storage but a &mut to it where it would be is created, even if only temporarily, and that has to be dereferenceable. If it was all raw pointers, you should just get a one-past-the-end pointer (modulo the alignment issue that the if fixes) which is never dereferenced and only flows into a zero-sized slice.

But if the other PR removes the support for the shared root from this method anyway, then this is all moot. I think I'd prefer the comment on as_leaf_mut to be made more precise and this method to not gain any comment in this PR.

I think I'm starting to understand both your comments now. Though if I were to try to correct support for the shared root in into_key_slice_mut, I'd still just copy the else-part of into_key_slice, kick out some consts and splash around some muts until the compiler is happy.

But yes, it's moot, because the callers that want to mutate trees, grant a tree its own root before poking around in slices; and the callers that merely want mutable access to values (i.e. get_mut) don't need into_key_slice_mut to handle shared roots.

hanna-kruppe · 2020-01-03T13:46:28Z

src/liballoc/collections/btree/node.rs

        self.node.as_ptr()
    }

+    /// The caller must ensure that this is a proper node, and not the shared root.


(I believe this is propagated from into_key_slice_mut, if I'm correct in my review comment there and that method doesn't require this precondition, then this instance should also be removed.)

hanna-kruppe · 2020-01-03T14:10:12Z

src/liballoc/collections/btree/node.rs

@@ -370,23 +370,32 @@ impl<BorrowType, K, V, Type> NodeRef<BorrowType, K, V, Type> {
        NodeRef { height: self.height, node: self.node, root: self.root, _marker: PhantomData }
    }

-    /// Assert that this is indeed a proper leaf node, and not the shared root.
+    /// The caller must ensure that this is a proper node, and not the shared root.


Nit: since "proper node" is the antonym of "shared root", I would prefer

Suggested change

/// The caller must ensure that this is a proper node, and not the shared root.

/// The caller must ensure that this is a proper node, not the shared root.

As-is, it sounds a little too much like a conjunction IMO: "this must be a proper node, and furthermore it must not be the shared root" vs the intended "this must be a proper node (reminder: proper node means any node other than the shared root)".

(This applies to all copies of this phrase.)

I just tried to recycle as much as possible from the original. There is no definition of proper node elsewhere, or any kind of properness, so why not just say "The caller must ensure that this(*) is not the shared root"?

(*) or "this node" or "self" or "we are" or ...

Oh yeah, that's probably better.

Co-Authored-By: Robin Kruppe <robin.kruppe@gmail.com>

hanna-kruppe · 2020-01-04T13:38:28Z

LGTM now, thanks! @bors r+

bors · 2020-01-04T13:38:30Z

📌 Commit 92acdc8 has been approved by rkruppe

…ruppe Tweak and extend internal BTreeMap documentation, including debug asserts. Gathered from work on various other pull requests (e.g. rust-lang#67725, rust-lang#67686).

@ghost

Rollup of 4 pull requests Successful merges: - #67137 (libstd uses `core::panic::Location` where possible.) - #67709 (Introduce an option for disabling deduplication of diagnostics) - #67775 (Make "use $crate" a hard error) - #67812 (Tweak and extend internal BTreeMap documentation, including debug asserts.) Failed merges: r? @ghost

rust-highfive assigned hanna-kruppe Jan 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2020

ssomers commented Jan 2, 2020

View reviewed changes

src/liballoc/collections/btree/map.rs Outdated Show resolved Hide resolved

ssomers mentioned this pull request Jan 2, 2020

Bundle and document 6 BTreeMap navigation algorithms #67073

Merged

hanna-kruppe suggested changes Jan 3, 2020

View reviewed changes

ssomers force-pushed the btreemap_internal_doc branch from 1930ad9 to d63b450 Compare January 4, 2020 00:47

Tweak and extend internal documentation, including debug asserts.

92acdc8

Co-Authored-By: Robin Kruppe <robin.kruppe@gmail.com>

ssomers force-pushed the btreemap_internal_doc branch from d63b450 to 92acdc8 Compare January 4, 2020 13:07

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 4, 2020

Dylan-DPC-zz mentioned this pull request Jan 4, 2020

Rollup of 4 pull requests #67874

Merged

bors merged commit 92acdc8 into rust-lang:master Jan 4, 2020

ssomers mentioned this pull request Jan 7, 2020

Remove leaf bias from BTreeMap's BoxedNode and NodeRef #67980

Closed

ssomers deleted the btreemap_internal_doc branch January 7, 2020 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak and extend internal BTreeMap documentation, including debug asserts. #67812

Tweak and extend internal BTreeMap documentation, including debug asserts. #67812

ssomers commented Jan 2, 2020 •

edited

Loading

rust-highfive commented Jan 2, 2020

ssomers Jan 2, 2020

hanna-kruppe Jan 3, 2020

ssomers Jan 3, 2020 •

edited

Loading

ssomers Jan 4, 2020

hanna-kruppe Jan 4, 2020

ssomers Jan 4, 2020

ssomers Jan 4, 2020

hanna-kruppe Jan 4, 2020

hanna-kruppe Jan 3, 2020

ssomers Jan 3, 2020

ssomers Jan 3, 2020 •

edited

Loading

hanna-kruppe Jan 3, 2020

ssomers Jan 4, 2020

hanna-kruppe Jan 3, 2020

hanna-kruppe Jan 3, 2020

ssomers Jan 3, 2020

hanna-kruppe Jan 3, 2020

hanna-kruppe commented Jan 4, 2020

bors commented Jan 4, 2020

	/// The caller must ensure that this is a proper node, and not the shared root.
	/// The caller must ensure that this is a proper node, not the shared root.

Tweak and extend internal BTreeMap documentation, including debug asserts. #67812

Tweak and extend internal BTreeMap documentation, including debug asserts. #67812

Conversation

ssomers commented Jan 2, 2020 • edited Loading

rust-highfive commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssomers Jan 3, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssomers Jan 3, 2020 • edited Loading

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

Choose a reason for hiding this comment

hanna-kruppe commented Jan 4, 2020

bors commented Jan 4, 2020

ssomers commented Jan 2, 2020 •

edited

Loading

ssomers Jan 3, 2020 •

edited

Loading

ssomers Jan 3, 2020 •

edited

Loading