Improve backtrace formating while panicking. #38165

Yamakaky · Dec 4, 2016

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 · Dec 4, 2016

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 · Dec 4, 2016

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 · Dec 5, 2016

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 · Dec 5, 2016

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

Yamakaky · Dec 5, 2016

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 · Dec 5, 2016

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 · Dec 5, 2016

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 · Dec 5, 2016

estebank reviewed Dec 5, 2016

View changes

estebank · Dec 5, 2016

src/libstd/sys_common/gnu/libbacktrace.rs

@@ -173,7 +178,7 @@ pub fn print(w: &mut Write, idx: isize, addr: *mut libc::c_void,
        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() };
-            output_fileline(w, file, line, i == FILELINE_SIZE - 1)?;
+            output_fileline(w, file, line, i == FILELINE_SIZE - 1, format)?;


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)?; }

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

Yamakaky · Dec 5, 2016

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 · Dec 5, 2016

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 · Dec 6, 2016

Can someone test on windows?

Yamakaky · Dec 10, 2016

Updated first post.
r? @alexcrichton

alexcrichton · Dec 12, 2016

alexcrichton reviewed Dec 12, 2016

View changes

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 · Dec 12, 2016

configure

@@ -1877,6 +1877,8 @@ fi

 msg
 copy_if_changed ${CFG_SRC_DIR}${INPUT_MAKEFILE} ./Makefile
+# Export CFG_BUILD_DIR for use in backtrace unwinding.
+sed -i -re 's/^(CFG_BUILD_DIR)/export \1/' config.tmp


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

See #38165 (comment)

alexcrichton · Dec 12, 2016

src/libstd/sys_common/backtrace.rs

    }

    let val = match env::var_os("RUST_BACKTRACE") {
-        Some(x) => if &x == "0" { 1 } else { 2 },
-        None => 1,
+        Some(x) => if ["no", "0"].iter().any(|val| &x == *val) {


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

Fixed in ddfef15.

alexcrichton · Dec 12, 2016

src/libstd/sys_common/backtrace.rs

+            None
+        } else if &x == "full" {
+            Some(PrintFormat::Full)
+        } else if ["short", "yes", "1"].iter().any(|val| &x == *val)  {


Could this just be collapsed with the case below?

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

alexcrichton · Dec 12, 2016

src/libstd/sys_common/backtrace.rs

@@ -25,45 +27,107 @@ pub const HEX_WIDTH: usize = 18;
 #[cfg(target_pointer_width = "32")]
 pub const HEX_WIDTH: usize = 10;

+#[derive(Debug, Copy, Clone, Eq, PartialEq)]
+pub enum PrintFormat {
+    Full = 2,


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

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

alexcrichton · Dec 12, 2016

src/libstd/sys_common/backtrace.rs

+    let mut already_printed = false;
+    if format == PrintFormat::Short && file_path.is_absolute() {
+        if let Ok(cwd) = env::current_dir() {
+            let buildroot = Path::new(env!("CFG_BUILD_DIR"));


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

See #38165 (comment)

Fixed in 7780ef8

alexcrichton · Dec 12, 2016

src/libstd/sys_common/backtrace.rs

+    let file_path = Path::new(file);
+    let mut already_printed = false;
+    if format == PrintFormat::Short && file_path.is_absolute() {
+        if let Ok(cwd) = env::current_dir() {


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

Oh yeah, right. Fixed in 5c08069

alexcrichton · Dec 12, 2016

src/libstd/sys_common/backtrace.rs

+    if format == PrintFormat::Short && file_path.is_absolute() {
+        if let Ok(cwd) = env::current_dir() {
+            let buildroot = Path::new(env!("CFG_BUILD_DIR"));
+            let libpath = Path::new("/tmp/libpath");


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

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 · Dec 12, 2016

src/libstd/sys/unix/backtrace/tracing/gcc_s.rs

+            if let Some(mangled_symbol_name) = resolve_symname(symaddr) {
+                let magics_begin = [
+                    "_ZN3std3sys3imp9backtrace7tracing3imp5write",
+                    "_ZN3std9panicking",


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

Sure, that would just need a bit of refactoring.

Yamakaky · Dec 13, 2016

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 · Dec 13, 2016

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

Yamakaky · Dec 13, 2016

@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 · Dec 13, 2016

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 · Dec 14, 2016

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 · Dec 14, 2016

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 · Dec 15, 2016

@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 · Dec 16, 2016

alexcrichton reviewed Dec 16, 2016

View changes

alexcrichton · Dec 16, 2016

src/libstd/sys/unix/backtrace/tracing/gcc_s.rs

+        // See libunwind:src/unwind/Backtrace.c for the return values.
+        // No, there is no doc.
+        let ret = match result_unwind {
+            uw::_URC_END_OF_STACK => {


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

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

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?

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

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.

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 ^^)

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?

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

alexcrichton · Dec 16, 2016

src/libstd/sys/windows/backtrace/mod.rs

        let addr = frame.AddrPC.Offset;
        if addr == frame.AddrReturn.Offset || addr == 0 ||
           frame.AddrReturn.Offset == 0 { break }

+        frames[i] = addr;


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

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

alexcrichton · Dec 16, 2016

src/libstd/sys/windows/backtrace/printing/msvc.rs


-        let name = if ret == c::TRUE {
+        if ret == c::TRUE {
            let ptr = info.Name.as_ptr() as *const c_char;
            Some(CStr::from_ptr(ptr).to_bytes())
        } else {
            None
        };


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

Coding without compiler is not a good idea

alexcrichton · Dec 16, 2016

src/libstd/sys/unix/backtrace/printing/dladdr.rs

-            CStr::from_ptr(info.dli_sname).to_bytes()
-        }))
+        Some(unsafe {
+            CStr::from_ptr(info.dli_sname).to_str().ok()


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

Oops, the "Some" shouldn't be there

alexcrichton · Dec 16, 2016

src/libstd/sys/windows/backtrace/printing/msvc.rs

+fn _foreach_symbol_fileline<F>(symbol_addr: Frame,
+                               mut f: F,
+                               context: &BacktraceContext) -> io::Result<bool>
+where F: FnMut(&[u8], libc::c_int) -> io::Result<()>


Stylistically we typically indent where clauses like these over one

alexcrichton · Dec 16, 2016

src/libstd/sys_common/backtrace.rs

+        output(w, index, *frame, resolve_symname(*frame, &context), format)?;
+        if foreach_symbol_fileline(*frame, |file, line| {
+            output_fileline(w, file, line, format)
+        }, &context)? {


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

yes, you're right

alexcrichton · Dec 16, 2016

src/libstd/sys_common/backtrace.rs

+        } else if &x == "full" {
+            Some(PrintFormat::Full)
+        } else if ["short", "yes", "1"].iter().any(|val| &x == *val)  {
+            Some(PrintFormat::Short)


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

alexcrichton · Dec 19, 2016

alexcrichton reviewed Dec 19, 2016

View changes

alexcrichton · Dec 19, 2016

src/libstd/sys/unix/backtrace/tracing/gcc_s.rs

+                match cx.last_error {
+                    Some(err) => panic!("Fatal error while unwinding: {}",
+                                        err),
+                    None => panic!("Fatal error while unwinding"),


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.

What do you suggest as return value?

Oh a bool or a Result would be fine

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

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?

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

That's what I did, finally.

alexcrichton · Dec 19, 2016

alexcrichton reviewed Dec 19, 2016

View changes

alexcrichton · Dec 19, 2016

src/libstd/sys/unix/backtrace/tracing/gcc_s.rs

-        match print(cx.writer, cx.idx, ip, symaddr) {
-            Ok(()) => {}
-            Err(e) => { cx.last_error = Some(e); }
+        if cx.last_error.is_some() {


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 · Dec 21, 2016

alexcrichton reviewed Dec 21, 2016

View changes

alexcrichton · Dec 21, 2016

src/libstd/sys/unix/backtrace/tracing/gcc_s.rs

            }
-            _ => Ok(()),
+            _ => unreachable!("\"{:?}\" should not be triggered",
+                              result_unwind),


Could this return an error?

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

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.

petrochenkov · Feb 27, 2017

@bors r+

bors · Feb 27, 2017

📌 Commit 52bed53 has been approved by petrochenkov

bors · Feb 27, 2017

⌛️ Testing commit 52bed53 with merge bea2b27...

bors · Feb 27, 2017

💔 Test failed - status-appveyor

petrochenkov · Feb 27, 2017

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 · Feb 27, 2017

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 · Feb 27, 2017

petrochenkov reviewed Feb 27, 2017

View changes

petrochenkov · Feb 27, 2017

src/test/run-pass/backtrace.rs

-        assert!(!s.contains(symbol),
-                "{} should be removed from the backtrace\n{}",
-                symbol, s);
-    }
    assert!(s.contains(" 0:"), "the frame number should start at 0");

    // Only on linux for _start and __libc_start_main


This one failed on Travis too.

petrochenkov · Feb 27, 2017

@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 · Feb 27, 2017

@bors r+

bors · Feb 27, 2017

📌 Commit 6398b20 has been approved by petrochenkov

bors · Feb 27, 2017

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

bors · Feb 27, 2017

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

Yamakaky · Feb 27, 2017

\o/
Finally!

jdm · Mar 16, 2017

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

Yamakaky · Mar 16, 2017

You should open an issue.

rust-highfive assigned brson Dec 4, 2016

Yamakaky force-pushed the Yamakaky:better-backtrace branch from e94d312 to ebc1ce9 Dec 5, 2016

Yamakaky force-pushed the Yamakaky:better-backtrace branch to 1aa97b5 Dec 6, 2016

rust-highfive assigned alexcrichton and unassigned brson Dec 10, 2016

Yamakaky force-pushed the Yamakaky:better-backtrace branch from 8e563c1 to 4447538 Dec 15, 2016

Yamakaky force-pushed the Yamakaky:better-backtrace branch from 86a862d to 52bed53 Feb 27, 2017

This test is too hard to maintain cross-platform

Verified

This commit was signed with a verified signature.

Yamakaky Yamakaky

GPG key ID: 1F5120C66C0B64F7 Learn about signing commits

Loading status checks…

6398b20

Yamakaky force-pushed the Yamakaky:better-backtrace branch from 9cfd356 to 6398b20 Feb 27, 2017

bors merged commit 6398b20 into rust-lang:master Feb 27, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

Yamakaky deleted the Yamakaky:better-backtrace branch Feb 27, 2017

Yamakaky referenced this pull request Mar 2, 2017
Closed
Better backtrace cleaning on panic #40201

alygin referenced this pull request Mar 13, 2017
Merged
RUN: Support short backtraces #1066

jdm referenced this pull request Mar 16, 2017
Open
Implement filtering behaviour like RUST_BACKTRACE=short #38

petrochenkov referenced this pull request Mar 21, 2017
Closed
Ignore more frames on backtrace unwinding. #40264

F001 referenced this pull request May 14, 2017
Closed
update backtrace formating text tips #41989

estebank referenced this pull request Sep 22, 2017
Closed
Filter stdlib out of stack traces #25621

main-- referenced this pull request Dec 16, 2017
Open
Debuginfo-based panic locations #2154

SimonSapin referenced this pull request Jan 14, 2018
Open
Short backtrace have regressed #47429

rust-lang/rust

Join GitHub today

Improve backtrace formating while panicking. #38165

Conversation

Yamakaky commented Dec 4, 2016 • edited

rust-highfive assigned brson Dec 4, 2016

This comment has been minimized.

rust-highfive commented Dec 4, 2016

This comment has been minimized.

Yamakaky commented Dec 4, 2016 • edited

This comment has been minimized.

Yamakaky commented Dec 5, 2016 • edited

This comment has been minimized.

Mark-Simulacrum commented Dec 5, 2016

This comment has been minimized.

Yamakaky commented Dec 5, 2016

This comment has been minimized.

Yamakaky commented Dec 5, 2016

Yamakaky force-pushed the Yamakaky:better-backtrace branch from e94d312 to ebc1ce9 Dec 5, 2016

This comment has been minimized.

Yamakaky commented Dec 5, 2016

estebank reviewed Dec 5, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Yamakaky commented Dec 5, 2016

This comment has been minimized.

Yamakaky commented Dec 5, 2016

This comment has been minimized.

Yamakaky commented Dec 6, 2016

Yamakaky force-pushed the Yamakaky:better-backtrace branch to 1aa97b5 Dec 6, 2016

This comment has been minimized.

Yamakaky commented Dec 10, 2016

rust-highfive assigned alexcrichton and unassigned brson Dec 10, 2016

alexcrichton reviewed Dec 12, 2016 View changes

alexcrichton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Yamakaky Dec 13, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Yamakaky commented Dec 13, 2016

This comment has been minimized.

Yamakaky commented Dec 13, 2016

This comment has been minimized.

Yamakaky commented Dec 13, 2016

This comment has been minimized.

Mark-Simulacrum commented Dec 13, 2016

This comment has been minimized.

Yamakaky commented Dec 14, 2016

This comment has been minimized.

Yamakaky commented Dec 14, 2016 • edited

Yamakaky force-pushed the Yamakaky:better-backtrace branch from 8e563c1 to 4447538 Dec 15, 2016

This comment has been minimized.

Yamakaky commented Dec 15, 2016

alexcrichton reviewed Dec 16, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Yamakaky commented Dec 4, 2016 •

edited

Yamakaky commented Dec 4, 2016 •

edited

Yamakaky commented Dec 5, 2016 •

edited

Yamakaky force-pushed the Yamakaky:better-backtrace branch from `e94d312` to `ebc1ce9` Dec 5, 2016

estebank reviewed Dec 5, 2016

View changes

Yamakaky force-pushed the Yamakaky:better-backtrace branch to `1aa97b5` Dec 6, 2016

alexcrichton reviewed Dec 12, 2016

View changes

Yamakaky Dec 13, 2016 •

edited

Yamakaky commented Dec 14, 2016 •

edited

Yamakaky force-pushed the Yamakaky:better-backtrace branch from `8e563c1` to `4447538` Dec 15, 2016

alexcrichton reviewed Dec 16, 2016

View changes

alexcrichton reviewed Dec 19, 2016

View changes

alexcrichton reviewed Dec 19, 2016

View changes

alexcrichton reviewed Dec 21, 2016

View changes