Skip to content

Format symbols under shared frames - #79692

Closed
KodrAus wants to merge 2 commits into
rust-lang:masterfrom
KodrAus:feat/panic-frame-fmt
Closed

Format symbols under shared frames#79692
KodrAus wants to merge 2 commits into
rust-lang:masterfrom
KodrAus:feat/panic-frame-fmt

Conversation

@KodrAus

@KodrAusKodrAus commented Dec 4, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

r? @cramertj

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2020
@jyn514jyn514 added A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows T-libs-api [DEPRECATED; DO NOT USE] labels Dec 4, 2020
@KodrAus

Copy link
Copy Markdown
ContributorAuthor

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

@cramertj

Copy link
Copy Markdown
Member

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

@KodrAus

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@KodrAusKodrAus 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
Copy Markdown
ContributorAuthor

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

@KodrAus

Copy link
Copy Markdown
ContributorAuthor

r? @sfackler

@sfackler

Copy link
Copy Markdown
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

KodrAus commented Jan 19, 2021

Copy link
Copy Markdown
ContributorAuthor

@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.

@JohnCSimonJohnCSimon 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
@camelidcamelid 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
@JohnCSimonJohnCSimon 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
Copy Markdown
ContributorAuthor

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!

@KodrAusKodrAus closed this Mar 18, 2021
@yaahc

Copy link
Copy Markdown
Member

I'll take this one over

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 21, 2021
@JohnTitorJohnTitor mentioned this pull request Jun 21, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 21, 2021
@JohnTitorJohnTitor mentioned this pull request Jun 21, 2021
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
github-actionsBot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
github-actionsBot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-runtimeArea: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflowsPG-error-handlingProject group: Error handling (https://github.com/rust-lang/project-error-handling)S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@KodrAus@rust-highfive@cramertj@sfackler@yaahc@JohnCSimon@jyn514@camelid