| Message ID | 81534fde7cdfc6acea4889d886fbefdd606630fb.1635019124.git.mschiffer@universe-factory.net |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user/signal: Map exit signals in SIGCHLD siginfo_t | expand |
On 23/10/2021 21:59, Matthias Schiffer wrote: > When converting a siginfo_t from waitid(), the interpretation of si_status > depends on the value of si_code: For CLD_EXITED, it is an exit code and > should be copied verbatim. For other codes, it is a signal number > (possibly with additional high bits from ptrace) that should be mapped. > > This code was previously changed in commit 1c3dfb506ea3 > ("linux-user/signal: Decode waitid si_code"), but the fix was > incomplete. ping > > Tested with the following test program: > > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > #include <sys/wait.h> > > int main() { > pid_t pid = fork(); > if (pid == 0) { > exit(12); > } else { > siginfo_t siginfo = {}; > waitid(P_PID, pid, &siginfo, WEXITED); > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > } > > pid = fork(); > if (pid == 0) { > raise(SIGUSR2); > } else { > siginfo_t siginfo = {}; > waitid(P_PID, pid, &siginfo, WEXITED); > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > } > } > > Output with an x86_64 host and mips64el target before 1c3dfb506ea3 > (incorrect: exit code 12 is translated like a signal): > > Code: 1, status: 17 > Code: 2, status: 17 > > After 1c3dfb506ea3 (incorrect: signal number is not translated): > > Code: 1, status: 12 > Code: 2, status: 12 > > With this patch: > > Code: 1, status: 12 > Code: 2, status: 17 > > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> > --- > linux-user/signal.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 14d8fdfde152..8e3af98ec0a7 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo, > case TARGET_SIGCHLD: > tinfo->_sifields._sigchld._pid = info->si_pid; > tinfo->_sifields._sigchld._uid = info->si_uid; > - tinfo->_sifields._sigchld._status = info->si_status; > + if (si_code == CLD_EXITED) > + tinfo->_sifields._sigchld._status = info->si_status; > + else > + tinfo->_sifields._sigchld._status > + = host_to_target_signal(info->si_status & 0x7f) > + | (info->si_status & ~0x7f); > tinfo->_sifields._sigchld._utime = info->si_utime; > tinfo->_sifields._sigchld._stime = info->si_stime; > si_type = QEMU_SI_CHLD;
CC'ing Alistair and Andreas that were involved in original fix 1c3dfb506ea3 ("linux-user/signal:
Decode waitid si_code")
Thanks,
Laurent
Le 23/10/2021 à 21:59, Matthias Schiffer a écrit :
> When converting a siginfo_t from waitid(), the interpretation of si_status
> depends on the value of si_code: For CLD_EXITED, it is an exit code and
> should be copied verbatim. For other codes, it is a signal number
> (possibly with additional high bits from ptrace) that should be mapped.
>
> This code was previously changed in commit 1c3dfb506ea3
> ("linux-user/signal: Decode waitid si_code"), but the fix was
> incomplete.
>
> Tested with the following test program:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/wait.h>
>
> int main() {
> pid_t pid = fork();
> if (pid == 0) {
> exit(12);
> } else {
> siginfo_t siginfo = {};
> waitid(P_PID, pid, &siginfo, WEXITED);
> printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
> }
>
> pid = fork();
> if (pid == 0) {
> raise(SIGUSR2);
> } else {
> siginfo_t siginfo = {};
> waitid(P_PID, pid, &siginfo, WEXITED);
> printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
> }
> }
>
> Output with an x86_64 host and mips64el target before 1c3dfb506ea3
> (incorrect: exit code 12 is translated like a signal):
>
> Code: 1, status: 17
> Code: 2, status: 17
>
> After 1c3dfb506ea3 (incorrect: signal number is not translated):
>
> Code: 1, status: 12
> Code: 2, status: 12
>
> With this patch:
>
> Code: 1, status: 12
> Code: 2, status: 17
>
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
> linux-user/signal.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 14d8fdfde152..8e3af98ec0a7 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> case TARGET_SIGCHLD:
> tinfo->_sifields._sigchld._pid = info->si_pid;
> tinfo->_sifields._sigchld._uid = info->si_uid;
> - tinfo->_sifields._sigchld._status = info->si_status;
> + if (si_code == CLD_EXITED)
> + tinfo->_sifields._sigchld._status = info->si_status;
> + else
> + tinfo->_sifields._sigchld._status
> + = host_to_target_signal(info->si_status & 0x7f)
> + | (info->si_status & ~0x7f);
> tinfo->_sifields._sigchld._utime = info->si_utime;
> tinfo->_sifields._sigchld._stime = info->si_stime;
> si_type = QEMU_SI_CHLD;
>
Well, the original fix 1c3dfb506ea3 did clearly improve things for me, but it wasn't complete yet. At some point I gave up on finding a minimal reproducer for my remaining problems (futex-related hangs in a complex python+bash app). So, this *may* be the missing piece. Will test, but that takes a few days. Andreas Am Sonntag, 19. Dezember 2021, 16:55:16 CET schrieb Laurent Vivier: > CC'ing Alistair and Andreas that were involved in original fix 1c3dfb506ea3 ("linux-user/signal: > Decode waitid si_code") > > Thanks, > Laurent > > Le 23/10/2021 à 21:59, Matthias Schiffer a écrit : > > When converting a siginfo_t from waitid(), the interpretation of si_status > > depends on the value of si_code: For CLD_EXITED, it is an exit code and > > should be copied verbatim. For other codes, it is a signal number > > (possibly with additional high bits from ptrace) that should be mapped. > > > > This code was previously changed in commit 1c3dfb506ea3 > > ("linux-user/signal: Decode waitid si_code"), but the fix was > > incomplete. > > > > Tested with the following test program: > > > > #include <stdio.h> > > #include <stdlib.h> > > #include <unistd.h> > > #include <sys/wait.h> > > > > int main() { > > pid_t pid = fork(); > > if (pid == 0) { > > exit(12); > > } else { > > siginfo_t siginfo = {}; > > waitid(P_PID, pid, &siginfo, WEXITED); > > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > > } > > > > pid = fork(); > > if (pid == 0) { > > raise(SIGUSR2); > > } else { > > siginfo_t siginfo = {}; > > waitid(P_PID, pid, &siginfo, WEXITED); > > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > > } > > } > > > > Output with an x86_64 host and mips64el target before 1c3dfb506ea3 > > (incorrect: exit code 12 is translated like a signal): > > > > Code: 1, status: 17 > > Code: 2, status: 17 > > > > After 1c3dfb506ea3 (incorrect: signal number is not translated): > > > > Code: 1, status: 12 > > Code: 2, status: 12 > > > > With this patch: > > > > Code: 1, status: 12 > > Code: 2, status: 17 > > > > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> > > --- > > linux-user/signal.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/linux-user/signal.c b/linux-user/signal.c > > index 14d8fdfde152..8e3af98ec0a7 100644 > > --- a/linux-user/signal.c > > +++ b/linux-user/signal.c > > @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo, > > case TARGET_SIGCHLD: > > tinfo->_sifields._sigchld._pid = info->si_pid; > > tinfo->_sifields._sigchld._uid = info->si_uid; > > - tinfo->_sifields._sigchld._status = info->si_status; > > + if (si_code == CLD_EXITED) > > + tinfo->_sifields._sigchld._status = info->si_status; > > + else > > + tinfo->_sifields._sigchld._status > > + = host_to_target_signal(info->si_status & 0x7f) > > + | (info->si_status & ~0x7f); > > tinfo->_sifields._sigchld._utime = info->si_utime; > > tinfo->_sifields._sigchld._stime = info->si_stime; > > si_type = QEMU_SI_CHLD; > > > > >
Am Sonntag, 19. Dezember 2021, 00:32:09 CET schrieb Matthias Schiffer: > On 23/10/2021 21:59, Matthias Schiffer wrote: > > When converting a siginfo_t from waitid(), the interpretation of si_status > > depends on the value of si_code: For CLD_EXITED, it is an exit code and > > should be copied verbatim. For other codes, it is a signal number > > (possibly with additional high bits from ptrace) that should be mapped. > > > > This code was previously changed in commit 1c3dfb506ea3 > > ("linux-user/signal: Decode waitid si_code"), but the fix was > > incomplete. > > ping > Sorry I can't say anything about this. The hangs that I experience seem to be unrelated to the patch (no improvement, but also no worsening).
Le 23/10/2021 à 21:59, Matthias Schiffer a écrit : > When converting a siginfo_t from waitid(), the interpretation of si_status > depends on the value of si_code: For CLD_EXITED, it is an exit code and > should be copied verbatim. For other codes, it is a signal number > (possibly with additional high bits from ptrace) that should be mapped. > > This code was previously changed in commit 1c3dfb506ea3 > ("linux-user/signal: Decode waitid si_code"), but the fix was > incomplete. > > Tested with the following test program: > > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > #include <sys/wait.h> > > int main() { > pid_t pid = fork(); > if (pid == 0) { > exit(12); > } else { > siginfo_t siginfo = {}; > waitid(P_PID, pid, &siginfo, WEXITED); > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > } > > pid = fork(); > if (pid == 0) { > raise(SIGUSR2); > } else { > siginfo_t siginfo = {}; > waitid(P_PID, pid, &siginfo, WEXITED); > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > } > } > > Output with an x86_64 host and mips64el target before 1c3dfb506ea3 > (incorrect: exit code 12 is translated like a signal): > > Code: 1, status: 17 > Code: 2, status: 17 > > After 1c3dfb506ea3 (incorrect: signal number is not translated): > > Code: 1, status: 12 > Code: 2, status: 12 > > With this patch: > > Code: 1, status: 12 > Code: 2, status: 17 > > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> > --- > linux-user/signal.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 14d8fdfde152..8e3af98ec0a7 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo, > case TARGET_SIGCHLD: > tinfo->_sifields._sigchld._pid = info->si_pid; > tinfo->_sifields._sigchld._uid = info->si_uid; > - tinfo->_sifields._sigchld._status = info->si_status; > + if (si_code == CLD_EXITED) > + tinfo->_sifields._sigchld._status = info->si_status; > + else > + tinfo->_sifields._sigchld._status > + = host_to_target_signal(info->si_status & 0x7f) > + | (info->si_status & ~0x7f); > tinfo->_sifields._sigchld._utime = info->si_utime; > tinfo->_sifields._sigchld._stime = info->si_stime; > si_type = QEMU_SI_CHLD; Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Le 23/10/2021 à 21:59, Matthias Schiffer a écrit : > When converting a siginfo_t from waitid(), the interpretation of si_status > depends on the value of si_code: For CLD_EXITED, it is an exit code and > should be copied verbatim. For other codes, it is a signal number > (possibly with additional high bits from ptrace) that should be mapped. > > This code was previously changed in commit 1c3dfb506ea3 > ("linux-user/signal: Decode waitid si_code"), but the fix was > incomplete. > > Tested with the following test program: > > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > #include <sys/wait.h> > > int main() { > pid_t pid = fork(); > if (pid == 0) { > exit(12); > } else { > siginfo_t siginfo = {}; > waitid(P_PID, pid, &siginfo, WEXITED); > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > } > > pid = fork(); > if (pid == 0) { > raise(SIGUSR2); > } else { > siginfo_t siginfo = {}; > waitid(P_PID, pid, &siginfo, WEXITED); > printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); > } > } > > Output with an x86_64 host and mips64el target before 1c3dfb506ea3 > (incorrect: exit code 12 is translated like a signal): > > Code: 1, status: 17 > Code: 2, status: 17 > > After 1c3dfb506ea3 (incorrect: signal number is not translated): > > Code: 1, status: 12 > Code: 2, status: 12 > > With this patch: > > Code: 1, status: 12 > Code: 2, status: 17 > > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> > --- > linux-user/signal.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 14d8fdfde152..8e3af98ec0a7 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo, > case TARGET_SIGCHLD: > tinfo->_sifields._sigchld._pid = info->si_pid; > tinfo->_sifields._sigchld._uid = info->si_uid; > - tinfo->_sifields._sigchld._status = info->si_status; > + if (si_code == CLD_EXITED) > + tinfo->_sifields._sigchld._status = info->si_status; > + else > + tinfo->_sifields._sigchld._status > + = host_to_target_signal(info->si_status & 0x7f) > + | (info->si_status & ~0x7f); > tinfo->_sifields._sigchld._utime = info->si_utime; > tinfo->_sifields._sigchld._stime = info->si_stime; > si_type = QEMU_SI_CHLD; Applied to my linux-user-for-7.0 branch. Thanks, Laurent
diff --git a/linux-user/signal.c b/linux-user/signal.c index 14d8fdfde152..8e3af98ec0a7 100644 --- a/linux-user/signal.c +++ b/linux-user/signal.c @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo, case TARGET_SIGCHLD: tinfo->_sifields._sigchld._pid = info->si_pid; tinfo->_sifields._sigchld._uid = info->si_uid; - tinfo->_sifields._sigchld._status = info->si_status; + if (si_code == CLD_EXITED) + tinfo->_sifields._sigchld._status = info->si_status; + else + tinfo->_sifields._sigchld._status + = host_to_target_signal(info->si_status & 0x7f) + | (info->si_status & ~0x7f); tinfo->_sifields._sigchld._utime = info->si_utime; tinfo->_sifields._sigchld._stime = info->si_stime; si_type = QEMU_SI_CHLD;
When converting a siginfo_t from waitid(), the interpretation of si_status depends on the value of si_code: For CLD_EXITED, it is an exit code and should be copied verbatim. For other codes, it is a signal number (possibly with additional high bits from ptrace) that should be mapped. This code was previously changed in commit 1c3dfb506ea3 ("linux-user/signal: Decode waitid si_code"), but the fix was incomplete. Tested with the following test program: #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <sys/wait.h> int main() { pid_t pid = fork(); if (pid == 0) { exit(12); } else { siginfo_t siginfo = {}; waitid(P_PID, pid, &siginfo, WEXITED); printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); } pid = fork(); if (pid == 0) { raise(SIGUSR2); } else { siginfo_t siginfo = {}; waitid(P_PID, pid, &siginfo, WEXITED); printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status); } } Output with an x86_64 host and mips64el target before 1c3dfb506ea3 (incorrect: exit code 12 is translated like a signal): Code: 1, status: 17 Code: 2, status: 17 After 1c3dfb506ea3 (incorrect: signal number is not translated): Code: 1, status: 12 Code: 2, status: 12 With this patch: Code: 1, status: 12 Code: 2, status: 17 Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> --- linux-user/signal.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)