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

Improve comments for the default backtrace printer #133882

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 5, 2024

The existing comments were misleading, confusing, and outdated.

Take this comment for example:

// Any frames between `__rust_begin_short_backtrace` and `__rust_end_short_backtrace`
// are omitted from the backtrace in short mode, `__rust_end_short_backtrace` will be
// called before the panic hook, so we won't ignore any frames if there is no
// invoke of `__rust_begin_short_backtrace`.

this is just wrong. here is an example (full) backtrace:

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s
     Running `/home/jyn/.local/lib/cargo/target/debug/example`
called `Option::unwrap()` on a `None` value
stack backtrace:
   0:     0x56499698c595 - std::backtrace_rs::backtrace::libunwind::trace::h5ef2cc16e9a7415a
   1:     0x56499698c595 - std::backtrace_rs::backtrace::trace_unsynchronized::h9b5e016e9075f714
   2:     0x56499698c595 - std::sys_common::backtrace::_print_fmt::h2f62c7f9ff224e93
   3:     0x56499698c595 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hbe51682735731910
   4:     0x5649969aa26b - core::fmt::rt::Argument::fmt::h1994ab2b310d665e
   5:     0x5649969aa26b - core::fmt::write::hade58a36d63468d7
   6:     0x56499698a43f - std::io::Write::write_fmt::h16145587d801a9ab
   7:     0x56499698c36e - std::sys_common::backtrace::_print::ha8082e56201dadb4
   8:     0x56499698c36e - std::sys_common::backtrace::print::he30f96b4e7f6cbfd
   9:     0x56499698d709 - std::panicking::default_hook::{{closure}}::hf0801f6b18a968d3
  10:     0x56499698d4ac - std::panicking::default_hook::hd2defec7eda5aeb0
  11:     0x56499698dc31 - std::panicking::rust_panic_with_hook::hde93283600065c53
  12:     0x56499698daf3 - std::panicking::begin_panic_handler::{{closure}}::h5e151adbdb7ec0c1
  13:     0x56499698ca59 - std::sys_common::backtrace::__rust_end_short_backtrace::he36a1407e0f77700
  14:     0x56499698d7d4 - rust_begin_unwind
  15:     0x5649969a9503 - core::panicking::panic_fmt::h2380d41365f95412
  16:     0x5649969a958c - core::panicking::panic::h38cf8db80e8c6e67
  17:     0x5649969a93e9 - core::option::unwrap_failed::he72696e53ff29a05
  18:     0x5649969722b6 - core::option::Option<T>::unwrap::hb574dc0dc1703062
  19:     0x5649969722b6 - example::main::h7a867aafacd93d75
  20:     0x5649969721db - core::ops::function::FnOnce::call_once::h734f99a5e57291b7
  21:     0x56499697226e - std::sys_common::backtrace::__rust_begin_short_backtrace::h02f5d58c351c4756
  22:     0x564996972241 - std::rt::lang_start::{{closure}}::h8b134fe2c31a4355
  23:     0x564996988662 - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h88d7bb571ee2aaf4
  24:     0x564996988662 - std::panicking::try::do_call::hfb78dfb6599c871d
  25:     0x564996988662 - std::panicking::try::habd041c8c4c8e50c
  27:     0x564996988662 - std::rt::lang_start_internal::{{closure}}::h227591a6f9c0879e
  28:     0x564996988662 - std::panicking::try::do_call::h3c5878333c38916a
  29:     0x564996988662 - std::panicking::try::h5af7b3a127cdae70
  31:     0x564996988662 - std::rt::lang_start_internal::hbc85e809eeace0dd
  32:     0x56499697221a - std::rt::lang_start::ha1eb16922c9cb224
  33:     0x5649969722ee - main
  34:     0x7f031962a1ca - __libc_start_call_main
  35:     0x7f031962a28b - __libc_start_main_impl
  36:     0x5649969720a5 - _start
  37:                0x0 - <unknown>

note particularly frames 13-21, from start_backtrace to end_backtrace. with PrintFmt::Short, these are the only frames that are printed; i.e. we are doing the exact opposite of the comment.

r? @saethlin

The existing comments were misleading, confusing, and wrong.

Take this comment for example:
```
// Any frames between `__rust_begin_short_backtrace` and `__rust_end_short_backtrace`
// are omitted from the backtrace in short mode, `__rust_end_short_backtrace` will be
// called before the panic hook, so we won't ignore any frames if there is no
// invoke of `__rust_begin_short_backtrace`.
```

this is just wrong. here is an example (full) backtrace:

