Improve backtrace formating while panicking.

[Yamakaky]

Fixes #37783.

Done:

• Fix alignment of file paths for better readability
• RUST_BACKTRACE=full prints all the informations (current behaviour)
• RUST_BACKTRACE=(short|yes) is the default and does:
  • Skip irrelevant frames at the beginning and the end
  • Remove function address
  • Remove the current directory from the absolute paths
  • Remove ::hfabe6541873 at the end of the symbols
• RUST_BACKTRACE=(0|no) disables the backtrace.
• RUST_BACKTRACE=<everything else> is equivalent to short for
  backward compatibility.
• doc
• More uniform printing across platforms.

Removed, TODO in a new PR:

• Remove path prefix for libraries and libstd

Example of short backtrace:

fn fail() {
    panic!();
}

fn main() {
    let closure = || fail();
    closure();
}

Short:

thread 'main' panicked at 'explicit panic', t.rs:2
Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: t::fail
            at ./t.rs:2
   1: t::main::{{closure}}
            at ./t.rs:6
   2: t::main
            at ./t.rs:7

Full:

thread 'main' panicked at 'This function never returns!', t.rs:2
stack backtrace:
   0:     0x558ddf666478 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hec84c9dd8389cc5d
                               at /home/yamakaky/dev/rust/rust/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x558ddf65d90e - std::sys_common::backtrace::_print::hfa25f8b31f4b4353
                               at /home/yamakaky/dev/rust/rust/src/libstd/sys_common/backtrace.rs:71
   2:     0x558ddf65cb5e - std::sys_common::backtrace::print::h9b711e11ac3ba805
                               at /home/yamakaky/dev/rust/rust/src/libstd/sys_common/backtrace.rs:60
   3:     0x558ddf66796e - std::panicking::default_hook::{{closure}}::h736d216e74748044
                               at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:355
   4:     0x558ddf66743c - std::panicking::default_hook::h16baff397e46ea10
                               at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:371
   5:     0x558ddf6682bc - std::panicking::rust_panic_with_hook::h6d5a9bb4eca42c80
                               at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:559
   6:     0x558ddf64ea93 - std::panicking::begin_panic::h17dc549df2f10b99
                               at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:521
   7:     0x558ddf64ec42 - t::diverges::he6bc43fc925905f5
                               at /tmp/p/t.rs:2
   8:     0x558ddf64ec5a - t::main::h0ffc20356b8a69c0
                               at /tmp/p/t.rs:6
   9:     0x558ddf6687f5 - core::ops::FnOnce::call_once::hce41f19c0db56f93
  10:     0x558ddf667cde - std::panicking::try::do_call::hd4c8c97efb4291df
                               at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:464
  11:     0x558ddf698d77 - __rust_try
  12:     0x558ddf698c57 - __rust_maybe_catch_panic
                               at /home/yamakaky/dev/rust/rust/src/libpanic_unwind/lib.rs:98
  13:     0x558ddf667adb - std::panicking::try::h2c56ed2a59ec1d12
                               at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:440
  14:     0x558ddf66cc9a - std::panic::catch_unwind::h390834e0251cc9af
                               at /home/yamakaky/dev/rust/rust/src/libstd/panic.rs:361
  15:     0x558ddf6809ee - std::rt::lang_start::hb73087428e233982
                               at /home/yamakaky/dev/rust/rust/src/libstd/rt.rs:57
  16:     0x558ddf64ec92 - main
  17:     0x7fecb869e290 - __libc_start_main
  18:     0x558ddf64e8b9 - _start
  19:                0x0 - <unknown>

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Yamakaky]

Any idea about how I could get access to the build root path?
Also, I'm thinking about using std::env::current_dir, but can I use it in the unwinding?

[Yamakaky]

Result formatting with RUST_BACKTRACE=short:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/libcore/result.rs:837
stack backtrace:
   1:    std::sys::imp::backtrace::tracing::imp::write::hfc2edb670e5eda97
   2:    std::panicking::default_hook::{{closure}}::hc66a547fab0b4d38
   3:    std::panicking::default_hook::h7fba1b2f69474bdc
   4:    std::panicking::rust_panic_with_hook::h5d3597668c9f0035
   5:    std::panicking::begin_panic::hfaa38fdbc7d103f6
   6:    std::panicking::begin_panic_fmt::h69e0397ed8fc5362
   7:    rust_begin_unwind
   8:    core::panicking::panic_fmt::h2ba266031787cf7b
   9:    core::result::unwrap_failed::h0a78b8b4914ea99c
             at /home/yamakaky/dev/rust/rust/src/libcore/macros.rs:29
  10:    <core::result::Result<T, E>>::unwrap::h2f7c43c0e2d4b7cd
             at /home/yamakaky/dev/rust/rust/src/libcore/result.rs:737
  11:    t::main::h86d9b74d94eb7fff
             at /tmp/pote/t.rs:2
  12:    __rust_maybe_catch_panic
  13:    std::rt::lang_start::ha33d35ddc5bfd7d2
  14:    main
  15:    __libc_start_main
  16:    _start

and with RUST_BACKTRACE=full:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/libcore/result.rs:837
stack backtrace:
   1:     0x56381e42288e - std::sys::imp::backtrace::tracing::imp::write::hfc2edb670e5eda97
   2:     0x56381e424b91 - std::panicking::default_hook::{{closure}}::hc66a547fab0b4d38
   3:     0x56381e424776 - std::panicking::default_hook::h7fba1b2f69474bdc
   4:     0x56381e424f78 - std::panicking::rust_panic_with_hook::h5d3597668c9f0035
   5:     0x56381e424e12 - std::panicking::begin_panic::hfaa38fdbc7d103f6
   6:     0x56381e424d50 - std::panicking::begin_panic_fmt::h69e0397ed8fc5362
   7:     0x56381e424cd1 - rust_begin_unwind
   8:     0x56381e46db2f - core::panicking::panic_fmt::h2ba266031787cf7b
   9:     0x56381e41cc55 - core::result::unwrap_failed::h0a78b8b4914ea99c
                               at /home/yamakaky/dev/rust/rust/src/libcore/macros.rs:29
  10:     0x56381e41c99b - <core::result::Result<T, E>>::unwrap::h2f7c43c0e2d4b7cd
                               at /home/yamakaky/dev/rust/rust/src/libcore/result.rs:737
  11:     0x56381e41ccd8 - t::main::h86d9b74d94eb7fff
                               at /tmp/pote/t.rs:2
  12:     0x56381e42f0f6 - __rust_maybe_catch_panic
  13:     0x56381e4255a8 - std::rt::lang_start::ha33d35ddc5bfd7d2
  14:     0x56381e41cd12 - main
  15:     0x7f6dc966c290 - __libc_start_main
  16:     0x56381e41c7f9 - _start

I plan to mask 1-8 and 12-16 with short. I think that's what you care about most of the time.

[Mark-Simulacrum]

Those appear to be the same, did you copy the wrong thing in?

[Yamakaky]

For now they are nearly the same, except the address which is not printed with short.
An option would be to first construct a Vec<{demangled, addr, [files]}>, then print it. Is it OK to alloc in this place, though?
Also, can I use std::env::current_dir in this place?

[Yamakaky]

In fact, that may not be necessary. We can simply recognized the mangled form of std::panicking::*, rust_begin_unwind, main (14 in the stacktrace above) and _rust_maybe_catch_main to determine the range of frames to print, and use a double pass. That should catch most of the unwanted frames.

[Yamakaky]

With panic! and without the ::h234567898765 part:

thread 'main' panicked at 'explicit panic', t.rs:2
stack backtrace:
   1:    std::sys::imp::backtrace::tracing::imp::write
   2:    std::panicking::default_hook::{{closure}}
   3:    std::panicking::default_hook
   4:    std::panicking::rust_panic_with_hook
   5:    std::panicking::begin_panic
             at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:417
   6:    t::main
             at /tmp/pote/t.rs:2
   7:    __rust_maybe_catch_panic
   8:    std::rt::lang_start
   9:    main
  10:    __libc_start_main
  11:    _start

[estebank]

I believe you should be able to accomplish item 4 in #37783 by doing something like

let mut print_line = if format == backtrace::PrintFormat::Short { false } else { true };
for (i, &(file, line)) in fileline_buf[..fileline_count].iter().enumerate() {
    if file.is_null() { continue; } // just to be sure
    let file = unsafe { CStr::from_ptr(file).to_bytes() };
    if format == backtrace::PrintFormat::Short && !print_line {
        let magic_str = "core::panicking::panic_fmt";
        if file[..magic_str.len()] == magic_str.as_bytes() {
            print_line = true
        }
        continue;
    }
    output_fileline(w, file, line, i == FILELINE_SIZE - 1, format)?;
}

[Yamakaky]

No, this loop only prints the at file:line lines.

[Yamakaky]

It's getting better!

thread 'main' panicked at 'explicit panic', t.rs:2
stack backtrace:
{ skipping 5 frames }
   6:    t::test
             at /tmp/pote/t.rs:2
   7:    t::main
             at /tmp/pote/t.rs:5
{ skipping 6 frames }

compared to

thread 'main' panicked at 'explicit panic', t.rs:2
stack backtrace:
   1:     0x559456964347 - std::sys::imp::backtrace::tracing::imp::write::hd29ea4b7d57abadd
   2:     0x559456969781 - std::panicking::default_hook::{{closure}}::h7cb71ec6c6050f50
   3:     0x559456968376 - std::panicking::default_hook::h7fba1b2f69474bdc
   4:     0x559456968978 - std::panicking::rust_panic_with_hook::h1b9f3a413c4c29f3
   5:     0x559456960263 - std::panicking::begin_panic::h5f62bd622a6e3525
                               at /home/yamakaky/dev/rust/rust/src/libstd/panicking.rs:417
   6:     0x559456960412 - t::test::hc6402ffe28ce1338
                               at /tmp/pote/t.rs:2
   7:     0x55945696042c - t::main::h86d9b74d94eb7fff
                               at /tmp/pote/t.rs:5
   8:     0x5594569710d6 - __rust_maybe_catch_panic
   9:     0x559456968fa8 - std::rt::lang_start::ha33d35ddc5bfd7d2
  10:     0x559456960472 - main
  11:     0x7f05f168c290 - __libc_start_main
  12:     0x559456960089 - _start

[Yamakaky]

Added removal of path prefix. It prints ./<path> for files in the current directory, at builtin file <file> for std and such, and at library file <file> for libraries. For now, the path are hardcoded for my system. I need to get the buildroot path, and the paths from -L. Any idea?

thread 'main' panicked at 'explicit panic', t.rs:2
stack backtrace:
{ skipping 5 frames }
   6:    t::test
             at ./t.rs:2
   7:    t::main::{{closure}}
             at ./t.rs:6
   8:    <core::result::Result<T, E>>::map
             at builtin file libcore/result.rs:465
   9:    t::main
             at ./t.rs:6
{ skipping 6 frames }

[Yamakaky]

Can someone test on windows?

[Yamakaky]

Updated first post.
r? @alexcrichton

[alexcrichton]

Thanks for the PR! I like the look and feel of the new output.

Could most of the changes be moved to the common implementation of backtraces? I don't see many changes to the Windows side of things so it'd be helpful to just define all the skipping and such logic in one location rather than in multiple.

[alexcrichton]

This seems sort of suspicious, how come this was necessary?

[Yamakaky]

See #38165 (comment)

[alexcrichton]

In the interest of being as conservative as possible here, could we leave this as just recognizing 0?

[Yamakaky]

Fixed in ddfef15.

[alexcrichton]

Could this just be collapsed with the case below?

[Yamakaky]

Are you sure? I find it cleaner with the "catch anything" separated.

[alexcrichton]

These values seem to be quite subtle, could you be sure to comment as to their purpose here?

[Yamakaky]

OK. In fact, it controls if the unwinding should use the short or long backtrace format.

[alexcrichton]

Could this use option_env and handle the None case to ensure that we can build the standard library separately if need be?

