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

Format symbols under shared frames #79692

Closed
wants to merge 2 commits into from
Closed

Format symbols under shared frames #79692

wants to merge 2 commits into from

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Dec 4, 2020

Part of #71706
Also related to #72981

This changes the format used for backtraces in the currently unstable std::backtrace to align them with backtraces produced by panic!. It involves un-nesting symbols that share frames in std::backtrace to appear as if they belong under individual frames.

before:

  14: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
      std::panicking::try::do_call
             at src/libstd/panicking.rs:297
      std::panicking::try
             at src/libstd/panicking.rs:274
      std::panic::catch_unwind
             at src/libstd/panic.rs:394
      std::rt::lang_start_internal
             at src/libstd/rt.rs:51

after:

  14: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  15: std::panicking::try::do_call
             at src/libstd/panicking.rs:297
  16: std::panicking::try
             at src/libstd/panicking.rs:274
  17: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  18: std::rt::lang_start_internal
             at src/libstd/rt.rs:51

Since we don't share too much code between these formats right now or really check the Display formats for backtraces, they might end up diverging again.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2020
@jyn514 jyn514 added A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 4, 2020
@KodrAus
Copy link
Contributor Author

KodrAus commented Dec 4, 2020

Ah! Looks like we do have a bit of coverage for the panic format afterall 👀

@cramertj
Copy link
Member

cramertj commented Dec 8, 2020

This looks reasonable to me, but it looks like CI is unhappy? r=me with that fixed.

@KodrAus
Copy link
Contributor Author

KodrAus commented Dec 16, 2020

We discussed this at a recent Libs meeting and thought making these formats consistent the other way was preferable. So instead of changing the way frames from panic!s are printed to show symbols that share a frame differently, we change the currently unstable Backtrace format to not show symbols that share a frame differently.

@KodrAus
Copy link
Contributor Author

KodrAus commented Dec 21, 2020

I've gone ahead and updated the description here to reflect the actual change being made, which is to std::backtrace, and not to panic! backtraces.

This should be ready for another review now.

Also cc @rust-lang/project-error-handling

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

This comment has been minimized.

@KodrAus
Copy link
Contributor Author

KodrAus commented Jan 14, 2021

@cramertj Are you still happy with this with the change made to Backtrace instead of panic!?

@KodrAus
Copy link
Contributor Author

KodrAus commented Jan 19, 2021

r? @sfackler

@rust-highfive rust-highfive assigned sfackler and unassigned cramertj Jan 19, 2021
@sfackler
Copy link
Member

I'm a bit confused as to the state of the PR - it looks like now it's just a purely stylistic change to the code, with no actual behavioral difference?

@KodrAus
Copy link
Contributor Author

KodrAus commented Jan 19, 2021

@sfackler it’s a bit subtle, but the difference is that we move the call to .frame() within the loop so it’s called for each symbol, instead of for each frame. It slightly changes the output format of std::backtrace backtraces to look like panic! backtraces. There’s an example in the OP.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2021
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2021
@KodrAus
Copy link
Contributor Author

KodrAus commented Mar 18, 2021

I no longer have the bandwidth to carry this forward but if somebody else would like to pick it up sometime in the future then please feel free!

@KodrAus KodrAus closed this Mar 18, 2021
@yaahc
Copy link
Member

yaahc commented Mar 19, 2021

I'll take this one over

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 21, 2021
…r=yaahc

Reopen rust-lang#79692 (Format symbols under shared frames)

Reopening rust-lang#79692.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 21, 2021
…r=yaahc

Reopen rust-lang#79692 (Format symbols under shared frames)

Reopening rust-lang#79692.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 22, 2021
Rollup of 11 pull requests

Successful merges:

 - rust-lang#85054 (Revert SGX inline asm syntax)
 - rust-lang#85182 (Move `available_concurrency` implementation to `sys`)
 - rust-lang#86037 (Add `io::Cursor::{remaining, remaining_slice, is_empty}`)
 - rust-lang#86114 (Reopen rust-lang#79692 (Format symbols under shared frames))
 - rust-lang#86297 (Allow to pass arguments to rustdoc-gui tool)
 - rust-lang#86334 (Resolve type aliases to the type they point to in intra-doc links)
 - rust-lang#86367 (Fix comment about rustc_inherit_overflow_checks in abs().)
 - rust-lang#86381 (Add regression test for issue rust-lang#39161)
 - rust-lang#86387 (Remove `#[allow(unused_lifetimes)]` which is now unnecessary)
 - rust-lang#86398 (Add regression test for issue rust-lang#54685)
 - rust-lang#86493 (Say "this enum variant takes"/"this struct takes" instead of "this function takes")

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows PG-error-handling Project group: Error handling (https://github.com/rust-lang/project-error-handling) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants