BTreeMap: enforce the panic rule imposed by `replace` #75071

ssomers · 2020-08-02T20:22:14Z

Also, reveal the unsafe parts in the closures fed to it.

pickfire · 2020-08-03T14:28:39Z

library/alloc/src/collections/btree/navigate.rs


 impl<BorrowType, K, V> Handle<NodeRef<BorrowType, K, V, marker::Leaf>, marker::Edge> {
    /// Given a leaf edge handle, returns [`Result::Ok`] with a handle to the neighboring KV
    /// on the right side, which is either in the same leaf node or in an ancestor node.
    /// If the leaf edge is the last one in the tree, returns [`Result::Err`] with the root node.
+    /// Cannot panic.


Suggested change

/// Cannot panic.

///

/// # Safety

///

/// Cannot panic.

Should it be like this to make it more distinct and easier to check? "Cannot panic" after a paragraph might be easy to miss. (usually safety is only used for unsafe function)

In general I don't like my wording either but I sure don't know what to do here. Some points:

It's rather distinct from the safety of calling this function, it's a guarantee that this function offers to callers that do not handle panic well (*), and a reminder for implementers.

I have seen "SAFETY" and "Safety" but not "# Safety".

Speaking of #, I did see the #[no_panic] attribute (or what's it called) in what seems to be the only attempt that survived to deal with this formally while diving into RFC 1736 and related. I'm not suggesting to use that crate of course, but maybe reuse the notation as a comment.

(*) But one could state that all functions in node.rs cannot panic. Whenever I make them, by accident or attempt to debug, I'm almost guaranteed a double panic. For instance, because VacantEntry::insert first increases the length, then tries to add an element, and if that fails, the drop handler tries to delete that unborn element.

It's rather distinct from the safety of calling this function, it's a guarantee that this function offers to callers that do not handle panic well (*), and a reminder for implementers.

Yes, this sort of stuff are usually let implementors to see.

I have seen "SAFETY" and "Safety" but not "# Safety".

I see that "SAFETY" is used mainly inside code comments for each unsafe use. But here "# Safety" (it also exists somewhere) is used to discuss the safety required when implementing it as part of the doc comments.

This shouldn't be made into a safety section (that only makes sense for unsafe functions). However, adding a bit of text saying something like "panicking here leads to UB (or aborts) elsewhere in this module" would be fine.

Hmm, repeat that text in all 28 places? For most functions, it's not "in this module" but in the navigate module.

I'm quite surprised there is no equivalent to noexecept in C++, apparently not even a convention.

How about a very explicit panicless_ prefix on each name?

We could add that into our main module doc but I think it is hard for everyone to read all of that.

I don't know what either of you mean with "that" and if I did, I probably still wouldn't know how to proceed.

@Mark-Simulacrum Can you please share some thoughts to proceed? I don't know what to say.

A reminder how we came here: Mark introduced the replace function to capture a pattern that had been scattered throughout iterator implementations. I got away then without checking or documenting compliance to the no-panic rule. Then Ralf in #73971 asked to add "a comment saying that, and also make sure that indeed next_kv, unwrap_unchecked and next_leaf_edge never panic?" That trickled down to tens of functions receiving a comment in this PR. I'm not at all sure that is what Ralf meant, it could just mean to have a single comment in the new take_mut function, the same as Mark wrote (or pasted) in replace (as well as removing the debug_asserts eager to panic).

I would put either nothing or a "[no_panic]" comment there - it looks less casual, and it's a term you can look up. I haven't found any explanation of "unwind(aborts)", and that (as an attribute) it's used on functions interfacing with C++.

here "# Safety" (it also exists somewhere) is used to discuss the safety required when implementing it as part of the doc comments.

Oh, I see, it's essential to get proper formatting, similar to the examples, such as NonNull::new_unchecked, with bigger blocks recently added. It's not important for all the private modules, but I better get used to doing it properly.

nbdd0121 · 2020-08-07T01:50:46Z

How about just make that function safe instead?

pub fn replace_with<T>(val: &mut T, f: impl FnOnce(T)->T) {
    struct PanicGuard;
    impl Drop for PanicGuard {
        fn drop(&mut self) {
            // replace_with is not safe when f panics, so use a guard to
            // trigger a double panic which will abort without drop.
            //
            // This could also be std::intrinsics::abort. 
            panic!("closure passed to replace_with must not panick");
        }
    }
    let guard = PanicGuard;
    unsafe {
        let value = ptr::read(val);
        let value = f(value);
        ptr::write(val, value);
    }
    mem::forget(guard);
}

(Taken from one of my personal project)

ssomers · 2020-08-07T08:07:01Z

Thanks @nbdd0121, I've been wanting to figure out how the "proper" take_mut would work.

But I don't quite understand the word "just". I do understand that it makes the function formally safe, but does it change the no-panic rule on the closure? Does it change how much we want to document or formally enforce the no-panic rule? And again, I'm not saying that anyone asked to document or formally enforce the no-panic rule, that was just my interpretation.

ssomers · 2020-08-07T10:16:47Z

According to fickle benchmarks, the abort guarantee comes with a small price, but using intrinsics::abort instead of panic! turns it into a small gain for IntoIter.

ssomers · 2020-08-07T11:37:46Z

I think I got it now: the abort guarantee replaces the (what I called) "no-panic" rule with a "beware that panics abort unconditionally" rule. There is no need to ban debug_asserts: they still guarantee a condition in debug builds at no cost in release builds, but they loose the additional benefit of nice error reporting, particularly during testing.

In reality, none of the functions building on the primitives down in internal modules handle panics well, except by coincidence. So I think there is no point in writing a comment on a particular primitive function that we know it is invoked in a way that guarantees a panic would cause an abort, because it might also be invoked in a way that doesn't guarantee that.

nbdd0121 · 2020-08-07T14:45:41Z

library/alloc/src/collections/btree/navigate.rs

+/// `Option::unwrap` without the promise to panic if the option contains no value.
+/// Instead, the entire process would be aborted.
+fn unwrap_or_abort<T>(val: Option<T>) -> T {
+    val.unwrap_or_else(|| intrinsics::abort())
+}


This is not necessary. Just let unwrap_unchecked panic in debug build, and let PanicGuard guarantee the safety. They do both end up aborting, but with unwrap_unchecked we have stack backtrace.

I saw it as an opportunity, not a necessity. The opportunity to avoid an unsafe call and to avoid the performance backlash of unwrap_unchecked, although in navigate.rs, benchmarks are not at all clear which of unwrap, unwrap_unchecked or unwrap_or_abort they think is faster.

As to a stack trace, the abort seems better to me, in my experience, on Windows, and I just confirmed on Linux. RUST_BACKTRACE seems completely useless in these double panic situations. The abort gives an illegal instruction, the debugger takes you right to the action at the top of the call stack, easier than if you have that unwinding kind of stuff on top.

The opportunity to avoid an unsafe call

Calling unwrap_unchecked is not unsafe anymore with PanicGuard.

RUST_BACKTRACE seems completely useless in these double panic situations.

Shouldn't be the case. I just confirmed RUST_BACKTRACE will produce backtrace just fine. Backtrace is generally preferred to just aborting when the situation allows, because it gives you more information without debugger.

avoid the performance backlash of unwrap_unchecked

unwrap_unchecked uses unreachable_unchecked(), and it is supposed to be faster. The current performance drawback is due to a bug in LLVM (#74615). If that get's fixed, unwrap_unchecked would be much faster than unwrap_or_abort.

Calling unwrap_unchecked is not unsafe anymore with PanicGuard.

I don't believe you. If there is nothing to unwrap, our call to unwrap_unchecked reaches UB and anything can happen. Not a problem, because the function whose body we're talking about is also tagged unsafe and documented to the caller when this could happen. But still unsafe in my book.

My point was that the call requires an unsafe block, even if there wasn't any UB lurking, so reading the code has a cognitive overhead. You could say that having to understand yet another variation on unwrap also has that overhead. Also, I struggle with the fact/perception that aborting is safe and UB, which includes having the luck to recover and the chance to save a user's work, is unsafe. Now I think that aborting, although classified as safe, is far worse than unsafe, so unwrap_or_abort is worse than unsafe { unwrap_unchecked }.

I just confirmed RUST_BACKTRACE will produce backtrace just fine.

Well in simple situations, sure. But do you mean in a double panic situation, in liballoc code as compiled by x.py with optimize=false, debug=true, debug-assertions=true?

Sorry, I misread your comment, I mean calling replace with is not unsafe. You're correct that's unsafe.

Abort is obvious safe, but it is definitely something to avoid if not necessary. For BTreeMap's case though, we know that the option is not None, so use unwrap_unchecked provide a good chance for LLVM to optimise things out (if there isn't a I-slow bug).

I do mean a double panic situation for backtrace. A very simple demonstration on playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4692ea8a7750c37bc3c55144e5fbb12c.

Good idea. I stuffed a slight variation:

struct P; impl Drop for P { fn drop(&mut self) { panic!(); } } fn main() { let _p = P; panic!(); } #[test] fn test_double_panic() { main() }

in library\alloc\tests\btree\extra.rs, hooked it up in mod.rs, and then when I rustc extra.rs && extra.exe, I have the trace. But up in the top directory, there is no trace for python x.py test --stage 0 library/alloc --test-args --test-threads=1 --test-args --format=pretty --test-args test_double_panic. One of the things it does let out is that the cargo command includes "--features" "panic-unwind backtrace compiler-builtins-c".

PS ok, duh, they are using different rust versions, but the direct rustc output behaves the same for every rustup channel.

Did you add RUST_BACKTRACE=1 and --test-args --nocapture?

--nocapture

🤦‍♂️

nbdd0121 · 2020-08-07T18:31:46Z

According to fickle benchmarks, the abort guarantee comes with a small price, but using intrinsics::abort instead of panic! turns it into a small gain for IntoIter.

Interesting, PanicGuard::drop should only be executed when the stack unwinds. I looked the assembly for a simple use-case and it doesn't see to have any difference for the normal path.

Mark-Simulacrum · 2020-08-07T18:40:55Z

@bors r+ rollup=never

I think there may be further improvements or discussion, but the safe variant used here seems good enough to me.

bors · 2020-08-07T18:40:57Z

📌 Commit 734fc04 has been approved by Mark-Simulacrum

ssomers · 2020-08-07T19:38:59Z

I looked the assembly for a simple use-case and it doesn't see to have any difference for the normal path.

Okay, but the pointer to the string argument of panic!() must go in the abnormal path, so code size is different, right? Which I suppose motivates the optimizer to (not) inline or reorder, crosses code page boundaries, makes the CPU caches hit sweet or sour spots and what not. I don't know for sure if that's even partially true, but it's enough for me to accept the occasional 10-20% changes in some benchmarks when they don't even touch the code I just changed.

bors · 2020-08-07T21:48:38Z

⌛ Testing commit 734fc04 with merge c2d1b0d...

bors · 2020-08-08T00:00:39Z

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

rust-highfive assigned Mark-Simulacrum Aug 2, 2020

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

pickfire reviewed Aug 3, 2020

View reviewed changes

ssomers force-pushed the btree_cleanup_5 branch 2 times, most recently from 82c6d95 to 06c84e7 Compare August 7, 2020 12:44

nbdd0121 suggested changes Aug 7, 2020

View reviewed changes

BTreeMap: enforce the panic rule imposed by replace

734fc04

ssomers force-pushed the btree_cleanup_5 branch from 06c84e7 to 734fc04 Compare August 7, 2020 17:51

ssomers changed the title ~~BTreeMap: respect the panic rule imposed by replace~~ BTreeMap: enforce the panic rule imposed by replace Aug 7, 2020

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 8, 2020

bors merged commit c2d1b0d into rust-lang:master Aug 8, 2020

This was referenced Aug 8, 2020

BTreeMap mutable iterators should not take any reference to visited nodes during iteration #73971

Merged

BTreeMap: introduce marker::ValMut and reserve Mut for unique access #75200

Merged

ssomers deleted the btree_cleanup_5 branch August 8, 2020 08:09

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BTreeMap: enforce the panic rule imposed by `replace` #75071

BTreeMap: enforce the panic rule imposed by `replace` #75071

ssomers commented Aug 2, 2020

pickfire Aug 3, 2020 •

edited

Loading

ssomers Aug 3, 2020 •

edited

Loading

pickfire Aug 3, 2020

Mark-Simulacrum Aug 3, 2020

ssomers Aug 3, 2020

pickfire Aug 4, 2020

ssomers Aug 5, 2020

pickfire Aug 6, 2020

ssomers Aug 6, 2020

ssomers Aug 11, 2020 •

edited

Loading

nbdd0121 commented Aug 7, 2020

ssomers commented Aug 7, 2020 •

edited

Loading

ssomers commented Aug 7, 2020

ssomers commented Aug 7, 2020 •

edited

Loading

nbdd0121 Aug 7, 2020

ssomers Aug 7, 2020

nbdd0121 Aug 7, 2020 •

edited

Loading

ssomers Aug 7, 2020 •

edited

Loading

nbdd0121 Aug 7, 2020

ssomers Aug 7, 2020 •

edited

Loading

nbdd0121 Aug 7, 2020

ssomers Aug 7, 2020

nbdd0121 commented Aug 7, 2020

Mark-Simulacrum commented Aug 7, 2020

bors commented Aug 7, 2020

ssomers commented Aug 7, 2020

bors commented Aug 7, 2020

bors commented Aug 8, 2020

BTreeMap: enforce the panic rule imposed by replace #75071

BTreeMap: enforce the panic rule imposed by replace #75071

Conversation

ssomers commented Aug 2, 2020

pickfire Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

ssomers Aug 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 Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

nbdd0121 commented Aug 7, 2020

ssomers commented Aug 7, 2020 • edited Loading

ssomers commented Aug 7, 2020

ssomers commented Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

ssomers Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssomers Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 commented Aug 7, 2020

Mark-Simulacrum commented Aug 7, 2020

bors commented Aug 7, 2020

ssomers commented Aug 7, 2020

bors commented Aug 7, 2020

bors commented Aug 8, 2020

BTreeMap: enforce the panic rule imposed by `replace` #75071

BTreeMap: enforce the panic rule imposed by `replace` #75071

pickfire Aug 3, 2020 •

edited

Loading

ssomers Aug 3, 2020 •

edited

Loading

ssomers Aug 11, 2020 •

edited

Loading

ssomers commented Aug 7, 2020 •

edited

Loading

ssomers commented Aug 7, 2020 •

edited

Loading

nbdd0121 Aug 7, 2020 •

edited

Loading

ssomers Aug 7, 2020 •

edited

Loading

ssomers Aug 7, 2020 •

edited

Loading