Expose Frames Iterator for the Backtrace Type #78299

seanchen1991 · 2020-10-23T18:58:09Z

Adds the ability to iterate over the frames of a Backtrace by exposing the frames method.

rust-highfive · 2020-10-23T18:58:12Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

seanchen1991 · 2020-10-23T18:58:39Z

r? @KodrAus

KodrAus · 2020-10-24T05:06:08Z

cc @rust-lang/project-error-handling

KodrAus · 2020-10-24T05:10:17Z

Yep! These are the types I had in mind. So a few things we'll need to do are:

Fill in the frames method.
Determine semantics of frames when the backtrace hasn't actually been captured (should it be an empty slice? Or should frames return a Option<&[BacktraceFrame]>?) An empty slice seems reasonable to me.
Add some docs to the now public BacktraceFrame type and frames method.
Add some tests.

Once we're ready to go:

Create a tracking issue.
Update the #[rustc_unstable] attributes to point to it.

seanchen1991 · 2020-11-03T18:48:09Z

I think returning an empty slice makes sense if the backtrace hasn't been captured. One thing I'm not sure about is whether the captured frames need to be resolved. Or can the frames method simply return the captured frames after acquiring them from the mutex?

yaahc · 2020-11-06T01:46:26Z

I think returning an empty slice makes sense if the backtrace hasn't been captured. One thing I'm not sure about is whether the captured frames need to be resolved. Or can the frames method simply return the captured frames after acquiring them from the mutex?

My expectation is that it will need to trigger the frame resolution if it hasn't already happened.

KodrAus · 2020-11-06T09:45:32Z

@yaahc Sorry, I should've been more precise. I meant returning an empty slice makes sense if backtraces are unsupported.

KodrAus · 2020-11-06T09:50:52Z

library/std/src/backtrace.rs

+    pub fn frames(&self) -> &[BacktraceFrame] {
+        let frames = match self.inner {
+            Inner::Captured(c) => {
+               let captured = c.lock().unwrap();


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 => &[], } } }

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.

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.

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?

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

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.

KodrAus · 2020-11-12T20:44:55Z

We may want to look at the formatting use-case a bit more here too to figure out how we want to implement Debug and Display on Frames, and whether we want methods for slicing that return something displayable so that you could use this for limiting or filtering the number of frames to print.

Maybe we could implement Iterator for Frames and FromIterator for Backtrace, so that you could write:

println!("{}", bt.frames().take(5).collect::<Backtrace>());

seanchen1991 · 2020-11-12T21:26:54Z

We may want to look at the formatting use-case a bit more here too to figure out how we want to implement Debug and Display on Frames, and whether we want methods for slicing that return something displayable so that you could use this for limiting or filtering the number of frames to print.

Maybe we could implement Iterator for Frames and FromIterator for Backtrace, so that you could write:
println!("{}", bt.frames().take(5).collect::<Backtrace>());

Sounds good to me.

Separate question: It looks like tests go in the src/backtrace/tests.rs. As far as how we test this, would it be easier to wait until Debug and Display are implemented on Frames so that we can more easily inspect the contents of the frames iterator?

KodrAus · 2020-11-12T21:47:51Z

We'll want to think about the FromIterator implementation a bit more, because we can't mark trait implementations as unstable so if we added one to Backtrace we'd be committing to it right away. We can just focus on the Frames API for now and revisit it later on.

It looks like tests go in the src/backtrace/tests.rs. As far as how we test this, would it be easier to wait until Debug and Display are implemented on Frames so that we can more easily inspect the contents of the frames iterator?

Ah that src/backtrace/tests.rs module is actually a child module of src/backtrace.rs so we can just poke around Backtrace internals directly in there and don't have to rely on formatting unless we want to. That's all because the file structure of:

/
-   backtrace/
     -  tests.rs
-   backtrace.rs

is equivalent to:

/
-   backtrace/
    -   mod.rs
    -   tests.rs

If that makes sense 🙂

seanchen1991 · 2020-11-30T21:20:11Z

I'm working through compiler errors that I get when running ./x.py test library/std. In the frames method outlined above, the line Frames { inner: captured.lock().unwrap().frames.clone() } fails to compile since the BacktraceFrame type doesn't implement Clone.

Dropping in a bunch of #[derive(Clone)]s (specifically on the BacktraceFrame, RawFrame, BacktraceSymbol, and BytesOrWide types) does silence the compiler errors, but is that the way to go to address this issue?

rust-log-analyzer · 2020-12-17T22:01:35Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

GITHUB_ACTION=run6
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=seanchen1991
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_249d1ac0-901f-4495-acb2-144733715881
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=master
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_249d1ac0-901f-4495-acb2-144733715881
GITHUB_REF=refs/pull/78299/merge
GITHUB_REPOSITORY_OWNER=rust-lang
GITHUB_RETENTION_DAYS=90
GITHUB_RUN_ID=429132655
GITHUB_RUN_NUMBER=21583
---
Building wheels for collected packages: PyYAML
  Running setup.py bdist_wheel for PyYAML: started
  Running setup.py bdist_wheel for PyYAML: finished with status 'error'
  Failed building wheel for PyYAML
  Complete output from command /usr/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-cwqt4sc4/PyYAML/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" bdist_wheel -d /tmp/tmpw6r8snlkpip-wheel- --python-tag cp36:
     or: -c --help [cmd1 cmd2 ...]
     or: -c --help-commands
     or: -c cmd --help
  
---
    Checking hashbrown v0.9.0
    Checking miniz_oxide v0.4.0
    Checking object v0.22.0
    Checking addr2line v0.14.0
error[E0520]: `Item` specializes an item from a parent `impl`, but that item is not marked `default`
    |
    |
520 |     type Item = BacktraceFrame;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot specialize default item `Item`
    = note: parent implementation is in crate `core`


error[E0520]: `IntoIter` specializes an item from a parent `impl`, but that item is not marked `default`
    |
    |
521 |     type IntoIter = alloc_crate::vec::IntoIter<Self::Item>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot specialize default item `IntoIter`
    = note: parent implementation is in crate `core`


error[E0520]: `into_iter` specializes an item from a parent `impl`, but that item is not marked `default`
    |
523 | /     fn into_iter(self) -> Self::IntoIter {
524 | |         self.inner.into_iter()
525 | |     }
525 | |     }
    | |_____^ cannot specialize default item `into_iter`
    = note: parent implementation is in crate `core`

error: aborting due to 3 previous errors


For more information about this error, try `rustc --explain E0520`.
error: could not compile `std`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:40
== clock drift check ==
  local time: Thu Dec 17 22:01:02 UTC 2020

rust-log-analyzer · 2020-12-18T21:23:54Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

GITHUB_ACTION=run6
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=seanchen1991
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_17de49bb-0b2f-4129-83c3-4c1f4bc2f78c
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=master
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_17de49bb-0b2f-4129-83c3-4c1f4bc2f78c
GITHUB_REF=refs/pull/78299/merge
GITHUB_REPOSITORY_OWNER=rust-lang
GITHUB_RETENTION_DAYS=90
GITHUB_RUN_ID=431348371
GITHUB_RUN_NUMBER=21640
---
Collecting six>=1.5 (from python-dateutil<3.0.0,>=2.1; python_version >= "2.7"->botocore==1.12.197->awscli)
Building wheels for collected packages: PyYAML
  Running setup.py bdist_wheel for PyYAML: started
  Running setup.py bdist_wheel for PyYAML: finished with status 'error'
  Complete output from command /usr/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-px0rgmtz/PyYAML/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" bdist_wheel -d /tmp/tmpcfnsw5e5pip-wheel- --python-tag cp36:
     or: -c --help [cmd1 cmd2 ...]
     or: -c --help-commands
     or: -c cmd --help
  
---
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Checking which error codes lack tests...
Found 434 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/library/std/src/backtrace.rs:158: trailing whitespace
tidy error: /checkout/library/std/src/backtrace.rs:512: trailing whitespace
tidy error: /checkout/library/std/src/backtrace.rs:514: trailing whitespace
tidy error: /checkout/library/std/src/backtrace.rs: too many trailing newlines (2)
tidy error: /checkout/library/std/src/backtrace/tests.rs:73: trailing whitespace
some tidy checks failed

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"
expected success, got: exit code: 1

KodrAus

Thanks for working on this @seanchen1991!

I think the Mutex<Capture> is giving us a little trouble here. Would you be happy to try replacing it with a SyncLazy<Capture>? That way we can safely return borrows from it without the potential for deadlocks. The first caller that attempts to access the capture will perform the work of capturing it.

KodrAus · 2020-12-19T01:37:42Z

library/std/src/backtrace.rs

+        if let Inner::Captured(captured) = &self.inner {
+            let frames = &captured.lock().unwrap().frames;
+            Frames {
+                inner: frames.iter().map(|frame| frame.clone()).collect::<Vec<BacktraceFrame>>(),


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(),

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?

KodrAus · 2020-12-19T02:10:17Z

library/std/src/backtrace.rs

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


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.

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

KodrAus · 2021-01-06T00:24:55Z

I've opened #80736 which avoids using Mutex for synchronizing the lazy resolution, but doesn't go as far as resolving individual frames lazily yet. It's at least a bit of a semantic cleanup so should give us a clearer idea of how to do that later.

KodrAus · 2021-01-06T01:22:42Z

cc @rust-lang/project-error-handling

use Once instead of Mutex to manage capture resolution For rust-lang#78299 This allows us to return borrows of the captured backtrace frames that are tied to a borrow of the Backtrace itself, instead of to some short-lived Mutex guard. We could alternatively share `&Mutex<Capture>`s and lock on-demand, but then we could potentially forget to call `resolve()` before working with the capture. It also makes it semantically clearer what synchronization is needed on the capture. cc `@seanchen1991` `@rust-lang/project-error-handling`

bors · 2021-01-13T07:48:47Z

☔ The latest upstream changes (presumably #80960) made this pull request unmergeable. Please resolve the merge conflicts.

rust-log-analyzer · 2021-01-13T20:44:51Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

extracting /checkout/obj/build/cache/2020-11-19/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:18

bjorn3 · 2021-01-13T20:58:06Z

library/std/src/backtrace.rs

@@ -476,3 +511,24 @@ impl RawFrame {
        }
    }
 }
+
+#[unstable(feature = "backtrace_frames", issue = "79676")]
+impl<'a> AsRef<[BacktraceFrame]> for Frames<'a> {


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

seanchen1991 · 2021-01-13T23:42:28Z

Closing this and re-opening from a clean branch.

seanchen1991 added 2 commits October 23, 2020 13:53

Expose BacktraceFrame as public

372ba21

Add empty frames method to Backtrace type

9b8e4fc

rust-highfive assigned m-ou-se Oct 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2020

rust-highfive assigned KodrAus and unassigned m-ou-se Oct 23, 2020

Fill in frames function

e07b25d

KodrAus reviewed Nov 6, 2020

View reviewed changes

seanchen1991 added 4 commits November 9, 2020 15:51

Add Frames struct

277a2a4

Add as_ref method for Frames type

b4e21e6

Remove Backtrace::frames method

43f2774

Remove unnecessary newlines

7331efe

KodrAus mentioned this pull request Nov 12, 2020

Tracking issue for RFC 2504, "Fix the Error trait" #53487

Open

9 tasks

Merge branch 'master' of github.com:rust-lang/rust

b43bcb6

Add frames method that doesn't borrow from lock

6df53a1

yaahc mentioned this pull request Nov 12, 2020

Add column number support to Backtrace #79002

Merged

seanchen1991 added 3 commits December 2, 2020 15:38

Add private clone methods to Frame and BacktraceFrame

37f4f13

Add additional unstable feature flags

747bb91

Fix a type in Frames::clone

c5d5912

seanchen1991 mentioned this pull request Dec 3, 2020

Tracking Issue for backtrace_frames #79676

Open

3 tasks

seanchen1991 added 9 commits December 3, 2020 15:47

Add tracking issue

30c5494

Fix ownership issues

4e6d2ef

Add doc comments to Frames and BacktraceFrame

e7df885

Derive Debug on Frames, BacktraceFrame, and RawFrame

61b198f

Tie Frames to Backtrace via PhantomData

b4175b1

Add generate_fake_backtrace fn to backtrace/tests.rs

b30f662

Add test for empty frames iterator

c624d22

Impl Iterator for Frames

de98734

Fix IntoIter type

da2e4a9

seanchen1991 added 2 commits December 18, 2020 11:51

Get empty iterator test passing

0199300

Add test to check frames iterator count

48e6a38

KodrAus reviewed Dec 19, 2020

View reviewed changes

Remove iterator impl on Backtrace Frame

ff81eb1

KodrAus mentioned this pull request Jan 6, 2021

use Once instead of Mutex to manage capture resolution #80736

Merged

KodrAus added the PG-error-handling Project group: Error handling (https://github.com/rust-lang/project-error-handling) label Jan 6, 2021

Merge upstream changes

43b9783

bjorn3 reviewed Jan 13, 2021

View reviewed changes

seanchen1991 closed this Jan 13, 2021

seanchen1991 mentioned this pull request Feb 8, 2021

Add Frames Iterator for Backtrace #81022

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose Frames Iterator for the Backtrace Type #78299

Expose Frames Iterator for the Backtrace Type #78299

seanchen1991 commented Oct 23, 2020

rust-highfive commented Oct 23, 2020

seanchen1991 commented Oct 23, 2020

KodrAus commented Oct 24, 2020

KodrAus commented Oct 24, 2020

seanchen1991 commented Nov 3, 2020

yaahc commented Nov 6, 2020

KodrAus commented Nov 6, 2020

KodrAus Nov 6, 2020

KodrAus Nov 6, 2020

KodrAus Nov 9, 2020

seanchen1991 Nov 9, 2020

KodrAus Nov 12, 2020

bjorn3 Dec 4, 2020

KodrAus commented Nov 12, 2020

seanchen1991 commented Nov 12, 2020

KodrAus commented Nov 12, 2020

seanchen1991 commented Nov 30, 2020

rust-log-analyzer commented Dec 17, 2020

rust-log-analyzer commented Dec 18, 2020

KodrAus left a comment

KodrAus Dec 19, 2020

seanchen1991 Jan 5, 2021

KodrAus Dec 19, 2020

bjorn3 Dec 19, 2020

KodrAus commented Jan 6, 2021

KodrAus commented Jan 6, 2021

bors commented Jan 13, 2021

rust-log-analyzer commented Jan 13, 2021

bjorn3 Jan 13, 2021

seanchen1991 commented Jan 13, 2021

-                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(),

Expose Frames Iterator for the Backtrace Type #78299

Expose Frames Iterator for the Backtrace Type #78299

Conversation

seanchen1991 commented Oct 23, 2020

rust-highfive commented Oct 23, 2020

seanchen1991 commented Oct 23, 2020

KodrAus commented Oct 24, 2020

KodrAus commented Oct 24, 2020

seanchen1991 commented Nov 3, 2020

yaahc commented Nov 6, 2020

KodrAus commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented Nov 12, 2020

seanchen1991 commented Nov 12, 2020

KodrAus commented Nov 12, 2020

seanchen1991 commented Nov 30, 2020

rust-log-analyzer commented Dec 17, 2020

rust-log-analyzer commented Dec 18, 2020

KodrAus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented Jan 6, 2021

KodrAus commented Jan 6, 2021

bors commented Jan 13, 2021

rust-log-analyzer commented Jan 13, 2021

Choose a reason for hiding this comment

seanchen1991 commented Jan 13, 2021