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

use Once instead of Mutex to manage capture resolution #80736

Merged
merged 1 commit into from
Jan 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 35 additions & 9 deletions library/std/src/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,12 @@ mod tests;
// a backtrace or actually symbolizing it.

use crate::backtrace_rs::{self, BytesOrWideString};
use crate::cell::UnsafeCell;
use crate::env;
use crate::ffi::c_void;
use crate::fmt;
use crate::sync::atomic::{AtomicUsize, Ordering::SeqCst};
use crate::sync::Mutex;
use crate::sync::Once;
use crate::sys_common::backtrace::{lock, output_filename};
use crate::vec::Vec;

Expand Down Expand Up @@ -132,7 +133,7 @@ pub enum BacktraceStatus {
enum Inner {
Unsupported,
Disabled,
Captured(Mutex<Capture>),
Captured(LazilyResolvedCapture),
}

struct Capture {
Expand Down Expand Up @@ -171,12 +172,11 @@ enum BytesOrWide {

impl fmt::Debug for Backtrace {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
let mut capture = match &self.inner {
let capture = match &self.inner {
Inner::Unsupported => return fmt.write_str("<unsupported>"),
Inner::Disabled => return fmt.write_str("<disabled>"),
Inner::Captured(c) => c.lock().unwrap(),
Inner::Captured(c) => c.force(),
};
capture.resolve();

let frames = &capture.frames[capture.actual_start..];

Expand Down Expand Up @@ -331,7 +331,7 @@ impl Backtrace {
let inner = if frames.is_empty() {
Inner::Unsupported
} else {
Inner::Captured(Mutex::new(Capture {
Inner::Captured(LazilyResolvedCapture::new(Capture {
actual_start: actual_start.unwrap_or(0),
frames,
resolved: false,
Expand All @@ -355,12 +355,11 @@ impl Backtrace {

impl fmt::Display for Backtrace {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
let mut capture = match &self.inner {
let capture = match &self.inner {
Inner::Unsupported => return fmt.write_str("unsupported backtrace"),
Inner::Disabled => return fmt.write_str("disabled backtrace"),
Inner::Captured(c) => c.lock().unwrap(),
Inner::Captured(c) => c.force(),
};
capture.resolve();

let full = fmt.alternate();
let (frames, style) = if full {
Expand Down Expand Up @@ -404,6 +403,33 @@ impl fmt::Display for Backtrace {
}
}

struct LazilyResolvedCapture {
sync: Once,
capture: UnsafeCell<Capture>,
}

impl LazilyResolvedCapture {
fn new(capture: Capture) -> Self {
LazilyResolvedCapture { sync: Once::new(), capture: UnsafeCell::new(capture) }
}

fn force(&self) -> &Capture {
self.sync.call_once(|| {
// SAFETY: This exclusive reference can't overlap with any others
// `Once` guarantees callers will block until this closure returns
// `Once` also guarantees only a single caller will enter this closure
unsafe { &mut *self.capture.get() }.resolve();
});

// SAFETY: This shared reference can't overlap with the exclusive reference above
unsafe { &*self.capture.get() }
}
}

// SAFETY: Access to the inner value is synchronized using a thread-safe `Once`
// So long as `Capture` is `Sync`, `LazilyResolvedCapture` is too
unsafe impl Sync for LazilyResolvedCapture where Capture: Sync {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not visible in the diff, but we do have a test to ensure Backtrace is Send + Sync


impl Capture {
fn resolve(&mut self) {
// If we're already resolved, nothing to do!
Expand Down
5 changes: 4 additions & 1 deletion library/std/src/backtrace/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use super::*;
#[test]
fn test_debug() {
let backtrace = Backtrace {
inner: Inner::Captured(Mutex::new(Capture {
inner: Inner::Captured(LazilyResolvedCapture::new(Capture {
actual_start: 1,
resolved: true,
frames: vec![
Expand Down Expand Up @@ -54,4 +54,7 @@ fn test_debug() {
\n]";

assert_eq!(format!("{:#?}", backtrace), expected);

// Format the backtrace a second time, just to make sure lazily resolved state is stable
assert_eq!(format!("{:#?}", backtrace), expected);
}