Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals
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
Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals
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 17d0 nip 161c lr
> > > 7fff93c55100 code 2 in pandafault[1000+1]
> > > After this patch, a page fault looks like:
> > >pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr
> > > 7fffb63e5100 code 2 in pandafault[13a2a+1]
> ]]
> > > 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
Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals
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
Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals
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 17d0 nip 161c lr
> > 7fff93c55100 code 2 in pandafault[1000+1]
> > After this patch, a page fault looks like:
> >pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr
> > 7fffb63e5100 code 2 in pandafault[13a2a+1]
]]
> > 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";
}
Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals
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 17d0 nip 161c lr
7fff93c55100 code 2 in pandafault[1000+1]
After this patch, a page fault looks like:
pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100
code 2 in pandafault[13a2a+1]
Signed-off-by: Murilo Opsfelder Araujo
---
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);
