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

Add Frames Iterator for Backtrace #81022

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

seanchen1991
Copy link
Member

@seanchen1991 seanchen1991 commented Jan 14, 2021

Second attempt at adding the ability to iterate over the frames of a Backtrace by exposing the frames method.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 14, 2021
@seanchen1991
Copy link
Member Author

r? @KodrAus

@rust-log-analyzer
Copy link
Collaborator

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)
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 435 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/library/std/src/backtrace/tests.rs:75: trailing whitespace
tidy error: /checkout/library/std/src/backtrace/tests.rs:99: trailing whitespace
tidy error: /checkout/library/std/src/backtrace.rs:159: trailing whitespace
tidy error: /checkout/library/std/src/backtrace.rs:377: trailing whitespace
tidy error: /checkout/library/std/src/backtrace.rs:517: 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

@@ -147,11 +147,15 @@ fn _assert_send_sync() {
_assert::<Backtrace>();
}

struct BacktraceFrame {
/// A single frame of a backtrace.
#[derive(Debug)]
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 probably want to implement Debug manually for this type to match the format produced by Backtrace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that within scope for this PR? Or should that perhaps go in a separate one?

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 it would be within scope, since we're making this type public now.

@seanchen1991
Copy link
Member Author

seanchen1991 commented Jan 19, 2021

@KodrAus I added a Debug implementation for BacktraceFrame, adhering to Backtrace's Debug impl as best I could. One question I have is in the Backtrace impl, it has

for frame in frames {
    if frame.frame.ip().is_null() {
        continue;
    }

So some frames are skipped and not printed. I'm not sure what to do in the same situation for a single BacktraceFrame. We just not print anything in that case, right?

@KodrAus
Copy link
Contributor

KodrAus commented Jan 22, 2021

Thanks @seanchen1991! This looks good to me. The main thing I was looking at with the debug format was trying to make symbols look the same.

I think we're ready to merge this! Would you be happy to squash your commits down? r=me after that.

I did have one more thought on making the return type some BacktraceFames<'a> instead of &'a [BacktraceFrame], but we can explore that asynchronously in the tracking issue.

@yaahc
Copy link
Member

yaahc commented Feb 1, 2021

@bors r=KodrAus

@bors
Copy link
Contributor

bors commented Feb 1, 2021

@yaahc: 🔑 Insufficient privileges: Not in reviewers

@seanchen1991
Copy link
Member Author

@bors r=KodrAus

@bors
Copy link
Contributor

bors commented Feb 1, 2021

@seanchen1991: 🔑 Insufficient privileges: Not in reviewers

@KodrAus
Copy link
Contributor

KodrAus commented Feb 2, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 2, 2021

📌 Commit 050643a has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 2, 2021
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#80629 (Add lint for 2229 migrations)
 - rust-lang#81022 (Add Frames Iterator for Backtrace)
 - rust-lang#81481 (move some tests)
 - rust-lang#81485 (Add some tests for associated-type-bounds issues)
 - rust-lang#81492 (rustdoc: Note why `rustdoc::html::markdown` is public)
 - rust-lang#81577 (const_evaluatable: consider sub-expressions to be evaluatable)
 - rust-lang#81599 (Implement `TrustedLen` for `Fuse<I: TrustedLen>`)
 - rust-lang#81608 (Improve handling of spans around macro result parse errors)
 - rust-lang#81609 (Remove the remains of query categories)
 - rust-lang#81630 (Fix overflowing text on mobile when sidebar is displayed)
 - rust-lang#81631 (Remove unneeded `mut` variable)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f61ab58 into rust-lang:master Feb 2, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 2, 2021
impl<'a> Backtrace {
/// Returns an iterator over the backtrace frames.
#[unstable(feature = "backtrace_frames", issue = "79676")]
pub fn frames(&'a self) -> &'a [BacktraceFrame] {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better if this were an iterator? That could make lazily walking the stack cheaper at some point in the future.

E.g. Java introduced a StackWalker API to make partial stack traces cheaper.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was indeed discussed. There's some additional discussion and context that isn't apparent because it's in the previous iteration of this PR. Apologies for that.

You can read some of that discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants