Ignore panic functions in backtraces. Print inlined functions on Windows #45637

Zoxc · 2017-10-30T20:31:42Z

For the following program,

fn main() {
    panic!("hello world");
}

we now get this stack trace:

thread 'main' panicked at 'hello world', test.rs:2:4
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: test::main
             at .\test.rs:2

instead of this one:

thread 'main' panicked at 'hello world', test.rs:2:4
stack backtrace:
   0: std::sys_common::backtrace::_print
             at C:\projects\rust\src\libstd\sys_common\backtrace.rs:92
   1: std::panicking::default_hook::{{closure}}
             at C:\projects\rust\src\libstd\panicking.rs:380
   2: std::panicking::default_hook
             at C:\projects\rust\src\libstd\panicking.rs:397
   3: std::panicking::rust_panic_with_hook
             at C:\projects\rust\src\libstd\panicking.rs:577
   4: std::panicking::begin_panic<str*>
             at C:\projects\rust\src\libstd\panicking.rs:538
   5: test::main
             at .\test.rs:2
   6: panic_unwind::__rust_maybe_catch_panic
             at C:\projects\rust\src\libpanic_unwind\lib.rs:99
   7: std::rt::lang_start
             at C:\projects\rust\src\libstd\rt.rs:52
   8: __scrt_common_main_seh
             at f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283
   9: BaseThreadInitThunk

This introduces a str macro fragment used to redirect panic! called with string literals to a non-generic function. This redirection is also an optimization (and matches what libcore does).

kennytm · 2017-10-30T21:07:55Z

cc #40264 (@Yamakaky)

cc #35625 (:literal macro specifier)

Yamakaky · 2017-10-30T21:26:48Z

SGTM!
BTW, is the backtrace pruning feature on nightly? When i try, it does the old behaviour.

petrochenkov · 2017-10-30T22:20:53Z

src/libsyntax/ext/tt/macro_parser.rs