```
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s
     Running `/home/jyn/.local/lib/cargo/target/debug/example`
called `Option::unwrap()` on a `None` value
stack backtrace:
   0:     0x56499698c595 - std::backtrace_rs::backtrace::libunwind::trace::h5ef2cc16e9a7415a
   1:     0x56499698c595 - std::backtrace_rs::backtrace::trace_unsynchronized::h9b5e016e9075f714
   2:     0x56499698c595 - std::sys_common::backtrace::_print_fmt::h2f62c7f9ff224e93
   3:     0x56499698c595 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hbe51682735731910
   4:     0x5649969aa26b - core::fmt::rt::Argument::fmt::h1994ab2b310d665e
   5:     0x5649969aa26b - core::fmt::write::hade58a36d63468d7
   6:     0x56499698a43f - std::io::Write::write_fmt::h16145587d801a9ab
   7:     0x56499698c36e - std::sys_common::backtrace::_print::ha8082e56201dadb4
   8:     0x56499698c36e - std::sys_common::backtrace::print::he30f96b4e7f6cbfd
   9:     0x56499698d709 - std::panicking::default_hook::{{closure}}::hf0801f6b18a968d3
  10:     0x56499698d4ac - std::panicking::default_hook::hd2defec7eda5aeb0
  11:     0x56499698dc31 - std::panicking::rust_panic_with_hook::hde93283600065c53
  12:     0x56499698daf3 - std::panicking::begin_panic_handler::{{closure}}::h5e151adbdb7ec0c1
  13:     0x56499698ca59 - std::sys_common::backtrace::__rust_end_short_backtrace::he36a1407e0f77700
  14:     0x56499698d7d4 - rust_begin_unwind
  15:     0x5649969a9503 - core::panicking::panic_fmt::h2380d41365f95412
  16:     0x5649969a958c - core::panicking::panic::h38cf8db80e8c6e67
  17:     0x5649969a93e9 - core::option::unwrap_failed::he72696e53ff29a05
  18:     0x5649969722b6 - core::option::Option<T>::unwrap::hb574dc0dc1703062
  19:     0x5649969722b6 - example::main::h7a867aafacd93d75
  20:     0x5649969721db - core::ops::function::FnOnce::call_once::h734f99a5e57291b7
  21:     0x56499697226e - std::sys_common::backtrace::__rust_begin_short_backtrace::h02f5d58c351c4756
  22:     0x564996972241 - std::rt::lang_start::{{closure}}::h8b134fe2c31a4355
  23:     0x564996988662 - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h88d7bb571ee2aaf4
  24:     0x564996988662 - std::panicking::try::do_call::hfb78dfb6599c871d
  25:     0x564996988662 - std::panicking::try::habd041c8c4c8e50c
  27:     0x564996988662 - std::rt::lang_start_internal::{{closure}}::h227591a6f9c0879e
  28:     0x564996988662 - std::panicking::try::do_call::h3c5878333c38916a
  29:     0x564996988662 - std::panicking::try::h5af7b3a127cdae70
  31:     0x564996988662 - std::rt::lang_start_internal::hbc85e809eeace0dd
  32:     0x56499697221a - std::rt::lang_start::ha1eb16922c9cb224
  33:     0x5649969722ee - main
  34:     0x7f031962a1ca - __libc_start_call_main
  35:     0x7f031962a28b - __libc_start_main_impl
  36:     0x5649969720a5 - _start
  37:                0x0 - <unknown>
```

note particularly frames 13-21, from start_backtrace to end_backtrace. with PrintFmt::Short, these are the *only* frames that are printed; i.e. we are doing the exact opposite of the comment.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 5, 2024
@saethlin
Copy link
Member

saethlin commented Dec 5, 2024

This makes so much more sense. It's a bummer that we still start printing at the end_backtrace frame and stop printing at the start_backtrace frame, I'd expect their names to be flipped but such a change would be much more invasive, and possibly annoy some users.

❤️

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Dec 5, 2024

📌 Commit 736c61e has been approved by saethlin

It is now in the queue for this repository.

@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 Dec 5, 2024
@saethlin
Copy link
Member

saethlin commented Dec 5, 2024

@bors rollup=always

I think I am more tired than I thought

@jyn514
Copy link
Member Author

jyn514 commented Dec 5, 2024

bors rollup=always

I think I am more tired than I thought

you’re just keeping a consistent style with the existing codebase

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127565 (Teach rustc about the Xtensa VaListImpl)
 - rust-lang#133844 (clarify simd_relaxed_fma non-determinism)
 - rust-lang#133867 (Fix "std" support status of some tier 3 targets)
 - rust-lang#133882 (Improve comments for the default backtrace printer)
 - rust-lang#133888 (Improve bootstrap job objects)
 - rust-lang#133898 (skip `setup::Hook` on non-git sources)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127565 (Teach rustc about the Xtensa VaListImpl)
 - rust-lang#133844 (clarify simd_relaxed_fma non-determinism)
 - rust-lang#133867 (Fix "std" support status of some tier 3 targets)
 - rust-lang#133882 (Improve comments for the default backtrace printer)
 - rust-lang#133888 (Improve bootstrap job objects)
 - rust-lang#133898 (skip `setup::Hook` on non-git sources)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit aaea63e into rust-lang:master Dec 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
Rollup merge of rust-lang#133882 - jyn514:doc-backtraces, r=saethlin

Improve comments for the default backtrace printer

The existing comments were misleading, confusing, and outdated.

Take this comment for example:
```
// Any frames between `__rust_begin_short_backtrace` and `__rust_end_short_backtrace`
// are omitted from the backtrace in short mode, `__rust_end_short_backtrace` will be
// called before the panic hook, so we won't ignore any frames if there is no
// invoke of `__rust_begin_short_backtrace`.
```

this is just wrong. here is an example (full) backtrace:

<details>

```
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s
     Running `/home/jyn/.local/lib/cargo/target/debug/example`
called `Option::unwrap()` on a `None` value
stack backtrace:
   0:     0x56499698c595 - std::backtrace_rs::backtrace::libunwind::trace::h5ef2cc16e9a7415a
   1:     0x56499698c595 - std::backtrace_rs::backtrace::trace_unsynchronized::h9b5e016e9075f714
   2:     0x56499698c595 - std::sys_common::backtrace::_print_fmt::h2f62c7f9ff224e93
   3:     0x56499698c595 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hbe51682735731910
   4:     0x5649969aa26b - core::fmt::rt::Argument::fmt::h1994ab2b310d665e
   5:     0x5649969aa26b - core::fmt::write::hade58a36d63468d7
   6:     0x56499698a43f - std::io::Write::write_fmt::h16145587d801a9ab
   7:     0x56499698c36e - std::sys_common::backtrace::_print::ha8082e56201dadb4
   8:     0x56499698c36e - std::sys_common::backtrace::print::he30f96b4e7f6cbfd
   9:     0x56499698d709 - std::panicking::default_hook::{{closure}}::hf0801f6b18a968d3
  10:     0x56499698d4ac - std::panicking::default_hook::hd2defec7eda5aeb0
  11:     0x56499698dc31 - std::panicking::rust_panic_with_hook::hde93283600065c53
  12:     0x56499698daf3 - std::panicking::begin_panic_handler::{{closure}}::h5e151adbdb7ec0c1
  13:     0x56499698ca59 - std::sys_common::backtrace::__rust_end_short_backtrace::he36a1407e0f77700
  14:     0x56499698d7d4 - rust_begin_unwind
  15:     0x5649969a9503 - core::panicking::panic_fmt::h2380d41365f95412
  16:     0x5649969a958c - core::panicking::panic::h38cf8db80e8c6e67
  17:     0x5649969a93e9 - core::option::unwrap_failed::he72696e53ff29a05
  18:     0x5649969722b6 - core::option::Option<T>::unwrap::hb574dc0dc1703062
  19:     0x5649969722b6 - example::main::h7a867aafacd93d75
  20:     0x5649969721db - core::ops::function::FnOnce::call_once::h734f99a5e57291b7
  21:     0x56499697226e - std::sys_common::backtrace::__rust_begin_short_backtrace::h02f5d58c351c4756
  22:     0x564996972241 - std::rt::lang_start::{{closure}}::h8b134fe2c31a4355
  23:     0x564996988662 - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h88d7bb571ee2aaf4
  24:     0x564996988662 - std::panicking::try::do_call::hfb78dfb6599c871d
  25:     0x564996988662 - std::panicking::try::habd041c8c4c8e50c
  27:     0x564996988662 - std::rt::lang_start_internal::{{closure}}::h227591a6f9c0879e
  28:     0x564996988662 - std::panicking::try::do_call::h3c5878333c38916a
  29:     0x564996988662 - std::panicking::try::h5af7b3a127cdae70
  31:     0x564996988662 - std::rt::lang_start_internal::hbc85e809eeace0dd
  32:     0x56499697221a - std::rt::lang_start::ha1eb16922c9cb224
  33:     0x5649969722ee - main
  34:     0x7f031962a1ca - __libc_start_call_main
  35:     0x7f031962a28b - __libc_start_main_impl
  36:     0x5649969720a5 - _start
  37:                0x0 - <unknown>
```

</details>

note particularly frames 13-21, from start_backtrace to end_backtrace. with PrintFmt::Short, these are the *only* frames that are printed; i.e. we are doing the exact opposite of the comment.

r? ``@saethlin``
@jyn514 jyn514 deleted the doc-backtraces branch December 6, 2024 18:32
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants