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

Filter frames by __rust_end_short_backtrace and __rust_begin_short_backtrace #502

Open
rukai opened this issue Dec 7, 2022 · 7 comments

Comments

@rukai
Copy link

rukai commented Dec 7, 2022

rustc inserts frames std::sys_common::backtrace::__rust_end_short_backtrace and std::sys_common::backtrace::__rust_begin_short_backtrace so that the internal backtrace implementation can filter out noise from backtrace capturing.

Can we make use of these in the backtrace crate in order to have equivalent noise removal?

I'm not sure if these frame names are stable but I figure that we can just fall back to the old filtering implementation if they are missing?

Edit:
Oh huh, I just realized even std::backtrace::Backtrace isnt filtering these out... I filed an issue for it here: rust-lang/rust#105413

Example:

With the following program:

use backtrace::Backtrace;

fn setup() {
    std::panic::set_hook(Box::new(|panic| {
        println!("{:?}", Backtrace::new());
    }));
}

fn run() {
    panic!("OOPS");
}

fn main() {
    setup();
    run();
}

Currently the backtrace looks like this:

   0: foo::setup::{{closure}}
             at src/main.rs:4:26
   1: std::panicking::rust_panic_with_hook
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:702:17
   2: std::panicking::begin_panic_handler::{{closure}}
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:586:13
   3: std::sys_common::backtrace::__rust_end_short_backtrace
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/sys_common/backtrace.rs:138:18
   4: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   5: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   6: foo::run
             at src/main.rs:9:5
   7: foo::main
             at src/main.rs:14:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5
   9: std::sys_common::backtrace::__rust_begin_short_backtrace
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/sys_common/backtrace.rs:122:18
  10: std::rt::lang_start::{{closure}}
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/rt.rs:166:18
  11: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:283:13
      std::panicking::try::do_call
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:492:40
      std::panicking::try
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:456:19
      std::panic::catch_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panic.rs:137:14
      std::rt::lang_start_internal::{{closure}}
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/rt.rs:148:48
      std::panicking::try::do_call
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:492:40
      std::panicking::try
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:456:19
      std::panic::catch_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panic.rs:137:14
      std::rt::lang_start_internal
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/rt.rs:148:20
  12: std::rt::lang_start
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/rt.rs:165:17
  13: main
  14: <unknown>
  15: __libc_start_main
  16: _start
             at /build/glibc/src/glibc/csu/../sysdeps/x86_64/start.S:115

With filtering it would look like:

   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: foo::run
             at src/main.rs:9:5
   3: foo::main
             at src/main.rs:14:5
   4: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5

We could keep the full unfiltered version available via "{:#?}" formatting.

@mitsuhiko
Copy link
Contributor

I would prefer if this crate does not try to be clever about stack traces and instead leaves this up to the user of the library. In particular due to the fact that the top of the stack requires different handling with regards to the instruction addr compared to all other frames makes it for some cases important to understand if the stack has been truncated or not.

@bjorn3
Copy link
Member

bjorn3 commented Jan 12, 2023

Backtrace-rs already filters itself away, so the top of the printed backtrace is not the top of the actual stack. Also the libstd api allows you to choose if anything should be filtered or not.

@eddyb
Copy link
Member

eddyb commented Mar 31, 2023

I would prefer if this crate does not try to be clever about stack traces and instead leaves this up to the user of the library.

Surely it could just be something that can be toggled by the user? (and not impact existing users)

Also, something that came up while @Gankra and I were discussing such functionality: what if not just the filtering, but __rust_begin_short_backtrace/__rust_end_short_backtrace themselves were provided by backtrace (as a "please hide these implementation details" bracketing system), and then libstd also used that?

That way, nothing would be hardcoding implementation details of anything else, and the backtrace crate could even check for its specific full symbol names (e.g. backtrace::short::begin/backtrace::short::end etc.), instead of just .contains with the function names.

@Gankra
Copy link

Gankra commented Mar 31, 2023

I was considering proposing adding some alternatives to Backtrace::frames. Specifically:

  • Backtrace::short_frames_strict: reproduces the current behaviour of std where it only uses the two delimiter frames
  • Backtrace::short_frames: as above, but also does "nice" cleanups like hiding rust_begin_unwind and panic_fmt which are still garbage frames as far as end users are concerned.

These APIs should exist because without them people don't know how to properly use this library and are shipping broken things like just using a hardcoded .skip(N) on the frames, absent any docs or APIs to guide them:

@Gankra
Copy link

Gankra commented Mar 31, 2023

tbc I'm happy to implement this if it will be accepted. until then i'm just going to implement the same logic in the above libraries to fix the ecosystem

@Gankra
Copy link

Gankra commented Mar 31, 2023

Also the libstd api allows you to choose if anything should be filtered or not.

Also I couldn't find any such API in std::backtrace? Are you just referring to env vars?

@Gankra
Copy link

Gankra commented Mar 31, 2023

I've created the backtrace-ext crate with a short_frames_strict function as described above. miette now uses it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants