hide panic internals in short backtraces

[jyn514]

right now, nearly all foo.expect("bar") backtraces start like this:

stack backtrace:
   0: rust_begin_unwind
             at /rustc/7f2fc33da6633f5a764ddc263c769b6b2873d167/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_fmt
             at /rustc/7f2fc33da6633f5a764ddc263c769b6b2873d167/library/core/src/panicking.rs:72:14
   2: core::panicking::panic_display
             at /rustc/7f2fc33da6633f5a764ddc263c769b6b2873d167/library/core/src/panicking.rs:256:5
   3: core::panicking::panic_str
             at /rustc/7f2fc33da6633f5a764ddc263c769b6b2873d167/library/core/src/panicking.rs:231:5
   4: core::option::expect_failed
             at /rustc/7f2fc33da6633f5a764ddc263c769b6b2873d167/library/core/src/option.rs:1994:5
   5: core::option::Option<T>::expect
             at /rustc/7f2fc33da6633f5a764ddc263c769b6b2873d167/library/core/src/option.rs:895:21

this is not super useful. it doesn't add any information, other than maybe the very last frame which says you called expect(). it would be nice to omit anything in the core::panicking module from the backtrace; and maybe expect_failed and the core::ops::function::FnOnce::call_once that show up at the end of the backtrace as well. people can always opt back in with RUST_BACKTRACE=full.

the code for this lives in

rust/library/std/src/sys_common/backtrace.rs

Lines 77 to 81 in 378a43a

	// 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`.
	if print_fmt == PrintFmt::Short {

@rustbot label +T-libs +A-runtime

[workingjubilee]

I think the easiest way to do this would be to try to move the __rust_begin_short_backtrace and __rust_end_short_backtrace symbols around. That way everything continues working as normal.

It should be fine if this accidentally results in some duplication in the full trace. The filtering must already be resilient against the fact that we are neither guaranteed that these symbols only appear once, or at all.

[jyn514]

I think the easiest way to do this would be to try to move the __rust_begin_short_backtrace and __rust_end_short_backtrace symbols around. That way everything continues working as normal.

i don't see how this helps? if you move it to expect_failed, you now need a bunch of begin_short_backtrace stubs, one for each possible approach to the panic machinery. i see 10 calls to panic_fmt alone, that doesn't seem maintainable.

the backtrace crate gives us access to the module names, that seems much simpler than trying to hack it by inserting custom frames.

[workingjubilee]

Hmm. Making the backtrace filtering logic more fragile does not seem automatically "simpler" to me, since now it has to handle a number of other conditions and it has to be aware that it can only exclude these frames at the beginning of the (shortened) backtrace, not if they somehow appear elsewhere, but I would be willing to review such a PR.

[conradludgate]

For short backtraces, I would also like to propose trimming the pre-main frames too

[saethlin]

I think that's in the OP:

and the core::ops::function::FnOnce::call_once that show up at the end of the backtrace as well.

I've been thinking about how to do this. I agree with Jubilee's concern that just going off module path is fragile, but I'm not totally sure that alternative approaches are worth the complexity.

Omitting the FnOnce::call_once entry is straightforward in that the heuristic is just to omit the frame before we encounter __rust_begin_short_backtrace. I don't see a straightforward way to make this happen because the backtrace printing needs to manage state in a way it doesn't right now. I don't see any fragility, we should just do this.

Omitting frames based on the core::panicking path looks like a good heuristic based on the backtrace for assert_eq!, but I'm wary of what would happen in a backtrace that exits the module then comes back to it. Perhaps we can also make this resilient this by adding yet more state to the backtrace printing? If we keep track of the fact that saw __rust_end_short_backtrace then continue skipping frames until we get something that isn't rust_begin_unwind or in core::panicking... maybe that works well enough? At that point though I'd really like to see a refactor of the backtrace printing state machine. It's quite unwieldy already.