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

Add hint about the return code of panic! #42670

Merged
merged 4 commits into from
Jul 12, 2017

Conversation

dns2utf8
Copy link
Contributor

I hope the link works on all cases, since the unreachable doc is copied to std:: as well.

@rust-highfive
Copy link
Collaborator

r? @sfackler

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

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2017
@sfackler
Copy link
Member

r? @steveklabnik

cc @rust-lang/docs

@@ -24,6 +24,8 @@
/// The multi-argument form of this macro panics with a string and has the
/// `format!` syntax for building a string.
///
/// If the main thread panics it will return with code `101`.
Copy link
Member

Choose a reason for hiding this comment

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

Is this something that is guaranteed to happen across all platforms?

Copy link
Member

Choose a reason for hiding this comment

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

it's currently hard-coded; I don't know if we want to commit to this

@rust-lang/lang

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

This seems fine but I'm not sure we want to lock into 101

@@ -24,6 +24,8 @@
/// The multi-argument form of this macro panics with a string and has the
/// `format!` syntax for building a string.
///
/// If the main thread panics it will return with code `101`.
Copy link
Member

Choose a reason for hiding this comment

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

it's currently hard-coded; I don't know if we want to commit to this

@rust-lang/lang

@dns2utf8
Copy link
Contributor Author

Is there a particular reason for this return code?
I could add a hint like "this is an implementaiton detail and may change"

@frewsxcv
Copy link
Member

Something also to consider: although unusual, it's possible to catch panics, in which case this error code mention doesn't really apply

@withoutboats
Copy link
Contributor

Seems like we can't change the default, people may be relying on it. Someday maybe we could let you set the error code, no idea how though.

@arielb1 arielb1 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2017
@alexcrichton
Copy link
Member

This seems like a case where we want to document the current behavior but not commit to it. We've done this a few other places in libstd, can we say what happens today and then explicitly state that it may change in the future?

@steveklabnik
Copy link
Member

@alexcrichton yeah we've done

/// # Current Implementation

in the past, could do that here as well. See https://doc.rust-lang.org/stable/std/primitive.slice.html#current-implementation

@steveklabnik
Copy link
Member

@dns2utf8 ping! Any chance to update this PR according to the most recent comments?

@aidanhs aidanhs added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 5, 2017
@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Jul 5, 2017

Yes, I will make it according to "current implementation"

@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Jul 5, 2017

I think setting the error code could be done with the macro. Maybe something like this:

panic!("my {} panic", "fancy" -> 23);

Where 23 would be the returned error code. The std::panic::set_hook would then also require the function to return a number.

@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Jul 5, 2017

I noticed something odd std::sys::unix::abort_internal() is implemented with libc::abort() which just exports the function from libc.

Using the following program but the return code is 134. Do you know why?

#include <stdlib.h>

int main() {

	// this will make the programm return 134 on linux-amd64 platform
	abort();

	return 42;
}

@joshtriplett
Copy link
Member

@dns2utf8 134 is the return code for a SIGABRT:

/tmp$ (sleep 1000 ; echo return code: $?) &
[1] 3006
/tmp$ pidof sleep
3008
/tmp$ kill -ABRT 3008
bash: line 6:  3008 Aborted                 sleep 1000
return code: 134
[1]+  Done                    ( sleep 1000; echo return code: $? )

@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Jul 5, 2017

Interesting, I did not know that.
Should I rebase the PR before merge? Would you like me to change something else?

@arielb1 arielb1 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 Jul 11, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jul 11, 2017

You have an overly-long line, please fix:

[00:02:23] tidy error: /checkout/src/libstd/macros.rs:29: line longer than 100 chars

I'll also ping @steveklabnik for a review.

@dns2utf8
Copy link
Contributor Author

Cool, thank you.

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jul 11, 2017

📌 Commit 133c1bc has been approved by steveklabnik

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 12, 2017
…klabnik

Add hint about the return code of panic!

I hope the link works on all cases, since the `unreachable` doc is copied to `std::` as well.
bors added a commit that referenced this pull request Jul 12, 2017
Rollup of 8 pull requests

- Successful merges: #42670, #42826, #43000, #43011, #43098, #43100, #43136, #43137
- Failed merges:
@bors bors merged commit 133c1bc into rust-lang:master Jul 12, 2017
@dns2utf8 dns2utf8 deleted the panic_return_code branch July 13, 2017 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.