| Message ID | 20180731145020.14009-6-muriloo@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | powerpc: Modernize unhandled signals message | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | Test checkpatch on branch next |
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit : > This adds a human-readable name in the unhandled signal message. > > Before this patch, a page fault looked like: > > pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000] > > After this patch, a page fault looks like: > > pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000] > > Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > --- > arch/powerpc/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 37 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index 1c4f06fca370..e71f12bca146 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler); > #define TM_DEBUG(x...) do { } while(0) > #endif > > +static const char *signames[SIGRTMIN + 1] = { > + "UNKNOWN", > + "SIGHUP", // 1 > + "SIGINT", // 2 > + "SIGQUIT", // 3 > + "SIGILL", // 4 > + "unhandled trap", // 5 = SIGTRAP > + "SIGABRT", // 6 = SIGIOT > + "bus error", // 7 = SIGBUS > + "floating point exception", // 8 = SIGFPE > + "illegal instruction", // 9 = SIGILL > + "SIGUSR1", // 10 > + "segfault", // 11 = SIGSEGV > + "SIGUSR2", // 12 > + "SIGPIPE", // 13 > + "SIGALRM", // 14 > + "SIGTERM", // 15 > + "SIGSTKFLT", // 16 > + "SIGCHLD", // 17 > + "SIGCONT", // 18 > + "SIGSTOP", // 19 > + "SIGTSTP", // 20 > + "SIGTTIN", // 21 > + "SIGTTOU", // 22 > + "SIGURG", // 23 > + "SIGXCPU", // 24 > + "SIGXFSZ", // 25 > + "SIGVTALRM", // 26 > + "SIGPROF", // 27 > + "SIGWINCH", // 28 > + "SIGIO", // 29 = SIGPOLL = SIGLOST > + "SIGPWR", // 30 > + "SIGSYS", // 31 = SIGUNUSED > +}; I don't think is is worth having that full table when we only use a few of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/) I would suggest to instead use a function like this: static const char *signame(int signr) { if (signr == SIGBUS) return "bus error"; if (signr == SIGFPE) return "floating point exception"; if (signr == SIGILL) return "illegal instruction"; if (signr == SIGILL) return "segfault"; if (signr == SIGTRAP) return "unhandled trap"; return "unknown signal"; } Christophe > + > /* > * Trap & Exception support > */ > @@ -314,8 +349,8 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code, > if (!unhandled_signal(current, signr)) > return; > > - pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x", > - current->comm, current->pid, signr, > + pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x", > + current->comm, current->pid, signames[signr], signr, > addr, regs->nip, regs->link, code); > > print_vma_addr(KERN_CONT " in ", regs->nip); >
On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote: > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit : > > This adds a human-readable name in the unhandled signal message. > > Before this patch, a page fault looked like: > > pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000] > > After this patch, a page fault looks like: > > pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000] ]] > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c [] > > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler); > > #define TM_DEBUG(x...) do { } while(0) > > #endif > > > > +static const char *signames[SIGRTMIN + 1] = { > > + "UNKNOWN", > > + "SIGHUP", // 1 > > + "SIGINT", // 2 [] > I don't think is is worth having that full table when we only use a few > of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/) > > I would suggest to instead use a function like this: > > static const char *signame(int signr) > { > if (signr == SIGBUS) > return "bus error"; > if (signr == SIGFPE) > return "floating point exception"; > if (signr == SIGILL) > return "illegal instruction"; > if (signr == SIGILL) > return "segfault"; > if (signr == SIGTRAP) > return "unhandled trap"; > return "unknown signal"; > } trivia: Unless the if tests are ordered most to least likely, perhaps it would be better to use a switch/case and let the compiler decide. switch (signr) { case SIGBUS: return "bus error"; case SIGFPE: return "floating point exception"; case SIGILL: return "illegal instruction"; case SIGSEGV: return "segfault"; case SIGTRAP: return "unhandled trap"; } return "unknown signal"; }
On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote: > On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote: > > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit : > > I would suggest to instead use a function like this: > > > > static const char *signame(int signr) > > { > > if (signr == SIGBUS) > > return "bus error"; > > if (signr == SIGFPE) > > return "floating point exception"; > > if (signr == SIGILL) > > return "illegal instruction"; > > if (signr == SIGILL) > > return "segfault"; > > if (signr == SIGTRAP) > > return "unhandled trap"; > > return "unknown signal"; > > } > > trivia: > > Unless the if tests are ordered most to least likely, > perhaps it would be better to use a switch/case and > let the compiler decide. That would also show there are two entries for SIGILL (here and in the original patch), one of them very wrong. Check the table with psignal or something? Segher
On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote: > On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote: > > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit : > > > This adds a human-readable name in the unhandled signal message. > > > Before this patch, a page fault looked like: > > > pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000] > > > After this patch, a page fault looks like: > > > pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000] > ]] > > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > [] > > > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler); > > > #define TM_DEBUG(x...) do { } while(0) > > > #endif > > > > > > +static const char *signames[SIGRTMIN + 1] = { > > > + "UNKNOWN", > > > + "SIGHUP", // 1 > > > + "SIGINT", // 2 > [] > > I don't think is is worth having that full table when we only use a few > > of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/) > > > > I would suggest to instead use a function like this: > > > > static const char *signame(int signr) > > { > > if (signr == SIGBUS) > > return "bus error"; > > if (signr == SIGFPE) > > return "floating point exception"; > > if (signr == SIGILL) > > return "illegal instruction"; > > if (signr == SIGILL) > > return "segfault"; > > if (signr == SIGTRAP) > > return "unhandled trap"; > > return "unknown signal"; > > } > > trivia: > > Unless the if tests are ordered most to least likely, > perhaps it would be better to use a switch/case and > let the compiler decide. > > switch (signr) { > case SIGBUS: return "bus error"; > case SIGFPE: return "floating point exception"; > case SIGILL: return "illegal instruction"; > case SIGSEGV: return "segfault"; > case SIGTRAP: return "unhandled trap"; > } > return "unknown signal"; > } > Hi, Joe, Christophe. That's a nice enhancement. I'll do that in my next respin. Cheers Murilo
Hi, Segher. On Wed, Aug 01, 2018 at 02:49:03AM -0500, Segher Boessenkool wrote: > On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote: > > On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote: > > > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit : > > > I would suggest to instead use a function like this: > > > > > > static const char *signame(int signr) > > > { > > > if (signr == SIGBUS) > > > return "bus error"; > > > if (signr == SIGFPE) > > > return "floating point exception"; > > > if (signr == SIGILL) > > > return "illegal instruction"; > > > if (signr == SIGILL) > > > return "segfault"; > > > if (signr == SIGTRAP) > > > return "unhandled trap"; > > > return "unknown signal"; > > > } > > > > trivia: > > > > Unless the if tests are ordered most to least likely, > > perhaps it would be better to use a switch/case and > > let the compiler decide. > > That would also show there are two entries for SIGILL (here and in the > original patch), one of them very wrong. Good catch. I'll take care of that in my next respin. > Check the table with psignal or something? Nice suggestion. Thanks! Cheers Murilo
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index 1c4f06fca370..e71f12bca146 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler); #define TM_DEBUG(x...) do { } while(0) #endif +static const char *signames[SIGRTMIN + 1] = { + "UNKNOWN", + "SIGHUP", // 1 + "SIGINT", // 2 + "SIGQUIT", // 3 + "SIGILL", // 4 + "unhandled trap", // 5 = SIGTRAP + "SIGABRT", // 6 = SIGIOT + "bus error", // 7 = SIGBUS + "floating point exception", // 8 = SIGFPE + "illegal instruction", // 9 = SIGILL + "SIGUSR1", // 10 + "segfault", // 11 = SIGSEGV + "SIGUSR2", // 12 + "SIGPIPE", // 13 + "SIGALRM", // 14 + "SIGTERM", // 15 + "SIGSTKFLT", // 16 + "SIGCHLD", // 17 + "SIGCONT", // 18 + "SIGSTOP", // 19 + "SIGTSTP", // 20 + "SIGTTIN", // 21 + "SIGTTOU", // 22 + "SIGURG", // 23 + "SIGXCPU", // 24 + "SIGXFSZ", // 25 + "SIGVTALRM", // 26 + "SIGPROF", // 27 + "SIGWINCH", // 28 + "SIGIO", // 29 = SIGPOLL = SIGLOST + "SIGPWR", // 30 + "SIGSYS", // 31 = SIGUNUSED +}; + /* * Trap & Exception support */ @@ -314,8 +349,8 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code, if (!unhandled_signal(current, signr)) return; - pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x", - current->comm, current->pid, signr, + pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x", + current->comm, current->pid, signames[signr], signr, addr, regs->nip, regs->link, code); print_vma_addr(KERN_CONT " in ", regs->nip);
This adds a human-readable name in the unhandled signal message. Before this patch, a page fault looked like: pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000] After this patch, a page fault looks like: pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000] Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> --- arch/powerpc/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)