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

Print our own callstack on panics #1622

Merged
merged 1 commit into from Mar 21, 2023
Merged

Conversation

emilk
Copy link
Member

@emilk emilk commented Mar 20, 2023

The panic handler prints very long filenames, which can include sensitive paths. By cutting out the sensitive parts we get two benefits:

  • Users can copy-paste the callstack from the terminal into a rerun issue without any sensitive paths leaking
  • The callstack is much more readable.

Before:

[2023-03-20T14:51:43Z INFO  rerun::run] Loading "../api_demo.rrd"…
thread 'main' panicked at 'assertion failed: from.start() != from.end()', /Users/emilk/.cargo/registry/src/github.com-1ecc6299db9ec823/emath-0.21.0/src/lib.rs:142:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14
   2: core::panicking::panic
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:111:5
   3: emath::rect_transform::RectTransform::transform_pos
   4: emath::rect_transform::RectTransform::transform_rect
             at /Users/emilk/.cargo/registry/src/github.com-1ecc6299db9ec823/emath-0.21.0/src/rect_transform.rs:52:18
   5: re_viewer::ui::view_spatial::ui::create_labels
             at ./crates/re_viewer/src/ui/view_spatial/ui.rs:616:13

After:

[2023-03-20T14:49:46Z INFO  rerun::run] Loading "../api_demo.rrd"…

thread 'main' panicked at 'assertion failed: from.start() != from.end()', emath-0.21.0/src/lib.rs:142

   7: core::panicking::panic_fmt
             at core/src/panicking.rs:64:14
   8: core::panicking::panic
             at core/src/panicking.rs:111:5
   9: emath::rect_transform::RectTransform::transform_pos
      emath::rect_transform::RectTransform::transform_rect
             at emath-0.21.0/src/rect_transform.rs:52:18
  10: re_viewer::ui::view_spatial::ui::create_labels
             at re_viewer/src/ui/view_spatial/ui.rs:616:13
…

Checklist

@emilk emilk added 🧑‍💻 dev experience developer experience (excluding CI) 💣 crash crash, deadlock/freeze, do-no-start labels Mar 20, 2023
@emilk emilk added the enhancement New feature or request label Mar 20, 2023
@emilk emilk force-pushed the emilk/nicer-callstack-on-panic branch from 0e0610a to b0021f2 Compare March 20, 2023 14:56
@@ -183,21 +222,28 @@ fn callstack_from(start_pattern: &str) -> String {
let mut stack = stack.as_str();

// Trim the top (closest to the panic handler) to cut out some noise:
if let Some(start_offset) = stack.find(start_pattern) {
stack = &stack[start_offset + start_pattern.len()..];
if let Some(offset) = stack.find(start_pattern) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Doing these trim operations on the string-concatenated callstack after it's been converted to a string still seems a bit awkward with the rfind('\n') business. This seems like it could maybe be more cleanly pushed down into backtrace_to_string as an operation on for frame in backtrace.frames().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree its ugly, but the backtrace crate is a bit too opaque (non-pub) to do anything nicer.

@jleibs
Copy link
Member

jleibs commented Mar 20, 2023

The After version reads so much more nicely. I love it.

@emilk emilk merged commit b5a96ca into main Mar 21, 2023
@emilk emilk deleted the emilk/nicer-callstack-on-panic branch March 21, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💣 crash crash, deadlock/freeze, do-no-start 🧑‍💻 dev experience developer experience (excluding CI) enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants