-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement RFC 1328 Custom Panic Handlers #30485
Conversation
I'm unfortunately not sure how to test the panic when modifying the handler during a panic behavior because compiletest is unhappy with a SIGABRT. |
cc #30449 |
Looks like there may be a legit travis failure |
@@ -21,6 +21,8 @@ use sync::{Arc, Mutex, RwLock}; | |||
use sys_common::unwind; | |||
use thread::Result; | |||
|
|||
pub use panicking::{take_handler, set_handler, PanicInfo, Location}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to define these in this module? They seems closely related to it at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to, but the panicking module needs access to all of the internals and the privacy doesn't work in a reasonable way. They could be moved in a world with rust-lang/rfcs#1422.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, that's fine for now
Could you also add some tests for cases like:
Some of these may be best as a run-pass instead of run-fail as well, and some may also require the "age-old trick" of exec-self-again and then looking at the return status of that. |
} | ||
|
||
/// Returns information about the location from which the panic originated, | ||
/// if available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this explain a bit more that although Some
is always returned today it's not guaranteed to always be available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
73a06ad
to
1c65e71
Compare
Something seems to be bogus with fat pointer comparisons. Does it not compare each component? This lldb output seems to imply that we should not be inside of this if block.
|
We might also want to test the interaction with double panics (including panic->panic->abort and panic->set_handler->panic->abort). |
Ok, it appears that comparison isn't working because the two fat pointers are ending up with different vtables, which is pretty weird but may or may not be intended. I guess I'll add an extra layer of indirection and store a |
776c25e
to
55de212
Compare
@alexcrichton Updated. |
unsafe { | ||
let lock = HANDLER_LOCK.write(); | ||
let old_handler = HANDLER; | ||
HANDLER = Handler::Custom(Box::into_raw(Box::new(handler))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Box::new
allocation here can probably move outside the lock
Just one nit but otherwise looks good to me, thanks @sfackler! r=me with that |
55de212
to
f1148a5
Compare
@bors r=alexcrichton |
📌 Commit f1148a5 has been approved by |
⌛ Testing commit f1148a5 with merge dd40b17... |
⛄ The build was interrupted to prioritize another pull request. |
⌛ Testing commit f1148a5 with merge 24580e6... |
⛄ The build was interrupted to prioritize another pull request. |
r? @alexcrichton