@@ -509,6 +509,7 @@ fn may_begin_with(name: &str, token: &Token) -> bool {
    }

    match name {
+        "str" => token.can_begin_str(),


All the previous matchers went through RFC process.
Well, at least I'm pretty sure this addition should not be hidden in a PR named "Ignore panic functions in backtraces".

Zoxc · 2017-10-31T18:01:11Z

I removed the str macro fragment specifier from this PR.

This does not seem to work on OS X, probably because the backtraces are incorrect there.

Zoxc · 2017-11-01T01:14:06Z

I changed how the implemented worked. Now panic functions passes a pointer to themselves into the panic handler. Backtraces then ignore functions in the backtrace before that pointer. Any functions after mark_start (calls main and thread functions) are also emitted from backtraces.

Zoxc · 2017-11-01T10:01:21Z

r? @alexcrichton

alexcrichton · 2017-11-01T15:44:17Z

src/libcore/panicking.rs

+#[cold]
+fn panic_dispatch(fmt: fmt::Arguments,
+                 file_line_col: &(&'static str, u32, u32),
+                 entry_point: &usize) -> ! {


How come this is passed as &usize? Can't this just be a usize?

I think passing locals by reference also prevents tail call optimization. I'm not really sure that this or the asm! trick works for function which never return. Currently these do not get tail call optimization applied, but I don't see a reason for that. Perhaps LLVM skips that optimization on cold functions?

I'm a little wary to have this and rely on this, are we sure that this doesn't get optimized away even with LTO? If we leave this in can it at least be documented in the source as to why it's a reference?

My guess for why this isn't optimized is because fmt::Arguments is a pretty big struct that's copied around the stack, and LLVM is probably conservative with the tail-call for now. Despite that though this seems like something that's pretty brittle to rely on?

alexcrichton · 2017-11-01T15:45:00Z

src/libstd/panicking.rs

@@ -546,8 +554,9 @@ pub fn begin_panic<M: Any + Send>(msg: M, file_line_col: &(&'static str, u32, u3
 /// run panic hooks, and then delegate to the actual implementation of panics.
 #[inline(never)]
 #[cold]
-fn rust_panic_with_hook(msg: Box<Any + Send>,
-                        file_line_col: &(&'static str, u32, u32)) -> ! {
+pub(crate) fn rust_panic_with_hook(msg: Box<Any + Send>,


How come pub(crate) is needed here?

Probably just a leftover from the earlier version.

alexcrichton · 2017-11-01T15:47:25Z

src/libstd/sys_common/backtrace.rs

+pub fn mark_start(f: &mut FnMut()) {
+    f();
+    unsafe {
+        asm!("" ::: "memory" : "volatile"); // A dummy statement to prevent tail call optimization


Does #[inline(never)] not work for inhibiting inlining and providing this as a marker?

It does not. It just prevents inlining, not the removal of stack frames.

Instead of asm!, does std::sync::atomic::compiler_fence() work?

It shouldn't.

alexcrichton · 2017-11-01T15:47:44Z

src/libstd/sys_common/backtrace.rs

+struct Function(*const ());
+unsafe impl Sync for Function {}
+
+static MARK_START: Function = Function(mark_start as *const ());


Couldn't this just store fn(...) instead of using casts and unsafe impls?

I think so. An earlier version had an array of functions, which needed casts.

alexcrichton · 2017-11-04T15:16:00Z

src/libtest/lib.rs

@@ -1484,7 +1484,10 @@ pub fn run_test(opts: &TestOpts,
 /// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
 #[inline(never)]
 fn __rust_begin_short_backtrace<F: FnOnce()>(f: F) {


This presumably no longer works now that the libstd implementation was updated?

Yeah this is now broken. We could work around it by exporting mark_start as an unstable item.

bors · 2017-11-09T20:47:13Z

☔ The latest upstream changes (presumably #45725) made this pull request unmergeable. Please resolve the merge conflicts.

Zoxc · 2017-11-15T17:38:53Z

Now this also prints inlined functions in backtraces on Windows.

@alexcrichton I'd like this to get merged in time for the next beta so I can get more complete backtraces in rustc.

cc @retep998

bjorn3 · 2017-11-12T10:05:28Z

src/libcore/panicking.rs

-    unsafe { panic_impl(fmt, file, line, col) }
-}
+    unsafe { panic_impl(fmt, file, line, col, entry_point) }
+}


Missing newline

kennytm · 2017-11-22T11:06:19Z

Ping @alexcrichton (also @retep998) is there any unresolved questions for this PR?

kennytm · 2017-11-29T11:14:31Z

Triage ping @alexcrichton! There are some responses from @Zoxc on #45637 (review) and #45637 (comment)

cc @rust-lang/libs

alexcrichton · 2017-11-30T16:18:22Z

src/libtest/lib.rs

-fn __rust_begin_short_backtrace<F: FnOnce()>(f: F) {
-    f()
+/// Clean the backtrace by using std::rt::mark_backtrace_start
+#[inline(always)]


Does this need to be either #[inline] or #[inline(always)]? If so, can it be documented as to why?

I don't think the attribute is needed here.

alexcrichton · 2017-11-30T16:19:41Z

Nice on the windows bits! Could those perhaps be moved to a separate PR? They seem unambiguously good to me while I'm still pretty uneasy on the other parts.

It seems like relying on the exact addresses of functions is quite brittle? I'm worried that this will cause regressions over time as we continue to upgrade LLVM. Is there perhaps something we can do to make this more bullet-proof rather than "insert something and pray llvm doesn't optimize it"?

Zoxc · 2017-12-01T23:43:50Z

I don't think there's anything we can do to make it bulletproof without modifying LLVM. If it breaks, it will just return to the status quo, so I don't think that's a good reason to avoid landing this. Backtraces are unfortunately best effort due to other reasons anyway.

alexcrichton · 2017-12-04T18:59:26Z

I'm personally sort of wary for that though in that this if we go down this road it's a commitment for us to make that we will receive, review, and maintain patches to fix this functionality as it regresses over time. In that sense I'm not comfortable myself r+'ing this but would want a larger sign-off from perhaps @rust-lang/libs on this functionality.

alexcrichton · 2017-12-13T21:25:38Z

src/libstd/panicking.rs

+
+#[cold]
+#[inline(never)]
+#[cfg_attr(not(stage0), notail_when_called)]


Could you add comment as well to functions in this crate for why this attribute is applied?

alexcrichton · 2017-12-13T21:26:37Z

Looks good to me, thanks! A few comments and a fixup to some indentation and otherwise looks ready to go.

eddyb · 2017-12-13T21:42:15Z

src/librustc_trans/mir/block.rs

@@ -137,6 +137,8 @@ impl<'a, 'tcx> MirContext<'a, 'tcx> {
                    this.set_debug_loc(&ret_bcx, terminator.source_info);
                    this.store_return(&ret_bcx, ret_dest, &fn_ty.ret, invokeret);
                }
+
+                None


This should probably be invokeret.

That would be bad, since InvokeInst is unrelated to CallInst and does not have a notail attribute.

They're both CallSite though?

I don't think that has a setTailCallKind method.

Oh, my bad, I just checked again and that is indeed that case. That's a big troubling, because an invoke can become a call through optimizations :/.

We should probably panic if we need to add notail on invokes then. I'll do that to check that we don't emit any invokes here.

It seems like we do emit invokes here.

~~I guess the attribute should require non-Rust ABI which implies nounwind?~~

Err, it should always be an invoke if it's calling a panic entry-point with unwinding enabled.

Isn't invoke only used when you need cleanup? Normal call instructions can also unwind and should be used here

Right, unwinding + cleanup.

eddyb · 2017-12-13T21:42:43Z

src/test/ui/feature-gate-notail_when_called.rs

+// except according to those terms.
+
+#[notail_when_called]
+fn main() {}


Missing newline at the end of file.

eddyb · 2017-12-14T09:27:49Z

src/rustllvm/RustWrapper.cpp

@@ -1226,6 +1226,10 @@ extern "C" LLVMValueRef LLVMRustBuildCall(LLVMBuilderRef B, LLVMValueRef Fn,
      unwrap(Fn), makeArrayRef(unwrap(Args), NumArgs), Bundles, Name));
 }

+extern "C" void LLVMRustSetCallNoTail(LLVMValueRef Call) {
+  dyn_cast<CallInst>(unwrap(Call))->setTailCallKind(CallInst::TCK_NoTail);


Okay, then I'd make this if (auto CI = dyn_cast<CallInst>(unwrap(Call))) CI->... just so you can pass an invoke instruction.

bors · 2017-12-17T18:53:53Z

☔ The latest upstream changes (presumably #46436) made this pull request unmergeable. Please resolve the merge conflicts.

…y points in backtraces

Zoxc · 2017-12-24T02:24:08Z

@alexcrichton It turns out notail cannot be applied to invoke instructions in LLVM, and LLVM can convert invoke instructions into call instructions (without notail).

bors · 2017-12-25T07:30:42Z

☔ The latest upstream changes (presumably #46914) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2018-01-03T15:54:11Z

@Zoxc that's unfortunate :(

Would it be possible to land the fixes for inlined functions on Windows and perhaps follow-up with an upstream bug with LLVM to add this feature?

kennytm · 2018-01-17T11:18:07Z

Hi @Zoxc, is this PR currently blocked by #47252, and is there a workaround for notail invoke or a LLVM bug report we can follow as mentioned in #45637 (comment)? Thanks!

Zoxc · 2018-01-17T22:04:49Z

There isn't any workaround nor have I filed a LLVM issue.

I should still split out the fixes to mark the start of backtraces though, which addresses #47429.

@alexcrichton

Print inlined functions on Windows Split from #45637 r? @alexcrichton

@alexcrichton

Print inlined functions on Windows Split from #45637 r? @alexcrichton

shepmaster · 2018-01-26T18:29:08Z

Ping from triage, @Zoxc ! Have you "split out the fixes to mark the start of backtraces"? If so, should this PR be closed?

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2017

petrochenkov reviewed Oct 30, 2017

View reviewed changes

Zoxc force-pushed the backtraces branch 2 times, most recently from 2507e67 to d046344 Compare October 31, 2017 16:38

Zoxc force-pushed the backtraces branch from d046344 to 64a7e31 Compare November 1, 2017 01:11

rust-highfive assigned alexcrichton Nov 1, 2017

alexcrichton reviewed Nov 4, 2017

View reviewed changes

Zoxc force-pushed the backtraces branch from 64a7e31 to ade3117 Compare November 5, 2017 17:37

Zoxc force-pushed the backtraces branch 2 times, most recently from 15bff34 to 915b654 Compare November 15, 2017 17:35

Zoxc force-pushed the backtraces branch 2 times, most recently from d63555a to 03e28c9 Compare November 15, 2017 18:28

bjorn3 requested changes Nov 15, 2017

View reviewed changes

Zoxc force-pushed the backtraces branch from 03e28c9 to f4455e3 Compare November 16, 2017 07:38

Zoxc changed the title ~~Ignore panic functions in backtraces~~ Ignore panic functions in backtraces. Print inlined functions on Windows Nov 16, 2017

Zoxc force-pushed the backtraces branch from f4455e3 to 288afe3 Compare November 16, 2017 15:42

kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 29, 2017

alexcrichton reviewed Nov 30, 2017

View reviewed changes

alexcrichton reviewed Dec 13, 2017

View reviewed changes

eddyb reviewed Dec 13, 2017

View reviewed changes

eddyb reviewed Dec 14, 2017

View reviewed changes

Zoxc added 4 commits December 23, 2017 23:58

Ignore panic related functions, functions before main and thread entr…

609fee7

…y points in backtraces

Make libtest have clean backtraces. Fix some nits.

f5f8d0e

Print inlined functions on Windows

13d806c

Remove useless attribute

28d983a

Zoxc force-pushed the backtraces branch from 91a0b97 to 28d983a Compare December 23, 2017 23:19

Zoxc mentioned this pull request Jan 7, 2018

Print inlined functions on Windows #47252

Merged

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2018

Zoxc mentioned this pull request Jan 14, 2018

Short backtrace have regressed #47429

Closed

bors added a commit that referenced this pull request Jan 25, 2018

Auto merge of #47252 - Zoxc:backtrace-win, r=alexcrichton

2b6205e

Print inlined functions on Windows Split from #45637 r? @alexcrichton

bors added a commit that referenced this pull request Jan 26, 2018

Auto merge of #47252 - Zoxc:backtrace-win, r=alexcrichton

a97cd17

Print inlined functions on Windows Split from #45637 r? @alexcrichton

Zoxc closed this Jan 28, 2018

Zoxc mentioned this pull request Jan 28, 2018

Ignore functions before main and thread entry points in backtraces #47824

Closed

Zoxc mentioned this pull request Apr 9, 2018

Filter backtrace #48702

Closed

Ignore panic functions in backtraces. Print inlined functions on Windows #45637

Ignore panic functions in backtraces. Print inlined functions on Windows #45637

Conversation

Zoxc commented Oct 30, 2017

kennytm commented Oct 30, 2017

Yamakaky commented Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Zoxc commented Oct 31, 2017

Zoxc commented Nov 1, 2017

Zoxc commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 9, 2017

Zoxc commented Nov 15, 2017

Choose a reason for hiding this comment

kennytm commented Nov 22, 2017

kennytm commented Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Nov 30, 2017

Zoxc commented Dec 1, 2017

alexcrichton commented Dec 4, 2017

Choose a reason for hiding this comment

alexcrichton commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 17, 2017

Zoxc commented Dec 24, 2017

bors commented Dec 25, 2017

alexcrichton commented Jan 3, 2018

kennytm commented Jan 17, 2018

Zoxc commented Jan 17, 2018

shepmaster commented Jan 26, 2018

Yamakaky commented Oct 30, 2017 •

edited

Loading

kennytm commented Nov 29, 2017 •

edited

Loading

eddyb Dec 14, 2017 •

edited

Loading