Make SIGSEGV handler emit nicer backtraces #113565

workingjubilee · 2023-07-11T08:12:58Z

This annotates the code heavily with comments to explain what is going on, for the benefit of other compiler contributors. The backtrace also emits appropriate comments to clarify, to a programmer who may not know why a bunch of file paths and hexadecimal blather was just dumped into stderr, what is going on. Finally, it detects cycles and uses their regularity to avoid repeating a bunch of text. The previous backtraces we were emitting was extremely unfriendly, potentially confusing, and often alarming, and this makes things almost "nice".

We can't necessarily make them much nicer than this, because a signal handler must use "signal-safe" functions. This precludes conveniences like dynamic allocations. Fortunately, Rust's stdlib has allocation-free formatting, but it may hinder integrating this error with our localization middleware, as I wasn't able to clearly ascertain, at a glance, whether there was a zero-alloc path through it.

r? @Nilstrieb

workingjubilee · 2023-07-11T08:35:27Z

emitted output now looks like:

error: rustc interrupted by SIGSEGV, printing stack trace:

./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x106c9f6)[0x7f6e63b4d9f6]
/usr/lib/libc.so.6(+0x39ab0)[0x7f6e6274bab0]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x650251a)[0x7f6e68fe351a]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x64fb1f3)[0x7f6e68fdc1f3]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x654343e)[0x7f6e6902443e]

...this goes on for a bit...

./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x6543868)[0x7f6e69024868]

please report a bug to https://github.com/rust-lang/rust

I don't know if this error message can be localized compatibly with our Fluent infrastructure, because of the exceedingly particular requirements around stack/heap handling inside a signal handler. Will have to look into that.

workingjubilee · 2023-07-11T08:58:50Z

Ah.

impl<'args> TranslateError<'args> {
    // some code
    // then...
    pub fn fluent(
        id: &'args Cow<'args, str>,
        args: &'args FluentArgs<'args>,
        errs: Vec<FluentError>,
    ) -> Self {

I looked into it. The answer is it should almost certainly not be without changes to the Fluent API surface, which uses Vecs and the like. We do not want to touch those while inside a signal handler. Allocating or deallocating anything is forbidden. We probably don't want to even think about Boxes. ( Sorry Boxy. )

Nilstrieb

thanks, that already looks better

compiler/rustc_driver_impl/src/lib.rs

workingjubilee · 2023-07-11T15:44:08Z

The tail of the error now looks like:


error: stack trace dumped due to SIGSEGV, possible stack overflow
error: please report a bug to https://github.com/rust-lang/rust
Segmentation fault (core dumped)

workingjubilee · 2023-07-13T06:17:28Z

New output now looks like... no trimming, this was previously a large backtrace that maxed out the 256 frames via stack overflow:

$ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc overflowy.rs
error: rustc interrupted by SIGSEGV, printing backtrace

./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x8ca553)[0x7f394028a553]
/usr/lib/libc.so.6(+0x39ab0)[0x7f393f628ab0]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x24db98a)[0x7f3941e9b98a]

### cycle encountered after 3 frames with period 5
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x24d4663)[0x7f3941e94663]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x251c8ae)[0x7f3941edc8ae]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x251ccd8)[0x7f3941edccd8]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x251cda3)[0x7f3941edcda3]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x24db9a4)[0x7f3941e9b9a4]
### recursed 50 times

./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x24d4663)[0x7f3941e94663]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x251c8ae)[0x7f3941edc8ae]
./build/x86_64-unknown-linux-gnu/stage1/bin/../lib/librustc_driver-edec4982b354a535.so(+0x251ccd8)[0x7f3941edccd8]

note: rustc unexpectedly overflowed its stack! this is a bug
note: maximum backtrace depth reached, frames may have been lost
note: we would appreciate a report at https://github.com/rust-lang/rust
Segmentation fault (core dumped)

thomcc · 2023-07-14T01:10:59Z

compiler/rustc_driver_impl/src/signal_handler.rs

+        sa.sa_sigaction = print_stack_trace as libc::sighandler_t;
+        sa.sa_flags = libc::SA_NODEFER | libc::SA_RESETHAND | libc::SA_ONSTACK;
+        libc::sigemptyset(&mut sa.sa_mask);
+        libc::sigaction(libc::SIGSEGV, &sa, ptr::null_mut());


We should probably backtrace on SIGBUS too, since wild pointers can trigger sigbus just as easily (okay, maybe not just as easily, but, easily).

I was going to look into what sort of signals we wanted to extend to after, since what I really started digging into the signal handler for is to set up a SIGILL handler for when ~~the compiler needs a hot pad and some soup~~ LLVM, running in our process space, runs into one of its llvm_unreachables that on x86-64, thanks to the CMake options we build LLVM with, are compiled into ud2 instead of UB (because they're @llvm.trap()).

thomcc · 2023-07-14T01:11:41Z

compiler/rustc_driver_impl/src/signal_handler.rs

+/// When SIGSEGV is delivered to the process, print a stack trace and then exit.
+pub(super) fn install() {
+    unsafe {
+        let alt_stack_size: usize = min_sigstack_size() + 64 * 1024;


Is 64k still definitely enough stack space for this more complicated handler to run in?

So far on my machine yes, but then again my machine is probably answering getauxval(AT_MINSIGSTKSZ) with a nonzero value.

This was a good question to ask because I found a spot where a small change removes an entire panic branch (and simplifies the code in general).

thomcc · 2023-07-14T01:21:32Z

We should also consider having an env var¹ or something you can set to symbolicate that output with backtrace_symbols, since it's often enough fine.

(Note that we should just leak the result of backtrace_symbols though -- technically you can free it with free but this crashes a shocking amount of the time, expecially if you're using an overridden malloc impl)

Ofc, we should read it when installing the handler, not inside the handler. ↩

workingjubilee · 2023-07-14T01:26:59Z

I was thinking about making that the RUST_BACKTRACE=full behavior.

"oh you want the ENTIRE backtrace, do you? well..."

compiler/rustc_driver_impl/src/signal_handler.rs

workingjubilee · 2023-07-15T08:20:17Z

I'm sorry?

workingjubilee · 2023-07-15T08:32:37Z

Oh, HEAD is fucked again.

...to both compiler contributors and other rustc invokers. The previous error messaging was extremely unfriendly, and potentially both confusing and alarming. The entire modules is extracted into a new file to help ease of reading, and plenty of comments get layered in. The design of the messaging is focused on preventing the overall purpose of the output from being too opaque in common cases, so users understand why it is there. There's not an immediate need for it being actionable, but some suggestions are offered anyways.

This cleans up the formatting of the backtrace considerably, dodging the fact that a backtrace with recursion normally emits hundreds of lines. Instead, simply write repeated symbols, and add how many times the recursion occurred. Also, it makes it look more like the "normal" backtrace. Some fine details in the code are important for reducing the amount of possible panic branches.

First, we reuse the `MAX_FRAMES` constant. Co-authored-by: erikdesjardins <erikdesjardins@users.noreply.github.com> Then we choose an arbitrary recursion depth for the other case. In this case, I used 2d20. Honest.

workingjubilee · 2023-07-22T06:26:00Z

Nils is /afk
r? compiler

workingjubilee · 2023-08-03T23:09:29Z

orz
r? compiler

pnkfelix · 2023-08-17T14:56:56Z

r? @pnkfelix

pnkfelix · 2023-08-30T00:21:42Z

compiler/rustc_driver_impl/src/signal_handler.rs

+    // Begin elaborating return addrs into symbols and writing them directly to stderr
+    // Most backtraces are stack overflow, most stack overflows are from recursion
+    // Check for cycles before writing 250 lines of the same ~5 symbols
+    let cycled = |(runner, walker)| runner == walker;


This is interesting... I'm trying to decide whether there's any world where the "normal" stack backtrace mechanism could benefit from an attempt to compress its presentation in the same manner that you are doing here. (e.g. if code is tracking a depth counter or some other exhaustion-prevention mechanism, and then panicking explicitly rather than allowing the stack to actually overflow...)

But I suspect that this code belongs here and only here because in vast majority of cases, that kind of unrestricted recursion is going to end with a SIGSEGV and handled here.

I think we could lift cycle-detection algorithms up into other parts of rustc, since in many cases we run afoul of essentially unbounded recursion, but it's probably not the case that this one is sufficiently general yet to be used thusly.

pnkfelix · 2023-08-30T00:38:26Z

compiler/rustc_driver_impl/src/signal_handler.rs

+            // impossible.
+            return;
+        };
+


the code inside here might benefit from an ascii-art diagram or something.

My main concern is that even though I have passing familiarity with Floyd's tortoise and hare algorithm, I nonetheless cannot immediately tell whether you are e.g. reporting the minimal period λ, or if you are only guaranteeing that you are reporting some period ν that is a multiple of λ. (See the linked wikipedia page for where those greek letter cames from.)

... or better still: Would it make any sense to factor this out into its own generic routine that can have concrete unit tests, and then instantiate it here?

... anyway, on the other hand, this code has waited long enough to land, and it certainly looks like an improvement. Further improvements like what I am suggesting here can be follow-on PR's.

Sure! I want rustc to have a SIGILL handler as well, so I'm definitely going to be doing some followups here and imposing better factorization. It just seemed premature to do anything like that, or at least I wasn't entirely sure what would be best to factor.

pnkfelix · 2023-08-30T00:41:11Z

compiler/rustc_driver_impl/src/signal_handler.rs

@@ -87,14 +87,15 @@ extern "C" fn print_stack_trace(_: libc::c_int) {
    raw_errln!("");
    written += rem.len() + 1;

-    if cyclic || stack.len() == 256 {
+    let random_depth = || 8 * 16; // chosen by random diceroll (2d20)


(... I don't understand why this is a closure versus just a let-bound usize.)

pnkfelix · 2023-08-30T00:42:16Z

@bors r+ rollup

bors · 2023-08-30T00:42:18Z

📌 Commit 3dee977 has been approved by pnkfelix

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#113565 (Make SIGSEGV handler emit nicer backtraces) - rust-lang#114704 (parser: not insert dummy field in struct) - rust-lang#115272 (miri/diagnostics: don't forget to print_backtrace when ICEing on unexpected errors) - rust-lang#115313 (Make `get_return_block()` return `Some` only for HIR nodes in body) - rust-lang#115347 (suggest removing `impl` in generic trait bound position) - rust-lang#115355 (new solver: handle edge case of a recursion limit of 0) - rust-lang#115363 (Don't suggest adding parentheses to call an inaccessible method.) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned Nilstrieb Jul 11, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 11, 2023

workingjubilee mentioned this pull request Jul 11, 2023

Excessively chaining operators causes a stack overflow in rustc #109648

Closed

workingjubilee linked an issue Jul 11, 2023 that may be closed by this pull request

Excessively chaining operators causes a stack overflow in rustc #109648

Closed

Nilstrieb reviewed Jul 11, 2023

View reviewed changes

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved

workingjubilee force-pushed the better-signal-handler-message branch 2 times, most recently from 366afb0 to ebf1bfc Compare July 11, 2023 15:21

workingjubilee linked an issue Jul 11, 2023 that may be closed by this pull request

rustc segfault (on ridiculous unlikely code) #102248

Closed

workingjubilee changed the title ~~Explain SIGSEGV backtrace handler~~ Make SIGSEGV handler emit nicer backtraces Jul 13, 2023

This comment has been minimized.

Sign in to view

thomcc reviewed Jul 14, 2023

View reviewed changes

workingjubilee force-pushed the better-signal-handler-message branch from ce21d91 to e756ccd Compare July 14, 2023 01:17

workingjubilee force-pushed the better-signal-handler-message branch from e756ccd to 7f3bc3b Compare July 14, 2023 01:37

erikdesjardins reviewed Jul 15, 2023

View reviewed changes

compiler/rustc_driver_impl/src/signal_handler.rs Outdated Show resolved Hide resolved

compiler/rustc_driver_impl/src/signal_handler.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

workingjubilee and others added 3 commits July 19, 2023 00:15

Clarify arbitrary constants

3dee977

First, we reuse the `MAX_FRAMES` constant. Co-authored-by: erikdesjardins <erikdesjardins@users.noreply.github.com> Then we choose an arbitrary recursion depth for the other case. In this case, I used 2d20. Honest.

workingjubilee force-pushed the better-signal-handler-message branch from 86bbb9d to 3dee977 Compare July 19, 2023 07:22

rustbot assigned eholk Jul 22, 2023

rustbot unassigned Nilstrieb Jul 22, 2023

rustbot assigned cjgillot and unassigned eholk Aug 3, 2023

rustbot assigned pnkfelix and unassigned cjgillot Aug 17, 2023

pnkfelix reviewed Aug 30, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2023

matthiaskrgr mentioned this pull request Aug 30, 2023

Rollup of 7 pull requests #115370

Merged

bors merged commit dafea5f into rust-lang:master Aug 30, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Aug 30, 2023

workingjubilee deleted the better-signal-handler-message branch August 30, 2023 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SIGSEGV handler emit nicer backtraces #113565

Make SIGSEGV handler emit nicer backtraces #113565

workingjubilee commented Jul 11, 2023 •

edited

workingjubilee commented Jul 11, 2023 •

edited

workingjubilee commented Jul 11, 2023

Nilstrieb left a comment

workingjubilee commented Jul 11, 2023

workingjubilee commented Jul 13, 2023

This comment has been minimized.

thomcc Jul 14, 2023

workingjubilee Jul 14, 2023

thomcc Jul 14, 2023

workingjubilee Jul 14, 2023

workingjubilee Jul 14, 2023 •

edited

thomcc commented Jul 14, 2023

workingjubilee commented Jul 14, 2023 •

edited

This comment has been minimized.

workingjubilee commented Jul 15, 2023

workingjubilee commented Jul 15, 2023

workingjubilee commented Jul 22, 2023

workingjubilee commented Aug 3, 2023

pnkfelix commented Aug 17, 2023

pnkfelix Aug 30, 2023

workingjubilee Aug 30, 2023

pnkfelix Aug 30, 2023 •

edited

workingjubilee Aug 30, 2023 •

edited

pnkfelix Aug 30, 2023

pnkfelix commented Aug 30, 2023

bors commented Aug 30, 2023

Make SIGSEGV handler emit nicer backtraces #113565

Make SIGSEGV handler emit nicer backtraces #113565

Conversation

workingjubilee commented Jul 11, 2023 • edited

workingjubilee commented Jul 11, 2023 • edited

workingjubilee commented Jul 11, 2023

Nilstrieb left a comment

Choose a reason for hiding this comment

workingjubilee commented Jul 11, 2023

workingjubilee commented Jul 13, 2023

This comment has been minimized.

thomcc Jul 14, 2023

Choose a reason for hiding this comment

workingjubilee Jul 14, 2023

Choose a reason for hiding this comment

thomcc Jul 14, 2023

Choose a reason for hiding this comment

workingjubilee Jul 14, 2023

Choose a reason for hiding this comment

workingjubilee Jul 14, 2023 • edited

Choose a reason for hiding this comment

thomcc commented Jul 14, 2023

Footnotes

workingjubilee commented Jul 14, 2023 • edited

This comment has been minimized.

workingjubilee commented Jul 15, 2023

workingjubilee commented Jul 15, 2023

workingjubilee commented Jul 22, 2023

workingjubilee commented Aug 3, 2023

pnkfelix commented Aug 17, 2023

pnkfelix Aug 30, 2023

Choose a reason for hiding this comment

workingjubilee Aug 30, 2023

Choose a reason for hiding this comment

pnkfelix Aug 30, 2023 • edited

Choose a reason for hiding this comment

workingjubilee Aug 30, 2023 • edited

Choose a reason for hiding this comment

pnkfelix Aug 30, 2023

Choose a reason for hiding this comment

pnkfelix commented Aug 30, 2023

bors commented Aug 30, 2023

workingjubilee commented Jul 11, 2023 •

edited

workingjubilee commented Jul 11, 2023 •

edited

workingjubilee Jul 14, 2023 •

edited

workingjubilee commented Jul 14, 2023 •

edited

pnkfelix Aug 30, 2023 •

edited

workingjubilee Aug 30, 2023 •

edited