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

Expose Frames Iterator for the Backtrace Type #78299

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
372ba21
Expose BacktraceFrame as public
seanchen1991 Oct 23, 2020
9b8e4fc
Add empty `frames` method to Backtrace type
seanchen1991 Oct 23, 2020
e07b25d
Fill in `frames` function
seanchen1991 Nov 3, 2020
277a2a4
Add `Frames` struct
seanchen1991 Nov 9, 2020
b4e21e6
Add `as_ref` method for `Frames` type
seanchen1991 Nov 9, 2020
43f2774
Remove `Backtrace::frames` method
seanchen1991 Nov 9, 2020
7331efe
Remove unnecessary newlines
seanchen1991 Nov 9, 2020
b43bcb6
Merge branch 'master' of github.com:rust-lang/rust
seanchen1991 Nov 12, 2020
6df53a1
Add `frames` method that doesn't borrow from lock
seanchen1991 Nov 12, 2020
37f4f13
Add private clone methods to Frame and BacktraceFrame
seanchen1991 Dec 2, 2020
747bb91
Add additional unstable feature flags
seanchen1991 Dec 2, 2020
c5d5912
Fix a type in Frames::clone
seanchen1991 Dec 2, 2020
30c5494
Add tracking issue
seanchen1991 Dec 3, 2020
4e6d2ef
Fix ownership issues
seanchen1991 Dec 3, 2020
e7df885
Add doc comments to `Frames` and `BacktraceFrame`
seanchen1991 Dec 3, 2020
61b198f
Derive Debug on `Frames`, `BacktraceFrame`, and `RawFrame`
seanchen1991 Dec 3, 2020
b4175b1
Tie `Frames` to `Backtrace` via PhantomData
seanchen1991 Dec 4, 2020
b30f662
Add `generate_fake_backtrace` fn to backtrace/tests.rs
seanchen1991 Dec 17, 2020
c624d22
Add test for empty frames iterator
seanchen1991 Dec 17, 2020
de98734
Impl `Iterator` for Frames
seanchen1991 Dec 17, 2020
da2e4a9
Fix IntoIter type
seanchen1991 Dec 17, 2020
0199300
Get empty iterator test passing
seanchen1991 Dec 18, 2020
48e6a38
Add test to check frames iterator count
seanchen1991 Dec 18, 2020
ff81eb1
Remove iterator impl on Backtrace Frame
seanchen1991 Jan 5, 2021
43b9783
Merge upstream changes
seanchen1991 Jan 13, 2021
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
67 changes: 66 additions & 1 deletion library/std/src/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ use crate::sync::atomic::{AtomicUsize, Ordering::SeqCst};
use crate::sync::Mutex;
use crate::sys_common::backtrace::{lock, output_filename};
use crate::vec::Vec;
use crate::marker::PhantomData;

/// A captured OS thread stack backtrace.
///
Expand Down Expand Up @@ -146,23 +147,38 @@ fn _assert_send_sync() {
_assert::<Backtrace>();
}

struct BacktraceFrame {
/// A single frame of a backtrace.
#[derive(Debug)]
#[unstable(feature = "backtrace_frames", issue = "79676")]
pub struct BacktraceFrame {
frame: RawFrame,
symbols: Vec<BacktraceSymbol>,
}

/// An iterator over the frames of a backtrace, created
/// by the [`frames`] method on [`Backtrace`].
#[derive(Debug)]
#[unstable(feature = "backtrace_frames", issue = "79676")]
pub struct Frames<'a> {
inner: Vec<BacktraceFrame>,
_backtrace: PhantomData<&'a Backtrace>
}

#[derive(Debug, Clone)]
enum RawFrame {
Actual(backtrace_rs::Frame),
#[cfg(test)]
Fake,
}

#[derive(Clone)]
struct BacktraceSymbol {
name: Option<Vec<u8>>,
filename: Option<BytesOrWide>,
lineno: Option<u32>,
}

#[derive(Clone)]
enum BytesOrWide {
Bytes(Vec<u8>),
Wide(Vec<u16>),
Expand Down Expand Up @@ -348,6 +364,25 @@ impl Backtrace {
}
}

impl<'a> Backtrace {
/// Returns an iterator over the backtrace frames.
#[unstable(feature = "backtrace_frames", issue = "79676")]
pub fn frames(&self) -> Frames<'a> {
if let Inner::Captured(captured) = &self.inner {
let frames = &captured.lock().unwrap().frames;
Frames {
inner: frames.iter().map(|frame| frame.clone()).collect::<Vec<BacktraceFrame>>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
inner: frames.iter().map(|frame| frame.clone()).collect::<Vec<BacktraceFrame>>(),
// NOTE: Frames are cloned to avoid returning references to them under the lock
// This could be avoided using a `SyncLazy` to initialize an immutable set of frames
inner: frames.iter().map(BacktraceFrame::clone).collect(),

Copy link
Member Author

Choose a reason for hiding this comment

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

So with this change, what's the way to access the type inside of the SyncLazy? Before with the Mutex we had

if let Inner::Captured(captured) = &self.inner {
  let frames = &captured.lock().unwrap().frames;

What would we need to change the let frames line into to access the underlying frames?

_backtrace: PhantomData
}
} else {
Frames {
inner: vec![],
_backtrace: PhantomData
}
}
}
}

impl fmt::Display for Backtrace {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
let mut capture = match &self.inner {
Expand Down Expand Up @@ -443,3 +478,33 @@ impl RawFrame {
}
}
}

#[unstable(feature = "backtrace_frames", issue = "79676")]
impl<'a> AsRef<[BacktraceFrame]> for Frames<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

This would be incompatible with lazy resolving of backtraces. Only an Iterator would be compatible.

fn as_ref(&self) -> &[BacktraceFrame] {
&self.inner
}
}


#[unstable(feature = "backtrace_frames", issue = "79676")]
impl BacktraceFrame {
// Private clone method so that we don't expose a
// public BacktraceFrame.clone() by deriving Clone
fn clone(&self) -> Self {
BacktraceFrame {
frame: self.frame.clone(),
symbols: self.symbols.clone(),
}
}
}

#[unstable(feature = "backtrace_frames", issue = "79676")]
impl<'a> Iterator for Frames<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll want to avoid adding an Iterator impl here just yet, because ideally we'll want to yield &'a BacktraceFrames, but we can't do that until we borrow the frames from the Backtrace instead of cloning them.

Copy link
Member

Choose a reason for hiding this comment

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

I think in the future it should actually lazily resolve the frames as .next() is called to reduce peak memory usage.

type Item = BacktraceFrame;

fn next(&mut self) -> Option<BacktraceFrame> {
self.inner.pop()
}
}

35 changes: 31 additions & 4 deletions library/std/src/backtrace/tests.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use super::*;

#[test]
fn test_debug() {
let backtrace = Backtrace {
fn generate_fake_backtrace() -> Backtrace {
Backtrace {
inner: Inner::Captured(Mutex::new(Capture {
actual_start: 1,
resolved: true,
Expand Down Expand Up @@ -40,7 +39,12 @@ fn test_debug() {
},
],
})),
};
}
}

#[test]
fn test_debug() {
let backtrace = generate_fake_backtrace();

#[rustfmt::skip]
let expected = "Backtrace [\
Expand All @@ -51,3 +55,26 @@ fn test_debug() {

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

#[test]
fn test_empty_frames_iterator() {
let empty_backtrace = Backtrace {
inner: Inner::Captured(Mutex::new(Capture {
actual_start: 1,
resolved: true,
frames: vec![],
}))
};

let iter = empty_backtrace.frames();

assert_eq!(iter.count(), 0);
}

#[test]
fn test_frames_iterator() {
let backtrace = generate_fake_backtrace();
let iter = backtrace.frames();

assert_eq!(iter.count(), 3);
}