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

Display human-readable descriptions for signal types in crash handlers #77286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 20, 2023

Previously, only the signal number was displayed:

handle_crash: Program crashed with signal 4

Now, the signal code and description are shown:

handle_crash: Program crashed with signal 4 (SIGILL) - Illegal instruction

This only affects Linux and macOS, not Windows.

Previously, only the signal number was displayed:

    handle_crash: Program crashed with signal 4

Now, the signal code and description are shown:

    handle_crash: Program crashed with signal 4 (SIGILL) - Illegal instruction

This only affects Linux and macOS, not Windows.
@Riteo
Copy link
Contributor

Riteo commented May 20, 2023

Nice! My bit/signal.h header has more signals. Should we add a description for them all? I guess we could use #ifdefs or perhaps follow a standard if there's any.

Excerpt from my bit/signal.h:

#define SIGHUP    1
#define SIGINT    2
#define SIGQUIT   3
#define SIGILL    4
#define SIGTRAP   5
#define SIGABRT   6
#define SIGIOT    SIGABRT
#define SIGBUS    7
#define SIGFPE    8
#define SIGKILL   9
#define SIGUSR1   10
#define SIGSEGV   11
#define SIGUSR2   12
#define SIGPIPE   13
#define SIGALRM   14
#define SIGTERM   15
#define SIGSTKFLT 16
#define SIGCHLD   17
#define SIGCONT   18
#define SIGSTOP   19
#define SIGTSTP   20
#define SIGTTIN   21
#define SIGTTOU   22
#define SIGURG    23
#define SIGXCPU   24
#define SIGXFSZ   25
#define SIGVTALRM 26
#define SIGPROF   27
#define SIGWINCH  28
#define SIGIO     29
#define SIGPOLL   29
#define SIGPWR    30
#define SIGSYS    31
#define SIGUNUSED SIGSYS

@RedworkDE
Copy link
Member

RedworkDE commented May 28, 2023

Should we add a description for them all?

The signal handler is only registered for SIGSEGV, SIGFPE, and SIGILL, so the other cases are dead anyways.

#ifdef CRASH_HANDLER_ENABLED
signal(SIGSEGV, handle_crash);
signal(SIGFPE, handle_crash);
signal(SIGILL, handle_crash);
#endif

(same for macos)

@Riteo
Copy link
Contributor

Riteo commented May 28, 2023

@RedworkDE

The signal handler is only registered for SIGSEGV, SIGFPE, and SIGILL, so the other cases are dead anyways.

Mh, this PR humanizes a few (arguably useful) extra signals though, so we either remove them from this PR or register them.

@Calinou
Copy link
Member Author

Calinou commented May 28, 2023

I don't think we should add more signals to the crash handler, so I think it's best to remove the extra cases I added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants