-
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
Add a fast path for std::thread::panicking
.
#72617
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -170,7 +170,7 @@ pub fn take_hook() -> Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send> { | |||||||||||||||||||||||||||||||||||||||||||||
fn default_hook(info: &PanicInfo<'_>) { | ||||||||||||||||||||||||||||||||||||||||||||||
// If this is a double panic, make sure that we print a backtrace | ||||||||||||||||||||||||||||||||||||||||||||||
// for this panic. Otherwise only print it if logging is enabled. | ||||||||||||||||||||||||||||||||||||||||||||||
let backtrace_env = if update_panic_count(0) >= 2 { | ||||||||||||||||||||||||||||||||||||||||||||||
let backtrace_env = if panic_count::get() >= 2 { | ||||||||||||||||||||||||||||||||||||||||||||||
RustBacktrace::Print(backtrace_rs::PrintFmt::Full) | ||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||
backtrace::rust_backtrace_env() | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -222,19 +222,56 @@ fn default_hook(info: &PanicInfo<'_>) { | |||||||||||||||||||||||||||||||||||||||||||||
#[cfg(not(test))] | ||||||||||||||||||||||||||||||||||||||||||||||
#[doc(hidden)] | ||||||||||||||||||||||||||||||||||||||||||||||
#[unstable(feature = "update_panic_count", issue = "none")] | ||||||||||||||||||||||||||||||||||||||||||||||
pub fn update_panic_count(amt: isize) -> usize { | ||||||||||||||||||||||||||||||||||||||||||||||
pub mod panic_count { | ||||||||||||||||||||||||||||||||||||||||||||||
use crate::cell::Cell; | ||||||||||||||||||||||||||||||||||||||||||||||
thread_local! { static PANIC_COUNT: Cell<usize> = Cell::new(0) } | ||||||||||||||||||||||||||||||||||||||||||||||
use crate::sync::atomic::{AtomicUsize, Ordering}; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Panic count for the current thread. | ||||||||||||||||||||||||||||||||||||||||||||||
thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = Cell::new(0) } | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Sum of panic counts from all threads. The purpose of this is to have | ||||||||||||||||||||||||||||||||||||||||||||||
// a fast path in `is_zero` (which is used by `panicking`). Access to | ||||||||||||||||||||||||||||||||||||||||||||||
// this variable can be always be done with relaxed ordering because | ||||||||||||||||||||||||||||||||||||||||||||||
// it is always guaranteed that, if `GLOBAL_PANIC_COUNT` is zero, | ||||||||||||||||||||||||||||||||||||||||||||||
// `LOCAL_PANIC_COUNT` will be zero. | ||||||||||||||||||||||||||||||||||||||||||||||
static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
pub fn increase() -> usize { | ||||||||||||||||||||||||||||||||||||||||||||||
GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed); | ||||||||||||||||||||||||||||||||||||||||||||||
LOCAL_PANIC_COUNT.with(|c| { | ||||||||||||||||||||||||||||||||||||||||||||||
let next = c.get() + 1; | ||||||||||||||||||||||||||||||||||||||||||||||
c.set(next); | ||||||||||||||||||||||||||||||||||||||||||||||
next | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
pub fn decrease() -> usize { | ||||||||||||||||||||||||||||||||||||||||||||||
GLOBAL_PANIC_COUNT.fetch_sub(1, Ordering::Relaxed); | ||||||||||||||||||||||||||||||||||||||||||||||
LOCAL_PANIC_COUNT.with(|c| { | ||||||||||||||||||||||||||||||||||||||||||||||
let next = c.get() - 1; | ||||||||||||||||||||||||||||||||||||||||||||||
c.set(next); | ||||||||||||||||||||||||||||||||||||||||||||||
next | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+249
to
+254
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
rust/src/libstd/thread/local.rs Lines 235 to 243 in 229e5b2
This will likely result in a double panic and thus process abort, but still. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds like it is worth a comment in the code. |
||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
PANIC_COUNT.with(|c| { | ||||||||||||||||||||||||||||||||||||||||||||||
let next = (c.get() as isize + amt) as usize; | ||||||||||||||||||||||||||||||||||||||||||||||
c.set(next); | ||||||||||||||||||||||||||||||||||||||||||||||
next | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
pub fn get() -> usize { | ||||||||||||||||||||||||||||||||||||||||||||||
LOCAL_PANIC_COUNT.with(|c| c.get()) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
pub fn is_zero() -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the The current call stack is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, that should probably be inline too. |
||||||||||||||||||||||||||||||||||||||||||||||
if GLOBAL_PANIC_COUNT.load(Ordering::Relaxed) == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||
// Fast path: if `GLOBAL_PANIC_COUNT` is zero, all threads | ||||||||||||||||||||||||||||||||||||||||||||||
// (including the current one) will have `LOCAL_PANIC_COUNT` | ||||||||||||||||||||||||||||||||||||||||||||||
// equal to zero, so TLS access can be avoided. | ||||||||||||||||||||||||||||||||||||||||||||||
true | ||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||
LOCAL_PANIC_COUNT.with(|c| c.get() == 0) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
#[cfg(test)] | ||||||||||||||||||||||||||||||||||||||||||||||
pub use realstd::rt::update_panic_count; | ||||||||||||||||||||||||||||||||||||||||||||||
pub use realstd::rt::panic_count; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/// Invoke a closure, capturing the cause of an unwinding panic if one occurs. | ||||||||||||||||||||||||||||||||||||||||||||||
pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>> { | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -284,7 +321,7 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>> | |||||||||||||||||||||||||||||||||||||||||||||
#[cold] | ||||||||||||||||||||||||||||||||||||||||||||||
unsafe fn cleanup(payload: *mut u8) -> Box<dyn Any + Send + 'static> { | ||||||||||||||||||||||||||||||||||||||||||||||
let obj = Box::from_raw(__rust_panic_cleanup(payload)); | ||||||||||||||||||||||||||||||||||||||||||||||
update_panic_count(-1); | ||||||||||||||||||||||||||||||||||||||||||||||
panic_count::decrease(); | ||||||||||||||||||||||||||||||||||||||||||||||
obj | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -314,7 +351,7 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>> | |||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/// Determines whether the current thread is unwinding because of panic. | ||||||||||||||||||||||||||||||||||||||||||||||
pub fn panicking() -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||
update_panic_count(0) != 0 | ||||||||||||||||||||||||||||||||||||||||||||||
!panic_count::is_zero() | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/// The entry point for panicking with a formatted message. | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -452,7 +489,7 @@ fn rust_panic_with_hook( | |||||||||||||||||||||||||||||||||||||||||||||
message: Option<&fmt::Arguments<'_>>, | ||||||||||||||||||||||||||||||||||||||||||||||
location: &Location<'_>, | ||||||||||||||||||||||||||||||||||||||||||||||
) -> ! { | ||||||||||||||||||||||||||||||||||||||||||||||
let panics = update_panic_count(1); | ||||||||||||||||||||||||||||||||||||||||||||||
let panics = panic_count::increase(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// If this is the third nested call (e.g., panics == 2, this is 0-indexed), | ||||||||||||||||||||||||||||||||||||||||||||||
// the panic hook probably triggered the last panic, otherwise the | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -514,7 +551,7 @@ fn rust_panic_with_hook( | |||||||||||||||||||||||||||||||||||||||||||||
/// This is the entry point for `resume_unwind`. | ||||||||||||||||||||||||||||||||||||||||||||||
/// It just forwards the payload to the panic runtime. | ||||||||||||||||||||||||||||||||||||||||||||||
pub fn rust_panic_without_hook(payload: Box<dyn Any + Send>) -> ! { | ||||||||||||||||||||||||||||||||||||||||||||||
update_panic_count(1); | ||||||||||||||||||||||||||||||||||||||||||||||
panic_count::increase(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
struct RewrapBox(Box<dyn Any + Send>); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
|
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.
This is a very strong statement to make, in particular about relaxed atomics! It took me some time to actually follow the argument here. You cannot, in general, relate the content of two different locations when you use relaxed accesses. In particular, "
GLOBAL_PANIC_COUNT
is zero" is an odd statement as atomics can appear to have different values in different threads. The only reason it works here is that one of the two is thread-local. I was also confused at first becausedecrease
decrementsGLOBAL
first, so there is a time whenGLOBAL
is already zero for the current thread butLOCAL
is still 1.So what about
Also, it would be good to explain in a comment why accessing an atomic global (potentially subject to contention) is faster than accessing a non-atomic thread local. That is rather non-intuitive IMO -- I would have expected the exact opposite.
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.
Getting a TLS pointer requires calling
__tls_get_addr
for ELF binaries when using the GD TLS model. This is not inlinable and may walk a linked list.https://github.com/bminor/glibc/blob/5f72f9800b250410cad3abfeeb09469ef12b2438/elf/dl-tls.c#L821
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.
Yes, that would have been a better description. I wrote the comment thinking about the
is_zero
function, which is the only place where the value ofGLOBAL_PANIC_COUNT
matters.You can compare here the assembly of a relaxed atomic load and a TLS read. The atomic load just needs a normal
mov
.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.
Assembly is basically gibberish to me.^^ But that's okay, a high-level explanation like @bjorn3 gave is totally sufficient. :)
So could you open a PR to include these comments, or should I do that?
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'll do it now.