| Message ID | 20191120145555.GA15154@ls3530.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] linux-user/strace: Add missing signal strings | expand |
On 11/20/19 3:55 PM, Helge Deller wrote: > Add the textual representations of some missing target signals. > > Signed-off-by: Helge Deller <deller@gmx.de> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 11/20/19 3:55 PM, Helge Deller wrote: > Add the textual representations of some missing target signals. > > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/linux-user/strace.c b/linux-user/strace.c > index 3d4d684450..de43238fa4 100644 > --- a/linux-user/strace.c > +++ b/linux-user/strace.c > @@ -146,6 +146,19 @@ print_signal(abi_ulong arg, int last) > case TARGET_SIGSTOP: signal_name = "SIGSTOP"; break; > case TARGET_SIGTTIN: signal_name = "SIGTTIN"; break; > case TARGET_SIGTTOU: signal_name = "SIGTTOU"; break; > + case TARGET_SIGIO: signal_name = "SIGIO"; break; > + case TARGET_SIGBUS: signal_name = "SIGBUS"; break; > + case TARGET_SIGPWR: signal_name = "SIGPWR"; break; > + case TARGET_SIGURG: signal_name = "SIGURG"; break; > + case TARGET_SIGSYS: signal_name = "SIGSYS"; break; > + case TARGET_SIGTRAP: signal_name = "SIGTRAP"; break; > + case TARGET_SIGXCPU: signal_name = "SIGXCPU"; break; > + case TARGET_SIGPROF: signal_name = "SIGPROF"; break; > + case TARGET_SIGTSTP: signal_name = "SIGTSTP"; break; > + case TARGET_SIGXFSZ: signal_name = "SIGXFSZ"; break; > + case TARGET_SIGWINCH: signal_name = "SIGWINCH"; break; > + case TARGET_SIGVTALRM: signal_name = "SIGVTALRM"; break; > + case TARGET_SIGSTKFLT: signal_name = "SIGSTKFLT"; break; > } > if (signal_name == NULL) { > print_raw_param("%ld", arg, last); > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Le 20/11/2019 à 15:55, Helge Deller a écrit : > Add the textual representations of some missing target signals. > > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/linux-user/strace.c b/linux-user/strace.c > index 3d4d684450..de43238fa4 100644 > --- a/linux-user/strace.c > +++ b/linux-user/strace.c > @@ -146,6 +146,19 @@ print_signal(abi_ulong arg, int last) > case TARGET_SIGSTOP: signal_name = "SIGSTOP"; break; > case TARGET_SIGTTIN: signal_name = "SIGTTIN"; break; > case TARGET_SIGTTOU: signal_name = "SIGTTOU"; break; > + case TARGET_SIGIO: signal_name = "SIGIO"; break; > + case TARGET_SIGBUS: signal_name = "SIGBUS"; break; > + case TARGET_SIGPWR: signal_name = "SIGPWR"; break; > + case TARGET_SIGURG: signal_name = "SIGURG"; break; > + case TARGET_SIGSYS: signal_name = "SIGSYS"; break; > + case TARGET_SIGTRAP: signal_name = "SIGTRAP"; break; > + case TARGET_SIGXCPU: signal_name = "SIGXCPU"; break; > + case TARGET_SIGPROF: signal_name = "SIGPROF"; break; > + case TARGET_SIGTSTP: signal_name = "SIGTSTP"; break; > + case TARGET_SIGXFSZ: signal_name = "SIGXFSZ"; break; > + case TARGET_SIGWINCH: signal_name = "SIGWINCH"; break; > + case TARGET_SIGVTALRM: signal_name = "SIGVTALRM"; break; > + case TARGET_SIGSTKFLT: signal_name = "SIGSTKFLT"; break; > } > if (signal_name == NULL) { > print_raw_param("%ld", arg, last); > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On Wed, Nov 20, 2019 at 3:57 PM Helge Deller <deller@gmx.de> wrote: > > Add the textual representations of some missing target signals. > > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/linux-user/strace.c b/linux-user/strace.c > index 3d4d684450..de43238fa4 100644 > --- a/linux-user/strace.c > +++ b/linux-user/strace.c > @@ -146,6 +146,19 @@ print_signal(abi_ulong arg, int last) > case TARGET_SIGSTOP: signal_name = "SIGSTOP"; break; > case TARGET_SIGTTIN: signal_name = "SIGTTIN"; break; > case TARGET_SIGTTOU: signal_name = "SIGTTOU"; break; > + case TARGET_SIGIO: signal_name = "SIGIO"; break; > + case TARGET_SIGBUS: signal_name = "SIGBUS"; break; > + case TARGET_SIGPWR: signal_name = "SIGPWR"; break; > + case TARGET_SIGURG: signal_name = "SIGURG"; break; > + case TARGET_SIGSYS: signal_name = "SIGSYS"; break; > + case TARGET_SIGTRAP: signal_name = "SIGTRAP"; break; > + case TARGET_SIGXCPU: signal_name = "SIGXCPU"; break; > + case TARGET_SIGPROF: signal_name = "SIGPROF"; break; > + case TARGET_SIGTSTP: signal_name = "SIGTSTP"; break; > + case TARGET_SIGXFSZ: signal_name = "SIGXFSZ"; break; > + case TARGET_SIGWINCH: signal_name = "SIGWINCH"; break; > + case TARGET_SIGVTALRM: signal_name = "SIGVTALRM"; break; > + case TARGET_SIGSTKFLT: signal_name = "SIGSTKFLT"; break; What about TARGET_SIGEMT, TARGET_SIGIOT ? Those are missing from MIPS-specific list of signals, and they won't be printed as strings. I think you should add: #if defined TARGET_SIGEMT case TARGET_SIGEMT: signal_name = "SIGEMT"; break; #endif case TARGET_SIGIOT: signal_name = "SIGIOT"; break; (I believe "#if defined"s is needed only for SIG_EMT, but doublecheck.) Without this, this patch favors other platforms over MIPS, which is certainly not a good/fair thing. There might be some similar case or two for other platforms too (alpha, sparc perhaps). Your reference should be kernel files: arch/<platform>/include/uapi/asm/sighal.h. In fact, there is some peace of kernell code that exactly deal with the same problem - getting the names of the signals. It is in security/apparmor/include/sig_names.h ( https://elixir.bootlin.com/linux/v5.4-rc8/source/security/apparmor/include/sig_names.h ) Since the file is short, I am inserting the whole content here: #include <linux/signal.h> #define SIGUNKNOWN 0 #define MAXMAPPED_SIG 35 #define MAXMAPPED_SIGNAME (MAXMAPPED_SIG + 1) #define SIGRT_BASE 128 /* provide a mapping of arch signal to internal signal # for mediation * those that are always an alias SIGCLD for SIGCLHD and SIGPOLL for SIGIO * map to the same entry those that may/or may not get a separate entry */ static const int sig_map[MAXMAPPED_SIG] = { [0] = MAXMAPPED_SIG, /* existence test */ [SIGHUP] = 1, [SIGINT] = 2, [SIGQUIT] = 3, [SIGILL] = 4, [SIGTRAP] = 5, /* -, 5, - */ [SIGABRT] = 6, /* SIGIOT: -, 6, - */ [SIGBUS] = 7, /* 10, 7, 10 */ [SIGFPE] = 8, [SIGKILL] = 9, [SIGUSR1] = 10, /* 30, 10, 16 */ [SIGSEGV] = 11, [SIGUSR2] = 12, /* 31, 12, 17 */ [SIGPIPE] = 13, [SIGALRM] = 14, [SIGTERM] = 15, #ifdef SIGSTKFLT [SIGSTKFLT] = 16, /* -, 16, - */ #endif [SIGCHLD] = 17, /* 20, 17, 18. SIGCHLD -, -, 18 */ [SIGCONT] = 18, /* 19, 18, 25 */ [SIGSTOP] = 19, /* 17, 19, 23 */ [SIGTSTP] = 20, /* 18, 20, 24 */ [SIGTTIN] = 21, /* 21, 21, 26 */ [SIGTTOU] = 22, /* 22, 22, 27 */ [SIGURG] = 23, /* 16, 23, 21 */ [SIGXCPU] = 24, /* 24, 24, 30 */ [SIGXFSZ] = 25, /* 25, 25, 31 */ [SIGVTALRM] = 26, /* 26, 26, 28 */ [SIGPROF] = 27, /* 27, 27, 29 */ [SIGWINCH] = 28, /* 28, 28, 20 */ [SIGIO] = 29, /* SIGPOLL: 23, 29, 22 */ [SIGPWR] = 30, /* 29, 30, 19. SIGINFO 29, -, - */ #ifdef SIGSYS [SIGSYS] = 31, /* 12, 31, 12. often SIG LOST/UNUSED */ #endif #ifdef SIGEMT [SIGEMT] = 32, /* 7, - , 7 */ #endif #if defined(SIGLOST) && SIGPWR != SIGLOST /* sparc */ [SIGLOST] = 33, /* unused on Linux */ #endif #if defined(SIGUNUSED) && \ defined(SIGLOST) && defined(SIGSYS) && SIGLOST != SIGSYS [SIGUNUSED] = 34, /* -, 31, - */ #endif }; /* this table is ordered post sig_map[sig] mapping */ static const char *const sig_names[MAXMAPPED_SIGNAME] = { "unknown", "hup", "int", "quit", "ill", "trap", "abrt", "bus", "fpe", "kill", "usr1", "segv", "usr2", "pipe", "alrm", "term", "stkflt", "chld", "cont", "stop", "stp", "ttin", "ttou", "urg", "xcpu", "xfsz", "vtalrm", "prof", "winch", "io", "pwr", "sys", "emt", "lost", "unused", "exists", /* always last existence test mapped to MAXMAPPED_SIG */ }; I think you should mirror the functionality from that file. Yours, Aleksandar > } > if (signal_name == NULL) { > print_raw_param("%ld", arg, last); >
On Thu, Nov 21, 2019 at 6:27 PM Laurent Vivier <laurent@vivier.eu> wrote: > > Le 20/11/2019 à 15:55, Helge Deller a écrit : > > Add the textual representations of some missing target signals. > > > > Signed-off-by: Helge Deller <deller@gmx.de> > > > > diff --git a/linux-user/strace.c b/linux-user/strace.c > > index 3d4d684450..de43238fa4 100644 > > --- a/linux-user/strace.c > > +++ b/linux-user/strace.c > > @@ -146,6 +146,19 @@ print_signal(abi_ulong arg, int last) > > case TARGET_SIGSTOP: signal_name = "SIGSTOP"; break; > > case TARGET_SIGTTIN: signal_name = "SIGTTIN"; break; > > case TARGET_SIGTTOU: signal_name = "SIGTTOU"; break; > > + case TARGET_SIGIO: signal_name = "SIGIO"; break; > > + case TARGET_SIGBUS: signal_name = "SIGBUS"; break; > > + case TARGET_SIGPWR: signal_name = "SIGPWR"; break; > > + case TARGET_SIGURG: signal_name = "SIGURG"; break; > > + case TARGET_SIGSYS: signal_name = "SIGSYS"; break; > > + case TARGET_SIGTRAP: signal_name = "SIGTRAP"; break; > > + case TARGET_SIGXCPU: signal_name = "SIGXCPU"; break; > > + case TARGET_SIGPROF: signal_name = "SIGPROF"; break; > > + case TARGET_SIGTSTP: signal_name = "SIGTSTP"; break; > > + case TARGET_SIGXFSZ: signal_name = "SIGXFSZ"; break; > > + case TARGET_SIGWINCH: signal_name = "SIGWINCH"; break; > > + case TARGET_SIGVTALRM: signal_name = "SIGVTALRM"; break; > > + case TARGET_SIGSTKFLT: signal_name = "SIGSTKFLT"; break; > > } > > if (signal_name == NULL) { > > print_raw_param("%ld", arg, last); > > > > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > Laurent, I have a significant objection over this patch (in this form). Please read my response in this thread. Thanks, Aleksandar
On 21.11.19 19:43, Aleksandar Markovic wrote: > On Wed, Nov 20, 2019 at 3:57 PM Helge Deller <deller@gmx.de> wrote: >> >> Add the textual representations of some missing target signals. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> >> diff --git a/linux-user/strace.c b/linux-user/strace.c >> index 3d4d684450..de43238fa4 100644 >> --- a/linux-user/strace.c >> +++ b/linux-user/strace.c >> @@ -146,6 +146,19 @@ print_signal(abi_ulong arg, int last) >> case TARGET_SIGSTOP: signal_name = "SIGSTOP"; break; >> case TARGET_SIGTTIN: signal_name = "SIGTTIN"; break; >> case TARGET_SIGTTOU: signal_name = "SIGTTOU"; break; >> + case TARGET_SIGIO: signal_name = "SIGIO"; break; >> + case TARGET_SIGBUS: signal_name = "SIGBUS"; break; >> + case TARGET_SIGPWR: signal_name = "SIGPWR"; break; >> + case TARGET_SIGURG: signal_name = "SIGURG"; break; >> + case TARGET_SIGSYS: signal_name = "SIGSYS"; break; >> + case TARGET_SIGTRAP: signal_name = "SIGTRAP"; break; >> + case TARGET_SIGXCPU: signal_name = "SIGXCPU"; break; >> + case TARGET_SIGPROF: signal_name = "SIGPROF"; break; >> + case TARGET_SIGTSTP: signal_name = "SIGTSTP"; break; >> + case TARGET_SIGXFSZ: signal_name = "SIGXFSZ"; break; >> + case TARGET_SIGWINCH: signal_name = "SIGWINCH"; break; >> + case TARGET_SIGVTALRM: signal_name = "SIGVTALRM"; break; >> + case TARGET_SIGSTKFLT: signal_name = "SIGSTKFLT"; break; > > What about TARGET_SIGEMT, TARGET_SIGIOT ? Those are missing from > MIPS-specific list of signals, and they won't be printed as strings. I > think you should add: > > #if defined TARGET_SIGEMT > case TARGET_SIGEMT: signal_name = "SIGEMT"; break; > #endif > case TARGET_SIGIOT: signal_name = "SIGIOT"; break; Please see my first version of the patch. There I added the TARGET_SIGIOT as comment. I assume there is a build issue if you add a case for TARGET_SIGIOT, since it probably conflicts with TARGET_SIGABRT. That's probably why TARGET_SIGIOT is commented out in linux-user/signal.c as well? For SIGEMT I don't know. It's only defined for MIPS, and I can't test-compile it. I'm happy to add the #ifdef mentioned above if you could test it. > (I believe "#if defined"s is needed only for SIG_EMT, but doublecheck.) Yes, it's not defined for any other arch. > Without this, this patch favors other platforms over MIPS, which is > certainly not a good/fair thing. Everyone loves the own baby :-) > There might be some similar case or two for other platforms too > (alpha, sparc perhaps). I think I checked with the table in linux-user/signal.c before sending. > Your reference should be kernel files: > arch/<platform>/include/uapi/asm/sighal.h. > > In fact, there is some peace of kernell code that exactly deal with > the same problem - getting the names of the signals. It is in > security/apparmor/include/sig_names.h > > ( https://elixir.bootlin.com/linux/v5.4-rc8/source/security/apparmor/include/sig_names.h > ) > > Since the file is short, I am inserting the whole content here: > > #include <linux/signal.h> > > #define SIGUNKNOWN 0 > #define MAXMAPPED_SIG 35 > #define MAXMAPPED_SIGNAME (MAXMAPPED_SIG + 1) > #define SIGRT_BASE 128 > > /* provide a mapping of arch signal to internal signal # for mediation > * those that are always an alias SIGCLD for SIGCLHD and SIGPOLL for SIGIO > * map to the same entry those that may/or may not get a separate entry > */ > static const int sig_map[MAXMAPPED_SIG] = { > [0] = MAXMAPPED_SIG, /* existence test */ > [SIGHUP] = 1, > [SIGINT] = 2, > [SIGQUIT] = 3, > [SIGILL] = 4, > [SIGTRAP] = 5, /* -, 5, - */ > [SIGABRT] = 6, /* SIGIOT: -, 6, - */ > [SIGBUS] = 7, /* 10, 7, 10 */ > [SIGFPE] = 8, > [SIGKILL] = 9, > [SIGUSR1] = 10, /* 30, 10, 16 */ > [SIGSEGV] = 11, > [SIGUSR2] = 12, /* 31, 12, 17 */ > [SIGPIPE] = 13, > [SIGALRM] = 14, > [SIGTERM] = 15, > #ifdef SIGSTKFLT > [SIGSTKFLT] = 16, /* -, 16, - */ > #endif > [SIGCHLD] = 17, /* 20, 17, 18. SIGCHLD -, -, 18 */ > [SIGCONT] = 18, /* 19, 18, 25 */ > [SIGSTOP] = 19, /* 17, 19, 23 */ > [SIGTSTP] = 20, /* 18, 20, 24 */ > [SIGTTIN] = 21, /* 21, 21, 26 */ > [SIGTTOU] = 22, /* 22, 22, 27 */ > [SIGURG] = 23, /* 16, 23, 21 */ > [SIGXCPU] = 24, /* 24, 24, 30 */ > [SIGXFSZ] = 25, /* 25, 25, 31 */ > [SIGVTALRM] = 26, /* 26, 26, 28 */ > [SIGPROF] = 27, /* 27, 27, 29 */ > [SIGWINCH] = 28, /* 28, 28, 20 */ > [SIGIO] = 29, /* SIGPOLL: 23, 29, 22 */ > [SIGPWR] = 30, /* 29, 30, 19. SIGINFO 29, -, - */ > #ifdef SIGSYS > [SIGSYS] = 31, /* 12, 31, 12. often SIG LOST/UNUSED */ > #endif > #ifdef SIGEMT > [SIGEMT] = 32, /* 7, - , 7 */ > #endif > #if defined(SIGLOST) && SIGPWR != SIGLOST /* sparc */ > [SIGLOST] = 33, /* unused on Linux */ > #endif > #if defined(SIGUNUSED) && \ > defined(SIGLOST) && defined(SIGSYS) && SIGLOST != SIGSYS > [SIGUNUSED] = 34, /* -, 31, - */ > #endif > }; > > /* this table is ordered post sig_map[sig] mapping */ > static const char *const sig_names[MAXMAPPED_SIGNAME] = { > "unknown", > "hup", > "int", > "quit", > "ill", > "trap", > "abrt", > "bus", > "fpe", > "kill", > "usr1", > "segv", > "usr2", > "pipe", > "alrm", > "term", > "stkflt", > "chld", > "cont", > "stop", > "stp", > "ttin", > "ttou", > "urg", > "xcpu", > "xfsz", > "vtalrm", > "prof", > "winch", > "io", > "pwr", > "sys", > "emt", > "lost", > "unused", > > "exists", /* always last existence test mapped to MAXMAPPED_SIG */ > }; > > I think you should mirror the functionality from that file. Simply mirroring files isn't good IMHO. The copy from bootlin above might not have all cases/fixes for all platforms (e.g. s390x, parisc, sparc, ...)? Helge
diff --git a/linux-user/strace.c b/linux-user/strace.c index 3d4d684450..de43238fa4 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -146,6 +146,19 @@ print_signal(abi_ulong arg, int last) case TARGET_SIGSTOP: signal_name = "SIGSTOP"; break; case TARGET_SIGTTIN: signal_name = "SIGTTIN"; break; case TARGET_SIGTTOU: signal_name = "SIGTTOU"; break; + case TARGET_SIGIO: signal_name = "SIGIO"; break; + case TARGET_SIGBUS: signal_name = "SIGBUS"; break; + case TARGET_SIGPWR: signal_name = "SIGPWR"; break; + case TARGET_SIGURG: signal_name = "SIGURG"; break; + case TARGET_SIGSYS: signal_name = "SIGSYS"; break; + case TARGET_SIGTRAP: signal_name = "SIGTRAP"; break; + case TARGET_SIGXCPU: signal_name = "SIGXCPU"; break; + case TARGET_SIGPROF: signal_name = "SIGPROF"; break; + case TARGET_SIGTSTP: signal_name = "SIGTSTP"; break; + case TARGET_SIGXFSZ: signal_name = "SIGXFSZ"; break; + case TARGET_SIGWINCH: signal_name = "SIGWINCH"; break; + case TARGET_SIGVTALRM: signal_name = "SIGVTALRM"; break; + case TARGET_SIGSTKFLT: signal_name = "SIGSTKFLT"; break; } if (signal_name == NULL) { print_raw_param("%ld", arg, last);
Add the textual representations of some missing target signals. Signed-off-by: Helge Deller <deller@gmx.de>