Repository navigation
Tracking Issue for panic_backtrace_config #93346
Description
Activity
- changed the title
[-]Tracking Issue for XXX[/-][+]Tracking Issue for panic_backtrace_config[/+]on Jan 26, 2022 - It's already somewhat annoying to import and/or re-type the
std::panic::prefix necessary to use these APIs, probably adding a second module to the mix isn't worth it.
I think it depends on how we expose the
ShortandFullformat displays to users. If we expect users to usefmtflags to configure the format when printingstd::backtrace::Backtracethen we'd probably want to associate the style with the panic module.I do worry though about how this style of API will look like in practice, I expect you'd probably have to write something like this every time you want to copy the
stdpanic hook behavior:match get_backtrace_style() { Short => // use correct fmt flags for short Full => // use correct fmt flags for full // ... }
Which feels a little unfortunate. This is a strawman proposal but I would personally rather use something that can be easily expressed in one line, first idea I can come up with is this:
println!("{}", backtrace.display(get_backtrace_style()));
And if we were to provide an adapter API for associating a style with a backtrace when printing it then I imagine we'd want to include the style type in the backtrace module.
- It's already somewhat annoying to import and/or re-type the
For the trivial "just print it" case, I suspect a direct
fn std::panic::report_backtrace()which internally prints to stderr may be better (or takes a&mut dyn Write, maybe) -- capturing and resolving the backtrace viastd::backtrace::Backtracein panic handling may want to e.g. avoid allocations and some of the other complexity associated with a more general API like std::backtrace::Backtrace.If you're doing something more complicated that doesn't just want to adjust the surrounding material but alter the backtrace printing directly, it may need some API design if we don't wan to stabilize some fairly low-level APIs (e.g., https://docs.rs/backtrace/latest/backtrace/fn.trace.html and similar) that may not be a good fit for std itself.
I'm not sure there's a great answer here, though; I think the tradeoffs are particularly acute around this surface area and difficult to judge.
For the trivial "just print it" case, I suspect a direct
fn std::panic::report_backtrace()which internally prints to stderr may be betteragreed, I've felt like we should be exposing the allocation-lite backtrace printing interface we use in the panic hook for a while now.
If you're doing something more complicated that doesn't just want to adjust the surrounding material but alter the backtrace printing directly, it may need some API design if we don't wan to stabilize some fairly low-level APIs (e.g., https://docs.rs/backtrace/latest/backtrace/fn.trace.html and similar) that may not be a good fit for std itself.
oh interesting, I can see us going this direction, though the usage of
traceseems somewhat opaque / complex so like you said we'd need some design discussion on that or some more informative examples. For now I am a big enough fan of the split you describe and think we should plan on keepingBacktraceStylein panic and add astd::panic::report_backtrace(&mut dyn Write)and let it read the backtrace style internally.Thinking about it though, this approach definitely reduces the value of the
get_backtrace_stylefunction, since it's not really making it easy to copy the behavior of the panic hook when printingstd::backtrace::Backtraces, and you wouldn't even need to use it when working withreport_backtrace.@rust-lang/libs-api I'd like to propose to stabilize these APIs as-is. The API matches what we have done with environment variables for a long time, just a bit more type-safe. There's more that can be done here (see the posts by @yaahc above), but I don't think any of this should be a blocker -- given in particular that this feature is on the critical path for reducing the use of
set_varin the ecosystem.set_backtrace_style()is almost the same as settingRUST_BACKTRACE, except that it doesn't enableBacktrace::capture(), right? Should it? Or is that intentional?Oh, interesting. On the one hand it seems strange if a
std::panicfunction affectsstd::backtracebehavior, on the other hand ideally this function can replace the environment variable entirely.Maybe the enum and methods should be moved to
std::backtrace?I suspect a direct
fn std::panic::report_backtrace()which internally prints to stderr may be betterSorry for interfering, though I'd suggest previous
Backtrace::display(...)API for one simple reason - ability to gather backtrace and then print it using builtin styles elsewhere. It's a bit inconvenient that you either use default default panic hook and get nice short stacktraces or have to emulate it by hand forstd::backtrace::Backtrace.Usually, we omit
get_from getters. So it'd be calledstd::panic::backtrace_styleand notstd::panic::get_backtrace_style.Reacted by Ralf Jung and Jonas Böttiger- We are getting closer to set_var being unsafe. It would be good to have this stable soon so that people can migrate away from a soon-to-be-unsafe API to something safe.
set_backtrace_style()is almost the same as settingRUST_BACKTRACE, except that it doesn't enableBacktrace::capture(), right? Should it? Or is that intentional?What does t-libs-api think is the better choice here? I don't have a strong opinion, but this API should ideally land in time for the edition.
- Currently implemented:
std::panic::set_backtrace_styleonly affects panics, but notBacktrace::capture. Doesn't provide a full replacement forset_var("RUST_BACKTRACE", _), but is probably good enough. - Have
std::panic::set_backtrace_stylealso affectBacktrace::capture. That's a somewhat odd coupling of module. - Move
set_backtrace_style(andget_backtrace_style) tostd::backtrace, to indicate that they affect all ways of capturing backtraces (viaBacktraceand inside panics). However,backtracedoesn't really make a difference between "short" and "full" so this is also a bit awkward.
Cc @rust-lang/libs-api
- Currently implemented:
We discussed this in the libs-api meeting today. The main point of contention is that we have 2 separate points where we configure the "backtrace mode":
RUST_BACKTRACEandRUST_LIB_BACKTRACE. There are valid use cases for configuring both:RUST_LIB_BACKTRACEcan be turned off to reduce the performance impact of backtrace capture in crates likeanyhow.RUST_BACKTRACEcan be turned on to always provide users with a backtrace for bug reports.
Our conclusion is that we should provide APIs to individually control both of these options:
// std::panic fn set_panic_backtrace_style(style: BacktraceStyle); fn panic_backtrace_style() -> BacktraceStyle; // std::backtrace fn set_backtrace_style(style: BacktraceStyle); fn backtrace_style() -> BacktraceStyle;
Some notable differences between this API and the existing one:
backtrace_styledoesn't return anOption. Missing backtrace support is an implementation detail that is only reported when actually trying to obtain a backtrace. The backtrace style API ignores this and only sets the "desired" panic style, independently of whether it is supported.- The 2 options are independent of each other: one is for the default panic hook, the other is for
Backtrace::capture. Each can be set independently and one does not affect the other. - If the style is not set then it will be initialized from environment variables only. Notably the default value for
backtrace_stylewill not reflect any previous calls toset_panic_backtrace_style.
Reacted by David Tolnay and Jonas PlatteMy understanding is that the consideration of making this possible in a thread-local manner has been pushed to third party libraries - meaning there is confidence that the standard library solution would leave sufficient hooks for third party libraries to do this efficiently - is that correct?
Is that also how this would be handled in an async context? A good usecase is
should_panictests where someone might disable backtraces for performance reasons, but only for those tests. They may be running in different threads, or might be async tests. Would that be easily handled under the current proposal (even if it's delegated to third party libraries)?Given the APIs currently exposed by
std, I think that it is actually not possible to cleanly provide a thread-local version ofset_backtrace_style()in a third-party panic hook. More precisely, I don't think it can be done without facing one of the following two undesirable consequences:- Break composability of third-party panic hooks
- Introduce a global mutex that is locked on every panic
Here is the reason: as a third-party crate, when I install a panic hook with
set_hook()(and, in some shiny future, its non-racyupdate_hook()replacement), I do not know whether the panic hook that is currently registered is the standard panic hook, or a third-party hook like the one provided by proptest'shandle_panicsfeature. I would like not to break these other third-party panic hooks, as I have no idea what they're trying to do, therefore I have no choice but to end my panic hook by calling the previously registered panic hook.Other third-party panic hooks that care about composability will do the same for the same reason, so eventually the stack of panic hooks will always call into the std panic hook, which itself will print a backtrace according to the current output of
get_backtrace_style().As a result, my third-party panic hook cannot prevent the std panic hook from printing a backtrace if my internal thread-local logic decides that it should not be printed, unless I engage in one of the following undesirable behaviors:
- Break panic hook composability by refusing to call the lower-level panic hook when a backtrace should not be printed, without actually knowing what this lower-level panic hook is supposed to do (it may not be the std hook, but a user-defined one whose purpose is unknown).
- Lock a global mutex on every panic, from the start of the global hook to its completion (recursive calls to former panic hooks included), to prevent concurrent accesses to the
backtrace_styleglobal state by thestdpanic hook. Then change that global state as my own backtrace-printing logic dictates for the duration of the lower-level panic hook.- Obviously, this global locking must be done carefully, thinking about e.g. where in the
stdpanic handling chain the recursive panic protection is located (is it handled before the std panic hook or inside of it?), otherwise deadlocks may ensue.
- Obviously, this global locking must be done carefully, thinking about e.g. where in the
Arguably, option 2 could be a reasonable tradeoff overall, given that panics are supposed to be rare and slow (so scalability is probably not a concern) and panics are not supposed to recurse (so I could probably just use a kind of recursive mutex that aborts when a recursive panic is detected).
But it's just a point to keep in mind: if it has to be done in a third-party crate, it will not be pretty. So it would be nicer if the std-exposed state was thread-local to begin with...
Feature gate:
#![feature(panic_backtrace_config)]This is a tracking issue for configuring the capture and display of backtraces in the default panic hook, as well as exposing that configuration to third-party libraries for usage.
Public API
This API is intended to form part of the strategy for addressing the unsoundness of the
std::env::set_varAPI on some platforms (mostly non-Windows). See #92431 (comment) for a summary.Steps / History
Unresolved Questions
std::backtrace?std::panic::prefix necessary to use these APIs, probably adding a second module to the mix isn't worth it.get_backtrace_stylebe exposed?Option<BacktraceStyle>return type looks a little weird, and may not mean the intuitive thing --NonerepresentsUnsupported, not "not set" or "don't print backtraces", as might be initially assumed.std::panic::report_backtrace(&mut dyn Write)instead which internally knows how to format things.