Panic machinery comments and tweaks #66766

RalfJung · 2019-11-26T07:24:39Z

This is mostly more comments, but I also renamed some things:

BoxMeUp::box_me_up is not terribly descriptive, and since this is a "take"-style method (the argument is &mut self but the return type is fully owned, even though you can't tell from the type) I chose a name involving "take".
continue_panic_fmt was very confusing as it was entirely unclear what was being continued -- for some time I thought "continue" might be the same as "resume" for a panic, but that's something entirely different. So I renamed this to begin_panic_handler, matching the begin_panic* theme of the other entry points.

…andler] directly The "continue" in the name was really confusing; it sounds way too much like "resume" which is a totally different concept around panics.

RalfJung · 2019-11-26T07:25:15Z

src/libstd/panicking.rs

            let data = match self.inner.take() {
                Some(a) => Box::new(a) as Box<dyn Any + Send>,
-                None => Box::new(()),
+                None => Box::new(()), // this should never happen: we got called twice


I was wondering if it is appropriate to call process::abort() here?

Maybe. I feel it’s not very important since this is a private API that has exactly one call site.

Let me re-phrase my question: is there any reason not to do that? Is there any legitimate use of this interface that calls take_box twice?

Actually what I am wondering about even more is what the method does not take self by-value?

This is an impl for a private type PanicPayload, whose value is only ever constructed in one place. A pointer to that value is __rust_start_panic, which we can consider a private implementation detail of the standard library. So both the definition and all uses of this API (all one of them) are under our control. So it doesn’t matter in my opinion what the code does in situations that we know never happen. There is no reason to abort, and there is no reason not to abort. Feel free to change this to abort if you’d like, I don’t mind r+’ing that change.

This method cannot take self by value because it is (only, ever) called by src/libpanic_unwind/lib.rs’s __rust_start_panic which dereferences a raw pointer (cast from usize) to get &mut &mut dyn BoxMeUp. It cannot (easily) be an owned Box<dyn BoxMeUp> because libpanic_unwind doesn’t depend on liballoc which defines Box. Oh and for the trait to be object-safe it might have to be self: Box<Self> or something.

It cannot (easily) be an owned Box because libpanic_unwind doesn’t depend on liballoc which defines Box

Ah, thanks, I added a comment along those lines (and made it abort). Please check.

SimonSapin

"continue" made some sense to me since it was called after "begin", but I like this new structure better. Thanks!

r=me with one code comment tweak

SimonSapin · 2019-11-26T08:04:02Z

src/libstd/panicking.rs

@@ -459,7 +458,9 @@ fn rust_panic_with_hook(payload: &mut dyn BoxMeUp,
        match HOOK {
            // Some platforms know that printing to stderr won't ever actually
            // print anything, and if that's the case we can skip the default
-            // hook.
+            // hook. Since string formatting happens lazily when calling `payload`
+            // methods, this means that with libpanic_abort, we don't format


panic_output().is_none() does not indicate libpanic_abort. It indicates a target such as wasm that doesn’t have anything like stderr, so we can’t print a message at all.

Well, with libpanic_unwind we call take_box() which will still trigger the formatting.

Oh right, I see what you meant now.

SimonSapin · 2019-11-26T08:43:05Z

Looks good, thanks!

@bors r+

bors · 2019-11-26T08:43:06Z

📌 Commit 4a19ef9 has been approved by SimonSapin

RalfJung · 2019-11-26T09:25:25Z

@bors r-

@SimonSapin it was pointed out to me on reddit that I forgot about one entry point, so I proposed a better name for that one as well. What do you think? This mirrors rust_panic_with_hook.

Dylan-DPC-zz · 2019-11-26T10:40:02Z

r? @SimonSapin

SimonSapin · 2019-11-26T14:38:52Z

@bors r+

bors · 2019-11-26T14:38:53Z

📌 Commit babe9fc has been approved by SimonSapin

@Dylan-DPC

Panic machinery comments and tweaks This is mostly more comments, but I also renamed some things: * `BoxMeUp::box_me_up` is not terribly descriptive, and since this is a "take"-style method (the argument is `&mut self` but the return type is fully owned, even though you can't tell from the type) I chose a name involving "take". * `continue_panic_fmt` was very confusing as it was entirely unclear what was being continued -- for some time I thought "continue" might be the same as "resume" for a panic, but that's something entirely different. So I renamed this to `begin_panic_handler`, matching the `begin_panic*` theme of the other entry points. r? @Dylan-DPC @SimonSapin

@Dylan-DPC

Panic machinery comments and tweaks This is mostly more comments, but I also renamed some things: * `BoxMeUp::box_me_up` is not terribly descriptive, and since this is a "take"-style method (the argument is `&mut self` but the return type is fully owned, even though you can't tell from the type) I chose a name involving "take". * `continue_panic_fmt` was very confusing as it was entirely unclear what was being continued -- for some time I thought "continue" might be the same as "resume" for a panic, but that's something entirely different. So I renamed this to `begin_panic_handler`, matching the `begin_panic*` theme of the other entry points. r? @Dylan-DPC @SimonSapin

@ghost

Rollup of 11 pull requests Successful merges: - #66379 (Rephrase docs in for ptr) - #66589 (Draw vertical lines correctly in compiler error messages) - #66613 (Allow customising ty::TraitRef's printing behavior) - #66766 (Panic machinery comments and tweaks) - #66791 (Handle GlobalCtxt directly from librustc_interface query system) - #66793 (Record temporary static references in generator witnesses) - #66808 (Cleanup error code) - #66826 (Clarifies how to tag users for assigning PRs) - #66837 (Clarify `{f32,f64}::EPSILON` docs) - #66844 (Miri: do not consider memory allocated by caller_location leaked) - #66872 (Minor documentation fix) Failed merges: r? @ghost

RalfJung added 4 commits November 25, 2019 12:14

Rename continue_panic_fmt to panic_handler, and make it the #[panic_h…

cd5d0c7

…andler] directly The "continue" in the name was really confusing; it sounds way too much like "resume" which is a totally different concept around panics.

better comment and rename BoxMeUp::box_me_up to take_box

08f779c

more panicking comments

3c48579

panic_handler -> begin_panic_handler (and more comments)

3a8e1b6

rust-highfive assigned Dylan-DPC-zz Nov 26, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2019

RalfJung commented Nov 26, 2019

View reviewed changes

RalfJung mentioned this pull request Nov 26, 2019

[WIP] Add panic implementation docs rust-lang/rustc-dev-guide#521

Merged

SimonSapin reviewed Nov 26, 2019

View reviewed changes

RalfJung added 3 commits November 26, 2019 09:24

abort on BoxMeUp misuse

3e96ca2

expand comment

61486f4

explain why __rust_start_panic does not take a Box

4a19ef9

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 Nov 26, 2019

rename update_count_then_panic -> rust_panic_without_hook

babe9fc

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 26, 2019

rust-highfive assigned SimonSapin and unassigned Dylan-DPC-zz Nov 26, 2019

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 26, 2019

RalfJung mentioned this pull request Nov 29, 2019

Rollup of 11 pull requests #66875

Closed

RalfJung mentioned this pull request Nov 29, 2019

Rollup of 11 pull requests #66879

Merged

bors merged commit babe9fc into rust-lang:master Nov 30, 2019

RalfJung deleted the panic-comments branch November 30, 2019 07:50

syvb mentioned this pull request Jan 29, 2024

Update uses of renamed BoxMeUp to PanicPayload rust-lang/rustc-dev-guide#1878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panic machinery comments and tweaks #66766

Panic machinery comments and tweaks #66766

RalfJung commented Nov 26, 2019

RalfJung Nov 26, 2019

SimonSapin Nov 26, 2019

RalfJung Nov 26, 2019

RalfJung Nov 26, 2019

SimonSapin Nov 26, 2019

RalfJung Nov 26, 2019

SimonSapin left a comment

SimonSapin Nov 26, 2019

RalfJung Nov 26, 2019

SimonSapin Nov 26, 2019

SimonSapin commented Nov 26, 2019

bors commented Nov 26, 2019

RalfJung commented Nov 26, 2019

Dylan-DPC-zz commented Nov 26, 2019

SimonSapin commented Nov 26, 2019

bors commented Nov 26, 2019

Panic machinery comments and tweaks #66766

Panic machinery comments and tweaks #66766

Conversation

RalfJung commented Nov 26, 2019

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

SimonSapin left a 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

SimonSapin commented Nov 26, 2019

bors commented Nov 26, 2019

RalfJung commented Nov 26, 2019

Dylan-DPC-zz commented Nov 26, 2019

SimonSapin commented Nov 26, 2019

bors commented Nov 26, 2019