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

Tracking Issue for panic_backtrace_config #93346

Open
1 of 3 tasks
Mark-Simulacrum opened this issue Jan 26, 2022 · 12 comments
Open
1 of 3 tasks

Tracking Issue for panic_backtrace_config #93346

Mark-Simulacrum opened this issue Jan 26, 2022 · 12 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 26, 2022

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

mod panic {
    #[derive(Copy, Clone, Debug, PartialEq, Eq)]
    #[non_exhaustive]
    pub enum BacktraceStyle {
        Short,
        Full,
        Off,
    }
    fn set_backtrace_style(BacktraceStyle);
    fn get_backtrace_style() -> Option<BacktraceStyle>;
}

This API is intended to form part of the strategy for addressing the unsoundness of the std::env::set_var API on some platforms (mostly non-Windows). See #92431 (comment) for a summary.

Steps / History

Unresolved Questions

  • Do we need to move to a thread-local or otherwise more customizable strategy for whether to capture backtraces? See this comment for some potential use cases for this.
    • Proposed answer: no, leave this for third-party hooks.
  • Bikeshed on naming of all the options, as usual.
  • Should BacktraceStyle be moved into std::backtrace?
    • 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.
  • Should get_backtrace_style be exposed?
    • The Option<BacktraceStyle> return type looks a little weird, and may not mean the intuitive thing -- None represents Unsupported, not "not set" or "don't print backtraces", as might be initially assumed.
    • We may want a std::panic::report_backtrace(&mut dyn Write) instead which internally knows how to format things.
@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jan 26, 2022
@Mark-Simulacrum Mark-Simulacrum changed the title Tracking Issue for XXX Tracking Issue for panic_backtrace_config Jan 26, 2022
@yaahc
Copy link
Member

yaahc commented 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 Short and Full format displays to users. If we expect users to use fmt flags to configure the format when printing std::backtrace::Backtrace then 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 std panic 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.

@Mark-Simulacrum
Copy link
Member Author

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 via std::backtrace::Backtrace in 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.

@yaahc
Copy link
Member

yaahc commented Jan 26, 2022

For the trivial "just print it" case, I suspect a direct fn std::panic::report_backtrace() which internally prints to stderr may be better

agreed, 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 trace seems 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 keeping BacktraceStyle in panic and add a std::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_style function, since it's not really making it easy to copy the behavior of the panic hook when printing std::backtrace::Backtraces, and you wouldn't even need to use it when working with report_backtrace.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 1, 2022
…r=yaahc

Support configuring whether to capture backtraces at runtime

Tracking issue: rust-lang#93346

This adds a new API to the `std::panic` module which configures whether and how the default panic hook will emit a backtrace when a panic occurs.

After discussion with `@yaahc` on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/backtrace.20lib.20vs.2E.20panic), this PR chooses to avoid adjusting or seeking to provide a similar API for the (currently unstable) std::backtrace API. It seems likely that the users of that API may wish to expose more specific settings rather than just a global one (e.g., emulating the `env_logger`, `tracing` per-module configuration) to avoid the cost of capture in hot code. The API added here could plausibly be copied and/or re-exported directly from std::backtrace relatively easily, but I don't think that's the right call as of now.

```rust
mod panic {
    #[derive(Copy, Clone, Debug, PartialEq, Eq)]
    #[non_exhaustive]
    pub enum BacktraceStyle {
        Short,
        Full,
        Off,
    }
    fn set_backtrace_style(BacktraceStyle);
    fn get_backtrace_style() -> Option<BacktraceStyle>;
}
```

Several unresolved questions:

* Do we need to move to a thread-local or otherwise more customizable strategy for whether to capture backtraces? See [this comment](rust-lang#79085 (comment)) for some potential use cases for this.
   * Proposed answer: no, leave this for third-party hooks.
* Bikeshed on naming of all the options, as usual.
* Should BacktraceStyle be moved into `std::backtrace`?
   * 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.

Note that PR rust-lang#79085 proposed a much simpler API, but particularly in light of the desire to fully replace setting environment variables via `env::set_var` to control the backtrace API, a more complete API seems preferable. This PR likely subsumes that one.
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 2, 2022
…yaahc

Support configuring whether to capture backtraces at runtime

Tracking issue: rust-lang#93346

This adds a new API to the `std::panic` module which configures whether and how the default panic hook will emit a backtrace when a panic occurs.

After discussion with `@yaahc` on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/backtrace.20lib.20vs.2E.20panic), this PR chooses to avoid adjusting or seeking to provide a similar API for the (currently unstable) std::backtrace API. It seems likely that the users of that API may wish to expose more specific settings rather than just a global one (e.g., emulating the `env_logger`, `tracing` per-module configuration) to avoid the cost of capture in hot code. The API added here could plausibly be copied and/or re-exported directly from std::backtrace relatively easily, but I don't think that's the right call as of now.

```rust
mod panic {
    #[derive(Copy, Clone, Debug, PartialEq, Eq)]
    #[non_exhaustive]
    pub enum BacktraceStyle {
        Short,
        Full,
        Off,
    }
    fn set_backtrace_style(BacktraceStyle);
    fn get_backtrace_style() -> Option<BacktraceStyle>;
}
```

Several unresolved questions:

* Do we need to move to a thread-local or otherwise more customizable strategy for whether to capture backtraces? See [this comment](rust-lang#79085 (comment)) for some potential use cases for this.
   * Proposed answer: no, leave this for third-party hooks.
* Bikeshed on naming of all the options, as usual.
* Should BacktraceStyle be moved into `std::backtrace`?
   * 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.

Note that PR rust-lang#79085 proposed a much simpler API, but particularly in light of the desire to fully replace setting environment variables via `env::set_var` to control the backtrace API, a more complete API seems preferable. This PR likely subsumes that one.
@RalfJung
Copy link
Member

@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_var in the ecosystem.

@RalfJung RalfJung added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Nov 11, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Nov 14, 2023

set_backtrace_style() is almost the same as setting RUST_BACKTRACE, except that it doesn't enable Backtrace::capture(), right? Should it? Or is that intentional?

@RalfJung
Copy link
Member

Oh, interesting. On the one hand it seems strange if a std::panic function affects std::backtrace behavior, on the other hand ideally this function can replace the environment variable entirely.

Maybe the enum and methods should be moved to std::backtrace?

@the8472 the8472 removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Nov 21, 2023
@target-san
Copy link

I suspect a direct fn std::panic::report_backtrace() which internally prints to stderr may be better

Sorry 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 for std::backtrace::Backtrace.

@tbu-
Copy link
Contributor

tbu- commented May 13, 2024

Usually, we omit get_ from getters. So it'd be called std::panic::backtrace_style and not std::panic::get_backtrace_style.

@RalfJung
Copy link
Member

RalfJung commented May 13, 2024 via email

@RalfJung
Copy link
Member

RalfJung commented May 13, 2024

@m-ou-se

set_backtrace_style() is almost the same as setting RUST_BACKTRACE, except that it doesn't enable Backtrace::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.

  1. Currently implemented: std::panic::set_backtrace_style only affects panics, but not Backtrace::capture. Doesn't provide a full replacement for set_var("RUST_BACKTRACE", _), but is probably good enough.
  2. Have std::panic::set_backtrace_style also affect Backtrace::capture. That's a somewhat odd coupling of module.
  3. Move set_backtrace_style (and get_backtrace_style) to std::backtrace, to indicate that they affect all ways of capturing backtraces (via Backtrace and inside panics). However, backtrace doesn't really make a difference between "short" and "full" so this is also a bit awkward.

Cc @rust-lang/libs-api

@m-ou-se m-ou-se added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label May 13, 2024
@Amanieu
Copy link
Member

Amanieu commented May 14, 2024

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_BACKTRACE and RUST_LIB_BACKTRACE. There are valid use cases for configuring both:

  • RUST_LIB_BACKTRACE can be turned off to reduce the performance impact of backtrace capture in crates like anyhow.
  • RUST_BACKTRACE can 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_style doesn't return an Option. 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_style will not reflect any previous calls to set_panic_backtrace_style.

@Amanieu Amanieu removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label May 14, 2024
@RalfJung
Copy link
Member

RalfJung commented May 15, 2024

@Amanieu thanks!

Will these use the same BacktraceStyle type? AFAIK, std::backtrace only supports "full" and "no" capture; there's no such thing as "short" there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants