| Message ID | 20220310105533.3012-1-chrubis@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | syscalls/waitid10: Fix on ARM, PPC and possibly others | expand |
Hi! > While integer division by zero does trap on x86_64 and causes the SIGFPE > signal to be delivered it's not the case on all architecutes. At least > on ARM and PPC64LE division by zero simply returns undefined result > instead. > > This patch adds raise(SIGFPE) at the end of the child as a fallback to > make sure the process is killed with the right signal on all > architectures. > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c > index 869ef18bd..8c351d120 100644 > --- a/testcases/kernel/syscalls/waitid/waitid10.c > +++ b/testcases/kernel/syscalls/waitid/waitid10.c > @@ -28,7 +28,10 @@ static void run(void) > volatile int a, zero = 0; > > a = 1 / zero; > - exit(a); > + > + tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE"); This patch inroduces 'set but not used' warning for the a variable so maybe the message should look like: tst_res(TINFO, "1/0 = %i raising SIGFPE", a); > + raise(SIGFPE); > } > > TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED)); > -- > 2.34.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> While integer division by zero does trap on x86_64 and causes the SIGFPE >> signal to be delivered it's not the case on all architecutes. At least >> on ARM and PPC64LE division by zero simply returns undefined result >> instead. Nit picking: even with this patch we are still testing undefined behaviour. There are six signals that can be delivered as a consequence of a hardware exception: SIGBUS, SIGEMT, SIGFPE, SIGILL, SIGSEGV, and SIGTRAP. Which of these signals is delivered, for any given hard- ware exception, is not documented and does not always make sense. If dividing by zero produces SIGEMT then it's still valid according to the specification. FPE does stand for floating point exception, but we are dividing integers. >> >> This patch adds raise(SIGFPE) at the end of the child as a fallback to >> make sure the process is killed with the right signal on all >> architectures. >> >> Signed-off-by: Cyril Hrubis <chrubis@suse.cz> >> --- >> testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c >> index 869ef18bd..8c351d120 100644 >> --- a/testcases/kernel/syscalls/waitid/waitid10.c >> +++ b/testcases/kernel/syscalls/waitid/waitid10.c >> @@ -28,7 +28,10 @@ static void run(void) >> volatile int a, zero = 0; >> >> a = 1 / zero; >> - exit(a); >> + >> + tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE"); > > This patch inroduces 'set but not used' warning for the a variable so > maybe the message should look like: > > tst_res(TINFO, "1/0 = %i raising SIGFPE", a); > >> + raise(SIGFPE); I'm wondering if we should branch on the architecture. If it's x86[_64] then we only do divide by zero as it's reasonable to think that if the signal is not raised then this is a bug. >> } >> >> TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED)); >> -- >> 2.34.1 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp > > -- > Cyril Hrubis > chrubis@suse.cz
Hi! > >> While integer division by zero does trap on x86_64 and causes the SIGFPE > >> signal to be delivered it's not the case on all architecutes. At least > >> on ARM and PPC64LE division by zero simply returns undefined result > >> instead. > > Nit picking: even with this patch we are still testing undefined > behaviour. > > There are six signals that can be delivered as a consequence of a > hardware exception: SIGBUS, SIGEMT, SIGFPE, SIGILL, SIGSEGV, and > SIGTRAP. Which of these signals is delivered, for any given > hard- ware exception, is not documented and does not always make > sense. > > If dividing by zero produces SIGEMT then it's still valid according to > the specification. FPE does stand for floating point exception, but we > are dividing integers. Actually as far as I can tell the POSIX says that for integer division by zero you shall get SIGFPE (and si_code in siginfo se tto FPE_INTDIV) if the operation traps. It seems to be pretty well defined: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html > >> > >> This patch adds raise(SIGFPE) at the end of the child as a fallback to > >> make sure the process is killed with the right signal on all > >> architectures. > >> > >> Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > >> --- > >> testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c > >> index 869ef18bd..8c351d120 100644 > >> --- a/testcases/kernel/syscalls/waitid/waitid10.c > >> +++ b/testcases/kernel/syscalls/waitid/waitid10.c > >> @@ -28,7 +28,10 @@ static void run(void) > >> volatile int a, zero = 0; > >> > >> a = 1 / zero; > >> - exit(a); > >> + > >> + tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE"); > > > > This patch inroduces 'set but not used' warning for the a variable so > > maybe the message should look like: > > > > tst_res(TINFO, "1/0 = %i raising SIGFPE", a); > > > >> + raise(SIGFPE); > > I'm wondering if we should branch on the architecture. If it's x86[_64] > then we only do divide by zero as it's reasonable to think that if the > signal is not raised then this is a bug. That would work too I guess.
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> >> While integer division by zero does trap on x86_64 and causes the SIGFPE >> >> signal to be delivered it's not the case on all architecutes. At least >> >> on ARM and PPC64LE division by zero simply returns undefined result >> >> instead. >> >> Nit picking: even with this patch we are still testing undefined >> behaviour. >> >> There are six signals that can be delivered as a consequence of a >> hardware exception: SIGBUS, SIGEMT, SIGFPE, SIGILL, SIGSEGV, and >> SIGTRAP. Which of these signals is delivered, for any given >> hard- ware exception, is not documented and does not always make >> sense. >> >> If dividing by zero produces SIGEMT then it's still valid according to >> the specification. FPE does stand for floating point exception, but we >> are dividing integers. > > Actually as far as I can tell the POSIX says that for integer division > by zero you shall get SIGFPE (and si_code in siginfo se tto FPE_INTDIV) > if the operation traps. It seems to be pretty well defined: > > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html > >> >> >> >> This patch adds raise(SIGFPE) at the end of the child as a fallback to >> >> make sure the process is killed with the right signal on all >> >> architectures. >> >> >> >> Signed-off-by: Cyril Hrubis <chrubis@suse.cz> >> >> --- >> >> testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++- >> >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c >> >> index 869ef18bd..8c351d120 100644 >> >> --- a/testcases/kernel/syscalls/waitid/waitid10.c >> >> +++ b/testcases/kernel/syscalls/waitid/waitid10.c >> >> @@ -28,7 +28,10 @@ static void run(void) >> >> volatile int a, zero = 0; >> >> >> >> a = 1 / zero; >> >> - exit(a); >> >> + >> >> + tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE"); >> > >> > This patch inroduces 'set but not used' warning for the a variable so >> > maybe the message should look like: >> > >> > tst_res(TINFO, "1/0 = %i raising SIGFPE", a); >> > >> >> + raise(SIGFPE); >> >> I'm wondering if we should branch on the architecture. If it's x86[_64] >> then we only do divide by zero as it's reasonable to think that if the >> signal is not raised then this is a bug. > > That would work too I guess. I would still use #ifdef to remove raise(SIGFPE) on x86. I think this makes it a more solid test on x86. As it is defined in the spec I guess you could do the divide by zero on other arches and we can review the results to see if any also raise SIGFPE. With that: Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
On 21. 03. 22 16:48, Richard Palethorpe wrote: > I'm wondering if we should branch on the architecture. If it's x86[_64] > then we only do divide by zero as it's reasonable to think that if the > signal is not raised then this is a bug. It's more likely to be a hardware bug/missing feature though. Do we really care? I'd argue that removing the division altogether and just calling raise(SIGFPE) in the child process is all we need in this particular test.
Hello, Martin Doucha <mdoucha@suse.cz> writes: > On 21. 03. 22 16:48, Richard Palethorpe wrote: >> I'm wondering if we should branch on the architecture. If it's x86[_64] >> then we only do divide by zero as it's reasonable to think that if the >> signal is not raised then this is a bug. > > It's more likely to be a hardware bug/missing feature though. Do we > really care? I'd argue that removing the division altogether and just > calling raise(SIGFPE) in the child process is all we need in this > particular test. I suppose it depends on if there is a substantial difference in how the signal is raised between div by zero and raise. I guess there is some configuration to trap the faulting instruction and raise a signal. I don't have a strong opinion as, by definintion, testing undefined behaviour has uncertain results.
Hi! > >> I'm wondering if we should branch on the architecture. If it's x86[_64] > >> then we only do divide by zero as it's reasonable to think that if the > >> signal is not raised then this is a bug. > > > > It's more likely to be a hardware bug/missing feature though. Do we > > really care? I'd argue that removing the division altogether and just > > calling raise(SIGFPE) in the child process is all we need in this > > particular test. > > I suppose it depends on if there is a substantial difference in how the > signal is raised between div by zero and raise. I guess there is some > configuration to trap the faulting instruction and raise a > signal. I guess that in the case of division by zero we end up in the kernel interrupt handler where the kernel looks up the process that was running when the interrupt has raised then it queues the signal delivery and so on. In the case of raise() we just do sysenter instruction which triggers different interrupt handler and the rest would be the same we queue the signal and so on. Which is why I think that there is some value in triggering the divison by zero on architectures that enable it by default because we execute kernel interrupt handler that is rarely being executed.
On Thu, Mar 10, 2022 at 11:53 AM Cyril Hrubis <chrubis@suse.cz> wrote: > > While integer division by zero does trap on x86_64 and causes the SIGFPE > signal to be delivered it's not the case on all architecutes. With gcc version 12.0.1 I don't get SIGFPE even on x86: int main(void) { volatile int a, zero = 0; a = 1 / zero; } 0x0000000000401020 <+0>: movl $0x0,-0x4(%rsp) 0x0000000000401028 <+8>: mov -0x4(%rsp),%eax 0x000000000040102c <+12>: lea 0x1(%rax),%edx 0x000000000040102f <+15>: cmp $0x2,%edx 0x0000000000401032 <+18>: mov $0x0,%edx 0x0000000000401037 <+23>: cmova %edx,%eax 0x000000000040103a <+26>: mov %eax,-0x8(%rsp) 0x000000000040103e <+30>: xor %eax,%eax 0x0000000000401040 <+32>: ret It does trigger with a small modification: @@ -25,9 +25,9 @@ static void run(void) pidchild = SAFE_FORK(); if (!pidchild) { - volatile int a, zero = 0; + volatile int a = 1, zero = 0; - a = 1 / zero; + a = a / zero; exit(a); } > At least > on ARM and PPC64LE division by zero simply returns undefined result > instead. > > This patch adds raise(SIGFPE) at the end of the child as a fallback to > make sure the process is killed with the right signal on all > architectures. > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c > index 869ef18bd..8c351d120 100644 > --- a/testcases/kernel/syscalls/waitid/waitid10.c > +++ b/testcases/kernel/syscalls/waitid/waitid10.c > @@ -28,7 +28,10 @@ static void run(void) > volatile int a, zero = 0; > > a = 1 / zero; > - exit(a); > + > + tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE"); > + > + raise(SIGFPE); I agree with Petr, that raise(SIGFPE) would be sufficient for this test. > } > > TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED)); > -- > 2.34.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp >
diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c index 869ef18bd..8c351d120 100644 --- a/testcases/kernel/syscalls/waitid/waitid10.c +++ b/testcases/kernel/syscalls/waitid/waitid10.c @@ -28,7 +28,10 @@ static void run(void) volatile int a, zero = 0; a = 1 / zero; - exit(a); + + tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE"); + + raise(SIGFPE); } TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED));
While integer division by zero does trap on x86_64 and causes the SIGFPE signal to be delivered it's not the case on all architecutes. At least on ARM and PPC64LE division by zero simply returns undefined result instead. This patch adds raise(SIGFPE) at the end of the child as a fallback to make sure the process is killed with the right signal on all architectures. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)