rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
537 stars 246 forks source link

Filter frames by __rust_end_short_backtrace and __rust_begin_short_backtrace #502

Open rukai opened 1 year ago

rukai commented 1 year ago

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: https://github.com/rust-lang/rust/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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