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.

Sign up for GitHub

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

Jump to bottom

Expose Frames Iterator for the Backtrace Type #78299

Closed
wants to merge 25 commits into from
Closed

Expose Frames Iterator for the Backtrace Type #78299

Changes from 3 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
16 changes: 15 additions & 1 deletion library/std/src/backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ fn _assert_send_sync() {
_assert::<Backtrace>();
}

struct BacktraceFrame {
#[unstable(feature = "backtrace_frames")]
pub struct BacktraceFrame {
frame: RawFrame,
symbols: Vec<BacktraceSymbol>,
}
Expand Down Expand Up @@ -346,6 +347,19 @@ impl Backtrace {
Inner::Captured(_) => BacktraceStatus::Captured,
}
}

/// Returns an iterator over the backtrace frames.
#[unstable(feature = "backtrace_frames")]
pub fn frames(&self) -> &[BacktraceFrame] {
let frames = match self.inner {
Inner::Captured(c) => {
let captured = c.lock().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a bit of an issue for us. We might end up needing to wrap this up in something like:

pub struct Frames<'a> {
    inner: Option<MutexGuard<'a, Capture>>,
}

impl<'a> AsRef<[BacktraceFrame]> for Frames<'a> {
    fn as_ref(&self) -> &[BacktraceFrame] {
        match self.inner {
            Some(captured) => &*captured.frames,
            None => &[],
        }
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason being the lifetime of the frames we can borrow from that locked mutex won't be enough for us to return a reference to the slice of frames.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this more, we probably don't want to hand out a quiet borrow to locked state, otherwise we might create the potential for deadlocks. Instead, we could come up with something like this:

pub struct Frames {
    inner: Vec<BacktraceFrame>,
}

impl<'a> AsRef<[BacktraceFrame]> for Frames<'a> {
    fn as_ref(&self) -> &[BacktraceFrame] {
        &self.inner
    }
}

And materialize that Vec under the lock.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not quite sure what you mean by "materialize that Vec under the lock". Is that just referring to the Frames type holding on to a Vec<BacktraceFrame> so that we don't need to manually deal with the lock?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, yes I meant making our frames() method look something like this:

impl Backtrace {
    pub fn frames(&self) -> Frames {
        if let Inner(captured) = self.inner {
            Frames {
                frames: captured.lock().unwrap().frames.clone(),
            }
        } else {
            Frames {
                frames: vec![],
            }
        }
    }
}

So that we're not handing out a borrow from that lock

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 Frames should have a lifetime tied to Backtrace even if it currently isn't used. In the future a pure rust unwinder may avoid the lock. We could then generate the frames on the fly in the Iterator impl for Frames, avoiding a memory allocation.

captured.frames
}
_ => vec![]
}
&frames
}
}

impl fmt::Display for Backtrace {
Expand Down