[Yamakaky]

See #38165 (comment)

[Yamakaky]

Fixed in 7780ef8

[alexcrichton]

Most of this can still be printed even if this fails, right?

[Yamakaky]

Oh yeah, right. Fixed in 5c08069

[alexcrichton]

Hm I'm curious where this path name came into effect?

[Yamakaky]

It's a dummy value, because the library directory is defined at compilation of the crate, not of stdlib. I still have to find a way to get it.

[alexcrichton]

This logic seems like we'd want to do this across all platforms, right? Perhaps this could go into the common module?

[Yamakaky]

Sure, that would just need a bit of refactoring.

[Yamakaky]

The CFG_BUILD_DIR stuff is a hack until I find a cleaner way to get the build dir. Any idea?
I didn't test on Windows, can someone test?

[Yamakaky]

Is it possible to also have the appveyor build? It would help me supporting windows.

[Yamakaky]

@alexcrichton A solution for cleaner code reuse would be that the unwinding function calls a platform-dependent function that generates some sort of Vec, then prints it. Not sure if it's an option, though, since it would allocate while unwinding.

[Mark-Simulacrum]

The platform specific option could potentially return an iterator instead of a vec to avoid the allocation (through impl Trait, or just explicitly naming it).

[Yamakaky]

I thought about that, but it may be complicated since the system specific code gives a callback to a C library that's invoked for each frame.

[Yamakaky]

• Sound like libunwind supports iterative unwinding unw_step, but not chosen for rust (see this comment for more background)
• On iOS, the code uses a builtin backtrace function which sets an array of pointers.
• Windows uses a frame iterator
• The only trouble is gcc. We could convert it to something like the iOS code easily.

What I propose is to add the structs Backtrace which implements Iterator<Frame>. These two structs would be defined in a system-specific manner, with a common trait for Frame. The only public interface for Frame would be functions to get a CStr symbol name, symbol address and file names and line number. I think it should work, and with only static dispatch. What do you think @alexcrichton?

[Yamakaky]

@alexcrichton I think I'm ready for a final review. I'm only on linux, so I was not able to test on windows and such. As I moved code around, there is probably at least some unused imports.
I removed the prefix removing for libraries and std as it involves more changes in rustc. That will be for another PR.

[alexcrichton]

I may have missed it, but where did these changes come from? How come we're handling different return codes now?

[Yamakaky]

http://git.savannah.gnu.org/gitweb/?p=libunwind.git;a=blob;f=src/unwind/Backtrace.c;hb=bc8698fd7ed13a629a8ec3cb2a89bd74f9d8b5c0
_Unwind_Backtrace only return some values.

[alexcrichton]

Sure yeah I can imagine that this function returns a number of values, but we weren't checking any of them before, only for errors. What prompted the change in logic?

[Yamakaky]

I thought it was better to handle error codes. Was I wrong?

[alexcrichton]

Oh no that's fine, I'm just trying to understand why. Did this come up during testing? Was this just done on a whim? Have these code paths been executed? etc.

I'm wary of these sorts of changes because libunwind's cross-platform behavior is notoriously finnicky, so I just wanted to dig into what prompted this change. My guess is that it's fine to land as such, but I just want to make sure.

[Yamakaky]

No, I just found it strange to ignore the error case (https://github.com/rust-lang/rust/blob/master/src/libstd/panicking.rs#L349, I'm watching ^^)

[alexcrichton]

Hm I feel like we're still not getting to an answer to my question. The code you linked to ignores errors because it has nowhere to pass the errors to and the information is just advisory, so it's fine to just ignore errors.

My question is why did this change happen? Is it purely being proactive? Was it reactive to some problem?

[Yamakaky]

No, I added it for the reasons explained above, without detected problems

[alexcrichton]

Below it printed addr - 1, but that seems to have been lost here?

[Yamakaky]

Yeah, not sure why there is this -1, but it was probably important XD

[alexcrichton]

This semicolon seems like it'd cause a compile failure? And the to_bytes doesn't seem to match the &'static str return type?

[Yamakaky]

Coding without compiler is not a good idea

[alexcrichton]

This type of Option<Option<&str>> seems to disagree with the tyep on the return of the function?

[Yamakaky]

Oops, the "Some" shouldn't be there

[alexcrichton]

Stylistically we typically indent where clauses like these over one

[Yamakaky]

OK

[alexcrichton]

The indentation here was a little confusing for me at first, perhaps the result of this could be bound to a let?

[Yamakaky]

yes, you're right

[alexcrichton]

I believe we can just remove this, right? (always fall through below)

[alexcrichton]

Er so to clarify, we definitely can't panic while producing a backtrace. This is called during a panic, which would be a recursive panic, which we don't want to abort on. If we do end up handling this then we need to punt the error upwards and let the caller deal with it.

[Yamakaky]

Ok

[Yamakaky]

What do you suggest as return value?

[alexcrichton]

Oh a bool or a Result would be fine

[Yamakaky]

Yeah, sure, now that there is no cx.next_error ^^

[Yamakaky]

In fact, if I replace it with a bool we will loose the io::Error on https://github.com/Yamakaky/rust/blob/better-backtrace/src/libstd/sys/windows/backtrace/mod.rs#L76. Is that what we want?

[Yamakaky]

An alternative would be to return the already-decoded frames on error. Are you OK with that?

[Yamakaky]

That's what I did, finally.

[alexcrichton]

I think this field is no longer necessary, right? It was only used when we could generate an error during a backtrace by calling write!, but with that no longer happening the body of this function is infallible.

[alexcrichton]

Could this return an error?

[Yamakaky]

I see a few possibilities:

• same handling than for _URC_FATAL_PHASE1_ERROR
• above + print an error message
• create a new error type and return it using io::ErrorKind::Other

[alexcrichton]

Er sorry, what I mean is that this must not panic. It should return an error of some kind (Err(()) is fine) so the caller can deal with the failure.

[bors]

📌 Commit 52bed53 has been approved by petrochenkov

[bors]

⌛ Testing commit 52bed53 with merge bea2b27...

[bors]

💔 Test failed - status-appveyor

[petrochenkov]

Both Travis and Appveyor failed.
Travis on the next test condition

'the full version of the backtrace should contain __libc_start_main', /checkout/src/test/run-pass/backtrace.rs:103

Appveyor on the "classic" one (WTF, I tested this locally in this exact configuration)

thread 'main' panicked at 'explicit panic', C:\projects\rust\src/test\run-pass\backtrace.rs:24
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: <std::time::SystemTimeError as core::fmt::Display>::fmt
   1: std::panicking::Location::line
   2: std::panicking::Location::line
   3: std::panicking::rust_panic_with_hook
   4: std::panicking::begin_panic<&str>
             at ./src\libstd\panicking.rs:511
   5: backtrace::foo
             at ./src\test\run-pass\backtrace.rs:24
   6: backtrace::main
             at ./src\test\run-pass\backtrace.rs:156
'

Time to drop both Make sure than the short... and Only on linux for _start... conditions.

[Yamakaky]

I'm thinking: what if we captured a stacktrace juste before the main, then substract it to the backtrace on panic? That way, at least the bottom of the trace would be correct all the time.

Maybe we could do the same for the top, in std::panicking::begin_panic, core::result::unwrap_failed... It would be easier to maintain.

Not sure how it would play with inlining. inline(never) should do the trick.

[petrochenkov]

This one failed on Travis too.

[petrochenkov]

@Yamakaky
Maybe as a next PR?
The problem of this patch is that it already tries to change far too much platform-specific code at once and is pending for too long.
If main and panic start can be reliably detected in presence of inlining and custom mains, then yes, it would be a more robust solution, this needs some experimentation.

[petrochenkov]

@bors r+

[bors]

📌 Commit 6398b20 has been approved by petrochenkov

[bors]

⌛ Testing commit 6398b20 with merge 4be034e...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 4be034e to master...

[Yamakaky]

\o/
Finally!

[jdm]

Does this also affect the output of https://crates.io/crates/backtrace or should we file a separate issue in that repository?

[Yamakaky]

You should open an issue.